Re: Extensible Rmgr for Table AMs - Mailing list pgsql-hackers

On Mon, Apr 4, 2022 at 8:33 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> I still may change it to go back to two RmgrTables (one for builtin and
> one for custom) to remove the lingering performance doubts. Other than
> that, and some cleanup, this is pretty close to the version I intend to
> commit.

I just had a quick look over the v6 patch, few comments:

1) Why can't rmid be chosen by the extensions in sequential order from
(129 till 255), say, to start with a columnar extension can choose
129, another extension can register 130 and so on right? This way, the
RmgrStartup, RmgrCleanup, and XLogDumpDisplayRecord can avoid looping
over all the 256 entries, with a global variable like
current_max_rmid(representing the current number of rmgers)? Is there
any specific reason to allow them to choose rmgrid randomly between
129 till 255? Am I missing some discussion here?
2) RM_MAX_ID is now UINT8_MAX - do we need to make it configurable? or
do we think that only a few (127) custom rmgrs can exist?
3) Do we need to talk about extensible rmgrs and
https://wiki.postgresql.org/wiki/ExtensibleRmgr in documentation
somewhere? This will help extension developers to refer when needed.
4) Do we need to change this description?
       <para>
        The default value of this setting is the empty string, which disables
        the feature.  It can be set to <literal>all</literal> to check all
        records, or to a comma-separated list of resource managers to check
        only records originating from those resource managers.  Currently,
        the supported resource managers are <literal>heap</literal>,
        <literal>heap2</literal>, <literal>btree</literal>,
<literal>hash</literal>,
        <literal>gin</literal>, <literal>gist</literal>,
<literal>sequence</literal>,
        <literal>spgist</literal>, <literal>brin</literal>, and
<literal>generic</literal>. Onl
        superusers can change this setting.
5) Do we need to add a PGDLLIMPORT qualifier to RegisterCustomRmgr()
(possibly for RmgrStartup, RmgrTable, RmgrCleanup as well?)  so that
the Windows versions of extensions can use this feature?
6) Should the below strcmp pg_strcmp? The custom rmgrs can still use
rm_name with different cases and below code fail to catch it?
+ 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)));
7) What's the intention of the below code? It seems like below it
checks if there's already a rmgr with the given name (RM_MAX_ID). Had
it been RM_MAX_BUILTIN_ID instead of  RM_MAX_ID, the error message
does make sense. Is the intention here to not have duplicate rmgrs in
the entire RM_MAX_ID(both builtin and custom)?
+ /* 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)));
8) Per https://wiki.postgresql.org/wiki/ExtensibleRmgr, 128 is
reserved, can we have it as a macro? Or something like (RM_MAX_ID/2+1)
+#define RM_MIN_CUSTOM_ID 128
9) Thinking if there's a way to test the code, maybe exposing
RegisterCustomRmgr as a function? I think we need to have a dummy
extension that will be used for testing this kind of patches/features
but that's a separate discussion IMO.

Regards,
Bharath Rupireddy.