Thread: Feedback on table expansion hook (including patch)
Hi,
I am looking for feedback on the possibility of adding a table expansion hook to PostgreSQL (see attached patch). The motivation for this is to allow extensions to optimize table expansion. In particular, TimescaleDB does its own table expansion in order to apply a number of optimizations, including partition pruning (note that TimescaleDB uses inheritance since PostgreSQL 9.6 rather than declarative partitioning ). There's currently no official hook for table expansion, but TimescaleDB has been using the get_relation_info hook for this purpose. Unfortunately, PostgreSQL 12 broke this for us since it moved expansion to a later stage where we can no longer control it without some pretty bad hacks. Given that PostgreSQL 12 changed the expansion state of a table for the get_relation_info hook, we are thinking about this as a regression and are wondering if this could be considered against the head of PG 12 or maybe even PG 13 (although we realize feature freeze has been reached)?
The attached patch is against PostgreSQL master (commit fb544735) and is about ~10 lines of code. It doesn't change any existing behavior; it only allows getting control of expand_inherited_rtentry, which would make a huge difference for TimescaleDB.
Best regards,
Erik
Engineering team lead
Timescale
Attachment
Re: Feedback on table expansion hook (including patch)
From
"Hans-Jürgen Schönig (PostgreSQL)"
Date:
that sounds really really useful.
i can see a ton of use cases for that.
we also toyed with the idea recently of having pluggable FSM strategies.
that one could be quite useful as well.
regards,
hans
On 07.05.2020, at 10:11, Erik Nordström <erik@timescale.com> wrote:<table-expansion-hook.patch>Hi,I am looking for feedback on the possibility of adding a table expansion hook to PostgreSQL (see attached patch). The motivation for this is to allow extensions to optimize table expansion. In particular, TimescaleDB does its own table expansion in order to apply a number of optimizations, including partition pruning (note that TimescaleDB uses inheritance since PostgreSQL 9.6 rather than declarative partitioning ). There's currently no official hook for table expansion, but TimescaleDB has been using the get_relation_info hook for this purpose. Unfortunately, PostgreSQL 12 broke this for us since it moved expansion to a later stage where we can no longer control it without some pretty bad hacks. Given that PostgreSQL 12 changed the expansion state of a table for the get_relation_info hook, we are thinking about this as a regression and are wondering if this could be considered against the head of PG 12 or maybe even PG 13 (although we realize feature freeze has been reached)?The attached patch is against PostgreSQL master (commit fb544735) and is about ~10 lines of code. It doesn't change any existing behavior; it only allows getting control of expand_inherited_rtentry, which would make a huge difference for TimescaleDB.Best regards,ErikEngineering team leadTimescale
--Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26, A-2700 Wiener Neustadt Web: https://www.cybertec-postgresql.com
On Thu, 7 May 2020 at 05:11, Erik Nordström <erik@timescale.com> wrote:
I am looking for feedback on the possibility of adding a table expansion hook to PostgreSQL (see attached patch). The motivation for this is to allow extensions to optimize table expansion. In particular, TimescaleDB does its own table expansion in order to apply a number of optimizations, including partition pruning (note that TimescaleDB uses inheritance since PostgreSQL 9.6 rather than declarative partitioning ). There's currently no official hook for table expansion, but TimescaleDB has been using the get_relation_info hook for this purpose. Unfortunately, PostgreSQL 12 broke this for us since it moved expansion to a later stage where we can no longer control it without some pretty bad hacks. Given that PostgreSQL 12 changed the expansion state of a table for the get_relation_info hook, we are thinking about this as a regression and are wondering if this could be considered against the head of PG 12 or maybe even PG 13 (although we realize feature freeze has been reached)?
I reviewed your patch and it looks good to me. You mentioned that it would be useful for partitioning using table inheritance but it could also be used for declarative partitioning (at least until someone decides to detach it from inheritance infrastructure).
Unfortunately, you showed up late here. Even though the hook is a straightforward feature, Postgres does not add new features to released versions or after the feature freeze.
The only point that I noticed was that you chose "control over" but similar code uses "control in".
Euler Taveira http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Status update for a commitfest entry. This patch implements useful improvement and the reviewer approved the code. It lacks a test, but looking at previously committedhooks, I think it is not mandatory. So, I move it to RFC. The new status of this patch is: Ready for Committer
Hi Erik, > 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(). This patch probably doesn't need yet another reviewer, but since there is a little controversy about if the hook should replace a procedure or be called after it, I decided to put my two cents in. The proposed approach is very flexible - it allows to modify the arguments, the result, to completely replace the procedure, etc. I don't think that calling a hook after the procedure was called (or before) will be very useful. The patch applies to `master` branch (6d177e28) and passes all the tests on MacOS. -- Best regards, Aleksander Alekseev
Hello, > 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(). > This patch is really useful. We are working on developing hypothetical partitioning as a feature of HypoPG[1][2], but we hit the same problem as TimescaleDB. Therefore we would also be thrilled to have that hook. Hypothetical partitioning allows users to define multiple partitioning schemes on real tables and real data hypothetically, and shows resulting queries' plan/cost with EXPLAIN using hypothetical partitioning schemes. Users can quickly check how their queries would behave if some tables were partitioned, and try different partitioning schemes. HypoPG does table expansion again according to the defined hypothetical partitioning schemes. For this purpose, we used get_relation_info hook, but in PG12, table expansion was moved, so we cannot do that using get_relation_info hook. This is exactly the same problem Erik has. Therefore the proposed hook would allow us to support hypothetical partitioning. [1] https://github.com/HypoPG/hypopg/tree/REL1_STABLE [2] https://github.com/HypoPG/hypopg/tree/master -- Best regards, Yuzuko Hosoya NTT Open Source Software Center
(Sorry about being very late to this thread.) On Sun, Mar 7, 2021 at 3:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Peter Eisentraut <peter.eisentraut@enterprisedb.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
On Wed, May 12, 2021 at 10:19:17PM +0900, Amit Langote wrote: > (Sorry about being very late to this thread.) > > > 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... > Since there is no clear definition here, we seems to be expecting an example of how the hook will be used and there have been no activity since may. I suggest we move this to Returned with feedback. Which I'll do in a couple hours. -- Jaime Casanova Director de Servicios Profesionales SystemGuards - Consultores de PostgreSQL