aboutsummaryrefslogtreecommitdiff
path: root/src/backend/commands/trigger.c
Commit message (Collapse)AuthorAge
* Add temporal PRIMARY KEY and UNIQUE constraintsPeter Eisentraut2024-01-24
| | | | | | | | | | | | Add WITHOUT OVERLAPS clause to PRIMARY KEY and UNIQUE constraints. These are backed by GiST indexes instead of B-tree indexes, since they are essentially exclusion constraints with = for the scalar parts of the key and && for the temporal part. Author: Paul A. Jungwirth <pj@illuminatedcomputing.com> Reviewed-by: Peter Eisentraut <peter@eisentraut.org> Reviewed-by: jian he <jian.universality@gmail.com> Discussion: https://www.postgresql.org/message-id/flat/CA+renyUApHgSZF9-nd-a0+OPGharLQLO=mDHcY4_qQ0+noCUVg@mail.gmail.com
* Prevent access to an unpinned buffer in BEFORE ROW UPDATE triggers.Tom Lane2024-01-14
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When ExecBRUpdateTriggers switches to a new target tuple as a result of the EvalPlanQual logic, it must form a new proposed update tuple. Since commit 86dc90056, that tuple (the result of ExecGetUpdateNewTuple) has been a virtual tuple that might contain pointers to by-ref fields of the new target tuple (in "oldslot"). However, immediately after that we materialize oldslot, causing it to drop its buffer pin, whereupon the by-ref pointers are unsafe to use. This is a live bug only when the new target tuple is in a different page than the original target tuple, since we do still hold a pin on the original one. (Before 86dc90056, there was no bug because the EPQ plantree would hold a pin on the new target tuple; but now that's not assured.) To fix, forcibly materialize the new tuple before we materialize oldslot. This costs nothing since we would have done that shortly anyway. The real-world impact of this is probably minimal. A visible failure could occur if the new target tuple's buffer were recycled for some other page in the short interval before we materialize newslot within the trigger-calling loop; but that's quite unlikely given that we'd just touched that page. There's a larger hazard that some other process could prune and repack that page within the window. We have lock on the new target tuple, but that wouldn't prevent it being moved on the page. Alexander Lakhin and Tom Lane, per bug #17798 from Alexander Lakhin. Back-patch to v14 where 86dc90056 came in. Discussion: https://postgr.es/m/17798-0907404928dcf0dd@postgresql.org
* Update copyright for 2024Bruce Momjian2024-01-03
| | | | | | | | Reported-by: Michael Paquier Discussion: https://postgr.es/m/ZZKTDPxBBMt3C0J9@paquier.xyz Backpatch-through: 12
* Fix code indentation violationsTomas Vondra2023-07-03
| | | | | | | Commits ce5aaea8cd, 2b8b2852bb and 28d03feac3 violated the expected code indentation rules, upsetting the new buildfarm member "koel." Discussion: https://postgr.es/m/ZKIU4mhWpgJOM0W0%40paquier.xyz
* Fix oversight in handling of modifiedCols since f24523672dTomas Vondra2023-07-02
| | | | | | | | | | | | | | | | | | | Commit f24523672d fixed a memory leak by moving the modifiedCols bitmap into the per-row memory context. In the case of AFTER UPDATE triggers, the bitmap is however referenced from an event kept until the end of the query, resulting in a use-after-free bug. Fixed by copying the bitmap into the AfterTriggerEvents memory context, which is the one where we keep the trigger events. There's only one place that needs to do the copy, but the memory context may not exist yet. Doing that in a separate function seems more readable. Report by Alexander Pyhalov, fix by me. Backpatch to 13, where the bitmap was added to the event by commit 71d60e2aa0. Reported-by: Alexander Pyhalov Backpatch-through: 13 Discussion: https://postgr.es/m/acddb17c89b0d6cb940eaeda18c08bbe@postgrespro.ru
* Fix another issue with ENABLE/DISABLE TRIGGER on partitioned tables.Tom Lane2023-04-05
| | | | | | | | | | | | | | | | | | | | | | | | | | In v13 and v14, the ENABLE/DISABLE TRIGGER USER variant malfunctioned on cloned triggers, failing to find the clones because it thought they were system triggers. Other variants of ENABLE/DISABLE TRIGGER would improperly apply a superuserness check. Fix by adjusting the is-it- a-system-trigger check to match reality in those branches. (As far as I can find, this is the only place that got it wrong.) There's no such bug in v15/HEAD, because we revised the catalog representation of system triggers to be what this code was expecting. However, add the test case to these branches anyway, because this area is visibly pretty fragile. Also remove an obsoleted comment. The recent v15/HEAD commit 6949b921d fixed a nearby bug. I now see that my commit message for that was inaccurate: the behavior of recursing to clone triggers is older than v15, but it didn't apply to the case in v13/v14 because in those branches parent partitioned tables have no pg_trigger entries for foreign-key triggers. But add the test case from that commit to v13/v14, just to show what is happening there. Per bug #17886 from DzmitryH. Discussion: https://postgr.es/m/17886-5406d5d828aa4aa3@postgresql.org
* Fix concurrent update issues with MERGE.Dean Rasheed2023-03-13
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | If MERGE attempts an UPDATE or DELETE on a table with BEFORE ROW triggers, or a cross-partition UPDATE (with or without triggers), and a concurrent UPDATE or DELETE happens, the merge code would fail. In some cases this would lead to a crash, while in others it would cause the wrong merge action to be executed, or no action at all. The immediate cause of the crash was the trigger code calling ExecGetUpdateNewTuple() as part of the EPQ mechanism, which fails because during a merge ri_projectNew is NULL, since merge has its own per-action projection information, which ExecGetUpdateNewTuple() knows nothing about. Fix by arranging for the trigger code to exit early, returning the TM_Result and TM_FailureData information, if a concurrent modification is detected, allowing the merge code to do the necessary EPQ handling in its own way. Similarly, prevent the cross-partition update code from doing any EPQ processing for a merge, allowing the merge code to work out what it needs to do. This leads to a number of simplifications in nodeModifyTable.c. Most notably, the ModifyTableContext->GetUpdateNewTuple() callback is no longer needed, and mergeGetUpdateNewTuple() can be deleted, since there is no longer any requirement for get-update-new-tuple during a merge. Similarly, ModifyTableContext->cpUpdateRetrySlot is no longer needed. Thus ExecGetUpdateNewTuple() and the retry_slot handling of ExecCrossPartitionUpdate() can be restored to how they were in v14, before the merge code was added, and ExecMergeMatched() no longer needs any special-case handling for cross-partition updates. While at it, tidy up ExecUpdateEpilogue() a bit, making it handle recheckIndexes locally, rather than passing it in as a parameter, ensuring that it is freed properly. This dates back to when it was split off from ExecUpdate() to support merge. Per bug #17809 from Alexander Lakhin, and follow-up investigation of bug #17792, also from Alexander Lakhin. Back-patch to v15, where MERGE was introduced, taking care to preserve backwards-compatibility of the trigger API in v15 for any extensions that might use it. Discussion: https://postgr.es/m/17809-9e6650bef133f0fe%40postgresql.org https://postgr.es/m/17792-0f89452029662c36%40postgresql.org
* Avoid failure when altering state of partitioned foreign-key triggers.Tom Lane2023-03-04
| | | | | | | | | | | | | | | | | | | | | | Beginning in v15, if you apply ALTER TABLE ENABLE/DISABLE TRIGGER to a partitioned table, it also affects the partitions' cloned versions of the affected trigger(s). The initial implementation of this located the clones by name, but that fails on foreign-key triggers which have names incorporating their own OIDs. We can fix that, and also make the behavior more bulletproof in the face of user-initiated trigger renames, by identifying the cloned triggers by tgparentid. Following the lead of earlier commits in this area, I took care not to break ABI in the v15 branch, even though I rather doubt there are any external callers of EnableDisableTrigger. While here, update the documentation, which was not touched when the semantics were changed. Per bug #17817 from Alan Hodgson. Back-patch to v15; older versions do not have this behavior. Discussion: https://postgr.es/m/17817-31dfb7c2100d9f3d@postgresql.org
* Update copyright for 2023Bruce Momjian2023-01-02
| | | | Backpatch-through: 11
* Refactor aclcheck functionsPeter Eisentraut2022-11-13
| | | | | | | | | | | | | | | | | | Instead of dozens of mostly-duplicate pg_foo_aclcheck() functions, write one common function object_aclcheck() that can handle almost all of them. We already have all the information we need, such as which system catalog corresponds to which catalog table and which column is the ACL column. There are a few pg_foo_aclcheck() that don't work via the generic function and have special APIs, so those stay as is. I also changed most pg_foo_aclmask() functions to static functions, since they are not used outside of aclchk.c. Reviewed-by: Corey Huinker <corey.huinker@gmail.com> Reviewed-by: Antonin Houska <ah@cybertec.at> Discussion: https://www.postgresql.org/message-id/flat/95c30f96-4060-2f48-98b5-a4392d3b6066@enterprisedb.com
* Refactor ownercheck functionsPeter Eisentraut2022-11-13
| | | | | | | | | | | | Instead of dozens of mostly-duplicate pg_foo_ownercheck() functions, write one common function object_ownercheck() that can handle almost all of them. We already have all the information we need, such as which system catalog corresponds to which catalog table and which column is the owner column. Reviewed-by: Corey Huinker <corey.huinker@gmail.com> Reviewed-by: Antonin Houska <ah@cybertec.at> Discussion: https://www.postgresql.org/message-id/flat/95c30f96-4060-2f48-98b5-a4392d3b6066@enterprisedb.com
* Correct error message for row-level triggers with transition tables on ↵Etsuro Fujita2022-11-04
| | | | | | | | | | | | | | | partitioned tables. "Triggers on partitioned tables cannot have transition tables." is incorrect as we allow statement-level triggers on partitioned tables to have transition tables. This has been wrong since commit 86f575948; back-patch to v11 where that commit came in. Reviewed by Tom Lane. Discussion: https://postgr.es/m/CAPmGK17gk4vXLzz2iG%2BG4LWRWCoVyam70nZ3OuGm1hMJwDrhcg%40mail.gmail.com
* Rename shadowed local variablesDavid Rowley2022-10-05
| | | | | | | | | | | | In a similar effort to f01592f91, here we mostly rename shadowed local variables to remove the warnings produced when compiling with -Wshadow=compatible-local. This fixes 63 warnings and leaves just 5. Author: Justin Pryzby, David Rowley Reviewed-by: Justin Pryzby Discussion https://postgr.es/m/20220817145434.GC26426%40telsasoft.com
* Fix tupdesc lifespan bug with AfterTriggersTableData.storeslot.Tom Lane2022-09-25
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Commit 25936fd46 adjusted things so that the "storeslot" we use for remapping trigger tuples would have adequate lifespan, but it neglected to consider the lifespan of the tuple descriptor that the slot depends on. It turns out that in at least some cases, the tupdesc we are passing is a refcounted tupdesc, and the refcount for the slot's reference can get assigned to a resource owner having different lifespan than the slot does. That leads to an error like "tupdesc reference 0x7fdef236a1b8 is not owned by resource owner SubTransaction". Worse, because of a second oversight in the same commit, we'd try to free the same tupdesc refcount again while cleaning up after that error, leading to recursive errors and an "ERRORDATA_STACK_SIZE exceeded" PANIC. To fix the initial problem, let's just make a non-refcounted copy of the tupdesc we're supposed to use. That seems likely to guard against additional problems, since there's no strong reason for this code to assume that what it's given is a refcounted tupdesc; in which case there's an independent hazard of the tupdesc having shorter lifespan than the slot does. (I didn't bother trying to free said copy, since it should go away anyway when the (sub) transaction context is cleaned up.) The other issue can be fixed by making the code added to AfterTriggerFreeQuery work like the rest of that function, ie be sure that it doesn't try to free the same slot twice in the event of recursive error cleanup. While here, also clean up minor stylistic issues in the test case added by 25936fd46: don't use "create or replace function", as any name collision within the tests is likely to have ill effects that that won't mask; and don't use function names as generic as trigger_function1, especially if you're not going to drop them at the end of the test stanza. Per bug #17607 from Thomas Mc Kay. Back-patch to v12, as the previous fix was. Discussion: https://postgr.es/m/17607-bd8ccc81226f7f80@postgresql.org
* Message style improvementsPeter Eisentraut2022-09-24
|
* Harmonize more parameter names in bulk.Peter Geoghegan2022-09-20
| | | | | | | | | | | | | | | | Make sure that function declarations use names that exactly match the corresponding names from function definitions in optimizer, parser, utility, libpq, and "commands" code, as well as in remaining library code. Do the same for all code related to frontend programs (with the exception of pg_dump/pg_dumpall related code). Like other recent commits that cleaned up function parameter names, this commit was written with help from clang-tidy. Later commits will handle ecpg and pg_dump/pg_dumpall. Author: Peter Geoghegan <pg@bowt.ie> Reviewed-By: David Rowley <dgrowleyml@gmail.com> Discussion: https://postgr.es/m/CAH2-WznJt9CMM9KJTMjJh_zbL5hD9oX44qdJ4aqZtjFi-zA3Tg@mail.gmail.com
* Split up guc.c for better build speed and ease of maintenance.Tom Lane2022-09-13
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | guc.c has grown to be one of our largest .c files, making it a bottleneck for compilation. It's also acquired a bunch of knowledge that'd be better kept elsewhere, because of our not very good habit of putting variable-specific check hooks here. Hence, split it up along these lines: * guc.c itself retains just the core GUC housekeeping mechanisms. * New file guc_funcs.c contains the SET/SHOW interfaces and some SQL-accessible functions for GUC manipulation. * New file guc_tables.c contains the data arrays that define the built-in GUC variables, along with some already-exported constant tables. * GUC check/assign/show hook functions are moved to the variable's home module, whenever that's clearly identifiable. A few hard- to-classify hooks ended up in commands/variable.c, which was already a home for miscellaneous GUC hook functions. To avoid cluttering a lot more header files with #include "guc.h", I also invented a new header file utils/guc_hooks.h and put all the GUC hook functions' declarations there, regardless of their originating module. That allowed removal of #include "guc.h" from some existing headers. The fallout from that (hopefully all caught here) demonstrates clearly why such inclusions are best minimized: there are a lot of files that, for example, were getting array.h at two or more levels of remove, despite not having any connection at all to GUCs in themselves. There is some very minor code beautification here, such as renaming a couple of inconsistently-named hook functions and improving some comments. But mostly this just moves code from point A to point B and deals with the ensuing needs for #include adjustments and exporting a few functions that previously weren't exported. Patch by me, per a suggestion from Andres Freund; thanks also to Michael Paquier for the idea to invent guc_funcs.c. Discussion: https://postgr.es/m/587607.1662836699@sss.pgh.pa.us
* Fix typo in 16d69ec29David Rowley2022-09-06
| | | | | | | As noted by Justin Pryzby, just I forgot to commit locally before creating a patch file. Discussion: https://postgr.es/m/20220901053146.GI31833@telsasoft.com
* Remove buggy and dead code from CreateTriggerFiringOnDavid Rowley2022-09-06
| | | | | | | | | | | | | | | | | | | | | | | | Here we remove some dead code from CreateTriggerFiringOn() which was attempting to find the relevant child partition index corresponding to the given indexOid. As it turned out, thanks to -Wshadow=compatible-local, this code was buggy as the code which was finding the child indexes assigned those to a shadowed variable that directly went out of scope. The code which thought it was looking at the List of child indexes was always referencing an empty List. On further investigation, this code is dead. We never call CreateTriggerFiringOn() passing a valid indexOid in a way that the function would actually ever execute the code in question. So, for lack of a way to test if a fix actually works, let's just remove the dead code instead. As a reminder, if there is ever a need to resurrect this code, an Assert() has been added to remind future feature developers that they might need to write some code to find the corresponding child index. Reported-by: Justin Pryzby Reviewed-by: Justin Pryzby Discussion: https://postgr.es/m/20220819211824.GX26426@telsasoft.com
* Further -Wshadow=compatible-local warning fixesDavid Rowley2022-08-24
| | | | | | | | | | | | | These should have been included in 421892a19 as these shadowed variable warnings can also be fixed by adjusting the scope of the shadowed variable to put the declaration for it in an inner scope. This is part of the same effort as f01592f91. By my count, this takes the warning count from 114 down to 106. Author: David Rowley and Justin Pryzby Discussion: https://postgr.es/m/CAApHDvrwLGBP%2BYw9vriayyf%3DXR4uPWP5jr6cQhP9au_kaDUhbA%40mail.gmail.com
* Fix ENABLE/DISABLE TRIGGER to handle recursion correctlyAlvaro Herrera2022-08-05
| | | | | | | | | | | | | | | | | | | | | | | | | | | | Using ATSimpleRecursion() in ATPrepCmd() to do so as bbb927b4db9b did is not correct, because ATPrepCmd() can't distinguish between triggers that may be cloned and those that may not, so would wrongly try to recurse for the latter category of triggers. So this commit restores the code in EnableDisableTrigger() that 86f575948c77 had added to do the recursion, which would do it only for triggers that may be cloned, that is, row-level triggers. This also changes tablecmds.c such that ATExecCmd() is able to pass the value of ONLY flag down to EnableDisableTrigger() using its new 'recurse' parameter. This also fixes what seems like an oversight of 86f575948c77 that the recursion to partition triggers would only occur if EnableDisableTrigger() had actually changed the trigger. It is more apt to recurse to inspect partition triggers even if the parent's trigger didn't need to be changed: only then can we be certain that all descendants share the same state afterwards. Backpatch all the way back to 11, like bbb927b4db9b. Care is taken not to break ABI compatibility (and that no catversion bump is needed.) Co-authored-by: Amit Langote <amitlangote09@gmail.com> Reviewed-by: Dmitry Koval <d.koval@postgrespro.ru> Discussion: https://postgr.es/m/CA+HiwqG-cZT3XzGAnEgZQLoQbyfJApVwOTQaCaas1mhpf+4V5A@mail.gmail.com
* Support TRUNCATE triggers on foreign tables.Fujii Masao2022-07-12
| | | | | | | | | | Now some foreign data wrappers support TRUNCATE command. So it's useful to support TRUNCATE triggers on foreign tables for audit logging or for preventing undesired truncation. Author: Yugo Nagata Reviewed-by: Fujii Masao, Ian Lawrence Barwick Discussion: https://postgr.es/m/20220630193848.5b02e0d6076b86617a915682@sraoss.co.jp
* Add support for MERGE SQL commandAlvaro Herrera2022-03-28
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | MERGE performs actions that modify rows in the target table using a source table or query. MERGE provides a single SQL statement that can conditionally INSERT/UPDATE/DELETE rows -- a task that would otherwise require multiple PL statements. For example, MERGE INTO target AS t USING source AS s ON t.tid = s.sid WHEN MATCHED AND t.balance > s.delta THEN UPDATE SET balance = t.balance - s.delta WHEN MATCHED THEN DELETE WHEN NOT MATCHED AND s.delta > 0 THEN INSERT VALUES (s.sid, s.delta) WHEN NOT MATCHED THEN DO NOTHING; MERGE works with regular tables, partitioned tables and inheritance hierarchies, including column and row security enforcement, as well as support for row and statement triggers and transition tables therein. MERGE is optimized for OLTP and is parameterizable, though also useful for large scale ETL/ELT. MERGE is not intended to be used in preference to existing single SQL commands for INSERT, UPDATE or DELETE since there is some overhead. MERGE can be used from PL/pgSQL. MERGE does not support targetting updatable views or foreign tables, and RETURNING clauses are not allowed either. These limitations are likely fixable with sufficient effort. Rewrite rules are also not supported, but it's not clear that we'd want to support them. Author: Pavan Deolasee <pavan.deolasee@gmail.com> Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Author: Amit Langote <amitlangote09@gmail.com> Author: Simon Riggs <simon.riggs@enterprisedb.com> Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com> Reviewed-by: Andres Freund <andres@anarazel.de> (earlier versions) Reviewed-by: Peter Geoghegan <pg@bowt.ie> (earlier versions) Reviewed-by: Robert Haas <robertmhaas@gmail.com> (earlier versions) Reviewed-by: Japin Li <japinli@hotmail.com> Reviewed-by: Justin Pryzby <pryzby@telsasoft.com> Reviewed-by: Tomas Vondra <tomas.vondra@enterprisedb.com> Reviewed-by: Zhihong Yu <zyu@yugabyte.com> Discussion: https://postgr.es/m/CANP8+jKitBSrB7oTgT9CY2i1ObfOt36z0XMraQc+Xrz8QB0nXA@mail.gmail.com Discussion: https://postgr.es/m/CAH2-WzkJdBuxj9PO=2QaO9-3h3xGbQPZ34kJH=HukRekwM-GZg@mail.gmail.com Discussion: https://postgr.es/m/20201231134736.GA25392@alvherre.pgsql
* Enforce foreign key correctly during cross-partition updatesAlvaro Herrera2022-03-20
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When an update on a partitioned table referenced in foreign key constraints causes a row to move from one partition to another, the fact that the move is implemented as a delete followed by an insert on the target partition causes the foreign key triggers to have surprising behavior. For example, a given foreign key's delete trigger which implements the ON DELETE CASCADE clause of that key will delete any referencing rows when triggered for that internal DELETE, although it should not, because the referenced row is simply being moved from one partition of the referenced root partitioned table into another, not being deleted from it. This commit teaches trigger.c to skip queuing such delete trigger events on the leaf partitions in favor of an UPDATE event fired on the root target relation. Doing so is sensible because both the old and the new tuple "logically" belong to the root relation. The after trigger event queuing interface now allows passing the source and the target partitions of a particular cross-partition update when registering the update event for the root partitioned table. Along with the two ctids of the old and the new tuple, the after trigger event now also stores the OIDs of those partitions. The tuples fetched from the source and the target partitions are converted into the root table format, if necessary, before they are passed to the trigger function. The implementation currently has a limitation that only the foreign keys pointing into the query's target relation are considered, not those of its sub-partitioned partitions. That seems like a reasonable limitation, because it sounds rare to have distinct foreign keys pointing to sub-partitioned partitions instead of to the root table. This misbehavior stems from commit f56f8f8da6af (which added support for foreign keys to reference partitioned tables) not paying sufficient attention to commit 2f178441044b (which had introduced cross-partition updates a year earlier). Even though the former commit goes back to Postgres 12, we're not backpatching this fix at this time for fear of destabilizing things too much, and because there are a few ABI breaks in it that we'd have to work around in older branches. It also depends on commit f4566345cf40, which had its own share of backpatchability issues as well. Author: Amit Langote <amitlangote09@gmail.com> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com> Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org> Reported-by: Eduard Català <eduard.catala@gmail.com> Discussion: https://postgr.es/m/CA+HiwqFvkBCmfwkQX_yBqv2Wz8ugUGiBDxum8=WvVbfU1TXaNg@mail.gmail.com Discussion: https://postgr.es/m/CAL54xNZsLwEM1XCk5yW9EqaRzsZYHuWsHQkA2L5MOSKXAwviCQ@mail.gmail.com
* Add API of sorts for transition table handling in trigger.cAlvaro Herrera2022-03-11
| | | | | | | | | | Preparatory patch for further additions in this area, particularly to allow MERGE to have separate transition tables for each action. Author: Pavan Deolasee <pavan.deolasee@gmail.com> Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org> Discussion: https://postgr.es/m/CABOikdNj+8HEJ5D8tu56mrPkjHVRrBb2_cdKWwpiYNcjXgDw8g@mail.gmail.com Discussion: https://postgr.es/m/20201231134736.GA25392@alvherre.pgsql
* Update copyright for 2022Bruce Momjian2022-01-07
| | | | Backpatch-through: 10
* Create foreign key triggers in partitioned tables tooAlvaro Herrera2022-01-05
| | | | | | | | | | | | | | | | | | | | | | | | | | While user-defined triggers defined on a partitioned table have a catalog definition for both it and its partitions, internal triggers used by foreign keys defined on partitioned tables only have a catalog definition for its partitions. This commit fixes that so that partitioned tables get the foreign key triggers too, just like user-defined triggers. Moreover, like user-defined triggers, partitions' internal triggers will now also have their tgparentid set appropriately. This is to allow subsequent commit(s) to make the foreign key related events to be fired in some cases using the parent table triggers instead of those of partitions'. This also changes what tgisinternal means in some cases. Currently, it means either that the trigger is an internal implementation object of a foreign key constraint, or a "child" trigger on a partition cloned from the trigger on the parent. This commit changes it to only mean the former to avoid confusion. As for the latter, it can be told by tgparentid being nonzero, which is now true both for user- defined and foreign key's internal triggers. Author: Amit Langote <amitlangote09@gmail.com> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com> Reviewed-by: Arne Roland <A.Roland@index.de> Discussion: https://postgr.es/m/CA+HiwqG7LQSK+n8Bki8tWv7piHD=PnZro2y6ysU2-28JS6cfgQ@mail.gmail.com
* Allow specifying column list for foreign key ON DELETE SET actionsPeter Eisentraut2021-12-08
| | | | | | | | | | | | | | | | | | | | | Extend the foreign key ON DELETE actions SET NULL and SET DEFAULT by allowing the specification of a column list, like CREATE TABLE posts ( ... FOREIGN KEY (tenant_id, author_id) REFERENCES users ON DELETE SET NULL (author_id) ); If a column list is specified, only those columns are set to null/default, instead of all the columns in the foreign-key constraint. This is useful for multitenant or sharded schemas, where the tenant or shard ID is included in the primary key of all tables but shouldn't be set to null. Author: Paul Martinez <paulmtz@google.com> Discussion: https://www.postgresql.org/message-id/flat/CACqFVBZQyMYJV=njbSMxf+rbDHpx=W=B7AEaMKn8dWn9OZJY7w@mail.gmail.com
* Remove newly added useless assertion checkAlvaro Herrera2021-07-26
| | | | | | | | | | | Coverity complained that my commit 80ba4bb38353 added a dubious coding for a consistency check that there isn't more than one row for a certain tgrelid/tgparentid combination. But we don't check for that explicitly anywhere else, and if we were to do it, it should be a full shouldn't-happen elog not just an assert. It doesn't seem that this is very important anyway, so remove it. Discussion: https://postgr.es/m/1337562.1627224583@sss.pgh.pa.us
* Make ALTER TRIGGER RENAME consistent for partitioned tablesAlvaro Herrera2021-07-22
| | | | | | | | | | | | | | | | Renaming triggers on partitioned tables had two problems: first, it did not recurse to renaming the triggers on the partitions; and second, it failed to prohibit renaming clone triggers. Having triggers with different names in partitions is pointless, and furthermore pg_dump would not preserve names for partitions anyway. Not backpatched -- making the ALTER TRIGGER throw an error in stable versions might cause problems for existing scripts. Co-authored-by: Arne Roland <A.Roland@index.de> Co-authored-by: Álvaro Herrera <alvherre@alvh.no-ip.org> Reviewed-by: Zhihong Yu <zyu@yugabyte.com> Discussion: https://postgr.es/m/d0fd7040c2fb4de1a111b9d9ccc456b8@index.de
* Preserve firing-on state when cloning row triggers to partitionsAlvaro Herrera2021-07-16
| | | | | | | | | | | | | | | When triggers are cloned from partitioned tables to their partitions, the 'tgenabled' flag (origin/replica/always/disable) was not propagated. Make it so that the flag on the trigger on partition is initially set to the same value as on the partitioned table. Add a test case to verify the behavior. Backpatch to 11, where this appeared in commit 86f575948c77. Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reported-by: Justin Pryzby <pryzby@telsasoft.com> Discussion: https://postgr.es/m/20200930223450.GA14848@telsasoft.com
* Improve error messages about mismatching relkindPeter Eisentraut2021-07-08
| | | | | | | | | | | | | | | | | | | | | | | | | Most error messages about a relkind that was not supported or appropriate for the command was of the pattern "relation \"%s\" is not a table, foreign table, or materialized view" This style can become verbose and tedious to maintain. Moreover, it's not very helpful: If I'm trying to create a comment on a TOAST table, which is not supported, then the information that I could have created a comment on a materialized view is pointless. Instead, write the primary error message shorter and saying more directly that what was attempted is not possible. Then, in the detail message, explain that the operation is not supported for the relkind the object was. To simplify that, add a new function errdetail_relkind_not_supported() that does this. In passing, make use of RELKIND_HAS_STORAGE() where appropriate, instead of listing out the relkinds individually. Reviewed-by: Michael Paquier <michael@paquier.xyz> Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org> Discussion: https://www.postgresql.org/message-id/flat/dc35a398-37d0-75ce-07ea-1dd71d98f8ec@2ndquadrant.com
* Fix access to no-longer-open relcache entry in logical-rep worker.Tom Lane2021-05-22
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | If we redirected a replicated tuple operation into a partition child table, and then tried to fire AFTER triggers for that event, the relation cache entry for the child table was already closed. This has no visible ill effects as long as the entry is still there and still valid, but an unluckily-timed cache flush could result in a crash or other misbehavior. To fix, postpone the ExecCleanupTupleRouting call (which is what closes the child table) until after we've fired triggers. This requires a bit of refactoring so that the cleanup function can have access to the necessary state. In HEAD, I took the opportunity to simplify some of worker.c's function APIs based on use of the new ApplyExecutionData struct. However, it doesn't seem safe/practical to back-patch that aspect, at least not without a lot of analysis of possible interactions with a04daa97a. In passing, add an Assert to afterTriggerInvokeEvents to catch such cases. This seems worthwhile because we've grown a number of fairly unstructured ways of calling AfterTriggerEndQuery. Back-patch to v13, where worker.c grew the ability to deal with partitioned target tables. Discussion: https://postgr.es/m/3382681.1621381328@sss.pgh.pa.us
* Initial pgindent and pgperltidy run for v14.Tom Lane2021-05-12
| | | | | | | | Also "make reformat-dat-files". The only change worthy of note is that pgindent messed up the formatting of launcher.c's struct LogicalRepWorkerId, which led me to notice that that struct wasn't used at all anymore, so I just took it out.
* Allow a partdesc-omitting-partitions to be cachedAlvaro Herrera2021-04-28
| | | | | | | | | | | | | | | | | | | | | Makes partition descriptor acquisition faster during the transient period in which a partition is in the process of being detached. This also adds the restriction that only one partition can be in pending-detach state for a partitioned table. While at it, return find_inheritance_children() API to what it was before 71f4c8c6f74b, and create a separate find_inheritance_children_extended() that returns detailed info about detached partitions. (This incidentally fixes a bug in 8aba9322511 whereby a memory context holding a transient partdesc is reparented to a NULL PortalContext, leading to permanent leak of that memory. The fix is to no longer rely on reparenting contexts to PortalContext. Reported by Amit Langote.) Per gripe from Amit Langote Discussion: https://postgr.es/m/CA+HiwqFgpP1LxJZOBYGt9rpvTjXXkg5qG2+Xch2Z1Q7KrqZR1A@mail.gmail.com
* Fix relcache inconsistency hazard in partition detachAlvaro Herrera2021-04-22
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | During queries coming from ri_triggers.c, we need to omit partitions that are marked pending detach -- otherwise, the RI query is tricked into allowing a row into the referencing table whose corresponding row is in the detached partition. Which is bogus: once the detach operation completes, the row becomes an orphan. However, the code was not doing that in repeatable-read transactions, because relcache kept a copy of the partition descriptor that included the partition, and used it in the RI query. This commit changes the partdesc cache code to only keep descriptors that aren't dependent on a snapshot (namely: those where no detached partition exist, and those where detached partitions are included). When a partdesc-without- detached-partitions is requested, we create one afresh each time; also, those partdescs are stored in PortalContext instead of CacheMemoryContext. find_inheritance_children gets a new output *detached_exist boolean, which indicates whether any partition marked pending-detach is found. Its "include_detached" input flag is changed to "omit_detached", because that name captures desired the semantics more naturally. CreatePartitionDirectory() and RelationGetPartitionDesc() arguments are identically renamed. This was noticed because a buildfarm member that runs with relcache clobbering, which would not keep the improperly cached partdesc, broke one test, which led us to realize that the expected output of that test was bogus. This commit also corrects that expected output. Author: Amit Langote <amitlangote09@gmail.com> Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Discussion: https://postgr.es/m/3269784.1617215412@sss.pgh.pa.us
* Postpone some stuff out of ExecInitModifyTable.Tom Lane2021-04-06
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Arrange to do some things on-demand, rather than immediately during executor startup, because there's a fair chance of never having to do them at all: * Don't open result relations' indexes until needed. * Don't initialize partition tuple routing, nor the child-to-root tuple conversion map, until needed. This wins in UPDATEs on partitioned tables when only some of the partitions will actually receive updates; with larger partition counts the savings is quite noticeable. Also, we can remove some sketchy heuristics in ExecInitModifyTable about whether to set up tuple routing. Also, remove execPartition.c's private hash table tracking which partitions were already opened by the ModifyTable node. Instead use the hash added to ModifyTable itself by commit 86dc90056. To allow lazy computation of the conversion maps, we now set ri_RootResultRelInfo in all child ResultRelInfos. We formerly set it only in some, not terribly well-defined, cases. This has user-visible side effects in that now more error messages refer to the root relation instead of some partition (and provide error data in the root's column order, too). It looks to me like this is a strict improvement in consistency, so I don't have a problem with the output changes visible in this commit. Extracted from a larger patch, which seemed to me to be too messy to push in one commit. Amit Langote, reviewed at different times by Heikki Linnakangas and myself Discussion: https://postgr.es/m/CA+HiwqG7ZruBmmih3wPsBZ4s0H2EhywrnXEduckY5Hr3fWzPWA@mail.gmail.com
* Rework planning and execution of UPDATE and DELETE.Tom Lane2021-03-31
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This patch makes two closely related sets of changes: 1. For UPDATE, the subplan of the ModifyTable node now only delivers the new values of the changed columns (i.e., the expressions computed in the query's SET clause) plus row identity information such as CTID. ModifyTable must re-fetch the original tuple to merge in the old values of any unchanged columns. The core advantage of this is that the changed columns are uniform across all tables of an inherited or partitioned target relation, whereas the other columns might not be. A secondary advantage, when the UPDATE involves joins, is that less data needs to pass through the plan tree. The disadvantage of course is an extra fetch of each tuple to be updated. However, that seems to be very nearly free in context; even worst-case tests don't show it to add more than a couple percent to the total query cost. At some point it might be interesting to combine the re-fetch with the tuple access that ModifyTable must do anyway to mark the old tuple dead; but that would require a good deal of refactoring and it seems it wouldn't buy all that much, so this patch doesn't attempt it. 2. For inherited UPDATE/DELETE, instead of generating a separate subplan for each target relation, we now generate a single subplan that is just exactly like a SELECT's plan, then stick ModifyTable on top of that. To let ModifyTable know which target relation a given incoming row refers to, a tableoid junk column is added to the row identity information. This gets rid of the horrid hack that was inheritance_planner(), eliminating O(N^2) planning cost and memory consumption in cases where there were many unprunable target relations. Point 2 of course requires point 1, so that there is a uniform definition of the non-junk columns to be returned by the subplan. We can't insist on uniform definition of the row identity junk columns however, if we want to keep the ability to have both plain and foreign tables in a partitioning hierarchy. Since it wouldn't scale very far to have every child table have its own row identity column, this patch includes provisions to merge similar row identity columns into one column of the subplan result. In particular, we can merge the whole-row Vars typically used as row identity by FDWs into one column by pretending they are type RECORD. (It's still okay for the actual composite Datums to be labeled with the table's rowtype OID, though.) There is more that can be done to file down residual inefficiencies in this patch, but it seems to be committable now. FDW authors should note several API changes: * The argument list for AddForeignUpdateTargets() has changed, and so has the method it must use for adding junk columns to the query. Call add_row_identity_var() instead of manipulating the parse tree directly. You might want to reconsider exactly what you're adding, too. * PlanDirectModify() must now work a little harder to find the ForeignScan plan node; if the foreign table is part of a partitioning hierarchy then the ForeignScan might not be the direct child of ModifyTable. See postgres_fdw for sample code. * To check whether a relation is a target relation, it's no longer sufficient to compare its relid to root->parse->resultRelation. Instead, check it against all_result_relids or leaf_result_relids, as appropriate. Amit Langote and Tom Lane Discussion: https://postgr.es/m/CA+HiwqHpHdqdDn48yCEhynnniahH78rwcrv1rEX65-fsZGBOLQ@mail.gmail.com
* Remove small inefficiency in ExecARDeleteTriggers/ExecARUpdateTriggers.Tom Lane2021-03-30
| | | | | | | | | | | | | | | | Whilst poking at nodeModifyTable.c, I chanced to notice that while its calls to ExecBR*Triggers and ExecIR*Triggers are protected by tests to see if there are any relevant triggers to fire, its calls to ExecAR*Triggers are not; the latter functions do the equivalent tests themselves. This seems possibly reasonable given the more complex conditions involved, but what's less reasonable is that the ExecAR* functions aren't careful to do no work when there is no work to be done. ExecARInsertTriggers gets this right, but the other two will both force creation of a slot that the query may have no use for. ExecARUpdateTriggers additionally performed a usually-useless ExecClearTuple() on that slot. This is probably all pretty microscopic in real workloads, but a cycle shaved is a cycle earned.
* ALTER TABLE ... DETACH PARTITION ... CONCURRENTLYAlvaro Herrera2021-03-25
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Allow a partition be detached from its partitioned table without blocking concurrent queries, by running in two transactions and only requiring ShareUpdateExclusive in the partitioned table. Because it runs in two transactions, it cannot be used in a transaction block. This is the main reason to use dedicated syntax: so that users can choose to use the original mode if they need it. But also, it doesn't work when a default partition exists (because an exclusive lock would still need to be obtained on it, in order to change its partition constraint.) In case the second transaction is cancelled or a crash occurs, there's ALTER TABLE .. DETACH PARTITION .. FINALIZE, which executes the final steps. The main trick to make this work is the addition of column pg_inherits.inhdetachpending, initially false; can only be set true in the first part of this command. Once that is committed, concurrent transactions that use a PartitionDirectory will include or ignore partitions so marked: in optimizer they are ignored if the row is marked committed for the snapshot; in executor they are always included. As a result, and because of the way PartitionDirectory caches partition descriptors, queries that were planned before the detach will see the rows in the detached partition and queries that are planned after the detach, won't. A CHECK constraint is created that duplicates the partition constraint. This is probably not strictly necessary, and some users will prefer to remove it afterwards, but if the partition is re-attached to a partitioned table, the constraint needn't be rechecked. Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reviewed-by: Amit Langote <amitlangote09@gmail.com> Reviewed-by: Justin Pryzby <pryzby@telsasoft.com> Discussion: https://postgr.es/m/20200803234854.GA24158@alvherre.pgsql
* Fix use-after-free bug with AfterTriggersTableData.storeslotAlvaro Herrera2021-02-27
| | | | | | | | | | | | | | | AfterTriggerSaveEvent() wrongly allocates the slot in execution-span memory context, whereas the correct thing is to allocate it in a transaction-span context, because that's where the enclosing AfterTriggersTableData instance belongs into. Backpatch to 12 (the test back to 11, where it works well with no code changes, and it's good to have to confirm that the case was previously well supported); this bug seems introduced by commit ff11e7f4b9ae. Reported-by: Bertrand Drouvot <bdrouvot@amazon.com> Author: Amit Langote <amitlangote09@gmail.com> Discussion: https://postgr.es/m/39a71864-b120-5a5c-8cc5-c632b6f16761@amazon.com
* Fix permission checks on constraint violation errors on partitions.Heikki Linnakangas2021-02-08
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | If a cross-partition UPDATE violates a constraint on the target partition, and the columns in the new partition are in different physical order than in the parent, the error message can reveal columns that the user does not have SELECT permission on. A similar bug was fixed earlier in commit 804b6b6db4. The cause of the bug is that the callers of the ExecBuildSlotValueDescription() function got confused when constructing the list of modified columns. If the tuple was routed from a parent, we converted the tuple to the parent's format, but the list of modified columns was grabbed directly from the child's RTE entry. ExecUpdateLockMode() had a similar issue. That lead to confusion on which columns are key columns, leading to wrong tuple lock being taken on tables referenced by foreign keys, when a row is updated with INSERT ON CONFLICT UPDATE. A new isolation test is added for that corner case. With this patch, the ri_RangeTableIndex field is no longer set for partitions that don't have an entry in the range table. Previously, it was set to the RTE entry of the parent relation, but that was confusing. NOTE: This modifies the ResultRelInfo struct, replacing the ri_PartitionRoot field with ri_RootResultRelInfo. That's a bit risky to backpatch, because it breaks any extensions accessing the field. The change that ri_RangeTableIndex is not set for partitions could potentially break extensions, too. The ResultRelInfos are visible to FDWs at least, and this patch required small changes to postgres_fdw. Nevertheless, this seem like the least bad option. I don't think these fields widely used in extensions; I don't think there are FDWs out there that uses the FDW "direct update" API, other than postgres_fdw. If there is, you will get a compilation error, so hopefully it is caught quickly. Backpatch to 11, where support for both cross-partition UPDATEs, and unique indexes on partitioned tables, were added. Reviewed-by: Amit Langote Security: CVE-2021-3393
* Remove bogus restriction from BEFORE UPDATE triggersAlvaro Herrera2021-01-28
| | | | | | | | | | | | | | | | | | | | | | In trying to protect the user from inconsistent behavior, commit 487e9861d0cf "Enable BEFORE row-level triggers for partitioned tables" tried to prevent BEFORE UPDATE FOR EACH ROW triggers from moving the row from one partition to another. However, it turns out that the restriction is wrong in two ways: first, it fails spuriously, preventing valid situations from working, as in bug #16794; and second, they don't protect from any misbehavior, because tuple routing would cope anyway. Fix by removing that restriction. We keep the same restriction on BEFORE INSERT FOR EACH ROW triggers, though. It is valid and useful there. In the future we could remove it by having tuple reroute work for inserts as it does for updates. Backpatch to 13. Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reported-by: Phillip Menke <pg@pmenke.de> Discussion: https://postgr.es/m/16794-350a655580fbb9ae@postgresql.org
* Pass down "logically unchanged index" hint.Peter Geoghegan2021-01-13
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | Add an executor aminsert() hint mechanism that informs index AMs that the incoming index tuple (the tuple that accompanies the hint) is not being inserted by execution of an SQL statement that logically modifies any of the index's key columns. The hint is received by indexes when an UPDATE takes place that does not apply an optimization like heapam's HOT (though only for indexes where all key columns are logically unchanged). Any index tuple that receives the hint on insert is expected to be a duplicate of at least one existing older version that is needed for the same logical row. Related versions will typically be stored on the same index page, at least within index AMs that apply the hint. Recognizing the difference between MVCC version churn duplicates and true logical row duplicates at the index AM level can help with cleanup of garbage index tuples. Cleanup can intelligently target tuples that are likely to be garbage, without wasting too many cycles on less promising tuples/pages (index pages with little or no version churn). This is infrastructure for an upcoming commit that will teach nbtree to perform bottom-up index deletion. No index AM actually applies the hint just yet. Author: Peter Geoghegan <pg@bowt.ie> Reviewed-By: Victor Yegorov <vyegorov@gmail.com> Discussion: https://postgr.es/m/CAH2-Wz=CEKFa74EScx_hFVshCOn6AA5T-ajFASTdzipdkLTNQQ@mail.gmail.com
* Update copyright for 2021Bruce Momjian2021-01-02
| | | | Backpatch-through: 9.5
* Provide the OR REPLACE option for CREATE TRIGGER.Tom Lane2020-11-14
| | | | | | | | | | | | | | | | | | This is mostly straightforward. However, we disallow replacing constraint triggers or changing the is-constraint property; perhaps that can be added later, but the complexity versus benefit tradeoff doesn't look very good. Also, no special thought is taken here for whether replacing an existing trigger should result in changes to queued-but-not-fired trigger actions. We just document that if you're surprised by the results, too bad, don't do that. (Note that any such pending trigger activity would have to be within the current session.) Takamichi Osumi, reviewed at various times by Surafel Temesgen, Peter Smith, and myself Discussion: https://postgr.es/m/0DDF369B45A1B44B8A687ED43F06557C010BC362@G01JPEXMBYT03
* In security-restricted operations, block enqueue of at-commit user code.Noah Misch2020-11-09
| | | | | | | | | | | | | | | | | | Specifically, this blocks DECLARE ... WITH HOLD and firing of deferred triggers within index expressions and materialized view queries. An attacker having permission to create non-temp objects in at least one schema could execute arbitrary SQL functions under the identity of the bootstrap superuser. One can work around the vulnerability by disabling autovacuum and not manually running ANALYZE, CLUSTER, REINDEX, CREATE INDEX, VACUUM FULL, or REFRESH MATERIALIZED VIEW. (Don't restore from pg_dump, since it runs some of those commands.) Plain VACUUM (without FULL) is safe, and all commands are fine when a trusted user owns the target object. Performance may degrade quickly under this workaround, however. Back-patch to 9.5 (all supported versions). Reviewed by Robert Haas. Reported by Etienne Stalmans. Security: CVE-2020-25695
* In INSERT/UPDATE, use the table's real tuple descriptor as target.Tom Lane2020-10-26
| | | | | | | | | | | | | | | | | | | | | | | Previously, ExecInitModifyTable relied on ExecInitJunkFilter, and thence ExecCleanTypeFromTL, to build the target descriptor from the query tlist. While we just checked (in ExecCheckPlanOutput) that the tlist produces compatible output, this is not a great substitute for the relation's actual tuple descriptor that's available from the relcache. For one thing, dropped columns will not be correctly marked attisdropped; it's a bit surprising that we've gotten away with that this long. But the real reason for being concerned with this is that using the table's descriptor means that the slot will have correct attrmissing data, allowing us to revert the klugy fix of commit ba9f18abd. (This commit undoes that one's changes in trigger.c, but keeps the new test case.) Thus we can solve the bogus-trigger-tuple problem with fewer cycles rather than more. No back-patch, since this doesn't fix any additional bug, and it seems somewhat more likely to have unforeseen side effects than ba9f18abd's narrow fix. Discussion: https://postgr.es/m/16644-5da7ef98a7ac4545@postgresql.org
* Fix corner case for a BEFORE ROW UPDATE trigger returning OLD.Tom Lane2020-10-25
| | | | | | | | | | | | | | | | | | | | | | | | | If the old row has any "missing" attributes that are supposed to be retrieved from an associated tuple descriptor, the wrong things happened because the trigger result is shoved directly into an executor slot that lacks the missing-attribute data. Notably, CHECK-constraint verification would incorrectly see those columns as NULL, and so would RETURNING-list evaluation. Band-aid around this by forcibly expanding the tuple before passing it to the trigger function. (IMO it was a fundamental misdesign to put the missing-attribute data into tuple constraints, which so much of the system considers to be optional. But we're probably stuck with that now, and will have to continue to apply band-aids as we find other places with similar issues.) Back-patch to v12. v11 would also have the issue, except that commit 920311ab1 already applied a similar band-aid. That forced expansion in more cases than seem really necessary, though, so this isn't a directly equivalent fix. Amit Langote, with some cosmetic changes by me Discussion: https://postgr.es/m/16644-5da7ef98a7ac4545@postgresql.org
* Fix ALTER TABLE .. ENABLE/DISABLE TRIGGER recursionAlvaro Herrera2020-10-20
| | | | | | | | | | | | | | | | More precisely, correctly handle the ONLY flag indicating not to recurse. This was implemented in 86f575948c77 by recursing in trigger.c, but that's the wrong place; use ATSimpleRecursion instead, which behaves properly. However, because legacy inheritance has never recursed in that situation, make sure to do that only for new-style partitioning. I noticed this problem while testing a fix for another bug in the vicinity. This has been wrong all along, so backpatch to 11. Discussion: https://postgr.es/m/20201016235925.GA29829@alvherre.pgsql