On Sun, 25 May 2025 at 13:41, Dean Rasheed <dean.a.rash...@gmail.com> wrote: > > On Sun, 25 May 2025 at 13:06, Tender Wang <tndrw...@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
From 74db4187272171853b69d4d8c7155a778846f299 Mon Sep 17 00:00:00 2001 From: Dean Rasheed <dean.a.rash...@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 <and...@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
