On Mon, Apr 4, 2022 at 8:33 AM Jeff Davis <pg...@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.
