Hi,
On Mon, Jan 31, 2022 at 06:36:59PM -0800, Jeff Davis wrote:
> On Tue, 2022-01-18 at 17:53 +0800, Julien Rouhaud wrote:
>
> > There's a bit of 0003 present in 002:
>
> I just squashed 0002 and 0003 together. Not large enough to keep
> separate.
Agreed.
> > + 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.
Ok.
> > +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.
Sure, but it seems something so fundamental that it should get done right
during the development phase rather than spending cycles in optimized builds to
check for it.
But even if that happened it would get caught by the final "not found" PANIC
anyway, and for end users I don't think that getting this error instead would
make much difference.
> > 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.
Agreed, API breaks happen often and that's not really a problem for third-party
extensions, especially if that comes with hard compile failure.
Other than that the patch looks good to me, as you said we just need a decision
on whether custom rmgrs are wanted or not.
