Horton

Re: Bug in query rewriter - hasModifyingCTE not getting set

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Greg Nancarrow <gregn4422@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2021-02-07T17:44:31Z
Lists: pgsql-hackers 
Greg Nancarrow <gregn4422@gmail.com> writes:
> On Sun, Feb 7, 2021 at 10:03 AM Tom Lane <tgl@sss.pgh.pa.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

Commits

  1. Fix rewriter to set hasModifyingCTE correctly on rewritten queries.

  2. Enable parallel SELECT for "INSERT INTO ... SELECT ...".

  3. Revert "Propagate CTE property flags when copying a CTE list into a rule."

  4. Propagate CTE property flags when copying a CTE list into a rule.