On Tue, Jun 6, 2017 at 2:15 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Robert Haas <robertmh...@gmail.com> writes: >> On Sat, Jun 3, 2017 at 10:39 PM, Thomas Munro >> <thomas.mu...@enterprisedb.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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers