Re: [PATCH 4/5] Add pg_xlogdump contrib module

On 2013-02-04 13:22:26 -0300, Alvaro Herrera wrote:
>
> I didn't like this bit too much:
>
> > diff --git a/contrib/pg_xlogdump/tables.c b/contrib/pg_xlogdump/tables.c
> > new file mode 100644
> > index 0000000..e947e0d
> > --- /dev/null
> > +++ b/contrib/pg_xlogdump/tables.c
> > @@ -0,0 +1,78 @@
>
> > +/*
> > + * RmgrTable linked only to functions available outside of the backend.
> > + *
> > + * needs to be synced with src/backend/access/transam/rmgr.c
> > + */
> > +const RmgrData RmgrTable[RM_MAX_ID + 1] = {
> > + {"XLOG", NULL, xlog_desc, NULL, NULL, NULL},
> > + {"Transaction", NULL, xact_desc, NULL, NULL, NULL},
>
> So I propose the following patch instead. This lets pg_xlogdump compile
> only the file it needs, and not concern itself with the rest of rmgr.c.

Hm. Not sure whats the advantage of that, duplicates about as much and
makes it harder to copy & paste between both files.
I am fine with it, I just don't see a clear advantage of either way.

> diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
> index ce9957e..b751882 100644
> --- a/src/include/access/xlog_internal.h
> +++ b/src/include/access/xlog_internal.h
> @@ -231,15 +231,17 @@ typedef struct xl_end_of_recovery
> struct XLogRecord;
>
> /*
> - * Method table for resource managers.
> + * Method tables for resource managers.
> *
> - * RmgrTable[] is indexed by RmgrId values (see rmgr.h).
> + * RmgrDescData (for textual descriptor functions) is split so that the file it
> + * lives in can be used by frontend programs.
> + *
> + * RmgrTable[] and RmgrDescTable[] are indexed by RmgrId values (see rmgr.h).
> */
> typedef struct RmgrData
> {
> const char *rm_name;
> void (*rm_redo) (XLogRecPtr lsn, struct XLogRecord *rptr);
> - void (*rm_desc) (StringInfo buf, uint8 xl_info, char *rec);
> void (*rm_startup) (void);
> void (*rm_cleanup) (void);
> bool (*rm_safe_restartpoint) (void);
> @@ -247,6 +249,14 @@ typedef struct RmgrData
>
> extern const RmgrData RmgrTable[];
>
> +typedef struct RmgrDescData
> +{
> + void (*rm_desc) (StringInfo buf, uint8 xl_info, char *rec);
> +} RmgrDescData;
> +
> +extern const RmgrDescData RmgrDescTable[];
> +

I think we would at least need to include rm_name here as well.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services