"tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> writes:
> The attached patch is based on your version. It includes cosmetic
> changes to use = instead of |= for boolean variable assignments.
I think that's less "cosmetic" than "gratuitous breakage". The point
here is that we are combining two rtables, so the query had better
end up with flags that describe the union of the rtables' properties.
Our regression tests are unfortunately not very thorough in this area,
so it doesn't surprise me that they fail to fall over.
After thinking about it for awhile, I'm okay with the concept of
attaching the source query's CTEs to the parent rule_action so far
as the semantics are concerned. But this patch fails to implement
that correctly. If we're going to do it like that, then the
ctelevelsup fields of any CTE RTEs that refer to those CTEs have
to be incremented when rule_action is different from sub_action,
because the CTEs are getting attached one level higher in the
query nest than the referencing RTEs are. The proposed test case
fails to expose this, because the rule action isn't INSERT/SELECT,
so the case of interest isn't being exercised at all. However,
it's harder than you might think to demonstrate a problem ---
I first tried
CREATE RULE bug6051_3_ins AS ON INSERT TO bug6051_3 DO INSTEAD
INSERT INTO bug6051_2 SELECT a FROM bug6051_3;
and that failed to fall over with the patch. Turns out that's
because the SELECT part is simple enough to be pulled up, and
the pull-up moves the CTE that's been put into it one level
higher, causing it to accidentally have the correct ctelevelsup
anyway. If you use an INSERT with a non-pull-up-able SELECT
then you can see the problem: this script
CREATE TEMP TABLE bug6051_2 (i int);
CREATE TEMP TABLE bug6051_3 AS
select a from generate_series(11,13) as a;
CREATE RULE bug6051_3_ins AS ON INSERT TO bug6051_3 DO INSTEAD
INSERT INTO bug6051_2 SELECT sum(a) FROM bug6051_3;
explain verbose
WITH t1 AS ( DELETE FROM bug6051_3 RETURNING * )
INSERT INTO bug6051_3 SELECT * FROM t1;
causes the patch to fail with
ERROR: could not find CTE "t1"
Now, we could potentially make this work if we wrote code to run
through the copied rtable entries (recursively) and increment the
appropriate ctelevelsup fields by one. That would essentially
have to be a variant of IncrementVarSublevelsUp that *only* acts
on ctelevelsup and not other level-dependent fields. That's
what I meant when I spoke of moving mountains: the amount of code
that would need to go into this seems out of all proportion to
the value. I think we should just throw an error, instead.
At least till such time as we see actual field complaints.
regards, tom lane