Re: Feedback on table expansion hook (including patch)

(Sorry about being very late to this thread.)

On Sun, Mar 7, 2021 at 3:09 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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.

Yeah, I do wonder if all of the things that are now done under
expand_inherited_rtentry() are not necessary for Timescale child
relations for the queries to work correctly? In 428b260f87, the
commit in v12 responsible for this discussion AFAICS, and more
recently in 86dc9005, we introduced a bunch of logic in the
exapnd_inherited_rtentry() path to do with adding junk columns to the
top-level query targetlist that was not there earlier. So I'd think
that expand_inherited_rtentry()'s job used to be much simpler pre-v12
so that an extension dealing with inheritance child relations could
more easily replicate its functionality, but that may not necessarily
be true anymore. Granted, a lot of that new logic is to account for
foreign table children, which perhaps doesn't matter to most
extensions. But I'd be more careful about the stuff added in
86dc9005, like add_row_identity_var/columns().

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

Interesting proposition...

--
Amit Langote
EDB: http://www.enterprisedb.com