Re: Extensible Rmgr for Table AMs

On Mon, 2022-03-28 at 11:00 -0700, Andres Freund wrote:
> > +/*
> > + * Start up all resource managers.
> > + */
> > +void
> > +StartupResourceManagers()
> 
> (void)

Fixed.

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

Not quite sure I understood this idea, do you mean dump all rmgrs and
the IDs when performing recovery?

> > +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?

Made it static inline.

> Shouldn't this continue to enforce RM_MAX_ID as well?

Done.

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

Done.

> 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.

Changed to only call it once and save it in a variable for each call
site where it makes sense.

> 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?

Good idea. I changed it to allow "custom###" to mean the custom rmgr
with ID ### (3 digits).

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.

Regards,
    Jeff Davis