Re: Feedback on table expansion hook (including patch)

Thank you all for the feedback and insights.

Yes, the intention is to *replace* expand_inherited_rtentry() in the same
way planner_hook replaces standard_planner().

Some background: TimescaleDB implements its own partitioning based on
inheritance that predates declarative partitioning. The extension would use
the table expansion hook to do its own table expansion based on
extension-specific metadata. There was a pretty clean (but still hacky) way
to do it via the get_relation_info_hook(), but PostgreSQL 12 changed the
order of events so that this hook no longer works for this purpose. For
PostgreSQL 12+, we'd have to copy/replace a lot of PostgreSQL code to make
our custom expansion still work, and the proposed hook would allow us to
get rid of this ugliness.

With the proposed table expansion hook, you could of course also first call
expand_inherited_rtentry() yourself and then modify the result or do
additional things. However, the way we'd like to use this in TimescaleDB is
to more-or-less replace the current expansion code since we do not rely on
declarative partitioning. I am not sure a more narrowly-scoped hook makes
sense, because it would tie you to a certain way of doing things. That
would defeat the purpose of the hook. Note that expand_inherited_rtenry()
immediately branches off based on type of relation: one branch for
inheritance, one for partitioning, and so on. So, doing this in a more
narrow scope would probably require one hook per relation type or at least
a common hook with some extra info on where you are in that expansion.
Another way of looking at this is to view TimescaleDB as offering a new
relation type for partitioning, so it is natural that it would have its own
expansion branch, just like inheritance and partitioning. There are a
couple of functions that might be useful to expose publicly, however, like
expand_single_inheritance_child() since it is called from both the
inheritance and the partitioning branch.

Looking at this problem more generally, though, it does seem to me that
PostgreSQL would benefit from a more general and official
table-partitioning API that would allow custom partitioning
implementations, or make it part of the table access method API. However,
while that is an interesting thing to explore, I think this table expansion
hook goes a long way until such an API is available.

I can provide a code example of how we'd use the table expansion hook in
TimescaleDB. A more standalone example probably requires a bit more work
though.

Best,

Erik

On Sat, Mar 6, 2021 at 7:38 PM David Fetter <david(at)fetter(dot)org> wrote:

> On Sat, Mar 06, 2021 at 01:09:10PM -0500, Tom Lane wrote:
> > Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> > > On 07.05.20 10:11, Erik Nordström wrote:
> > >> I am looking for feedback on the possibility of adding a table
> expansion
> > >> hook to PostgreSQL (see attached patch).
> >
> > > Unlike the get_relation_info_hook, your proposed hook would *replace*
> > > expand_inherited_rtentry() rather than just tack on additional
> actions.
> > > That seems awfully fragile. Could you do with a hook that does
> > > additional things rather than replace a whole chunk of built-in code?
> >
> > I suppose Erik is assuming that he could call expand_inherited_rtentry
> > (or better, the previous hook occupant) when his special case doesn't
> > apply. But I'm suspicious that he'd still end up duplicating large
> > chunks of optimizer/util/inherit.c in order to carry out the special
> > case, since almost all of that is private/static functions. It
> > does seem like a more narrowly-scoped hook might be better.
> >
> > Would it be unreasonable of us to ask for a worked-out example making
> > use of the proposed hook? That'd go a long way towards resolving the
> > question of whether you can do anything useful without duplicating
> > lots of code.
> >
> > I've also been wondering, given the table-AM projects that are
> > going on, whether we shouldn't refactor things to give partitioned
> > tables a special access method, and then shove most of the planner
> > and executor's hard-wired partitioning logic into access method
> > callbacks. That would make it a lot more feasible for extensions
> > to implement custom partitioning-like behavior ... or so I guess.
>
> That seems pretty reasonable. I suspect that that process will expose
> bits of the planning and execution machinery that have gotten less
> isolated than they should be.
>
> More generally, and I'll start a separate thread on this, we should be
> working up to including a reference implementation, however tiny, of
> every extension point we supply in order to ensure that our APIs are
> at a minimum reasonably usable and remain so.
>
> Best,
> David.
> --
> David Fetter <david(at)fetter(dot)org> http://fetter.org/
> Phone: +1 415 235 3778
>
> Remember to vote!
> Consider donating to Postgres: http://www.postgresql.org/about/donate
>