Re: Extensible Rmgr for Table AMs

Hi,

On 2022-03-23 21:43:08 -0700, Jeff Davis wrote:
> /* must be kept in sync with RmgrData definition in xlog_internal.h */
> #define PG_RMGR(symname,name,redo,desc,identify,startup,cleanup,mask,decode) \
> - { name, redo, desc, identify, startup, cleanup, mask, decode },
> + &(struct RmgrData){ name, redo, desc, identify, startup, cleanup, mask, decode },
>
> -const RmgrData RmgrTable[RM_MAX_ID + 1] = {
> +static RmgrData *RmgrTable[RM_MAX_ID + 1] = {
> #include "access/rmgrlist.h"
> };

I think this has been discussed before, but to me it's not obvious that it's a
good idea to change RmgrTable from RmgrData to RmgrData *. That adds an
indirection, without obvious benefit.

> +
> +/*
> + * Start up all resource managers.
> + */
> +void
> +StartupResourceManagers()

(void)

> +void
> +RegisterCustomRmgr(RmgrId rmid, RmgrData *rmgr)
> +{
> + if (rmid < RM_MIN_CUSTOM_ID)
> + ereport(PANIC, errmsg("custom rmgr id %d is out of range", rmid));
> +
> + if (!process_shared_preload_libraries_in_progress)
> + ereport(ERROR,
> + (errmsg("custom rmgr must be registered while initializing modules in shared_preload_libraries")));
> +
> + ereport(LOG,
> + (errmsg("registering custom rmgr \"%s\" with ID %d",
> + rmgr->rm_name, rmid)));
> +
> + if (RmgrTable[rmid] != NULL)
> + ereport(PANIC,
> + (errmsg("custom rmgr ID %d already registered with name \"%s\"",
> + rmid, RmgrTable[rmid]->rm_name)));
> +
> + /* check for existing rmgr with the same name */
> + for (int i = 0; i <= RM_MAX_ID; i++)
> + {
> + const RmgrData *existing_rmgr = RmgrTable[i];
> +
> + if (existing_rmgr == NULL)
> + continue;
> +
> + if (!strcmp(existing_rmgr->rm_name, rmgr->rm_name))
> + ereport(PANIC,
> + (errmsg("custom rmgr \"%s\" has the same name as builtin rmgr",
> + existing_rmgr->rm_name)));
> + }
> +
> + /* register it */
> + RmgrTable[rmid] = rmgr;
> +}

Random idea: Might be worth emitting the id->name mapping just after a redo
location is determined, to make it easier to debug things.

> +RmgrData *
> +GetRmgr(RmgrId rmid)
> +{
> + return RmgrTable[rmid];
> +}

Given this is so simple, why incur the cost of a function call? Rather than
continuing to expose RmgrTable and move GetRmgr() into the header, as a static
inline? As-is this also prevent the compiler from optimizing across repeated
GetRmgr() calls (which often won't be possible anyway, but still)..

> - if (record->xl_rmid > RM_MAX_ID)
> + if (record->xl_rmid > RM_MAX_BUILTIN_ID && record->xl_rmid < RM_MIN_CUSTOM_ID)
> {
> report_invalid_record(state,
> "invalid resource manager ID %u at %X/%X",

Shouldn't this continue to enforce RM_MAX_ID as well?

> @@ -1604,12 +1603,7 @@ PerformWalRecovery(void)
>
> InRedo = true;
>
> - /* Initialize resource managers */
> - for (rmid = 0; rmid <= RM_MAX_ID; rmid++)
> - {
> - if (RmgrTable[rmid].rm_startup != NULL)
> - RmgrTable[rmid].rm_startup();
> - }
> + StartupResourceManagers();

Personally I'd rather name it ResourceManagersStartup() or RmgrStartup().

> @@ -1871,7 +1860,7 @@ ApplyWalRecord(XLogReaderState *xlogreader, XLogRecord *record, TimeLineID *repl
> xlogrecovery_redo(xlogreader, *replayTLI);
>
> /* Now apply the WAL record itself */
> - RmgrTable[record->xl_rmid].rm_redo(xlogreader);
> + GetRmgr(record->xl_rmid)->rm_redo(xlogreader);
>
> /*
> * After redo, check whether the backup pages associated with the WAL

So we have just added one indirect call and one pointer indirection
(previously RmgrTable could be resolved by the linker, now it needs to be
dereferenced again), that's not too bad. I was afraid there'd be multiple
calls.

> @@ -2101,16 +2090,16 @@ xlog_outdesc(StringInfo buf, XLogReaderState *record)
> uint8 info = XLogRecGetInfo(record);
> const char *id;
>
> - appendStringInfoString(buf, RmgrTable[rmid].rm_name);
> + appendStringInfoString(buf, GetRmgr(rmid)->rm_name);
> appendStringInfoChar(buf, '/');
>
> - id = RmgrTable[rmid].rm_identify(info);
> + id = GetRmgr(rmid)->rm_identify(info);
> if (id == NULL)
> appendStringInfo(buf, "UNKNOWN (%X): ", info & ~XLR_INFO_MASK);
> else
> appendStringInfo(buf, "%s: ", id);
>
> - RmgrTable[rmid].rm_desc(buf, record);
> + GetRmgr(rmid)->rm_desc(buf, record);
> }

Like here. It's obviously not as performance critical as replay. But it's
still a shame to add 3 calls to GetRmgr, that each then need to dereference
RmgrTable. The compiler won't be able to optimize any of that away.

> @@ -117,8 +117,8 @@ LogicalDecodingProcessRecord(LogicalDecodingContext *ctx, XLogReaderState *recor
>
> rmid = XLogRecGetRmid(record);
>
> - if (RmgrTable[rmid].rm_decode != NULL)
> - RmgrTable[rmid].rm_decode(ctx, &buf);
> + if (GetRmgr(rmid)->rm_decode != NULL)
> + GetRmgr(rmid)->rm_decode(ctx, &buf);
> else
> {
> /* just deal with xid, and done */

This one might actually matter, overhead wise.

> @@ -473,7 +473,7 @@ static void
> XLogDumpDisplayRecord(XLogDumpConfig *config, XLogReaderState *record)
> {
> const char *id;
> - const RmgrDescData *desc = &RmgrDescTable[XLogRecGetRmid(record)];
> + const RmgrDescData *desc = GetRmgrDesc(XLogRecGetRmid(record));
> uint32 rec_len;
> uint32 fpi_len;
> RelFileNode rnode;
> @@ -658,7 +658,7 @@ XLogDumpDisplayStats(XLogDumpConfig *config, XLogDumpStats *stats)
> * calculate column totals.
> */
>
> - for (ri = 0; ri < RM_NEXT_ID; ri++)
> + for (ri = 0; ri < RM_MAX_ID; ri++)
> {
> total_count += stats->rmgr_stats[ri].count;
> total_rec_len += stats->rmgr_stats[ri].rec_len;

> @@ -694,6 +694,9 @@ XLogDumpDisplayStats(XLogDumpConfig *config, XLogDumpStats *stats)
> fpi_len = stats->rmgr_stats[ri].fpi_len;
> tot_len = rec_len + fpi_len;
>
> + if (ri > RM_MAX_BUILTIN_ID && count == 0)
> + continue;
> +

Ah, I see. I had written a concerned comment about the previous hunk...

> XLogDumpStatsRow(desc->rm_name,
> count, total_count, rec_len, total_rec_len,
> fpi_len, total_fpi_len, tot_len, total_len);
> @@ -913,16 +916,16 @@ main(int argc, char **argv)
> exit(EXIT_SUCCESS);
> }
>
> - for (i = 0; i <= RM_MAX_ID; i++)
> + for (i = 0; i <= RM_MAX_BUILTIN_ID; i++)
> {
> - if (pg_strcasecmp(optarg, RmgrDescTable[i].rm_name) == 0)
> + if (pg_strcasecmp(optarg, GetRmgrDesc(i)->rm_name) == 0)
> {
> config.filter_by_rmgr[i] = true;
> config.filter_by_rmgr_enabled = true;
> break;
> }
> }
> - if (i > RM_MAX_ID)
> + if (i > RM_MAX_BUILTIN_ID)
> {
> pg_log_error("resource manager \"%s\" does not exist",
> optarg);

So we can't filter by rmgr id for non-builtin rmgr's? That sucks. Maybe add a
custom:<i> syntax? Or generally allow numerical identifiers in addition to the
names?

Greetings,

Andres Freund