pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18634: Wrong varnullingrels with merge ... when not matched by source
9+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2024-10-03 34ae54af92 259a0a99fe Fix wrong varnullingrels error for MERGE WHEN NOT MATCHED BY SOURCE.
2024-10-03 d7d297f844 dddb5640c6 Fix incorrect non-strict join recheck in MERGE WHEN NOT MATCHED BY SOURCE.
* BUG #18634: Wrong varnullingrels with merge ... when not matched by source
@ 2024-09-26 18:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 9+ messages in thread
From: PG Bug reporting form @ 2024-09-26 18: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: 18634
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 17.0
Operating system: Ubuntu 22.04
Description:
The following script:
CREATE TABLE t (a int);
INSERT INTO t VALUES(1), (2);
CREATE VIEW v AS SELECT a FROM t WHERE EXISTS (SELECT 1 FROM t);
MERGE INTO v USING (SELECT * FROM generate_series(1,1)) AS s(a)
ON s.a = v.a WHEN NOT MATCHED BY SOURCE THEN DELETE;
produces:
ERROR: XX000: wrong varnullingrels (b) (expected (b 4)) for Var 5/1
LOCATION: search_indexed_tlist_for_var, setrefs.c:2847
At the same time,
MERGE INTO v USING (SELECT * FROM generate_series(1,1)) AS s(a)
ON s.a = v.a WHEN MATCHED THEN DELETE;
planned and executed with no error.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18634: Wrong varnullingrels with merge ... when not matched by source
@ 2024-09-26 22:41 Tom Lane <tgl@sss.pgh.pa.us>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 9+ messages in thread
From: Tom Lane @ 2024-09-26 22:41 UTC (permalink / raw)
To: exclusion@gmail.com; +Cc: pgsql-bugs@lists.postgresql.org
PG Bug reporting form <noreply@postgresql.org> writes:
> The following script:
> CREATE TABLE t (a int);
> INSERT INTO t VALUES(1), (2);
> CREATE VIEW v AS SELECT a FROM t WHERE EXISTS (SELECT 1 FROM t);
> MERGE INTO v USING (SELECT * FROM generate_series(1,1)) AS s(a)
> ON s.a = v.a WHEN NOT MATCHED BY SOURCE THEN DELETE;
> produces:
> ERROR: XX000: wrong varnullingrels (b) (expected (b 4)) for Var 5/1
> LOCATION: search_indexed_tlist_for_var, setrefs.c:2847
I haven't run this fully to ground, but what it looks like
is that preprocess_targetlist is generating row identity
Vars that lack required varnullingrels. I don't understand
though why this only seems to affect MERGE.
regards, tom lane
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18634: Wrong varnullingrels with merge ... when not matched by source
@ 2024-09-27 12:52 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 9+ messages in thread
From: Richard Guo @ 2024-09-27 12:52 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Sep 27, 2024 at 6:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
> > The following script:
> > CREATE TABLE t (a int);
> > INSERT INTO t VALUES(1), (2);
> > CREATE VIEW v AS SELECT a FROM t WHERE EXISTS (SELECT 1 FROM t);
>
> > MERGE INTO v USING (SELECT * FROM generate_series(1,1)) AS s(a)
> > ON s.a = v.a WHEN NOT MATCHED BY SOURCE THEN DELETE;
>
> > produces:
> > ERROR: XX000: wrong varnullingrels (b) (expected (b 4)) for Var 5/1
> > LOCATION: search_indexed_tlist_for_var, setrefs.c:2847
>
> I haven't run this fully to ground, but what it looks like
> is that preprocess_targetlist is generating row identity
> Vars that lack required varnullingrels. I don't understand
> though why this only seems to affect MERGE.
It looks like that preprocess_targetlist will add any vars used in
parse->mergeJoinCondition that belong to the source relation to the
processed tlist. This logic was introduced to support WHEN NOT
MATCHED BY SOURCE actions (see 0294df2f1). For such actions, the
source relation is on the nullable side of the outer join. But when
adding the vars used in the join condition that belong to source
relation to the tlist, we fail to mark them as nullable by the join.
I think we can check the jointype of the join between the target and
the source relation when adding the vars in mergeJoinCondition. If it
is JOIN_LEFT, we mark the vars that belong to source as nullable by
this join.
With this routine, ISTM we'd need a way for preprocess_targetlist to
access the JoinExpr that we build in transform_MERGE_to_join.
Thanks
Richard
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18634: Wrong varnullingrels with merge ... when not matched by source
@ 2024-09-27 15:06 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Dean Rasheed @ 2024-09-27 15:06 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, 27 Sept 2024 at 13:52, Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Fri, Sep 27, 2024 at 6:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > I haven't run this fully to ground, but what it looks like
> > is that preprocess_targetlist is generating row identity
> > Vars that lack required varnullingrels. I don't understand
> > though why this only seems to affect MERGE.
>
> It looks like that preprocess_targetlist will add any vars used in
> parse->mergeJoinCondition that belong to the source relation to the
> processed tlist. This logic was introduced to support WHEN NOT
> MATCHED BY SOURCE actions (see 0294df2f1). For such actions, the
> source relation is on the nullable side of the outer join. But when
> adding the vars used in the join condition that belong to source
> relation to the tlist, we fail to mark them as nullable by the join.
>
Yes, I reached the same conclusion.
The same goes for Vars in merge action quals and targetlists, and the
RETURNING list, but not the ones added for rowmarks, if I'm
understanding it correctly.
> I think we can check the jointype of the join between the target and
> the source relation when adding the vars in mergeJoinCondition. If it
> is JOIN_LEFT, we mark the vars that belong to source as nullable by
> this join.
>
> With this routine, ISTM we'd need a way for preprocess_targetlist to
> access the JoinExpr that we build in transform_MERGE_to_join.
>
Another option is to do it in transform_MERGE_to_join(). That feels
safer, because the jointree might have been modified by the time we
reach preprocess_targetlist().
Something like the attached.
Regards,
Dean
Attachments:
bug18634-fix.patch (text/x-patch, 3.7K) download
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
new file mode 100644
index a704045..322d511
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -257,6 +257,44 @@ transform_MERGE_to_join(Query *parse)
bms_make_singleton(joinrti));
/*
+ * Similarly, any non-target Vars in the join condition will be added to
+ * the targetlist by preprocess_targetlist(), and so must be marked as
+ * nullable by the join, for LEFT and FULL joins. The same goes for Vars
+ * in action quals, action targetlists and the RETURNING list.
+ */
+ if (jointype == JOIN_LEFT || jointype == JOIN_FULL)
+ {
+ Bitmapset *nontargetrels = NULL;
+
+ for (int i = 1; i < joinrti; i++)
+ if (i != parse->mergeTargetRelation)
+ nontargetrels = bms_add_member(nontargetrels, i);
+
+ parse->mergeJoinCondition =
+ add_nulling_relids(parse->mergeJoinCondition,
+ nontargetrels,
+ bms_make_singleton(joinrti));
+
+ foreach_node(MergeAction, action, parse->mergeActionList)
+ {
+ action->qual =
+ add_nulling_relids(action->qual,
+ nontargetrels,
+ bms_make_singleton(joinrti));
+
+ action->targetList = (List *)
+ add_nulling_relids((Node *) action->targetList,
+ nontargetrels,
+ bms_make_singleton(joinrti));
+ }
+
+ parse->returningList = (List *)
+ add_nulling_relids((Node *) parse->returningList,
+ nontargetrels,
+ bms_make_singleton(joinrti));
+ }
+
+ /*
* If there are any WHEN NOT MATCHED BY SOURCE actions, the executor will
* use the join condition to distinguish between MATCHED and NOT MATCHED
* BY SOURCE cases. Otherwise, it's no longer needed, and we set it to
diff --git a/src/test/regress/expected/merge.out b/src/test/regress/expected/merge.out
new file mode 100644
index 3d33259..ff0e167
--- a/src/test/regress/expected/merge.out
+++ b/src/test/regress/expected/merge.out
@@ -2689,6 +2689,27 @@ DETAIL: drop cascades to table measurem
drop cascades to table measurement_y2006m03
drop cascades to table measurement_y2007m01
DROP FUNCTION measurement_insert_trigger();
+--
+-- test for bug #18634
+--
+CREATE TABLE bug18634t (a int, b int);
+INSERT INTO bug18634t VALUES(1, 10), (2, 20);
+CREATE VIEW bug18634v AS
+ SELECT * FROM bug18634t WHERE EXISTS (SELECT 1 FROM bug18634t);
+CREATE TABLE bug18634s (a int, b int);
+INSERT INTO bug18634s VALUES (1, 2);
+MERGE INTO bug18634v t USING bug18634s s ON s.a = t.a
+WHEN MATCHED THEN UPDATE SET b = s.b
+WHEN NOT MATCHED BY SOURCE THEN DELETE;
+SELECT * FROM bug18634t;
+ a | b
+---+---
+ 1 | 2
+(1 row)
+
+DROP TABLE bug18634t CASCADE;
+NOTICE: drop cascades to view bug18634v
+DROP TABLE bug18634s;
-- prepare
RESET SESSION AUTHORIZATION;
-- try a system catalog
diff --git a/src/test/regress/sql/merge.sql b/src/test/regress/sql/merge.sql
new file mode 100644
index 92163ec..020d22e
--- a/src/test/regress/sql/merge.sql
+++ b/src/test/regress/sql/merge.sql
@@ -1710,6 +1710,26 @@ SELECT * FROM new_measurement ORDER BY c
DROP TABLE measurement, new_measurement CASCADE;
DROP FUNCTION measurement_insert_trigger();
+--
+-- test for bug #18634
+--
+CREATE TABLE bug18634t (a int, b int);
+INSERT INTO bug18634t VALUES(1, 10), (2, 20);
+CREATE VIEW bug18634v AS
+ SELECT * FROM bug18634t WHERE EXISTS (SELECT 1 FROM bug18634t);
+
+CREATE TABLE bug18634s (a int, b int);
+INSERT INTO bug18634s VALUES (1, 2);
+
+MERGE INTO bug18634v t USING bug18634s s ON s.a = t.a
+WHEN MATCHED THEN UPDATE SET b = s.b
+WHEN NOT MATCHED BY SOURCE THEN DELETE;
+
+SELECT * FROM bug18634t;
+
+DROP TABLE bug18634t CASCADE;
+DROP TABLE bug18634s;
+
-- prepare
RESET SESSION AUTHORIZATION;
* Re: BUG #18634: Wrong varnullingrels with merge ... when not matched by source
@ 2024-09-27 15:43 Tom Lane <tgl@sss.pgh.pa.us>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Tom Lane @ 2024-09-27 15:43 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Richard Guo <guofenglinux@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Fri, 27 Sept 2024 at 13:52, Richard Guo <guofenglinux@gmail.com> wrote:
>> I think we can check the jointype of the join between the target and
>> the source relation when adding the vars in mergeJoinCondition. If it
>> is JOIN_LEFT, we mark the vars that belong to source as nullable by
>> this join.
> Another option is to do it in transform_MERGE_to_join(). That feels
> safer, because the jointree might have been modified by the time we
> reach preprocess_targetlist().
Yeah, I think it's critical that these Vars be already correctly
marked before we engage in all the slicing-and-dicing that
prepjointree et al will do. As an example, it seems not impossible
for join removal to make wrong decisions if they aren't.
> Something like the attached.
Could use some comments ... but actually, now I'm confused about why
any of this is the right thing at all:
+ * Similarly, any non-target Vars in the join condition will be added to
+ * the targetlist by preprocess_targetlist(), and so must be marked as
+ * nullable by the join, for LEFT and FULL joins.
Why do we need these Vars in the tlist? If they're for re-evaluating
the join condition, isn't the already-nulled form of them the wrong
thing?
regards, tom lane
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18634: Wrong varnullingrels with merge ... when not matched by source
@ 2024-09-28 00:39 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 9+ messages in thread
From: Richard Guo @ 2024-09-28 00:39 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Dean Rasheed <dean.a.rasheed@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Sep 27, 2024 at 11:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Could use some comments ... but actually, now I'm confused about why
> any of this is the right thing at all:
>
> + * Similarly, any non-target Vars in the join condition will be added to
> + * the targetlist by preprocess_targetlist(), and so must be marked as
> + * nullable by the join, for LEFT and FULL joins.
>
> Why do we need these Vars in the tlist? If they're for re-evaluating
> the join condition, isn't the already-nulled form of them the wrong
> thing?
I have the same concern. I think we should NOT mark the vars in
mergeJoinCondition as nullable, as mergeJoinCondition acts as join
quals rather than filter quals at that outer join. Instead, we should
mark them nullable when they are pulled out and ready to be added to
the targetlist, if they are really needed in the targetlist.
Thanks
Richard
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18634: Wrong varnullingrels with merge ... when not matched by source
@ 2024-09-29 07:49 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Dean Rasheed @ 2024-09-29 07:49 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, 28 Sept 2024 at 01:40, Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Fri, Sep 27, 2024 at 11:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Could use some comments ... but actually, now I'm confused about why
> > any of this is the right thing at all:
> >
> > + * Similarly, any non-target Vars in the join condition will be added to
> > + * the targetlist by preprocess_targetlist(), and so must be marked as
> > + * nullable by the join, for LEFT and FULL joins.
> >
> > Why do we need these Vars in the tlist? If they're for re-evaluating
> > the join condition, isn't the already-nulled form of them the wrong
> > thing?
>
Doh. Yes, that's broken. A previous version of the WHEN NOT MATCHED BY
SOURCE patch used a qual of the form "src IS [NOT] NULL" to
distinguish between the MATCHED and NOT MATCHED BY SOURCE cases, but I
changed that to use the merge join condition as part of a larger
refactoring [1], which was a mistake.
[1] https://www.postgresql.org/message-id/CAEZATCV-7j0wq6T2AGsyRbaVY5muZ8KJ07L-%3D8Pvt%3Da3w1V5vA%40mail...
A simple case to demonstrate the problem is this:
CREATE TABLE src (a int, b text);
INSERT INTO src VALUES (1, 'src row');
CREATE TABLE tgt (a int, b text);
INSERT INTO tgt VALUES (NULL, 'tgt row');
MERGE INTO tgt USING src ON tgt.a IS NOT DISTINCT FROM src.a
WHEN MATCHED THEN UPDATE SET a = src.a, b = src.b
WHEN NOT MATCHED BY SOURCE THEN DELETE;
SELECT * FROM tgt;
a | b
---+---
|
(1 row)
which is wrong (it executes the MATCHED UPDATE action instead of the
NOT MATCHED BY SOURCE DELETE action because src.a is NULL above the
join).
The simplest fix is to add a "src IS NOT NULL" wholerow check to the
executor recheck condition, similar to the prior implementation, which
I've done in the v2 patch, attached.
A slightly better fix would be to *replace* the executor check with
that IS NOT NULL test, and not actually recheck the original join
condition at all. That's already sufficient for the initial check at
the top of ExecMergeMatched(), and it would only require a minor
change to the EPQ rechecking further down to make it work fully. It
would, however, make the field names wrong/misleading, so probably
they would need updating if we did that, making it unsuitable for
back-patching. Since this is really only an optimisation, and it's not
clear how much difference it would actually make anyway, I'm inclined
to leave it as a possible future enhancement.
> I have the same concern. I think we should NOT mark the vars in
> mergeJoinCondition as nullable, as mergeJoinCondition acts as join
> quals rather than filter quals at that outer join. Instead, we should
> mark them nullable when they are pulled out and ready to be added to
> the targetlist, if they are really needed in the targetlist.
>
Actually, the marking is done after building the join node, so it's
only marking a copy of the join condition, for use above the join. The
original condition inside the join node remains unmarked, so I think
it's right.
For the sake of the archives, this "wrong varnullingrels" error is not
limited to the join condition. With the same table/view definitions as
in the original report, it can also be reproduced with queries like
MERGE INTO v USING (SELECT * FROM generate_series(1,1)) AS s(a) ON true
WHEN MATCHED THEN UPDATE SET a = s.a
WHEN NOT MATCHED BY SOURCE THEN DELETE;
and
MERGE INTO v USING (SELECT * FROM generate_series(1,1)) AS s(a) ON true
WHEN NOT MATCHED BY SOURCE THEN DELETE
RETURNING s.a;
Basically, anything in the ModifyTable node that accesses source
relation Vars, if the source relation is on the outer side of the
join.
I spent some time trying to figure out why none of the existing tests
hit this error, and I think the reason is that all the previous tests
involved a plan where the ModifyTable node is directly on top of the
join node, so the top targetlist was the join node's targetlist, and
therefore wasn't marked. But in the example here, there is a one-time
filter Result node between the ModifyTable node and the join node,
which means the ModifyTable node pulls from the Result node, whose
output is marked as nullable, because it's above the join. That makes
the error somewhat rare, though maybe there are other cases that can
lead to a plan node being inserted between the ModifyTable node and
the join node.
It feels a bit unsatisfactory that this wasn't detectable with a
ModifyTable node directly on top of the join node, making the bug hard
to spot, but I don't know whether it would be feasible to do anything
about that.
I have split this into 2 commits, since it's really 2 separate bugs,
and tried to improve the commentary.
Regards,
Dean
Attachments: download all (2 files, .tar.gz)
v2-0001-Fix-incorrect-non-strict-join-recheck-in-MERGE-WH.patch (text/x-patch, 6.5K) download
From f587f0bc67995f4f9919d35180e2281efc7ddd4e Mon Sep 17 00:00:00 2001
From: Dean Rasheed <dean.a.rasheed@gmail.com>
Date: Sat, 28 Sep 2024 17:20:38 +0100
Subject: [PATCH v2 1/2] Fix incorrect non-strict join recheck in MERGE WHEN
NOT MATCHED BY SOURCE.
If a MERGE command contains WHEN NOT MATCHED BY SOURCE actions, the
merge join condition is used by the executor to distinguish MATCHED
from NOT MATCHED BY SOURCE cases. However, this qual is executed using
the output of the join subplan node, which nulls the output from the
source relation in the not matched case, and so the result may be
incorrect if the join condition is "non-strict" -- for example,
something like "src.col IS NOT DISTINCT FROM tgt.col".
Fix this by enhancing the join condition with an additional "src IS
NOT NULL" check, so that it does the right thing when evaluated using
the output of the join subplan.
---
src/backend/optimizer/prep/prepjointree.c | 66 +++++++++++++++++++++--
src/test/regress/expected/merge.out | 22 ++++++++
src/test/regress/sql/merge.sql | 18 +++++++
3 files changed, 101 insertions(+), 5 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index a70404558f..ca15994fdc 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -158,6 +158,9 @@ transform_MERGE_to_join(Query *parse)
int joinrti;
List *vars;
RangeTblRef *rtr;
+ FromExpr *target;
+ Node *source;
+ int sourcerti;
if (parse->commandType != CMD_MERGE)
return;
@@ -226,13 +229,36 @@ transform_MERGE_to_join(Query *parse)
* parse->jointree->quals are restrictions on the target relation (if the
* target relation is an auto-updatable view).
*/
+ /* target rel, with any quals */
rtr = makeNode(RangeTblRef);
rtr->rtindex = parse->mergeTargetRelation;
+ target = makeFromExpr(list_make1(rtr), parse->jointree->quals);
+
+ /* source rel (expect exactly one -- see transformMergeStmt()) */
+ Assert(list_length(parse->jointree->fromlist) == 1);
+ source = linitial(parse->jointree->fromlist);
+
+ /*
+ * index of source rel (expect either a RangeTblRef or a JoinExpr -- see
+ * transformFromClauseItem()).
+ */
+ if (IsA(source, RangeTblRef))
+ sourcerti = ((RangeTblRef *) source)->rtindex;
+ else if (IsA(source, JoinExpr))
+ sourcerti = ((JoinExpr *) source)->rtindex;
+ else
+ {
+ elog(ERROR, "unrecognized source node type: %d",
+ (int) nodeTag(source));
+ sourcerti = 0; /* keep compiler quiet */
+ }
+
+ /* Join the source and target */
joinexpr = makeNode(JoinExpr);
joinexpr->jointype = jointype;
joinexpr->isNatural = false;
- joinexpr->larg = (Node *) makeFromExpr(list_make1(rtr), parse->jointree->quals);
- joinexpr->rarg = linitial(parse->jointree->fromlist); /* source rel */
+ joinexpr->larg = (Node *) target;
+ joinexpr->rarg = source;
joinexpr->usingClause = NIL;
joinexpr->join_using_alias = NULL;
joinexpr->quals = parse->mergeJoinCondition;
@@ -261,9 +287,39 @@ transform_MERGE_to_join(Query *parse)
* use the join condition to distinguish between MATCHED and NOT MATCHED
* BY SOURCE cases. Otherwise, it's no longer needed, and we set it to
* NULL, saving cycles during planning and execution.
- */
- if (!have_action[MERGE_WHEN_NOT_MATCHED_BY_SOURCE])
- parse->mergeJoinCondition = NULL;
+ *
+ * We need to be careful though: the executor evaluates this condition
+ * using the output of the join subplan node, which nulls the output from
+ * the source relation when the join condition doesn't match. That risks
+ * producing incorrect results when rechecking using a "non-strict" join
+ * condition, such as "src.col IS NOT DISTINCT FROM tgt.col". To guard
+ * against that, we add an additional "src IS NOT NULL" check to the join
+ * condition, so that it does the right thing when performing a recheck
+ * based on the output of the join subplan.
+ */
+ if (have_action[MERGE_WHEN_NOT_MATCHED_BY_SOURCE])
+ {
+ Var *var;
+ NullTest *ntest;
+
+ /* source wholerow Var (nullable by the new join) */
+ var = makeWholeRowVar(rt_fetch(sourcerti, parse->rtable),
+ sourcerti, 0, false);
+ var->varnullingrels = bms_make_singleton(joinrti);
+
+ /* "src IS NOT NULL" check */
+ ntest = makeNode(NullTest);
+ ntest->arg = (Expr *) var;
+ ntest->nulltesttype = IS_NOT_NULL;
+ ntest->argisrow = false;
+ ntest->location = -1;
+
+ /* combine it with the original join condition */
+ parse->mergeJoinCondition =
+ (Node *) make_and_qual((Node *) ntest, parse->mergeJoinCondition);
+ }
+ else
+ parse->mergeJoinCondition = NULL; /* join condition not needed */
}
/*
diff --git a/src/test/regress/expected/merge.out b/src/test/regress/expected/merge.out
index 3d33259e8f..0e59bae1a7 100644
--- a/src/test/regress/expected/merge.out
+++ b/src/test/regress/expected/merge.out
@@ -2689,6 +2689,28 @@ DETAIL: drop cascades to table measurement_y2006m02
drop cascades to table measurement_y2006m03
drop cascades to table measurement_y2007m01
DROP FUNCTION measurement_insert_trigger();
+--
+-- test non-strict join clause
+--
+CREATE TABLE src (a int, b text);
+INSERT INTO src VALUES (1, 'src row');
+CREATE TABLE tgt (a int, b text);
+INSERT INTO tgt VALUES (NULL, 'tgt row');
+MERGE INTO tgt USING src ON tgt.a IS NOT DISTINCT FROM src.a
+ WHEN MATCHED THEN UPDATE SET a = src.a, b = src.b
+ WHEN NOT MATCHED BY SOURCE THEN DELETE
+ RETURNING merge_action(), src.*, tgt.*;
+ merge_action | a | b | a | b
+--------------+---+---+---+---------
+ DELETE | | | | tgt row
+(1 row)
+
+SELECT * FROM tgt;
+ a | b
+---+---
+(0 rows)
+
+DROP TABLE src, tgt;
-- prepare
RESET SESSION AUTHORIZATION;
-- try a system catalog
diff --git a/src/test/regress/sql/merge.sql b/src/test/regress/sql/merge.sql
index 92163ec9fe..2a7753c65b 100644
--- a/src/test/regress/sql/merge.sql
+++ b/src/test/regress/sql/merge.sql
@@ -1710,6 +1710,24 @@ SELECT * FROM new_measurement ORDER BY city_id, logdate;
DROP TABLE measurement, new_measurement CASCADE;
DROP FUNCTION measurement_insert_trigger();
+--
+-- test non-strict join clause
+--
+CREATE TABLE src (a int, b text);
+INSERT INTO src VALUES (1, 'src row');
+
+CREATE TABLE tgt (a int, b text);
+INSERT INTO tgt VALUES (NULL, 'tgt row');
+
+MERGE INTO tgt USING src ON tgt.a IS NOT DISTINCT FROM src.a
+ WHEN MATCHED THEN UPDATE SET a = src.a, b = src.b
+ WHEN NOT MATCHED BY SOURCE THEN DELETE
+ RETURNING merge_action(), src.*, tgt.*;
+
+SELECT * FROM tgt;
+
+DROP TABLE src, tgt;
+
-- prepare
RESET SESSION AUTHORIZATION;
--
2.43.0
v2-0002-Fix-varnullingrels-markings-for-MERGE-WHEN-NOT-MA.patch (text/x-patch, 5.7K) download
From 306f52996ea0adab560c4f201958fc253721428f Mon Sep 17 00:00:00 2001
From: Dean Rasheed <dean.a.rasheed@gmail.com>
Date: Sat, 28 Sep 2024 20:07:28 +0100
Subject: [PATCH v2 2/2] Fix varnullingrels markings for MERGE WHEN NOT MATCHED
BY SOURCE.
If a MERGE command contains WHEN NOT MATCHED BY SOURCE actions, the
source relation appears on the outer side of the join. Thus, any Vars
referring to the source in the merge join condition, actions, and
RETURNING list should be marked as nullable by the join, since they
are used in the ModifyTable node above the join. Note that this only
applies to the copy of join condition used in the executor to
distinguish MATCHED from NOT MATCHED BY SOURCE cases. The original
join condition inside the join node itself should not be modified.
Failure to do this led to a "wrong varnullingrels" error in the final
stage of query planning, in some circumstances. We happened to get
away without this in all previous tests, since they all involved a
ModifyTable node immediately above the join node, so that the top plan
targetlist coincided with the output of the join, and wasn't marked as
nullable. However, if another plan node, such as a one-time filter
Result node, gets inserted between the ModifyTable node and the join
node, then that changes.
---
src/backend/optimizer/prep/prepjointree.c | 39 +++++++++++++++++++++++
src/test/regress/expected/merge.out | 29 +++++++++++++++++
src/test/regress/sql/merge.sql | 21 ++++++++++++
3 files changed, 89 insertions(+)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index ca15994fdc..4d7f972caf 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -282,6 +282,45 @@ transform_MERGE_to_join(Query *parse)
bms_make_singleton(parse->mergeTargetRelation),
bms_make_singleton(joinrti));
+ /*
+ * If the source relation is on the outer side of the join, mark any
+ * source relation Vars in the join condition, actions, and RETURNING list
+ * as nullable by the join. These Vars will be added to the targetlist by
+ * preprocess_targetlist(), so it's important to mark them correctly here.
+ *
+ * It might seem that this is not necessary for Vars in the join
+ * condition, since it is inside the join, but it is also needed above the
+ * join (in the ModifyTable node) to distinguish between the MATCHED and
+ * NOT MATCHED BY SOURCE cases -- see ExecMergeMatched(). Note that this
+ * creates a modified copy of the join condition, for use above the join,
+ * without modifying the the original join condition, inside the join.
+ */
+ if (jointype == JOIN_LEFT || jointype == JOIN_FULL)
+ {
+ parse->mergeJoinCondition =
+ add_nulling_relids(parse->mergeJoinCondition,
+ bms_make_singleton(sourcerti),
+ bms_make_singleton(joinrti));
+
+ foreach_node(MergeAction, action, parse->mergeActionList)
+ {
+ action->qual =
+ add_nulling_relids(action->qual,
+ bms_make_singleton(sourcerti),
+ bms_make_singleton(joinrti));
+
+ action->targetList = (List *)
+ add_nulling_relids((Node *) action->targetList,
+ bms_make_singleton(sourcerti),
+ bms_make_singleton(joinrti));
+ }
+
+ parse->returningList = (List *)
+ add_nulling_relids((Node *) parse->returningList,
+ bms_make_singleton(sourcerti),
+ bms_make_singleton(joinrti));
+ }
+
/*
* If there are any WHEN NOT MATCHED BY SOURCE actions, the executor will
* use the join condition to distinguish between MATCHED and NOT MATCHED
diff --git a/src/test/regress/expected/merge.out b/src/test/regress/expected/merge.out
index 0e59bae1a7..c236f155c0 100644
--- a/src/test/regress/expected/merge.out
+++ b/src/test/regress/expected/merge.out
@@ -2711,6 +2711,35 @@ SELECT * FROM tgt;
(0 rows)
DROP TABLE src, tgt;
+
+--
+-- test for bug #18634 (wrong varnullingrels error)
+--
+CREATE TABLE bug18634t (a int, b int, c text);
+INSERT INTO bug18634t VALUES(1, 10, 'tgt1'), (2, 20, 'tgt2');
+CREATE VIEW bug18634v AS
+ SELECT * FROM bug18634t WHERE EXISTS (SELECT 1 FROM bug18634t);
+CREATE TABLE bug18634s (a int, b int, c text);
+INSERT INTO bug18634s VALUES (1, 2, 'src1');
+MERGE INTO bug18634v t USING bug18634s s ON s.a = t.a
+ WHEN MATCHED THEN UPDATE SET b = s.b
+ WHEN NOT MATCHED BY SOURCE THEN DELETE
+ RETURNING merge_action(), s.c, t.*;
+ merge_action | c | a | b | c
+--------------+------+---+----+------
+ UPDATE | src1 | 1 | 2 | tgt1
+ DELETE | | 2 | 20 | tgt2
+(2 rows)
+
+SELECT * FROM bug18634t;
+ a | b | c
+---+---+------
+ 1 | 2 | tgt1
+(1 row)
+
+DROP TABLE bug18634t CASCADE;
+NOTICE: drop cascades to view bug18634v
+DROP TABLE bug18634s;
-- prepare
RESET SESSION AUTHORIZATION;
-- try a system catalog
diff --git a/src/test/regress/sql/merge.sql b/src/test/regress/sql/merge.sql
index 2a7753c65b..ce9981d70b 100644
--- a/src/test/regress/sql/merge.sql
+++ b/src/test/regress/sql/merge.sql
@@ -1727,6 +1727,27 @@ MERGE INTO tgt USING src ON tgt.a IS NOT DISTINCT FROM src.a
SELECT * FROM tgt;
DROP TABLE src, tgt;
+
+--
+-- test for bug #18634 (wrong varnullingrels error)
+--
+CREATE TABLE bug18634t (a int, b int, c text);
+INSERT INTO bug18634t VALUES(1, 10, 'tgt1'), (2, 20, 'tgt2');
+CREATE VIEW bug18634v AS
+ SELECT * FROM bug18634t WHERE EXISTS (SELECT 1 FROM bug18634t);
+
+CREATE TABLE bug18634s (a int, b int, c text);
+INSERT INTO bug18634s VALUES (1, 2, 'src1');
+
+MERGE INTO bug18634v t USING bug18634s s ON s.a = t.a
+ WHEN MATCHED THEN UPDATE SET b = s.b
+ WHEN NOT MATCHED BY SOURCE THEN DELETE
+ RETURNING merge_action(), s.c, t.*;
+
+SELECT * FROM bug18634t;
+
+DROP TABLE bug18634t CASCADE;
+DROP TABLE bug18634s;
-- prepare
--
2.43.0
* Re: BUG #18634: Wrong varnullingrels with merge ... when not matched by source
@ 2024-09-30 02:16 Richard Guo <guofenglinux@gmail.com>
parent: Dean Rasheed <dean.a.rasheed@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Richard Guo @ 2024-09-30 02:16 UTC (permalink / raw)
To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Sun, Sep 29, 2024 at 3:49 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On Sat, 28 Sept 2024 at 01:40, Richard Guo <guofenglinux@gmail.com> wrote:
> > I have the same concern. I think we should NOT mark the vars in
> > mergeJoinCondition as nullable, as mergeJoinCondition acts as join
> > quals rather than filter quals at that outer join. Instead, we should
> > mark them nullable when they are pulled out and ready to be added to
> > the targetlist, if they are really needed in the targetlist.
>
> Actually, the marking is done after building the join node, so it's
> only marking a copy of the join condition, for use above the join. The
> original condition inside the join node remains unmarked, so I think
> it's right.
Ah yes, you are right. I thought the join node we build for MERGE
would use the marked mergeJoinCondition as join quals, but that's not
the case.
> I spent some time trying to figure out why none of the existing tests
> hit this error, and I think the reason is that all the previous tests
> involved a plan where the ModifyTable node is directly on top of the
> join node, so the top targetlist was the join node's targetlist, and
> therefore wasn't marked. But in the example here, there is a one-time
> filter Result node between the ModifyTable node and the join node,
> which means the ModifyTable node pulls from the Result node, whose
> output is marked as nullable, because it's above the join. That makes
> the error somewhat rare, though maybe there are other cases that can
> lead to a plan node being inserted between the ModifyTable node and
> the join node.
>
> It feels a bit unsatisfactory that this wasn't detectable with a
> ModifyTable node directly on top of the join node, making the bug hard
> to spot, but I don't know whether it would be feasible to do anything
> about that.
For an outer join, any vars appearing in its targetlist (and qpqual)
should be marked nullable if they are from the nullable side, because
they are logically above the join. However, when we fix up the
targetlist and qpqual, we don't have enough info available to identify
the nullingrel bits added by the outer join. So we have to use
superset matches rather than exact matches.
This is why we don't hit this error in cases where the ModifyTable
node is directly on top of the join node, even though we fail to mark
the vars in targetlist correctly.
In Alexander's case, there is a Result node in between. When we fix
up the targetlist for the Result node, we perform exact matches for
the nullingrel bits. That's how this issue is revealed.
Yeah, it's a bit unsatisfying that we can only perform superset
matches rather than exact matches for the Vars in the targetlist and
qpqual of an outer join. Maybe we can record the ojrelid of the outer
join in Join node, and then match these Vars with input Vars'
nullingrels plus this ojrelid. But when the outer joins are
re-ordered according to identity 3, it becomes very tricky to figure
out what the correct ojrelid(s) we should use for the outer join.
Thanks
Richard
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #18634: Wrong varnullingrels with merge ... when not matched by source
@ 2024-10-03 13:17 Dean Rasheed <dean.a.rasheed@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 0 replies; 9+ messages in thread
From: Dean Rasheed @ 2024-10-03 13:17 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, 30 Sept 2024 at 03:16, Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Sun, Sep 29, 2024 at 3:49 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> > I spent some time trying to figure out why none of the existing tests
> > hit this error, and I think the reason is that all the previous tests
> > involved a plan where the ModifyTable node is directly on top of the
> > join node, so the top targetlist was the join node's targetlist, and
> > therefore wasn't marked. But in the example here, there is a one-time
> > filter Result node between the ModifyTable node and the join node,
> > which means the ModifyTable node pulls from the Result node, whose
> > output is marked as nullable, because it's above the join. That makes
> > the error somewhat rare, though maybe there are other cases that can
> > lead to a plan node being inserted between the ModifyTable node and
> > the join node.
> >
> > It feels a bit unsatisfactory that this wasn't detectable with a
> > ModifyTable node directly on top of the join node, making the bug hard
> > to spot, but I don't know whether it would be feasible to do anything
> > about that.
>
> For an outer join, any vars appearing in its targetlist (and qpqual)
> should be marked nullable if they are from the nullable side, because
> they are logically above the join. However, when we fix up the
> targetlist and qpqual, we don't have enough info available to identify
> the nullingrel bits added by the outer join. So we have to use
> superset matches rather than exact matches.
>
> This is why we don't hit this error in cases where the ModifyTable
> node is directly on top of the join node, even though we fail to mark
> the vars in targetlist correctly.
>
OK, that makes sense, I think.
I have pushed and back-patched both fixes.
Regards,
Dean
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2024-10-03 13:17 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-09-26 18:00 BUG #18634: Wrong varnullingrels with merge ... when not matched by source PG Bug reporting form <noreply@postgresql.org>
2024-09-26 22:41 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-09-27 12:52 ` Richard Guo <guofenglinux@gmail.com>
2024-09-27 15:06 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2024-09-27 15:43 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-09-28 00:39 ` Richard Guo <guofenglinux@gmail.com>
2024-09-29 07:49 ` Dean Rasheed <dean.a.rasheed@gmail.com>
2024-09-30 02:16 ` Richard Guo <guofenglinux@gmail.com>
2024-10-03 13:17 ` Dean Rasheed <dean.a.rasheed@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