On Tue, 2022-01-18 at 17:53 +0800, Julien Rouhaud wrote:
> > Review on patch 0001 would be helpful. It makes decoding just
> > another
> > method of rmgr, which makes a lot of sense to me from a code
> > organization standpoint regardless of the other patches. Is there
> > any
> > reason not to do that?
>
> I think that it's a clean and sensible approach, so +1 for me.
Thank you, committed 0001. Other patches not committed yet.
> There's a bit of 0003 present in 002:
I just squashed 0002 and 0003 together. Not large enough to keep
separate.
> A few random comments on 0003:
>
> #define RM_MAX_ID (RM_NEXT_ID - 1)
> +#define RM_CUSTOM_MIN_ID 128
> +#define RM_CUSTOM_MAX_ID UINT8_MAX
>
> It would be a good idea to add a StaticAssertStmt here to make sure
> that
> there's no overlap in the ranges.
Done.
> +
> +/*
> + * RmgrId to use for extensions that require an RmgrId, but are
> still in
> + * development and have not reserved their own unique RmgrId yet.
> See:
> + * https://wiki.postgresql.org/wiki/ExtensibleRmgr
> + */
> +#define RM_EXPERIMENTAL_ID 128
>
> I'm a bit dubious about the whole "register your ID in the Wiki"
> approach. It
> might not be a problem since there probably won't be hundreds of
> users, and I
> don't have any better suggestion since it has to be consistent across
> nodes.
Agree, but I don't see a better approach, either.
I do some sanity checking, which should catch collisions when they
happen.
> + elog(LOG, "registering customer rmgr \"%s\" with ID %d",
> + rmgr->rm_name, rmid);
>
> Should it be a DEBUG message instead? Also s/customer/custom/
It seems like a fairly important thing to have in the log. Only a
couple extensions will ever encounter this message, and only at server
start.
Typo fixed.
> +RmgrData
> +GetCustomRmgr(RmgrId rmid)
> +{
> + if (rmid < RM_CUSTOM_MIN_ID || rmid > RM_CUSTOM_MAX_ID)
> + elog(PANIC, "custom rmgr id %d out of range", rmid);
>
> Should this be an assert?
If we make it an Assert, then it won't be caught in production builds.
> +#define GetBuiltinRmgr(rmid) RmgrTable[(rmid)]
> +#define GetRmgr(rmid) ((rmid < RM_CUSTOM_MIN_ID) ? \
> + GetBuiltinRmgr(rmid) : GetCustomRmgr(rmid))
>
> rmid should be protected in the macro.
Done.
> How you plan to mark it experimental?
I suppose it doesn't need to be marked explicitly -- there are other
APIs that change. For instance, the ProcessUtility_hook changed, and
that's used much more widely.
As long as we generally agree that some kind of custom rmgrs are the
way to go, if we change the API or implementation around from version
to version I can easily update my table access method.
Regards,
Jeff Davis