Re: Bug in query rewriter - hasModifyingCTE not getting set

Greg Nancarrow <gregn4422(at)gmail(dot)com> writes:
> On Sun, Feb 7, 2021 at 10:03 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think either the bit about rule_action is unnecessary, or most of
>> the code immediately above this is wrong, because it's only updating
>> flags in sub_action. Why do you think it's necessary to change
>> rule_action in addition to sub_action?

> I believe that the bit about rule_action IS necessary, as it's needed
> for the case of INSERT...SELECT, so that hasModifyingCTE is set on the
> rewritten INSERT (see comment above the call to
> getInsertSelectQuery(), and the "KLUDGE ALERT" comment within that
> function).

Hm. So after looking at this more, the problem is that the rewrite
is producing something equivalent to

INSERT INTO bug6051_2
(WITH t1 AS (DELETE FROM bug6051 RETURNING *) SELECT * FROM t1);

If you try to do that directly, the parser will give you the raspberry:

ERROR: WITH clause containing a data-modifying statement must be at the top level
LINE 2: (WITH t1 AS (DELETE FROM bug6051 RETURNING *) SELECT * FROM ...
^

The code throwing that error, in analyzeCTE(), explains

/*
* We disallow data-modifying WITH except at the top level of a query,
* because it's not clear when such a modification should be executed.
*/

That semantic issue doesn't get any less pressing just because the query
was generated by rewrite. So I now think that what we have to do is
throw an error if we have a modifying CTE and sub_action is different
from rule_action. Not quite sure how to phrase the error though.

In view of this, maybe the right thing is to disallow modifying CTEs
in rule actions in the first place. I see we already do that for
views (i.e. ON SELECT rules), but they're not really any safer in
other types of rules. Given that non-SELECT rules are an undertested
legacy thing, I'm not that excited about moving mountains to make
this case possible.

Anyway, I think I'm going to go revert the patch I crammed in last night.
There's more here than meets the eye, and right before a release is no
time to be fooling with an issue that's been there for years.

regards, tom lane