On Tue, May 2, 2017 at 3:01 AM, Robert Haas <robertmh...@gmail.com> wrote: > [...] > Only the rows in the parent show up in the transition table. But now > look what happens if I add an unrelated trigger that also uses > transition tables to the children: > > rhaas=# CREATE FUNCTION u() RETURNS trigger LANGUAGE plpgsql AS > $$begin null; end$$; > CREATE FUNCTION > rhaas=# CREATE TRIGGER x1 AFTER UPDATE ON p1 REFERENCING OLD TABLE AS > old NEW TABLE AS new FOR EACH STATEMENT EXECUTE PROCEDURE u(); > CREATE TRIGGER > rhaas=# UPDATE p SET b = 'whatever'; > NOTICE: table p > NOTICE: table p got value 0 > NOTICE: table p got value 1 > NOTICE: table p got value 2 > UPDATE 3 > > It seems pretty clear to me that this is busted. The existence of > trigger x1 on p1 shouldn't affect whether trigger x on p sees changes > to p1's rows in its transition tables.
Yikes -- agreed. See analysis and draft patch for discussion below. > Either all changes to any > descendants of p should be captured by the transition tables, or only > changes to the root table should be captured. If we do the former, > the restriction against using transition tables in triggers on > partitioned tables should be removed, I would think. If we do the > latter, then what we should document is not that partitioned tables > have a restriction that doesn't apply to inheritance but rather that > the restriction on the partitioned case flows from the fact that only > the parent's changes are captured, and the parent is always empty in > the partitioning case. In deciding between these two cases, we should > consider the case where the inheritance children have extra columns > and/or different column orderings. I think that we should only capture transition tuples captured from the explicitly named relation, since we only fire AFTER STATEMENT triggers on that relation. I see no inconsistency with the policy of rejecting transition tables on partitioned tables (as I proposed and Kevin accepted), because partitioned tables can't have any data so there would be no point. In contrast, every parent table in an inheritance hierarchy is also a regular table and can hold data, so I think we should allow transition tables on them, and capture transition tuples from that table only when you modify it directly. The transition table infrastructure only supports exactly one relation being modified at each query level, and it's a bug that this example captures tuples from p1 into the tuplestore intended for p's tuples even though it is not even going to fire the after statement trigger on p1. It's only a coincidence that the tuples have compatible TupleDescs. The pre-existing behaviour for triggers with inheritance is that STATEMENT triggers fire only for the directly named relation, but ROW triggers fire for all affected relations. The transition table patch didn't change that, but it added code to AfterTriggerSaveEvent, which is called by ExecAR(Insert|Update|Delete)Triggers, to capture transitions. That gets called for every updated relation (ie including partitions and inheritance sub-tables) to implement the ROW policy. It needs to be taught not to capture transition tuples from relations except the one directly named. One solution to this problem is for nodeModifyTable.c to tell the ExecAR* functions explicitly whether to capture transition tuples. It knows when it has modified the explicitly named relation in a hierarchy (mt_whichplan == 0) without rerouting via a partitioned table (mt_partition_dispatch_info == NULL). See attached patch for discussion (it lacks tests and needs better comments). Does this make sense? Do you see a better way?  https://www.postgresql.org/message-id/CACjxUsNhdm4ZCgaVreLK5kAwHTZUkqJAVXiywwi-HNVsuTLMnA%40mail.gmail.com -- Thomas Munro http://www.enterprisedb.com
Description: Binary data
-- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers