MERGE issues around inheritance
Hi,
In [1]/messages/by-id/smj2mopf4t654xinuyyq4azik2d5zp2g3lsv2o7vficegqq5c5@v35iq4llqjpw I added some verification to projection building, to check if the
tupleslot passed to ExecBuildProjectionInfo() is compatible with the target
list. One query in merge.sql [2]MERGE into measurement m USING new_measurement nm ON (m.city_id = nm.city_id and m.logdate=nm.logdate) WHEN MATCHED AND nm.peaktemp IS NULL THEN DELETE WHEN MATCHED THEN UPDATE SET peaktemp = greatest(m.peaktemp, nm.peaktemp), unitsales = m.unitsales + coalesce(nm.unitsales, 0) WHEN NOT MATCHED THEN INSERT (city_id, logdate, peaktemp, unitsales) VALUES (city_id, logdate, peaktemp, unitsales); got flagged.
Trying to debug that issue, I found another problem. This leaves us with:
1) w/ inheritance INSERTed rows are not returned by RETURNING. This seems to
just generally not work with MERGE
2) w/ inheritance the projection for insert that ExecInitMerge() builds
sometimes uses mismatching columns between the slot passed to
ExecBuildProjectionInfo() and tgtslot. The only reason this doesn't seem
to immediately crash is 1).
E.g. the attached repro-merge-inheritance.sql triggers [3]2025-05-21 11:24:55.111 EDT [1838296][client backend][0/16:0][psql] WARNING: type mismatch: resno 1: slot (type: 1700, name: dropme) vs expr (type: 25, name: key) 2025-05-21 11:24:55.112 EDT [1838296][client backend][0/16:0][psql] WARNING: TargetEntry: 2025-05-21 11:24:55.112 EDT [1838296][client backend][0/16:0][psql] DETAIL: {TARGETENTRY :expr {VAR :varno -1 :varattno 1 :vartype 25 :vartypmod -1 :varcollid 100 :varnullingrels (b) :varlevelsup 0 :varreturningtype 0 :varnosyn 2 :varattnosyn 1 :location 179 } :resno 1 :resname key :ressortgroupref 0 :resorigtbl 0 :resorigcol 0 :resjunk false } if the attached
debugging patch is applied.
I wouldn't be surprised if there were other issues around inheritance with
mismatched column (order), but I didn't look further.
Greetings,
Andres Freund
[1]: /messages/by-id/smj2mopf4t654xinuyyq4azik2d5zp2g3lsv2o7vficegqq5c5@v35iq4llqjpw
[2]: MERGE into measurement m USING new_measurement nm ON (m.city_id = nm.city_id and m.logdate=nm.logdate) WHEN MATCHED AND nm.peaktemp IS NULL THEN DELETE WHEN MATCHED THEN UPDATE SET peaktemp = greatest(m.peaktemp, nm.peaktemp), unitsales = m.unitsales + coalesce(nm.unitsales, 0) WHEN NOT MATCHED THEN INSERT (city_id, logdate, peaktemp, unitsales) VALUES (city_id, logdate, peaktemp, unitsales);
USING new_measurement nm ON
(m.city_id = nm.city_id and m.logdate=nm.logdate)
WHEN MATCHED AND nm.peaktemp IS NULL THEN DELETE
WHEN MATCHED THEN UPDATE
SET peaktemp = greatest(m.peaktemp, nm.peaktemp),
unitsales = m.unitsales + coalesce(nm.unitsales, 0)
WHEN NOT MATCHED THEN INSERT
(city_id, logdate, peaktemp, unitsales)
VALUES (city_id, logdate, peaktemp, unitsales);
triggers
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING: type mismatch: resno 1: slot (type: 0, name: ........pg.dropped.1........) vs expr (type: 23, name: city_id)
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING: TargetEntry:
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] DETAIL: {TARGETENTRY
:expr
{VAR
:varno -1
:varattno 3
:vartype 23
:vartypmod -1
:varcollid 0
:varnullingrels (b)
:varlevelsup 0
:varreturningtype 0
:varnosyn 2
:varattnosyn 1
:location 379
}
:resno 1
:resname city_id
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING: type mismatch: resno 2: slot (type: 23, name: peaktemp) vs expr (type: 1082, name: logdate)
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING: TargetEntry:
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] DETAIL: {TARGETENTRY
:expr
{VAR
:varno -1
:varattno 4
:vartype 1082
:vartypmod -1
:varcollid 0
:varnullingrels (b)
:varlevelsup 0
:varreturningtype 0
:varnosyn 2
:varattnosyn 2
:location 388
}
:resno 2
:resname logdate
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING: type mismatch: resno 3: slot (type: 1082, name: logdate) vs expr (type: 23, name: peaktemp)
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING: TargetEntry:
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] DETAIL: {TARGETENTRY
:expr
{VAR
:varno -1
:varattno 1
:vartype 23
:vartypmod -1
:varcollid 0
:varnullingrels (b)
:varlevelsup 0
:varreturningtype 0
:varnosyn 2
:varattnosyn 3
:location 397
}
:resno 3
:resname peaktemp
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING: type mismatch: resno 5: slot (type: 23, name: unitsales) vs expr (type: 25, name: last_action)
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] WARNING: TargetEntry:
2025-05-20 14:32:31.039 EDT [1617074][client backend][0/49:0][psql] DETAIL: {TARGETENTRY
:expr
{CONST
:consttype 25
:consttypmod -1
:constcollid 100
:constlen -1
:constbyval false
:constisnull false
:location 418
:constvalue 16 [ 64 0 0 0 109 101 114 103 101 45 105 110 115 101 114 116
]
}
:resno 5
:resname last_action
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
[3]: 2025-05-21 11:24:55.111 EDT [1838296][client backend][0/16:0][psql] WARNING: type mismatch: resno 1: slot (type: 1700, name: dropme) vs expr (type: 25, name: key) 2025-05-21 11:24:55.112 EDT [1838296][client backend][0/16:0][psql] WARNING: TargetEntry: 2025-05-21 11:24:55.112 EDT [1838296][client backend][0/16:0][psql] DETAIL: {TARGETENTRY :expr {VAR :varno -1 :varattno 1 :vartype 25 :vartypmod -1 :varcollid 100 :varnullingrels (b) :varlevelsup 0 :varreturningtype 0 :varnosyn 2 :varattnosyn 1 :location 179 } :resno 1 :resname key :ressortgroupref 0 :resorigtbl 0 :resorigcol 0 :resjunk false }
2025-05-21 11:24:55.112 EDT [1838296][client backend][0/16:0][psql] WARNING: TargetEntry:
2025-05-21 11:24:55.112 EDT [1838296][client backend][0/16:0][psql] DETAIL: {TARGETENTRY
:expr
{VAR
:varno -1
:varattno 1
:vartype 25
:vartypmod -1
:varcollid 100
:varnullingrels (b)
:varlevelsup 0
:varreturningtype 0
:varnosyn 2
:varattnosyn 1
:location 179
}
:resno 1
:resname key
:ressortgroupref 0
:resorigtbl 0
:resorigcol 0
:resjunk false
}
Attachments:
merge-debug.difftext/x-diff; charset=us-asciiDownload
diff --git i/src/backend/executor/execExpr.c w/src/backend/executor/execExpr.c
index f1569879b52..2972af3c189 100644
--- i/src/backend/executor/execExpr.c
+++ w/src/backend/executor/execExpr.c
@@ -346,6 +346,7 @@ ExecInitExprList(List *nodes, PlanState *parent)
return result;
}
+#include "nodes/print.h"
/*
* ExecBuildProjectionInfo
@@ -399,6 +400,24 @@ ExecBuildProjectionInfo(List *targetList,
AttrNumber attnum = 0;
bool isSafeVar = false;
+ if (slot && tle->expr != NULL)
+ {
+ Oid expr_type = exprType((Node *) tle->expr);
+ Form_pg_attribute slot_attr = TupleDescAttr(slot->tts_tupleDescriptor, tle->resno - 1);
+ Oid slot_type = slot_attr->atttypid;
+
+ if (expr_type != slot_type)
+ {
+ elog(WARNING, "type mismatch: resno %d: slot (type: %d, name: %s) vs expr (type: %d, name: %s)",
+ tle->resno, slot_type,
+ NameStr(slot_attr->attname),
+ expr_type,
+ tle->resname
+ );
+ elog_node_display(WARNING, "TargetEntry", tle, true);
+ }
+ }
+
/*
* If tlist expression is a safe non-system Var, use the fast-path
* ASSIGN_*_VAR opcodes. "Safe" means that we don't need to apply
On 2025-May-21, Andres Freund wrote:
Hi,
In [1] I added some verification to projection building, to check if the
tupleslot passed to ExecBuildProjectionInfo() is compatible with the target
list. One query in merge.sql [2] got flagged.Trying to debug that issue, I found another problem. This leaves us with:
1) w/ inheritance INSERTed rows are not returned by RETURNING. This seems to
just generally not work with MERGE
Hmm, curious. One thing to observe is that the original source tuple is
in the child table, but the tuple inserted by MERGE ends up in the
parent table. I'm guessing that the code gets confused as to the
relation that the tuple in the returned slot comes from, and that
somehow makes it somehow not "see" the tuple to process for RETURNING?
I dunno. CC'ing Dean, who is more familiar with this code than I am.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"I love the Postgres community. It's all about doing things _properly_. :-)"
(David Garamond)
Álvaro Herrera <alvherre@alvh.no-ip.org> 于2025年5月24日周六 17:11写道:
On 2025-May-21, Andres Freund wrote:
Hi,
In [1] I added some verification to projection building, to check if the
tupleslot passed to ExecBuildProjectionInfo() is compatible with thetarget
list. One query in merge.sql [2] got flagged.
Trying to debug that issue, I found another problem. This leaves us with:
1) w/ inheritance INSERTed rows are not returned by RETURNING. This
seems to
just generally not work with MERGE
Hmm, curious. One thing to observe is that the original source tuple is
in the child table, but the tuple inserted by MERGE ends up in the
parent table. I'm guessing that the code gets confused as to the
relation that the tuple in the returned slot comes from, and that
somehow makes it somehow not "see" the tuple to process for RETURNING?
I dunno. CC'ing Dean, who is more familiar with this code than I am.
In ExecMergeNotMatched(), we passed the mtstate->rootResultRelInfo to
ExecInsert().
In this case, the ri_projectReturning of mtstate->rootResultRelInfo is
NULL, in ExecInsert(),
the "if (resultRelInfo->ri_projectReturning)" branch will not run, so
inheritance INSERTed rows are not returned by RETURNING.
The mtstate->rootResultRelInfo assigned in ExecInitModifyTable() is only
here:
if (node->rootRelation > 0)
{
Assert(bms_is_member(node->rootRelation, estate->es_unpruned_relids));
mtstate->rootResultRelInfo = makeNode(ResultRelInfo);
ExecInitResultRelation(estate, mtstate->rootResultRelInfo,
node->rootRelation);
}
The ri_projectReturning is not assigned.
I try to pass resultRelInfo to ExecInsert, the inherited INSERTed rows are
returned by RETURNING.
But some test cases in regression failed.
--
Thanks,
Tender Wang
Tender Wang <tndrwang@gmail.com> 于2025年5月25日周日 19:17写道:
Álvaro Herrera <alvherre@alvh.no-ip.org> 于2025年5月24日周六 17:11写道:
On 2025-May-21, Andres Freund wrote:
Hi,
In [1] I added some verification to projection building, to check if the
tupleslot passed to ExecBuildProjectionInfo() is compatible with thetarget
list. One query in merge.sql [2] got flagged.
Trying to debug that issue, I found another problem. This leaves us
with:
1) w/ inheritance INSERTed rows are not returned by RETURNING. This
seems to
just generally not work with MERGE
Hmm, curious. One thing to observe is that the original source tuple is
in the child table, but the tuple inserted by MERGE ends up in the
parent table. I'm guessing that the code gets confused as to the
relation that the tuple in the returned slot comes from, and that
somehow makes it somehow not "see" the tuple to process for RETURNING?
I dunno. CC'ing Dean, who is more familiar with this code than I am.In ExecMergeNotMatched(), we passed the mtstate->rootResultRelInfo to
ExecInsert().
In this case, the ri_projectReturning of mtstate->rootResultRelInfo is
NULL, in ExecInsert(),
the "if (resultRelInfo->ri_projectReturning)" branch will not run, so
inheritance INSERTed rows are not returned by RETURNING.The mtstate->rootResultRelInfo assigned in ExecInitModifyTable() is only
here:
if (node->rootRelation > 0)
{
Assert(bms_is_member(node->rootRelation, estate->es_unpruned_relids));
mtstate->rootResultRelInfo = makeNode(ResultRelInfo);
ExecInitResultRelation(estate, mtstate->rootResultRelInfo,
node->rootRelation);
}
The ri_projectReturning is not assigned.I try to pass resultRelInfo to ExecInsert, the inherited INSERTed rows are
returned by RETURNING.
But some test cases in regression failed.
For a partitioned table, we must pass rootResultRelInfo to ExecInsert(). I
added the check before calling ExecInsert()
If it is a partitioned table, we continue to pass rootResultRelInfo.
Otherwise, we pass resultRelInfo.
Please see the attached diff file. The patch passed all regression test
cases.
--
Thanks,
Tender Wang
Attachments:
merge_inh.difftext/plain; charset=US-ASCII; name=merge_inh.diffDownload
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2bc89bf84dc..443d4523789 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -3612,6 +3612,7 @@ ExecMergeNotMatched(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
MergeActionState *action = (MergeActionState *) lfirst(l);
CmdType commandType = action->mas_action->commandType;
TupleTableSlot *newslot;
+ char relkind;
/*
* Test condition, if any.
@@ -3635,9 +3636,15 @@ ExecMergeNotMatched(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
*/
newslot = ExecProject(action->mas_proj);
mtstate->mt_merge_action = action;
+ relkind = mtstate->rootResultRelInfo->ri_RelationDesc->rd_rel->relkind;
- rslot = ExecInsert(context, mtstate->rootResultRelInfo,
+ if (relkind == RELKIND_PARTITIONED_TABLE)
+ rslot = ExecInsert(context, mtstate->rootResultRelInfo,
newslot, canSetTag, NULL, NULL);
+ else
+ rslot = ExecInsert(context, resultRelInfo,
+ newslot, canSetTag, NULL, NULL);
+
mtstate->mt_merge_inserted += 1;
break;
case CMD_NOTHING:
On Sun, 25 May 2025 at 13:06, Tender Wang <tndrwang@gmail.com> wrote:
For a partitioned table, we must pass rootResultRelInfo to ExecInsert(). I added the check before calling ExecInsert()
If it is a partitioned table, we continue to pass rootResultRelInfo. Otherwise, we pass resultRelInfo.
Please see the attached diff file. The patch passed all regression test cases.
No, I don't think that's the right fix. I'm looking at it now, and I
think I have a fix, but it's more complicated than that. I'll post an
update later.
Regards,
Dean
On Sun, 25 May 2025 at 13:41, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Sun, 25 May 2025 at 13:06, Tender Wang <tndrwang@gmail.com> wrote:
For a partitioned table, we must pass rootResultRelInfo to ExecInsert(). I added the check before calling ExecInsert()
If it is a partitioned table, we continue to pass rootResultRelInfo. Otherwise, we pass resultRelInfo.
Please see the attached diff file. The patch passed all regression test cases.No, I don't think that's the right fix. I'm looking at it now, and I
think I have a fix, but it's more complicated than that. I'll post an
update later.
The reason that MERGE must pass rootResultRelInfo to ExecInsert() for
a plain-inheritance table dates back to 387f9ed0a08. As that commit
demonstrates, it is possible for the parent to be excluded from the
plan, and so all of the entries in the resultRelInfo array may be for
different relations than rootResultRelInfo.
So there are indeed two related bugs here:
1. The projection built by ExecInitMerge() in the INSERT case may use
a tuple slot that's not compatible with the target table.
2. ExecInitModifyTable() does not initialize the WCO lists or
RETURNING list for rootResultRelInfo, so those never get executed.
As it happens, it is possible to construct cases where (1) causes a
crash, even without WCO's or a RETURNING list (see the first test case
in the attached patch), so this bug goes all the way back to v15,
where MERGE was introduced.
So I think we need to do something like the attached.
There is perhaps scope to reduce the code duplication between this and
ExecInitPartitionInfo(), but that'd likely make the patch bigger, so I
think it's best to leave that for now.
Regards,
Dean
Attachments:
v1-0001-Fix-MERGE-into-a-plain-inheritance-parent-table.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-MERGE-into-a-plain-inheritance-parent-table.patchDownload
From 74db4187272171853b69d4d8c7155a778846f299 Mon Sep 17 00:00:00 2001
From: Dean Rasheed <dean.a.rasheed@gmail.com>
Date: Sun, 25 May 2025 20:00:48 +0100
Subject: [PATCH v1] Fix MERGE into a plain inheritance parent table.
When a MERGE's target table is the parent of an inheritance tree, any
INSERT actions insert into the parent table using ModifyTableState's
rootResultRelInfo. However, there are at least two bugs in the way
this is initialized:
1. ExecInitMerge() incorrectly uses a ResultRelInfo entry from the
ModifyTableState's resultRelInfo array to build the insert projection,
which may not be compatible with the rootResultRelInfo.
2. ExecInitModifyTable() does not fully initialize rootResultRelInfo
(ri_WithCheckOptions, ri_WithCheckOptionExprs, ri_returningList, and
ri_projectReturning are not initialized).
This can lead to crashes, or a failure to check WCO's or to evaluate
the RETURNING list for MERGE INSERT actions.
Fix both these bugs in ExecInitMerge(), noting that it is only
necessary to fully initialize rootResultRelInfo for a MERGE with
INSERT actions, when the target table is an inheritance parent.
Backpatch to v15, where MERGE was introduced.
Reported-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/4rlmjfniiyffp6b3kv4pfy4jw3pciy6mq72rdgnedsnbsx7qe5@j5hlpiwdguvc
Backpatch-through: 15
---
src/backend/executor/nodeModifyTable.c | 123 ++++++++++++++++++++++++-
src/test/regress/expected/merge.out | 70 ++++++++++++++
src/test/regress/sql/merge.sql | 49 ++++++++++
3 files changed, 239 insertions(+), 3 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2bc89bf84dc..871c421fd5b 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -64,6 +64,7 @@
#include "nodes/nodeFuncs.h"
#include "optimizer/optimizer.h"
#include "rewrite/rewriteHandler.h"
+#include "rewrite/rewriteManip.h"
#include "storage/lmgr.h"
#include "utils/builtins.h"
#include "utils/datum.h"
@@ -3735,6 +3736,7 @@ ExecInitMerge(ModifyTableState *mtstate, EState *estate)
switch (action->commandType)
{
case CMD_INSERT:
+ /* INSERT actions always use rootRelInfo */
ExecCheckPlanOutput(rootRelInfo->ri_RelationDesc,
action->targetList);
@@ -3774,9 +3776,23 @@ ExecInitMerge(ModifyTableState *mtstate, EState *estate)
}
else
{
- /* not partitioned? use the stock relation and slot */
- tgtslot = resultRelInfo->ri_newTupleSlot;
- tgtdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
+ /*
+ * If the MERGE targets an inherited table, we insert
+ * into the root table, so we must initialize its
+ * "new" tuple slot, if not already done, and use its
+ * relation descriptor for the projection.
+ *
+ * For non-inherited tables, rootRelInfo and
+ * resultRelInfo are the same, and the "new" tuple
+ * slot will already have been initialized.
+ */
+ if (rootRelInfo->ri_newTupleSlot == NULL)
+ rootRelInfo->ri_newTupleSlot =
+ table_slot_create(rootRelInfo->ri_RelationDesc,
+ &estate->es_tupleTable);
+
+ tgtslot = rootRelInfo->ri_newTupleSlot;
+ tgtdesc = RelationGetDescr(rootRelInfo->ri_RelationDesc);
}
action_state->mas_proj =
@@ -3809,6 +3825,107 @@ ExecInitMerge(ModifyTableState *mtstate, EState *estate)
}
}
}
+
+ /*
+ * If the MERGE targets an inherited table, any INSERT actions will use
+ * rootRelInfo, and rootRelInfo will not be in the resultRelInfo array.
+ * Therefore we must initialize its WITH CHECK OPTION constraints and
+ * RETURNING projection, as ExecInitModifyTable did for the resultRelInfo
+ * entries.
+ *
+ * Note that the planner does not build a withCheckOptionList or
+ * returningList for the root relation, but as in ExecInitPartitionInfo,
+ * we can use the first resultRelInfo entry as a reference to calculate
+ * the attno's for the root table.
+ */
+ if (rootRelInfo != mtstate->resultRelInfo &&
+ rootRelInfo->ri_RelationDesc->rd_rel->relkind != RELKIND_PARTITIONED_TABLE &&
+ (mtstate->mt_merge_subcommands & MERGE_INSERT) != 0)
+ {
+ ModifyTable *node = (ModifyTable *) mtstate->ps.plan;
+ Relation rootRelation = rootRelInfo->ri_RelationDesc;
+ Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc;
+ int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex;
+ AttrMap *part_attmap = NULL;
+ bool found_whole_row;
+
+ if (node->withCheckOptionLists != NIL)
+ {
+ List *wcoList;
+ List *wcoExprs = NIL;
+
+ /* There should be as many WCO lists as result rels */
+ Assert(list_length(node->withCheckOptionLists) ==
+ list_length(node->resultRelations));
+
+ /* Use the first WCO list as a reference */
+ wcoList = linitial(node->withCheckOptionLists);
+
+ /* Convert any Vars in it to contain the root's attno's */
+ part_attmap =
+ build_attrmap_by_name(RelationGetDescr(rootRelation),
+ RelationGetDescr(firstResultRel),
+ false);
+
+ wcoList = (List *)
+ map_variable_attnos((Node *) wcoList,
+ firstVarno, 0,
+ part_attmap,
+ RelationGetForm(rootRelation)->reltype,
+ &found_whole_row);
+
+ foreach(lc, wcoList)
+ {
+ WithCheckOption *wco = lfirst_node(WithCheckOption, lc);
+ ExprState *wcoExpr = ExecInitQual(castNode(List, wco->qual),
+ &mtstate->ps);
+
+ wcoExprs = lappend(wcoExprs, wcoExpr);
+ }
+
+ rootRelInfo->ri_WithCheckOptions = wcoList;
+ rootRelInfo->ri_WithCheckOptionExprs = wcoExprs;
+ }
+
+ if (node->returningLists != NIL)
+ {
+ List *returningList;
+ TupleTableSlot *slot;
+
+ /* There should be as many returning lists as result rels */
+ Assert(list_length(node->returningLists) ==
+ list_length(node->resultRelations));
+
+ /* Use the first returning list as a reference */
+ returningList = linitial(node->returningLists);
+
+ /* Convert any Vars in it to contain the root's attno's */
+ if (part_attmap == NULL)
+ part_attmap =
+ build_attrmap_by_name(RelationGetDescr(rootRelation),
+ RelationGetDescr(firstResultRel),
+ false);
+
+ returningList = (List *)
+ map_variable_attnos((Node *) returningList,
+ firstVarno, 0,
+ part_attmap,
+ RelationGetForm(rootRelation)->reltype,
+ &found_whole_row);
+
+ rootRelInfo->ri_returningList = returningList;
+
+ /* Initialize the RETURNING projection */
+ Assert(mtstate->ps.ps_ResultTupleSlot != NULL);
+ slot = mtstate->ps.ps_ResultTupleSlot;
+ Assert(mtstate->ps.ps_ExprContext != NULL);
+ econtext = mtstate->ps.ps_ExprContext;
+ rootRelInfo->ri_projectReturning =
+ ExecBuildProjectionInfo(returningList, econtext, slot,
+ &mtstate->ps,
+ RelationGetDescr(rootRelation));
+ }
+ }
}
/*
diff --git a/src/test/regress/expected/merge.out b/src/test/regress/expected/merge.out
index bcd29668297..cf2219df754 100644
--- a/src/test/regress/expected/merge.out
+++ b/src/test/regress/expected/merge.out
@@ -2702,6 +2702,76 @@ SELECT * FROM new_measurement ORDER BY city_id, logdate;
1 | 01-17-2007 | |
(2 rows)
+-- MERGE into inheritance root table
+DROP TRIGGER insert_measurement_trigger ON measurement;
+ALTER TABLE measurement ADD CONSTRAINT mcheck CHECK (city_id = 0) NO INHERIT;
+EXPLAIN (COSTS OFF)
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100);
+ QUERY PLAN
+--------------------------------------------------------------------------
+ Merge on measurement m
+ Merge on measurement_y2007m01 m_1
+ -> Nested Loop Left Join
+ -> Result
+ -> Seq Scan on measurement_y2007m01 m_1
+ Filter: ((city_id = 1) AND (logdate = '01-17-2007'::date))
+(6 rows)
+
+BEGIN;
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100);
+SELECT * FROM ONLY measurement ORDER BY city_id, logdate;
+ city_id | logdate | peaktemp | unitsales
+---------+------------+----------+-----------
+ 0 | 07-21-2005 | 25 | 35
+ 0 | 01-17-2007 | 25 | 100
+(2 rows)
+
+ROLLBACK;
+ALTER TABLE measurement ENABLE ROW LEVEL SECURITY;
+ALTER TABLE measurement FORCE ROW LEVEL SECURITY;
+CREATE POLICY measurement_p ON measurement USING (peaktemp IS NOT NULL);
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, NULL, 100); -- should fail
+ERROR: new row violates row-level security policy for table "measurement"
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100); -- ok
+SELECT * FROM ONLY measurement ORDER BY city_id, logdate;
+ city_id | logdate | peaktemp | unitsales
+---------+------------+----------+-----------
+ 0 | 07-21-2005 | 25 | 35
+ 0 | 01-17-2007 | 25 | 100
+(2 rows)
+
+MERGE INTO measurement m
+ USING (VALUES (1, '01-18-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 200)
+RETURNING merge_action(), m.*;
+ merge_action | city_id | logdate | peaktemp | unitsales
+--------------+---------+------------+----------+-----------
+ INSERT | 0 | 01-18-2007 | 25 | 200
+(1 row)
+
DROP TABLE measurement, new_measurement CASCADE;
NOTICE: drop cascades to 3 other objects
DETAIL: drop cascades to table measurement_y2006m02
diff --git a/src/test/regress/sql/merge.sql b/src/test/regress/sql/merge.sql
index f7a19c0e7dd..2660b19f238 100644
--- a/src/test/regress/sql/merge.sql
+++ b/src/test/regress/sql/merge.sql
@@ -1722,6 +1722,55 @@ WHEN MATCHED THEN DELETE;
SELECT * FROM new_measurement ORDER BY city_id, logdate;
+-- MERGE into inheritance root table
+DROP TRIGGER insert_measurement_trigger ON measurement;
+ALTER TABLE measurement ADD CONSTRAINT mcheck CHECK (city_id = 0) NO INHERIT;
+
+EXPLAIN (COSTS OFF)
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100);
+
+BEGIN;
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100);
+SELECT * FROM ONLY measurement ORDER BY city_id, logdate;
+ROLLBACK;
+
+ALTER TABLE measurement ENABLE ROW LEVEL SECURITY;
+ALTER TABLE measurement FORCE ROW LEVEL SECURITY;
+CREATE POLICY measurement_p ON measurement USING (peaktemp IS NOT NULL);
+
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, NULL, 100); -- should fail
+
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100); -- ok
+SELECT * FROM ONLY measurement ORDER BY city_id, logdate;
+
+MERGE INTO measurement m
+ USING (VALUES (1, '01-18-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 200)
+RETURNING merge_action(), m.*;
+
DROP TABLE measurement, new_measurement CASCADE;
DROP FUNCTION measurement_insert_trigger();
--
2.43.0
Dean Rasheed <dean.a.rasheed@gmail.com> 于2025年5月26日周一 04:10写道:
On Sun, 25 May 2025 at 13:41, Dean Rasheed <dean.a.rasheed@gmail.com>
wrote:On Sun, 25 May 2025 at 13:06, Tender Wang <tndrwang@gmail.com> wrote:
For a partitioned table, we must pass rootResultRelInfo to
ExecInsert(). I added the check before calling ExecInsert()
If it is a partitioned table, we continue to pass rootResultRelInfo.
Otherwise, we pass resultRelInfo.
Please see the attached diff file. The patch passed all regression
test cases.
No, I don't think that's the right fix. I'm looking at it now, and I
think I have a fix, but it's more complicated than that. I'll post an
update later.The reason that MERGE must pass rootResultRelInfo to ExecInsert() for
a plain-inheritance table dates back to 387f9ed0a08. As that commit
demonstrates, it is possible for the parent to be excluded from the
plan, and so all of the entries in the resultRelInfo array may be for
different relations than rootResultRelInfo.
Thanks for the information. Passing resultRelInfo to ExecInsert() is wrong.
So there are indeed two related bugs here:
1. The projection built by ExecInitMerge() in the INSERT case may use
a tuple slot that's not compatible with the target table.2. ExecInitModifyTable() does not initialize the WCO lists or
RETURNING list for rootResultRelInfo, so those never get executed.As it happens, it is possible to construct cases where (1) causes a
crash, even without WCO's or a RETURNING list (see the first test case
in the attached patch), so this bug goes all the way back to v15,
where MERGE was introduced.So I think we need to do something like the attached.
I tested the attached patch, and there's no problem anymore. LGTM.
There is perhaps scope to reduce the code duplication between this and
ExecInitPartitionInfo(), but that'd likely make the patch bigger, so I
think it's best to leave that for now.
Agreed.
--
Thanks,
Tender Wang
Dean Rasheed <dean.a.rasheed@gmail.com> 于2025年5月26日周一 04:10写道:
On Sun, 25 May 2025 at 13:41, Dean Rasheed <dean.a.rasheed@gmail.com>
wrote:On Sun, 25 May 2025 at 13:06, Tender Wang <tndrwang@gmail.com> wrote:
For a partitioned table, we must pass rootResultRelInfo to
ExecInsert(). I added the check before calling ExecInsert()
If it is a partitioned table, we continue to pass rootResultRelInfo.
Otherwise, we pass resultRelInfo.
Please see the attached diff file. The patch passed all regression
test cases.
No, I don't think that's the right fix. I'm looking at it now, and I
think I have a fix, but it's more complicated than that. I'll post an
update later.The reason that MERGE must pass rootResultRelInfo to ExecInsert() for
a plain-inheritance table dates back to 387f9ed0a08. As that commit
demonstrates, it is possible for the parent to be excluded from the
plan, and so all of the entries in the resultRelInfo array may be for
different relations than rootResultRelInfo.
Hi Dean,
"it is possible for the parent to be excluded from the
plan and so all of the entries in the resultRelInfo array may be for
different relations than rootResultRelInfo."
I didn't fully understand the above sentence. Can you give me more
information or an example?
If the parent is excluded from the plan, the first entry in the
resultRelInfo array will not be the parent but some surviving child.
--
Thanks,
Tender Wang
On Mon, May 26, 2025 at 4:11 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Sun, 25 May 2025 at 13:41, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
2. ExecInitModifyTable() does not initialize the WCO lists or
RETURNING list for rootResultRelInfo, so those never get executed.As it happens, it is possible to construct cases where (1) causes a
crash, even without WCO's or a RETURNING list (see the first test case
in the attached patch), so this bug goes all the way back to v15,
where MERGE was introduced.So I think we need to do something like the attached.
There is perhaps scope to reduce the code duplication between this and
ExecInitPartitionInfo(), but that'd likely make the patch bigger, so I
think it's best to leave that for now.
+ Relation rootRelation = rootRelInfo->ri_RelationDesc;
+ Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc;
+ int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex;
firstResultRel may equal (==) to rootRelation,
in that case, we don't need to call map_variable_attnos?
we can
returningList = linitial(node->returningLists);
if (rel != firstResultRel)
{
/* Convert any Vars in it to contain the root's attno's */
part_attmap =
build_attrmap_by_name(RelationGetDescr(rel),
RelationGetDescr(firstResultRel),
false);
returningList = (List *)
map_variable_attnos((Node *) returningList,
firstVarno, 0,
part_attmap,
RelationGetForm(rel)->reltype,
&found_whole_row);
}
(i am not sure that will make code less readable).
we can unconditionally call ExecBuildProjectionInfo for rootRelInfo
within ExecInitModifyTable instead of in ExecInitMerge.
right after
/*
* Build a projection for each result rel.
*/
resultRelInfo = mtstate->resultRelInfo;
foreach(l, returningLists)
{
List *rlist = (List *) lfirst(l);
resultRelInfo->ri_returningList = rlist;
resultRelInfo->ri_projectReturning =
ExecBuildProjectionInfo(rlist, econtext, slot, &mtstate->ps,
resultRelInfo->ri_RelationDesc->rd_att);
resultRelInfo++;
}
This would make related initiation logic stay together, also harmless (i think).
what do you think?
jian he <jian.universality@gmail.com> 于2025年5月26日周一 17:30写道:
On Mon, May 26, 2025 at 4:11 AM Dean Rasheed <dean.a.rasheed@gmail.com>
wrote:On Sun, 25 May 2025 at 13:41, Dean Rasheed <dean.a.rasheed@gmail.com>
wrote:
2. ExecInitModifyTable() does not initialize the WCO lists or
RETURNING list for rootResultRelInfo, so those never get executed.As it happens, it is possible to construct cases where (1) causes a
crash, even without WCO's or a RETURNING list (see the first test case
in the attached patch), so this bug goes all the way back to v15,
where MERGE was introduced.So I think we need to do something like the attached.
There is perhaps scope to reduce the code duplication between this and
ExecInitPartitionInfo(), but that'd likely make the patch bigger, so I
think it's best to leave that for now.+ Relation rootRelation = rootRelInfo->ri_RelationDesc; + Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc; + int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex;firstResultRel may equal (==) to rootRelation,
in that case, we don't need to call map_variable_attnos?+ if (rootRelInfo != mtstate->resultRelInfo &&
+ rootRelInfo->ri_RelationDesc->rd_rel->relkind !=
RELKIND_PARTITIONED_TABLE &&
+ (mtstate->mt_merge_subcommands & MERGE_INSERT) != 0)
Above if already does the check.
--
Thanks,
Tender Wang
On Mon, 26 May 2025 at 10:30, jian he <jian.universality@gmail.com> wrote:
+ Relation rootRelation = rootRelInfo->ri_RelationDesc; + Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc; + int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex;firstResultRel may equal (==) to rootRelation,
in that case, we don't need to call map_variable_attnos?
Good point. I think that's by far the most common case, so that seems
like a worthwhile optimisation. v2 attached.
we can unconditionally call ExecBuildProjectionInfo for rootRelInfo
within ExecInitModifyTable instead of in ExecInitMerge.
right after/*
* Build a projection for each result rel.
*/
resultRelInfo = mtstate->resultRelInfo;
foreach(l, returningLists)
{
List *rlist = (List *) lfirst(l);
resultRelInfo->ri_returningList = rlist;
resultRelInfo->ri_projectReturning =
ExecBuildProjectionInfo(rlist, econtext, slot, &mtstate->ps,
resultRelInfo->ri_RelationDesc->rd_att);
resultRelInfo++;
}This would make related initiation logic stay together, also harmless (i think).
what do you think?
Well it would have to be done after calling ExecInitMerge() to set up
rootRelInfo->ri_returningList, but I don't think it really makes sense
to do it there. The patch intentionally only does it for a MERGE into
an inherited table when there are INSERT actions, and this also allows
the new code to be more consistent with ExecInitPartitionInfo().
Regards,
Dean
Attachments:
v2-0001-Fix-MERGE-into-a-plain-inheritance-parent-table.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Fix-MERGE-into-a-plain-inheritance-parent-table.patchDownload
From 1d20f593cbc3f454686f5880d24ee275c76cc736 Mon Sep 17 00:00:00 2001
From: Dean Rasheed <dean.a.rasheed@gmail.com>
Date: Sun, 25 May 2025 20:00:48 +0100
Subject: [PATCH v2] Fix MERGE into a plain inheritance parent table.
When a MERGE's target table is the parent of an inheritance tree, any
INSERT actions insert into the parent table using ModifyTableState's
rootResultRelInfo. However, there are at least two bugs in the way
this is initialized:
1. ExecInitMerge() incorrectly uses a ResultRelInfo entry from the
ModifyTableState's resultRelInfo array to build the insert projection,
which may not be compatible with the rootResultRelInfo.
2. ExecInitModifyTable() does not fully initialize rootResultRelInfo
(ri_WithCheckOptions, ri_WithCheckOptionExprs, ri_returningList, and
ri_projectReturning are not initialized).
This can lead to crashes, or a failure to check WCO's or to evaluate
the RETURNING list for MERGE INSERT actions.
Fix both these bugs in ExecInitMerge(), noting that it is only
necessary to fully initialize rootResultRelInfo for a MERGE with
INSERT actions, when the target table is an inheritance parent.
Backpatch to v15, where MERGE was introduced.
Reported-by: Andres Freund <andres@anarazel.de>
Author: Dean Rasheed <dean.a.rasheed@gmail.com>
Reviewed-by: Jian He <jian.universality@gmail.com>
Reviewed-by: Tender Wang <tndrwang@gmail.com>
Discussion: https://postgr.es/m/4rlmjfniiyffp6b3kv4pfy4jw3pciy6mq72rdgnedsnbsx7qe5@j5hlpiwdguvc
Backpatch-through: 15
---
src/backend/executor/nodeModifyTable.c | 135 ++++++++++++++++++++++++-
src/test/regress/expected/merge.out | 70 +++++++++++++
src/test/regress/sql/merge.sql | 49 +++++++++
3 files changed, 251 insertions(+), 3 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 2bc89bf84dc..8162b2af504 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -64,6 +64,7 @@
#include "nodes/nodeFuncs.h"
#include "optimizer/optimizer.h"
#include "rewrite/rewriteHandler.h"
+#include "rewrite/rewriteManip.h"
#include "storage/lmgr.h"
#include "utils/builtins.h"
#include "utils/datum.h"
@@ -3735,6 +3736,7 @@ ExecInitMerge(ModifyTableState *mtstate, EState *estate)
switch (action->commandType)
{
case CMD_INSERT:
+ /* INSERT actions always use rootRelInfo */
ExecCheckPlanOutput(rootRelInfo->ri_RelationDesc,
action->targetList);
@@ -3774,9 +3776,23 @@ ExecInitMerge(ModifyTableState *mtstate, EState *estate)
}
else
{
- /* not partitioned? use the stock relation and slot */
- tgtslot = resultRelInfo->ri_newTupleSlot;
- tgtdesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
+ /*
+ * If the MERGE targets an inherited table, we insert
+ * into the root table, so we must initialize its
+ * "new" tuple slot, if not already done, and use its
+ * relation descriptor for the projection.
+ *
+ * For non-inherited tables, rootRelInfo and
+ * resultRelInfo are the same, and the "new" tuple
+ * slot will already have been initialized.
+ */
+ if (rootRelInfo->ri_newTupleSlot == NULL)
+ rootRelInfo->ri_newTupleSlot =
+ table_slot_create(rootRelInfo->ri_RelationDesc,
+ &estate->es_tupleTable);
+
+ tgtslot = rootRelInfo->ri_newTupleSlot;
+ tgtdesc = RelationGetDescr(rootRelInfo->ri_RelationDesc);
}
action_state->mas_proj =
@@ -3809,6 +3825,119 @@ ExecInitMerge(ModifyTableState *mtstate, EState *estate)
}
}
}
+
+ /*
+ * If the MERGE targets an inherited table, any INSERT actions will use
+ * rootRelInfo, and rootRelInfo will not be in the resultRelInfo array.
+ * Therefore we must initialize its WITH CHECK OPTION constraints and
+ * RETURNING projection, as ExecInitModifyTable did for the resultRelInfo
+ * entries.
+ *
+ * Note that the planner does not build a withCheckOptionList or
+ * returningList for the root relation, but as in ExecInitPartitionInfo,
+ * we can use the first resultRelInfo entry as a reference to calculate
+ * the attno's for the root table.
+ */
+ if (rootRelInfo != mtstate->resultRelInfo &&
+ rootRelInfo->ri_RelationDesc->rd_rel->relkind != RELKIND_PARTITIONED_TABLE &&
+ (mtstate->mt_merge_subcommands & MERGE_INSERT) != 0)
+ {
+ ModifyTable *node = (ModifyTable *) mtstate->ps.plan;
+ Relation rootRelation = rootRelInfo->ri_RelationDesc;
+ Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc;
+ int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex;
+ AttrMap *part_attmap = NULL;
+ bool found_whole_row;
+
+ if (node->withCheckOptionLists != NIL)
+ {
+ List *wcoList;
+ List *wcoExprs = NIL;
+
+ /* There should be as many WCO lists as result rels */
+ Assert(list_length(node->withCheckOptionLists) ==
+ list_length(node->resultRelations));
+
+ /*
+ * Use the first WCO list as a reference. In the most common case,
+ * this will be for the same relation as rootRelInfo, and so there
+ * will be no need to adjust its attno's.
+ */
+ wcoList = linitial(node->withCheckOptionLists);
+ if (rootRelation != firstResultRel)
+ {
+ /* Convert any Vars in it to contain the root's attno's */
+ part_attmap =
+ build_attrmap_by_name(RelationGetDescr(rootRelation),
+ RelationGetDescr(firstResultRel),
+ false);
+
+ wcoList = (List *)
+ map_variable_attnos((Node *) wcoList,
+ firstVarno, 0,
+ part_attmap,
+ RelationGetForm(rootRelation)->reltype,
+ &found_whole_row);
+ }
+
+ foreach(lc, wcoList)
+ {
+ WithCheckOption *wco = lfirst_node(WithCheckOption, lc);
+ ExprState *wcoExpr = ExecInitQual(castNode(List, wco->qual),
+ &mtstate->ps);
+
+ wcoExprs = lappend(wcoExprs, wcoExpr);
+ }
+
+ rootRelInfo->ri_WithCheckOptions = wcoList;
+ rootRelInfo->ri_WithCheckOptionExprs = wcoExprs;
+ }
+
+ if (node->returningLists != NIL)
+ {
+ List *returningList;
+ TupleTableSlot *slot;
+
+ /* There should be as many returning lists as result rels */
+ Assert(list_length(node->returningLists) ==
+ list_length(node->resultRelations));
+
+ /*
+ * Use the first returning list as a reference. In the most common
+ * case, this will be for the same relation as rootRelInfo, and so
+ * there will be no need to adjust its attno's.
+ */
+ returningList = linitial(node->returningLists);
+ if (rootRelation != firstResultRel)
+ {
+ /* Convert any Vars in it to contain the root's attno's */
+ if (part_attmap == NULL)
+ part_attmap =
+ build_attrmap_by_name(RelationGetDescr(rootRelation),
+ RelationGetDescr(firstResultRel),
+ false);
+
+ returningList = (List *)
+ map_variable_attnos((Node *) returningList,
+ firstVarno, 0,
+ part_attmap,
+ RelationGetForm(rootRelation)->reltype,
+ &found_whole_row);
+ }
+
+ rootRelInfo->ri_returningList = returningList;
+
+ /* Initialize the RETURNING projection */
+ Assert(mtstate->ps.ps_ResultTupleSlot != NULL);
+ slot = mtstate->ps.ps_ResultTupleSlot;
+ Assert(mtstate->ps.ps_ExprContext != NULL);
+ econtext = mtstate->ps.ps_ExprContext;
+ rootRelInfo->ri_projectReturning =
+ ExecBuildProjectionInfo(returningList, econtext, slot,
+ &mtstate->ps,
+ RelationGetDescr(rootRelation));
+ }
+ }
}
/*
diff --git a/src/test/regress/expected/merge.out b/src/test/regress/expected/merge.out
index bcd29668297..cf2219df754 100644
--- a/src/test/regress/expected/merge.out
+++ b/src/test/regress/expected/merge.out
@@ -2702,6 +2702,76 @@ SELECT * FROM new_measurement ORDER BY city_id, logdate;
1 | 01-17-2007 | |
(2 rows)
+-- MERGE into inheritance root table
+DROP TRIGGER insert_measurement_trigger ON measurement;
+ALTER TABLE measurement ADD CONSTRAINT mcheck CHECK (city_id = 0) NO INHERIT;
+EXPLAIN (COSTS OFF)
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100);
+ QUERY PLAN
+--------------------------------------------------------------------------
+ Merge on measurement m
+ Merge on measurement_y2007m01 m_1
+ -> Nested Loop Left Join
+ -> Result
+ -> Seq Scan on measurement_y2007m01 m_1
+ Filter: ((city_id = 1) AND (logdate = '01-17-2007'::date))
+(6 rows)
+
+BEGIN;
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100);
+SELECT * FROM ONLY measurement ORDER BY city_id, logdate;
+ city_id | logdate | peaktemp | unitsales
+---------+------------+----------+-----------
+ 0 | 07-21-2005 | 25 | 35
+ 0 | 01-17-2007 | 25 | 100
+(2 rows)
+
+ROLLBACK;
+ALTER TABLE measurement ENABLE ROW LEVEL SECURITY;
+ALTER TABLE measurement FORCE ROW LEVEL SECURITY;
+CREATE POLICY measurement_p ON measurement USING (peaktemp IS NOT NULL);
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, NULL, 100); -- should fail
+ERROR: new row violates row-level security policy for table "measurement"
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100); -- ok
+SELECT * FROM ONLY measurement ORDER BY city_id, logdate;
+ city_id | logdate | peaktemp | unitsales
+---------+------------+----------+-----------
+ 0 | 07-21-2005 | 25 | 35
+ 0 | 01-17-2007 | 25 | 100
+(2 rows)
+
+MERGE INTO measurement m
+ USING (VALUES (1, '01-18-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 200)
+RETURNING merge_action(), m.*;
+ merge_action | city_id | logdate | peaktemp | unitsales
+--------------+---------+------------+----------+-----------
+ INSERT | 0 | 01-18-2007 | 25 | 200
+(1 row)
+
DROP TABLE measurement, new_measurement CASCADE;
NOTICE: drop cascades to 3 other objects
DETAIL: drop cascades to table measurement_y2006m02
diff --git a/src/test/regress/sql/merge.sql b/src/test/regress/sql/merge.sql
index f7a19c0e7dd..2660b19f238 100644
--- a/src/test/regress/sql/merge.sql
+++ b/src/test/regress/sql/merge.sql
@@ -1722,6 +1722,55 @@ WHEN MATCHED THEN DELETE;
SELECT * FROM new_measurement ORDER BY city_id, logdate;
+-- MERGE into inheritance root table
+DROP TRIGGER insert_measurement_trigger ON measurement;
+ALTER TABLE measurement ADD CONSTRAINT mcheck CHECK (city_id = 0) NO INHERIT;
+
+EXPLAIN (COSTS OFF)
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100);
+
+BEGIN;
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100);
+SELECT * FROM ONLY measurement ORDER BY city_id, logdate;
+ROLLBACK;
+
+ALTER TABLE measurement ENABLE ROW LEVEL SECURITY;
+ALTER TABLE measurement FORCE ROW LEVEL SECURITY;
+CREATE POLICY measurement_p ON measurement USING (peaktemp IS NOT NULL);
+
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, NULL, 100); -- should fail
+
+MERGE INTO measurement m
+ USING (VALUES (1, '01-17-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 100); -- ok
+SELECT * FROM ONLY measurement ORDER BY city_id, logdate;
+
+MERGE INTO measurement m
+ USING (VALUES (1, '01-18-2007'::date)) nm(city_id, logdate) ON
+ (m.city_id = nm.city_id and m.logdate=nm.logdate)
+WHEN NOT MATCHED THEN INSERT
+ (city_id, logdate, peaktemp, unitsales)
+ VALUES (city_id - 1, logdate, 25, 200)
+RETURNING merge_action(), m.*;
+
DROP TABLE measurement, new_measurement CASCADE;
DROP FUNCTION measurement_insert_trigger();
--
2.43.0
On Mon, 26 May 2025 at 07:46, Tender Wang <tndrwang@gmail.com> wrote:
Hi Dean,
"it is possible for the parent to be excluded from the
plan and so all of the entries in the resultRelInfo array may be for
different relations than rootResultRelInfo."I didn't fully understand the above sentence. Can you give me more information or an example?
If the parent is excluded from the plan, the first entry in the resultRelInfo array will not be the parent but some surviving child.
There's an example in the updated regression tests. A non-inherited
CHECK constraint on the parent causes the planner to exclude the
parent from the relations being scanned and from the resultRelInfo
array, so the first resultRelInfo entry is for a child relation.
Regards,
Dean
On Mon, 26 May 2025 at 10:39, Tender Wang <tndrwang@gmail.com> wrote:
jian he <jian.universality@gmail.com> 于2025年5月26日周一 17:30写道:
+ Relation rootRelation = rootRelInfo->ri_RelationDesc; + Relation firstResultRel = mtstate->resultRelInfo[0].ri_RelationDesc; + int firstVarno = mtstate->resultRelInfo[0].ri_RangeTableIndex;firstResultRel may equal (==) to rootRelation,
in that case, we don't need to call map_variable_attnos?+ if (rootRelInfo != mtstate->resultRelInfo && + rootRelInfo->ri_RelationDesc->rd_rel->relkind != RELKIND_PARTITIONED_TABLE && + (mtstate->mt_merge_subcommands & MERGE_INSERT) != 0)Above if already does the check.
That's a different check. "rootRelInfo != mtstate->resultRelInfo"
checks that we're dealing with an inheritance/partitioned case (see
the code in ExecInitModifyTable() that sets up rootRelInfo). However,
in the inherited case, when rootRelInfo and mtstate->resultRelInfo
point to different ResultRelInfo structures, it is possible (actually
quite likely) that they will internally point to the same Relation. In
that case, we do still need to set up the WCO list, RETURNING list and
projection for rootRelInfo, but we don't need to map attribute
numbers. Building the attribute map looks like it's O(n^2) in the
number of attributes, so it's worth avoiding if we can.
Regards,
Dean
Dean Rasheed <dean.a.rasheed@gmail.com> 于2025年5月26日周一 18:41写道:
On Mon, 26 May 2025 at 07:46, Tender Wang <tndrwang@gmail.com> wrote:
Hi Dean,
"it is possible for the parent to be excluded from the
plan and so all of the entries in the resultRelInfo array may be for
different relations than rootResultRelInfo."I didn't fully understand the above sentence. Can you give me more
information or an example?
If the parent is excluded from the plan, the first entry in the
resultRelInfo array will not be the parent but some surviving child.
There's an example in the updated regression tests. A non-inherited
CHECK constraint on the parent causes the planner to exclude the
parent from the relations being scanned and from the resultRelInfo
array, so the first resultRelInfo entry is for a child relation.
Yes, it is. I debugged the updated regression tests. It would crash if
resultRelInfo were used instead of rootResultInfo.
Is that the reason that we must use rootResultInfo? Are there other things
that I miss?
That's a different check. "rootRelInfo != mtstate->resultRelInfo"
checks that we're dealing with an inheritance/partitioned case (see
the code in ExecInitModifyTable() that sets up rootRelInfo). However,
in the inherited case, when rootRelInfo and mtstate->resultRelInfo
point to different ResultRelInfo structures, it is possible (actually
quite likely) that they will internally point to the same Relation. In
that case, we do still need to set up the WCO list, RETURNING list and
projection for rootRelInfo, but we don't need to map attribute
numbers. Building the attribute map looks like it's O(n^2) in the
number of attributes, so it's worth avoiding if we can.
Yeah, Jian's idea is right.
--
Thanks,
Tender Wang
On Mon, 26 May 2025 at 12:50, Tender Wang <tndrwang@gmail.com> wrote:
"it is possible for the parent to be excluded from the
plan and so all of the entries in the resultRelInfo array may be for
different relations than rootResultRelInfo."I didn't fully understand the above sentence. Can you give me more information or an example?
If the parent is excluded from the plan, the first entry in the resultRelInfo array will not be the parent but some surviving child.There's an example in the updated regression tests. A non-inherited
CHECK constraint on the parent causes the planner to exclude the
parent from the relations being scanned and from the resultRelInfo
array, so the first resultRelInfo entry is for a child relation.Yes, it is. I debugged the updated regression tests. It would crash if resultRelInfo were used instead of rootResultInfo.
Is that the reason that we must use rootResultInfo?
Yes. To work correctly for an inherited table, ExecInsert() must be
passed a pointer to a ResultRelInfo structure that points to the
parent table.
As I mentioned before, this goes back to commit 387f9ed0a08, so it's
worth reading that in more detail.
Prior to commit 387f9ed0a08, rootResultInfo was equal to resultRelInfo
for an inherited table, which was wrong because that could point to a
child table, if the parent table was excluded from the plan.
Commit 387f9ed0a08 fixed that by changing the planner so that it set
ModifyTable.rootRelation to the index of the parent table, if it was
an inherited table. As a result, starting from that commit, for an
inherited table, rootResultInfo is a different ResultRelInfo structure
which always points to the parent table, making it the correct thing
to pass to ExecInsert() under all circumstances.
The thing that was overlooked was that the separate ResultRelInfo
structure in rootResultInfo, and the insert projection, weren't being
correctly initialised for MERGE, which is what this patch aims to fix.
Unless there are any further comments, I plan to commit it in a day or
so.
Regards,
Dean
Dean Rasheed <dean.a.rasheed@gmail.com> 于2025年5月28日周三 18:26写道:
On Mon, 26 May 2025 at 12:50, Tender Wang <tndrwang@gmail.com> wrote:
"it is possible for the parent to be excluded from the
plan and so all of the entries in the resultRelInfo array may be for
different relations than rootResultRelInfo."I didn't fully understand the above sentence. Can you give me more
information or an example?
If the parent is excluded from the plan, the first entry in the
resultRelInfo array will not be the parent but some surviving child.
There's an example in the updated regression tests. A non-inherited
CHECK constraint on the parent causes the planner to exclude the
parent from the relations being scanned and from the resultRelInfo
array, so the first resultRelInfo entry is for a child relation.Yes, it is. I debugged the updated regression tests. It would crash if
resultRelInfo were used instead of rootResultInfo.
Is that the reason that we must use rootResultInfo?
Yes. To work correctly for an inherited table, ExecInsert() must be
passed a pointer to a ResultRelInfo structure that points to the
parent table.As I mentioned before, this goes back to commit 387f9ed0a08, so it's
worth reading that in more detail.Prior to commit 387f9ed0a08, rootResultInfo was equal to resultRelInfo
for an inherited table, which was wrong because that could point to a
child table, if the parent table was excluded from the plan.Commit 387f9ed0a08 fixed that by changing the planner so that it set
ModifyTable.rootRelation to the index of the parent table, if it was
an inherited table. As a result, starting from that commit, for an
inherited table, rootResultInfo is a different ResultRelInfo structure
which always points to the parent table, making it the correct thing
to pass to ExecInsert() under all circumstances.
Yeah. Your explanation above made me understand completely.
Thanks for your explanation.
The thing that was overlooked was that the separate ResultRelInfo
structure in rootResultInfo, and the insert projection, weren't being
correctly initialised for MERGE, which is what this patch aims to fix.
Yes, it is.
Unless there are any further comments, I plan to commit it in a day or
so.
I don't have any other comments. It looks good for me.
--
Thanks,
Tender Wang
On Wed, 28 May 2025 at 11:37, Tender Wang <tndrwang@gmail.com> wrote:
Dean Rasheed <dean.a.rasheed@gmail.com> 于2025年5月28日周三 18:26写道:
Unless there are any further comments, I plan to commit it in a day or so.
I don't have any other comments. It looks good for me.
Thanks for looking. I have committed this now.
Regards,
Dean