Re: [HACKERS] transition table behavior with inheritance appearsbroken (was: Declarative partitioning - another take)

On Tue, May 2, 2017 at 3:01 AM, Robert Haas <robertmhaas@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[1]), 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?

[1] https://www.postgresql.org/message-id/CACjxUsNhdm4ZCgaVreLK5kAwHTZUkqJAVXiywwi-HNVsuTLMnA%40mail.gmail.com

-- 
Thomas Munro
http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers