Thread: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values

The following bug has been logged on the website:

Bug reference:      17803
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 15.2
Operating system:   Ubuntu 22.04
Description:

The following queries:
CREATE TABLE t (a int, b int DEFAULT -1);
CREATE VIEW v AS SELECT * FROM t;
CREATE RULE vr AS ON INSERT TO v DO ALSO INSERT INTO t SELECT 1;
INSERT INTO v VALUES (0, DEFAULT), (1, DEFAULT);

lead to an assert for me:
Core was generated by `postgres: law regression [local] INSERT
                        '.
Program terminated with signal SIGABRT, Aborted.

warning: Section `.reg-xstate/1192422' in core file too small.
#0  __pthread_kill_implementation (no_tid=0, signo=6,
threadid=140044862031680) at ./nptl/pthread_kill.c:44
44      ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6,
threadid=140044862031680) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=140044862031680) at
./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=140044862031680, signo=signo@entry=6) at
./nptl/pthread_kill.c:89
#3  0x00007f5ebb90e476 in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4  0x00007f5ebb8f47f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x0000562faf172cf6 in ExceptionalCondition (conditionName=0x562faf346725
"rte->rtekind == RTE_VALUES", 
    errorType=0x562faf3462b3 "FailedAssertion", fileName=0x562faf34634c
"rewriteHandler.c", lineNumber=1600)
    at assert.c:69
#6  0x0000562faef2c936 in rewriteValuesRTEToNulls (parsetree=0x562fb1232028,
rte=0x562fb13002f0)
    at rewriteHandler.c:1600
#7  0x0000562faef30be1 in RewriteQuery (parsetree=0x562fb1232140,
rewrite_events=0x0, orig_rt_length=0)
    at rewriteHandler.c:3878
#8  0x0000562faef316da in QueryRewrite (parsetree=0x562fb1232140) at
rewriteHandler.c:4168
#9  0x0000562faef9dc61 in pg_rewrite_query (query=0x562fb1232140) at
postgres.c:792
#10 0x0000562faef9da2e in pg_analyze_and_rewrite_fixedparams
(parsetree=0x562fb1231f98, 
    query_string=0x562fb12311b0 "INSERT INTO v VALUES (0, DEFAULT), (1,
DEFAULT);", paramTypes=0x0, numParams=0, 
    queryEnv=0x0) at postgres.c:666
#11 0x0000562faef9e269 in exec_simple_query (
    query_string=0x562fb12311b0 "INSERT INTO v VALUES (0, DEFAULT), (1,
DEFAULT);") at postgres.c:1166
#12 0x0000562faefa3339 in PostgresMain (dbname=0x562fb125d658 "regression",
username=0x562fb125d638 "law")
    at postgres.c:4593
#13 0x0000562faeec8afb in BackendRun (port=0x562fb1254d70) at
postmaster.c:4511
#14 0x0000562faeec8382 in BackendStartup (port=0x562fb1254d70) at
postmaster.c:4239
#15 0x0000562faeec430f in ServerLoop () at postmaster.c:1806
#16 0x0000562faeec3a6c in PostmasterMain (argc=3, argv=0x562fb122c670) at
postmaster.c:1478
#17 0x0000562faedb79ad in main (argc=3, argv=0x562fb122c670) at main.c:202

Without asserts enabled I get:
ERROR:  unrecognized node type: 148
STATEMENT:  INSERT INTO v VALUES (0, DEFAULT), (1, DEFAULT);

It looks like this is originated from the commit 41531e42d
that was prepared only for "ALSO INSERT ... VALUES ..." (before it
INSERT SELECT worked just as incorrectly as INSERT VALUES),
and the following commits (ed4653db8,  b8f2687fd, 2605643a),
which massaged that code didn't fix things for this scenario yet.

On Tue, 21 Feb 2023 at 09:21, PG Bug reporting form
<noreply@postgresql.org> wrote:
>
> The following queries:
> CREATE TABLE t (a int, b int DEFAULT -1);
> CREATE VIEW v AS SELECT * FROM t;
> CREATE RULE vr AS ON INSERT TO v DO ALSO INSERT INTO t SELECT 1;
> INSERT INTO v VALUES (0, DEFAULT), (1, DEFAULT);
>
> lead to an assert for me:
>

Nice catch.

It looks like we need something like the attached, to deal with
product queries that are INSERT ... SELECT queries. In that case the
VALUES RTE will be at the same index, but in the SELECT part of the
product query, not the top-level product query itself.

Annoyingly, this can't use getInsertSelectQuery(), because the product
query will no longer have the OLD and NEW placeholder entries, so I
have just duplicated the relevant portions of its code, rather than
remove those checks from getInsertSelectQuery().

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> It looks like we need something like the attached, to deal with
> product queries that are INSERT ... SELECT queries. In that case the
> VALUES RTE will be at the same index, but in the SELECT part of the
> product query, not the top-level product query itself.

It seems like this bit:

+                    rtr = (RangeTblRef *) linitial(pt->jointree->fromlist);
+                    selectrte = rt_fetch(rtr->rtindex, pt->rtable);
+                    selectquery = selectrte->subquery;

is missing several essential checks.  Is the node extracted from
jointree->fromlist actually a RangeTblRef?  Seems like it could
be a JoinExpr or FromExpr instead; even if it can't be that today,
an IsA check is cheap future-proofing.  Likewise, once you've
got your hands on the RTE, you should check rtekind == RTE_SUBQUERY
rather than assuming it's safe to touch the subquery field.

(I see that getInsertSelectQuery isn't much better about this,
but we should fix that while we're at it.)

            regards, tom lane

On Tue, 21 Feb 2023 at 15:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> It seems like this bit:
>
> +                    rtr = (RangeTblRef *) linitial(pt->jointree->fromlist);
> +                    selectrte = rt_fetch(rtr->rtindex, pt->rtable);
> +                    selectquery = selectrte->subquery;
>
> is missing several essential checks.  Is the node extracted from
> jointree->fromlist actually a RangeTblRef?  Seems like it could
> be a JoinExpr or FromExpr instead; even if it can't be that today,
> an IsA check is cheap future-proofing.  Likewise, once you've
> got your hands on the RTE, you should check rtekind == RTE_SUBQUERY
> rather than assuming it's safe to touch the subquery field.
>
> (I see that getInsertSelectQuery isn't much better about this,
> but we should fix that while we're at it.)
>

Yeah, that makes sense. Something like this? (I think an elog() is
probably more useful than an Assert(), if we don't find what we
expect.)

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> Yeah, that makes sense. Something like this? (I think an elog() is
> probably more useful than an Assert(), if we don't find what we
> expect.)

I think it's fine to leave the checks on parsetree->jointree being
a FromExpr as Asserts, because that's assumed in a lot of places.
Rest of it is OK by me.

            regards, tom lane

21.02.2023 21:02, Tom Lane wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>> Yeah, that makes sense. Something like this? (I think an elog() is
>> probably more useful than an Assert(), if we don't find what we
>> expect.)
> I think it's fine to leave the checks on parsetree->jointree being
> a FromExpr as Asserts, because that's assumed in a lot of places.
> Rest of it is OK by me.
I have a minor question about the condition:
+                if (pt->commandType == CMD_INSERT ...
Is it possible to get another commandType there?
IIUC, we cant get into
         if (defaults_remaining && product_queries != NIL)
only for INSERT ...
In other words, are there other commands that we expect executing
following lines for?
values_rte = rt_fetch(values_rte_index, pt->rtable);
...
rewriteValuesRTEToNulls(pt, values_rte);
(ALSO MERGE is not supported, as I can see)

If the (pt->commandType == CMD_INSERT) check is for safety, maybe it
should be broader?

Thanks for the fix!
(My extra testing discovered no new anomalies in this area.)

Best regards,
Alexander

On Wed, 22 Feb 2023 at 08:00, Alexander Lakhin <exclusion@gmail.com> wrote:
>
> I have a minor question about the condition:
> +                if (pt->commandType == CMD_INSERT ...
> Is it possible to get another commandType there?

Yes, there could also be ON INSERT DO ALSO UPDATE/DELETE rules. The
new code is intended to apply only for DO ALSO INSERT .. SELECT rules
(where the VALUES RTE in "pt" isn't in the usual place), but other
types of command in the rule are also possible. As long as the
top-level command is a multi-row INSERT ... VALUES (), (), ... it can
reach this code block.

For something like an ON INSERT DO ALSO UPDATE/DELETE rule, the
previous code should in theory handle any remaining DEFAULT values
from the top-level INSERT, though I'm not sure how thoroughly that has
been tested. I'll take a look.

> Thanks for the fix!
> (My extra testing discovered no new anomalies in this area.)

Thanks for testing.

Regards,
Dean

22.02.2023 12:07, Dean Rasheed wrote:
> On Wed, 22 Feb 2023 at 08:00, Alexander Lakhin <exclusion@gmail.com> wrote:
>> I have a minor question about the condition:
>> +                if (pt->commandType == CMD_INSERT ...
>> Is it possible to get another commandType there?
> Yes, there could also be ON INSERT DO ALSO UPDATE/DELETE rules. The
> new code is intended to apply only for DO ALSO INSERT .. SELECT rules
> (where the VALUES RTE in "pt" isn't in the usual place), but other
> types of command in the rule are also possible. As long as the
> top-level command is a multi-row INSERT ... VALUES (), (), ... it can
> reach this code block.

Thanks! I understand this place better now.
I mistakenly supposed that VALUES RTE can intervene with INSERT only.
But while exploring how they can affect other statements,
I've found one more interesting thing:
CREATE TABLE t (a int, b int DEFAULT -1);
CREATE VIEW v AS SELECT * FROM t;
CREATE RULE vr AS ON INSERT TO v DO ALSO INSERT INTO t SELECT NEW.b;
INSERT INTO v VALUES(10, -1), (20, DEFAULT);
SELECT * FROM v;

  a  | b
----+----
  10 | -1
  20 | -1
  -1 | -1
     | -1

It seems that a NEW field filled correctly if it is not DEFAULT,
but DEFAULT is lost somehow.

Best regards,
Alexander

On Wed, 22 Feb 2023 at 12:00, Alexander Lakhin <exclusion@gmail.com> wrote:
>
> Thanks! I understand this place better now.
> I mistakenly supposed that VALUES RTE can intervene with INSERT only.
> But while exploring how they can affect other statements,
> I've found one more interesting thing:
> CREATE TABLE t (a int, b int DEFAULT -1);
> CREATE VIEW v AS SELECT * FROM t;
> CREATE RULE vr AS ON INSERT TO v DO ALSO INSERT INTO t SELECT NEW.b;
> INSERT INTO v VALUES(10, -1), (20, DEFAULT);
> SELECT * FROM v;
>
>   a  | b
> ----+----
>   10 | -1
>   20 | -1
>   -1 | -1
>      | -1
>

Yes, that's correct. Or at least that's the way it's always been. We
even have regression tests similar to that.

Any DEFAULT values from the top-level command that haven't been
replaced are set to NULL before the rule is evaluated, so a DEFAULT in
the top-level command becomes a NULL rather than a DEFAULT in the rule
action.

Arguably, that isn't what some users might expect, but it's
long-standing behaviour that would probably be more trouble than it's
worth to change. There are many other surprises in the way rules work,
which is why their use is discouraged, almost to the point of being
deprecated.

Regards,
Dean

22.02.2023 15:29, Dean Rasheed wrote:
> On Wed, 22 Feb 2023 at 12:00, Alexander Lakhin <exclusion@gmail.com> wrote:
>> CREATE RULE vr AS ON INSERT TO v DO ALSO INSERT INTO t SELECT NEW.b;
>> INSERT INTO v VALUES(10, -1), (20, DEFAULT);
>> SELECT * FROM v;
>>
>>    a  | b
>> ----+----
>>    10 | -1
>>    20 | -1
>>    -1 | -1
>>       | -1
>>
> Yes, that's correct. Or at least that's the way it's always been. We
> even have regression tests similar to that.

Thanks for the explanation! I agree that there must be strong reasons to
change a well-known and accepted behavior.
Please look at another anomaly, probably not related to the initial one,
but related to INSERT VALUES(...), (...), as I can see:
CREATE TABLE t (a int, b int DEFAULT -1);
CREATE VIEW v AS SELECT * FROM t;

CREATE RULE vr AS ON INSERT TO v DO ALSO INSERT INTO t
   SELECT * FROM (SELECT a FROM t WHERE NEW.a = t.a) tt;

INSERT INTO v VALUES (1), (2);
Leads to:
Core was generated by `postgres: law regression [local] 
INSERT                                       '.
Program terminated with signal SIGABRT, Aborted.

warning: Section `.reg-xstate/1345067' in core file too small.
#0  __pthread_kill_implementation (no_tid=0, signo=6, 
threadid=140349553121088) at ./nptl/pthread_kill.c:44
44      ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, 
threadid=140349553121088) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=140349553121088) at 
./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=140349553121088, signo=signo@entry=6) 
at ./nptl/pthread_kill.c:89
#3  0x00007fa5acf33476 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/posix/raise.c:26
#4  0x00007fa5acf197f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x000055643e045db9 in ExceptionalCondition 
(conditionName=0x55643e1fc08d "root->hasLateralRTEs",
     fileName=0x55643e1fbd1a "initsplan.c", lineNumber=2208) at assert.c:66
#6  0x000055643dd1211b in distribute_qual_to_rels (root=0x55643ed3d228, 
clause=0x55643ed5c130,
     jtitem=0x55643ed5d130, sjinfo=0x0, security_level=0, 
qualscope=0x55643ed5d2c0, ojscope=0x0,
     outerjoin_nonnullable=0x0, allow_equivalence=true, has_clone=false, 
is_clone=false,
     postponed_oj_qual_list=0x0) at initsplan.c:2208
#7  0x000055643dd11fcf in distribute_quals_to_rels (root=0x55643ed3d228, 
clauses=0x55643ed5c180,
     jtitem=0x55643ed5d130, sjinfo=0x0, security_level=0, 
qualscope=0x55643ed5d2c0, ojscope=0x0,
     outerjoin_nonnullable=0x0, allow_equivalence=true, has_clone=false, 
is_clone=false,
     postponed_oj_qual_list=0x0) at initsplan.c:2113
#8  0x000055643dd1073c in deconstruct_distribute (root=0x55643ed3d228, 
jtitem=0x55643ed5d130)
     at initsplan.c:1147
#9  0x000055643dd0f6ce in deconstruct_jointree (root=0x55643ed3d228) at 
initsplan.c:776
#10 0x000055643dd1476e in query_planner (root=0x55643ed3d228,
     qp_callback=0x55643dd1b11a <standard_qp_callback>, 
qp_extra=0x7ffc081eac20) at planmain.c:186
#11 0x000055643dd17319 in grouping_planner (root=0x55643ed3d228, 
tuple_fraction=0) at planner.c:1496
#12 0x000055643dd16998 in subquery_planner (glob=0x55643ed59468, 
parse=0x55643ec5e2c8, parent_root=0x0,
     hasRecursion=false, tuple_fraction=0) at planner.c:1065
#13 0x000055643dd14f36 in standard_planner (parse=0x55643ec5e2c8,
     query_string=0x55643ec5d568 "INSERT INTO v VALUES (1), (2);", 
cursorOptions=2048, boundParams=0x0)
     at planner.c:411
#14 0x000055643dd14c64 in planner (parse=0x55643ec5e2c8,
     query_string=0x55643ec5d568 "INSERT INTO v VALUES (1), (2);", 
cursorOptions=2048, boundParams=0x0)
     at planner.c:281
#15 0x000055643de60d18 in pg_plan_query (querytree=0x55643ec5e2c8,
     query_string=0x55643ec5d568 "INSERT INTO v VALUES (1), (2);", 
cursorOptions=2048, boundParams=0x0)
     at postgres.c:870
#16 0x000055643de60e75 in pg_plan_queries (querytrees=0x55643ed57828,
     query_string=0x55643ec5d568 "INSERT INTO v VALUES (1), (2);", 
cursorOptions=2048, boundParams=0x0)
     at postgres.c:962
#17 0x000055643de6128a in exec_simple_query (query_string=0x55643ec5d568 
"INSERT INTO v VALUES (1), (2);")
     at postgres.c:1159
#18 0x000055643de66456 in PostgresMain (dbname=0x55643ec95968 
"regression", username=0x55643ec5aaf8 "law")
     at postgres.c:4572
#19 0x000055643dd8a118 in BackendRun (port=0x55643ec860d0) at 
postmaster.c:4461
#20 0x000055643dd899a4 in BackendStartup (port=0x55643ec860d0) at 
postmaster.c:4189
#21 0x000055643dd85ce9 in ServerLoop () at postmaster.c:1779
#22 0x000055643dd85593 in PostmasterMain (argc=3, argv=0x55643ec58a30) 
at postmaster.c:1463
#23 0x000055643dc3f614 in main (argc=3, argv=0x55643ec58a30) at main.c:200

The more simple rule:
CREATE RULE vr AS ON INSERT TO v DO ALSO INSERT INTO t
   SELECT a FROM t WHERE NEW.a = t.a;
doesn't trigger the assertion failure,
and "INSERT INTO v VALUES (1);" with the complex rule too.

Maybe it's worth reporting it as another bug, and finish things with the
current one?
(Though I am still unsure, can we get
list_length(pt->jointree->fromlist) != 1 in the new condition?)

Best regards,
Alexander

On Thu, 23 Feb 2023 at 11:00, Alexander Lakhin <exclusion@gmail.com> wrote:
>
> Please look at another anomaly, probably not related to the initial one,
> but related to INSERT VALUES(...), (...), as I can see:
> CREATE TABLE t (a int, b int DEFAULT -1);
> CREATE VIEW v AS SELECT * FROM t;
>
> CREATE RULE vr AS ON INSERT TO v DO ALSO INSERT INTO t
>    SELECT * FROM (SELECT a FROM t WHERE NEW.a = t.a) tt;
>
> INSERT INTO v VALUES (1), (2);

[I just pushed the other fix]

This is definitely an independent bug, nothing to do with DEFAULTs,
and also not specific to multi-row VALUES lists either, since it also
fails with INSERT INTO v SELECT 1.

Apparently the rewriter is generating a query that the planner is not
able to handle, though I'm not clear on the precise details.

As another clue, if the rule query is modified to prevent subquery
pullup, by adding OFFSET 0, it fails in a different way, with

ERROR:  plan should not reference subplan's variable

in finalize_plan(). Not sure if that helps find the problem.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> This is definitely an independent bug, nothing to do with DEFAULTs,
> and also not specific to multi-row VALUES lists either, since it also
> fails with INSERT INTO v SELECT 1.

> Apparently the rewriter is generating a query that the planner is not
> able to handle, though I'm not clear on the precise details.

It looks to me like the rewriter is failing to set the rte->lateral flag
on the sub-select, or maybe the fault is even earlier in the parser.
That NEW reference sure looks like a lateral ref to me:

CREATE RULE vr AS ON INSERT TO v DO ALSO INSERT INTO t
   SELECT * FROM (SELECT a FROM t WHERE NEW.a = t.a) tt;

The planner is Asserting because it's seeing a reference to RTE 4
(the VALUES RTE) in a place where it'd only expect to see a reference
to RTE 8 (the sub-select's "t" rel) unless the query uses LATERAL.

Everything goes through fine if I manually add LATERAL:

regression=# CREATE or replace RULE vr AS ON INSERT TO v DO ALSO INSERT INTO t
  SELECT * FROM LATERAL (SELECT a FROM t WHERE NEW.a = t.a) tt;
CREATE RULE
regression=# INSERT INTO v VALUES (1), (2);
INSERT 0 2

Arguably, the user should have written LATERAL on that sub-select in
the first place, but we probably can't start enforcing that ex post
facto.  We'll have to do something that causes NEW (and OLD?) references
in sub-selects to generate a LATERAL marking silently.

Kinda surprising nobody noticed this before, because I'm sure it's
been busted a long time.

            regards, tom lane

On Thu, 23 Feb 2023 at 21:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> It looks to me like the rewriter is failing to set the rte->lateral flag
> on the sub-select, or maybe the fault is even earlier in the parser.
>
> Arguably, the user should have written LATERAL on that sub-select in
> the first place, but we probably can't start enforcing that ex post
> facto.  We'll have to do something that causes NEW (and OLD?) references
> in sub-selects to generate a LATERAL marking silently.
>

Yeah, my first thought was to try to fix it up in the rewriter, so
that it can deal with any existing stored rules.

Doing the attached fixes the reported issue, and all variants of it
that I could come up with, but I'm not entirely sure whether it needs
to be concerned about other rtekinds.

Regards,
Dean

On Thu, 23 Feb 2023 at 23:20, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> Doing the attached fixes the reported issue, and all variants of it
> that I could come up with, but I'm not entirely sure whether it needs
> to be concerned about other rtekinds.
>

I wasn't able to provoke a failure involving any other rtekind. It is
possible to break it using references to OLD in UPDATE rules though.

Here's an updated patch, now with test cases involving both OLD and
NEW references.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> Here's an updated patch, now with test cases involving both OLD and
> NEW references.

It might be worth doing

+        if (rte->rtekind == RTE_SUBQUERY && !rte->lateral &&
+            contain_vars_of_level((Node *) rte->subquery, 1))
+            rte->lateral = true;

so as to save the expense of contain_vars_of_level() when the flag
is already set.  However, it's arguable that no users would bother
with writing LATERAL, so this might be pointless.

More importantly, I think the comment could do with a bit more
information.  Maybe like

     /*
+     * Mark any subquery RTEs in the rule action as LATERAL if they contain
+     * Vars referring to the current query level (references to NEW/OLD).
+     * Those really are lateral references, but we've historically not
+     * required users to mark such subqueries with LATERAL explicitly.
+     * But the planner will complain if such Vars exist in a non-LATERAL
+     * subquery, so we have to fix things up here.
+     */

That might be overly verbose, but I think it's good to memorialize this
sort of info.

LGTM otherwise.

            regards, tom lane

On Fri, 24 Feb 2023 at 16:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> It might be worth doing
>
> +        if (rte->rtekind == RTE_SUBQUERY && !rte->lateral &&
> +            contain_vars_of_level((Node *) rte->subquery, 1))
> +            rte->lateral = true;
>
> so as to save the expense of contain_vars_of_level() when the flag
> is already set.  However, it's arguable that no users would bother
> with writing LATERAL, so this might be pointless.
>

OK, maybe not essential, but that does seem more consistent with what
we do in other places.

> More importantly, I think the comment could do with a bit more
> information.  Maybe like ...
>

Yeah I thought about adding more detail to that comment, but couldn't
come up with the right set of words. That looks reasonable to me.

Pushed.

Regards,
Dean

25.02.2023 18:08, Dean Rasheed wrote:
> Pushed.
Thanks for the fix!

Best regards,
Alexander