On Tue, Jun 6, 2017 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> On Tue, Jun 6, 2017 at 2:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 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.
>
> No, they don't necessarily. One case that will certainly not be
> covered by such a test is if the wCTE is an UPDATE or DELETE on
> an inheritance hierarchy: parse analysis has no idea whether the
> target table has children at all, let alone whether any of the
> children have triggers that use transition tables. You really want
> the test to happen after the planner has expanded inheritance.
If Kevin accepts my other patch or at least the approach, we won't
allow any child table triggers with transition tables, so we wouldn't
need to consider them here (triggers on the named result table see
tuples collected from children, but ROW triggers attached to children
can't have transition tables and STATEMENT triggers attached to
children don't fire). Though those decisions probably wouldn't be
future-proof, we'd only want this block in place until we had support
for wCTEs.
> Another problem is that the result of parse analysis is frozen for much
> longer than you're thinking about in the case of stored rules or views.
> We currently disallow views that contain writable CTEs, but I do not
> think there's such a prohibition for rules.
Ugh. Right. I can indeed circumvent the above check with a wCTE
hiding in a rule, so parse analysis is not the right time for the
check. The whole rewrite area is clearly something I need to go and
learn about. Thanks for the explanation.
--
Thomas Munro
http://www.enterprisedb.com