Re: PG10 transition tables, wCTEs and multiple operations on the same table

On Tue, Jun 6, 2017 at 2:15 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sat, Jun 3, 2017 at 10:39 PM, Thomas Munro
>> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>>> In the meantime, it seems like you agree that rejecting wCTEs that
>>> affect tables with triggers with transition tables is the best
>>> response to this bug report? Do you think that parse analysis is the
>>> right time to do the check? Here's a first attempt at that.
>
> FWIW, parse analysis is surely NOT the time for such a check. Triggers
> might get added to a table between analysis and execution. I think you
> might have to do it during executor startup.

Hmm. My understanding: In analyzeCTE(), where the check is done in
that patch, we hold a lock on the relation because we have a
RangeTblEntry. That means that no triggers can be added concurrently
in the case where you go on to execute the plan. If you save the plan
for later execution, triggers created between then and lock
reacquisition (because creating triggers touches pg_class) cause
reanalysis. This is not fundamentally different from altering the
table. For example, with that patch:

postgres=# prepare ps as with t as (insert into table1 values (1))
insert into table2 values ('hello');
PREPARE
postgres=# create trigger trig1 after insert on table1 referencing new
table as oo for each row execute procedure trigfunc();
CREATE TRIGGER
postgres=# execute ps;
ERROR: WITH clause cannot modify a table that has triggers with
transition tables

What am I missing?

>> I'm starting to like the approach of reverting the entire transition
>> tables patch. Failing to consider the possibility of a plan with
>> multiple ModifyTable nodes seems like a pretty fundamental design
>> mistake, and I'm not eager either to ship this with that broken or try
>> to fix it at this stage of the release cycle.

Not handling wCTEs or inheritance definitely counts as a howler, but
we seem tantalisingly close and it would be a shame to push the whole
feature back IMHO. I really hope we can move on to talking about
incremental matviews in the next cycle, which is why I've jumped on
every problem report so far.

I'm still waiting to hear from Kevin whether he thinks what I proposed
for the inheritance bug has legs. If so, my vote from the peanut
gallery would be to keep transition tables in the tree but block wCTEs
with transition tables, and then add support in the next release
(which I'd be happy to work on). Support will certainly be needed
ASAP, because it'd suck if incremental matview maintenance didn't work
just because you use wCTEs, and the 'action at a distance' factor
isn't a great user experience (adding a new trigger can in general
break existing queries, but breaking them just because they use wCTEs
is arguably surprising). On the other hand, if he thinks that the
inheritance bug is not adequately fixed by my proposal (with minor
tweaks) and needs a completely different approach, then I'm out (but
will be happy to help work on this stuff for PG11).

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