pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
17+ messages / 4 participants
[nested] [flat]
* BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-21 09:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 17+ messages in thread
From: PG Bug reporting form @ 2023-02-21 09:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
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.
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-21 11:56 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 17+ messages in thread
From: Dean Rasheed @ 2023-02-21 11:56 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
Attachments:
[text/x-patch] fix-default-handling-for-insert-select-rule.patch (3.9K, ../../CAEZATCUQtGx6gQtryMbC3ZsEd4GUcc5THVnkXP1F0Fan_uLG-Q@mail.gmail.com/2-fix-default-handling-for-insert-select-rule.patch)
download | inline diff:
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index c74bac2..5f3f680
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -3861,13 +3861,34 @@ RewriteQuery(Query *parsetree, List *rew
/*
* Each product query has its own copy of the VALUES RTE at the
* same index in the rangetable, so we must finalize each one.
+ *
+ * Note that if the product query is an INSERT ... SELECT, then
+ * the VALUES RTE will be at the same index in the SELECT part of
+ * the product query rather than the top-level product query
+ * itself.
*/
foreach(n, product_queries)
{
Query *pt = (Query *) lfirst(n);
- RangeTblEntry *values_rte = rt_fetch(values_rte_index,
- pt->rtable);
+ RangeTblEntry *values_rte;
+ if (pt->commandType == CMD_INSERT &&
+ list_length(pt->jointree->fromlist) == 1)
+ {
+ RangeTblRef *rtr;
+ RangeTblEntry *selectrte;
+ Query *selectquery;
+
+ rtr = (RangeTblRef *) linitial(pt->jointree->fromlist);
+ selectrte = rt_fetch(rtr->rtindex, pt->rtable);
+ selectquery = selectrte->subquery;
+
+ if (selectquery && IsA(selectquery, Query) &&
+ selectquery->commandType == CMD_SELECT)
+ pt = selectquery;
+ }
+
+ values_rte = rt_fetch(values_rte_index, pt->rtable);
rewriteValuesRTEToNulls(pt, values_rte);
}
}
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
new file mode 100644
index 2b578cc..3bc38b6
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -3296,6 +3296,25 @@ select * from base_tab_def order by a, c
| View default | | View default |
(22 rows)
+-- Test a DO ALSO INSERT ... SELECT rule
+drop rule base_tab_def_view_ins_rule on base_tab_def_view;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do also insert into base_tab_def (a, b, e) select new.a, new.b, 'xxx';
+truncate base_tab_def;
+insert into base_tab_def_view values (1, default, default, default, default);
+insert into base_tab_def_view values (2, default, default, default, default),
+ (3, default, default, default, default);
+select * from base_tab_def order by a, e NULLS FIRST;
+ a | b | c | d | e
+---+--------------+---------------+--------------+-----
+ 1 | View default | Table default | View default |
+ 1 | View default | Table default | | xxx
+ 2 | View default | Table default | View default |
+ 2 | View default | Table default | | xxx
+ 3 | View default | Table default | View default |
+ 3 | View default | Table default | | xxx
+(6 rows)
+
drop view base_tab_def_view;
drop table base_tab_def;
-- Test defaults with array assignments
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
new file mode 100644
index a6aebbc..00810ce
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1717,6 +1717,16 @@ insert into base_tab_def_view values (15
insert into base_tab_def_view values (17), (default);
select * from base_tab_def order by a, c NULLS LAST;
+-- Test a DO ALSO INSERT ... SELECT rule
+drop rule base_tab_def_view_ins_rule on base_tab_def_view;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do also insert into base_tab_def (a, b, e) select new.a, new.b, 'xxx';
+truncate base_tab_def;
+insert into base_tab_def_view values (1, default, default, default, default);
+insert into base_tab_def_view values (2, default, default, default, default),
+ (3, default, default, default, default);
+select * from base_tab_def order by a, e NULLS FIRST;
+
drop view base_tab_def_view;
drop table base_tab_def;
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-21 15:05 Tom Lane <tgl@sss.pgh.pa.us>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Tom Lane @ 2023-02-21 15:05 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-21 17:15 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 17+ messages in thread
From: Dean Rasheed @ 2023-02-21 17:15 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
Attachments:
[text/x-patch] fix-default-handling-for-insert-select-rule-v2.patch (5.8K, ../../CAEZATCWoXX9-+ZaaLiOMRsZgwmStayBGmCdA0BdwVzo5GCU51Q@mail.gmail.com/2-fix-default-handling-for-insert-select-rule-v2.patch)
download | inline diff:
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index c74bac2..b6452e9
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1604,7 +1604,6 @@ rewriteValuesRTEToNulls(Query *parsetree
List *newValues;
ListCell *lc;
- Assert(rte->rtekind == RTE_VALUES);
newValues = NIL;
foreach(lc, rte->values_lists)
{
@@ -3861,12 +3860,39 @@ RewriteQuery(Query *parsetree, List *rew
/*
* Each product query has its own copy of the VALUES RTE at the
* same index in the rangetable, so we must finalize each one.
+ *
+ * Note that if the product query is an INSERT ... SELECT, then
+ * the VALUES RTE will be at the same index in the SELECT part of
+ * the product query rather than the top-level product query
+ * itself.
*/
foreach(n, product_queries)
{
Query *pt = (Query *) lfirst(n);
- RangeTblEntry *values_rte = rt_fetch(values_rte_index,
- pt->rtable);
+ RangeTblEntry *values_rte;
+
+ if (pt->commandType == CMD_INSERT &&
+ pt->jointree && IsA(pt->jointree, FromExpr) &&
+ list_length(pt->jointree->fromlist) == 1)
+ {
+ Node *jtnode = (Node *) linitial(pt->jointree->fromlist);
+
+ if (IsA(jtnode, RangeTblRef))
+ {
+ int rtindex = ((RangeTblRef *) jtnode)->rtindex;
+ RangeTblEntry *src_rte = rt_fetch(rtindex, pt->rtable);
+
+ if (src_rte->rtekind == RTE_SUBQUERY &&
+ src_rte->subquery &&
+ IsA(src_rte->subquery, Query) &&
+ src_rte->subquery->commandType == CMD_SELECT)
+ pt = src_rte->subquery;
+ }
+ }
+
+ values_rte = rt_fetch(values_rte_index, pt->rtable);
+ if (values_rte->rtekind != RTE_VALUES)
+ elog(ERROR, "failed to find VALUES RTE in product query");
rewriteValuesRTEToNulls(pt, values_rte);
}
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
new file mode 100644
index 6f51c7e..58d962a
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -1008,16 +1008,20 @@ getInsertSelectQuery(Query *parsetree, Q
strcmp(rt_fetch(PRS2_NEW_VARNO, parsetree->rtable)->eref->aliasname,
"new") == 0)
return parsetree;
- Assert(parsetree->jointree && IsA(parsetree->jointree, FromExpr));
- if (list_length(parsetree->jointree->fromlist) != 1)
+ if (!(parsetree->jointree &&
+ IsA(parsetree->jointree, FromExpr) &&
+ list_length(parsetree->jointree->fromlist) == 1))
elog(ERROR, "expected to find SELECT subquery");
rtr = (RangeTblRef *) linitial(parsetree->jointree->fromlist);
- Assert(IsA(rtr, RangeTblRef));
+ if (!IsA(rtr, RangeTblRef))
+ elog(ERROR, "expected to find SELECT subquery");
selectrte = rt_fetch(rtr->rtindex, parsetree->rtable);
- selectquery = selectrte->subquery;
- if (!(selectquery && IsA(selectquery, Query) &&
- selectquery->commandType == CMD_SELECT))
+ if (!(selectrte->rtekind == RTE_SUBQUERY &&
+ selectrte->subquery &&
+ IsA(selectrte->subquery, Query) &&
+ selectrte->subquery->commandType == CMD_SELECT))
elog(ERROR, "expected to find SELECT subquery");
+ selectquery = selectrte->subquery;
if (list_length(selectquery->rtable) >= 2 &&
strcmp(rt_fetch(PRS2_OLD_VARNO, selectquery->rtable)->eref->aliasname,
"old") == 0 &&
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
new file mode 100644
index 2b578cc..3bc38b6
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -3296,6 +3296,25 @@ select * from base_tab_def order by a, c
| View default | | View default |
(22 rows)
+-- Test a DO ALSO INSERT ... SELECT rule
+drop rule base_tab_def_view_ins_rule on base_tab_def_view;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do also insert into base_tab_def (a, b, e) select new.a, new.b, 'xxx';
+truncate base_tab_def;
+insert into base_tab_def_view values (1, default, default, default, default);
+insert into base_tab_def_view values (2, default, default, default, default),
+ (3, default, default, default, default);
+select * from base_tab_def order by a, e NULLS FIRST;
+ a | b | c | d | e
+---+--------------+---------------+--------------+-----
+ 1 | View default | Table default | View default |
+ 1 | View default | Table default | | xxx
+ 2 | View default | Table default | View default |
+ 2 | View default | Table default | | xxx
+ 3 | View default | Table default | View default |
+ 3 | View default | Table default | | xxx
+(6 rows)
+
drop view base_tab_def_view;
drop table base_tab_def;
-- Test defaults with array assignments
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
new file mode 100644
index a6aebbc..00810ce
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1717,6 +1717,16 @@ insert into base_tab_def_view values (15
insert into base_tab_def_view values (17), (default);
select * from base_tab_def order by a, c NULLS LAST;
+-- Test a DO ALSO INSERT ... SELECT rule
+drop rule base_tab_def_view_ins_rule on base_tab_def_view;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do also insert into base_tab_def (a, b, e) select new.a, new.b, 'xxx';
+truncate base_tab_def;
+insert into base_tab_def_view values (1, default, default, default, default);
+insert into base_tab_def_view values (2, default, default, default, default),
+ (3, default, default, default, default);
+select * from base_tab_def order by a, e NULLS FIRST;
+
drop view base_tab_def_view;
drop table base_tab_def;
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-21 18:02 Tom Lane <tgl@sss.pgh.pa.us>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Tom Lane @ 2023-02-21 18:02 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-22 08:00 Alexander Lakhin <exclusion@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Lakhin @ 2023-02-22 08:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-22 09:07 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Dean Rasheed @ 2023-02-22 09:07 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-22 12:00 Alexander Lakhin <exclusion@gmail.com>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Lakhin @ 2023-02-22 12:00 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-22 12:29 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Dean Rasheed @ 2023-02-22 12:29 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-23 11:00 Alexander Lakhin <exclusion@gmail.com>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Lakhin @ 2023-02-23 11:00 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-23 12:00 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Dean Rasheed @ 2023-02-23 12:00 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-23 21:50 Tom Lane <tgl@sss.pgh.pa.us>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Tom Lane @ 2023-02-23 21:50 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-23 23:20 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 17+ messages in thread
From: Dean Rasheed @ 2023-02-23 23:20 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
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
Attachments:
[text/x-patch] add-lateral-markers.patch (1.2K, ../../CAEZATCUo-GCtup4_ve89pp+3XTBirqppi8fYv16J+uxnAcA4AQ@mail.gmail.com/2-add-lateral-markers.patch)
download | inline diff:
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index b6452e9..03a2050
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -421,12 +421,26 @@ rewriteRuleAction(Query *parsetree,
{
List *rtable_tail = sub_action->rtable;
List *perminfos_tail = sub_action->rteperminfos;
+ ListCell *lc;
/*
* RewriteQuery relies on the fact that RT entries from the original
* query appear at the start of the expanded rtable, so we put the
- * action's original table at the end of the list.
+ * action's original rtable at the end of the list.
+ *
+ * If there are any subquery entries in the action's original rtable
+ * that refer to the current query level (references to NEW/OLD), then
+ * mark those RTEs as LATERAL.
*/
+ foreach(lc, sub_action->rtable)
+ {
+ RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
+
+ if (rte->rtekind == RTE_SUBQUERY &&
+ contain_vars_of_level((Node *) rte->subquery, 1))
+ rte->lateral = true;
+ }
+
sub_action->rtable = copyObject(parsetree->rtable);
sub_action->rteperminfos = copyObject(parsetree->rteperminfos);
CombineRangeTables(&sub_action->rtable, &sub_action->rteperminfos,
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-24 08:50 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Dean Rasheed @ 2023-02-24 08:50 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
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
Attachments:
[text/x-patch] fix-lateral-refs-to-old-and-new-in-rules.patch (3.7K, ../../CAEZATCVMdsLhBj8BbGT3B-Y-4HpmBW8EdOZGhYk+KG6hXngrdQ@mail.gmail.com/2-fix-lateral-refs-to-old-and-new-in-rules.patch)
download | inline diff:
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index b6452e9..aeb757c
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -353,6 +353,7 @@ rewriteRuleAction(Query *parsetree,
Query *sub_action;
Query **sub_action_ptr;
acquireLocksOnSubLinks_context context;
+ ListCell *lc;
context.for_execute = true;
@@ -392,6 +393,19 @@ rewriteRuleAction(Query *parsetree,
PRS2_OLD_VARNO + rt_length, rt_index, 0);
/*
+ * Mark any subquery RTEs in the rule action as LATERAL if they contain
+ * Vars referring to the current query level (references to NEW/OLD).
+ */
+ foreach(lc, sub_action->rtable)
+ {
+ RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
+
+ if (rte->rtekind == RTE_SUBQUERY &&
+ contain_vars_of_level((Node *) rte->subquery, 1))
+ rte->lateral = true;
+ }
+
+ /*
* Generate expanded rtable consisting of main parsetree's rtable plus
* rule action's rtable; this becomes the complete rtable for the rule
* action. Some of the entries may be unused after we finish rewriting,
@@ -439,8 +453,6 @@ rewriteRuleAction(Query *parsetree,
*/
if (parsetree->hasSubLinks && !sub_action->hasSubLinks)
{
- ListCell *lc;
-
foreach(lc, parsetree->rtable)
{
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
@@ -538,8 +550,6 @@ rewriteRuleAction(Query *parsetree,
*/
if (parsetree->cteList != NIL && sub_action->commandType != CMD_UTILITY)
{
- ListCell *lc;
-
/*
* Annoying implementation restriction: because CTEs are identified by
* name within a cteList, we can't merge a CTE from the original query
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
new file mode 100644
index a3a5a62..e953d1f
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3083,6 +3083,31 @@ Rules:
drop table rule_t1, rule_dest;
--
+-- Test implicit LATERAL references to old/new in rules
+--
+CREATE TABLE rule_t1(a int, b text DEFAULT 'xxx', c int);
+CREATE VIEW rule_v1 AS SELECT * FROM rule_t1;
+CREATE RULE v1_ins AS ON INSERT TO rule_v1
+ DO ALSO INSERT INTO rule_t1
+ SELECT * FROM (SELECT a + 10 FROM rule_t1 WHERE a = NEW.a) tt;
+CREATE RULE v1_upd AS ON UPDATE TO rule_v1
+ DO ALSO UPDATE rule_t1 t
+ SET c = tt.a * 10
+ FROM (SELECT a FROM rule_t1 WHERE a = OLD.a) tt WHERE t.a = tt.a;
+INSERT INTO rule_v1 VALUES (1, 'a'), (2, 'b');
+UPDATE rule_v1 SET b = upper(b);
+SELECT * FROM rule_t1;
+ a | b | c
+----+-----+-----
+ 1 | A | 10
+ 2 | B | 20
+ 11 | XXX | 110
+ 12 | XXX | 120
+(4 rows)
+
+DROP TABLE rule_t1 CASCADE;
+NOTICE: drop cascades to view rule_v1
+--
-- check alter rename rule
--
CREATE TABLE rule_t1 (a INT);
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
new file mode 100644
index ac1a4ce..4caab34
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1047,6 +1047,23 @@ create rule rr as on update to rule_t1 d
drop table rule_t1, rule_dest;
--
+-- Test implicit LATERAL references to old/new in rules
+--
+CREATE TABLE rule_t1(a int, b text DEFAULT 'xxx', c int);
+CREATE VIEW rule_v1 AS SELECT * FROM rule_t1;
+CREATE RULE v1_ins AS ON INSERT TO rule_v1
+ DO ALSO INSERT INTO rule_t1
+ SELECT * FROM (SELECT a + 10 FROM rule_t1 WHERE a = NEW.a) tt;
+CREATE RULE v1_upd AS ON UPDATE TO rule_v1
+ DO ALSO UPDATE rule_t1 t
+ SET c = tt.a * 10
+ FROM (SELECT a FROM rule_t1 WHERE a = OLD.a) tt WHERE t.a = tt.a;
+INSERT INTO rule_v1 VALUES (1, 'a'), (2, 'b');
+UPDATE rule_v1 SET b = upper(b);
+SELECT * FROM rule_t1;
+DROP TABLE rule_t1 CASCADE;
+
+--
-- check alter rename rule
--
CREATE TABLE rule_t1 (a INT);
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-24 16:53 Tom Lane <tgl@sss.pgh.pa.us>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Tom Lane @ 2023-02-24 16:53 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-25 15:08 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 17+ messages in thread
From: Dean Rasheed @ 2023-02-25 15:08 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alexander Lakhin <exclusion@gmail.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values
@ 2023-02-25 16:00 Alexander Lakhin <exclusion@gmail.com>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Alexander Lakhin @ 2023-02-25 16:00 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
25.02.2023 18:08, Dean Rasheed wrote:
> Pushed.
Thanks for the fix!
Best regards,
Alexander
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2023-02-25 16:00 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-21 09:00 BUG #17803: Rule "ALSO INSERT ... SELECT ..." fails to substitute default values PG Bug reporting form <noreply@postgresql.org>
2023-02-21 11:56 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2023-02-21 15:05 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-02-21 17:15 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2023-02-21 18:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-02-22 08:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-02-22 09:07 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2023-02-22 12:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-02-22 12:29 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2023-02-23 11:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-02-23 12:00 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2023-02-23 21:50 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-02-23 23:20 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2023-02-24 08:50 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2023-02-24 16:53 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-02-25 15:08 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2023-02-25 16:00 ` Alexander Lakhin <exclusion@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox