Hello, ccing pgsql-hack...@postgresql.org
Upon investigation, it seems that the problem is caused by the following: The slot passed to the call to ExecProcessReturning() inside ExecInsert() is often a virtual tuple table slot. If there are system columns other than ctid and tableOid referenced in the RETURNING clause (not only xmin as in the bug report), it will lead to the ERROR as mentioned in this thread as virtual tuple table slots don't really store such columns. (ctid and tableOid are present directly in the TupleTableSlot struct and can be satisfied from there: refer: slot_getsysattr())) I have attached two alternate patches to solve the problem. Both patches use and share a mechanism to detect if there are any such system columns. This is done inside ExecBuildProjectionInfo() and we store this info inside the ProjectionInfo struct. Then based on this info, system columns are populated in a suitable slot, which is then passed on to ExecProcessReturning(). (If it is deemed that this operation only be done for RETURNING, we can just as easily do it in the callsite for ExecBuildProjectionInfo() in ExecInitModifyTable() for RETURNING instead of doing it inside ExecBuildProjectionInfo()) The first patch [1] explicitly creates a heap tuple table slot, fills in the system column values as we would do during heap_prepare_insert() and then passes that slot to ExecProcessReturning(). (We use a heap tuple table slot as it is guaranteed to support these attributes). The second patch [2] instead of relying on a heap tuple table slot, relies on ExecGetReturningSlot() for the right slot and table_tuple_fetch_row_version() to supply the system column values. It does make the assumption that the AM would supply a slot that will have these system columns. [1] v1-0001-Explicitly-supply-system-columns-for-INSERT.RETUR.patch [2] v1-0001-Use-table_tuple_fetch_row_version-to-supply-INSER.patch Regards, Soumyadeep (VMware)
From baa48a89e571405f4cd802f065d0f82131599f53 Mon Sep 17 00:00:00 2001
From: soumyadeep2007 <soumyadeep2007@gmail.com>
Date: Sun, 5 Jul 2020 10:23:30 -0700
Subject: [PATCH v1 1/1] Explicitly supply system columns for INSERT..RETURNING
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
If there are system columns other than ctid and tableOid in the
RETURNING clause for an INSERT, we must ensure that the slot we pass to
ExecProcessReturning() has those columns (ctid and tableOid are present
directly in the TupleTableSlot struct and can be satisifed from there:
refer: slot_getsysattr)).
This is in response to bug #16446 reported by Георгий Драк. In the bug
an INSERT..RETURNING statement fails with:
"ERROR: virtual tuple table slot does not have system
attributes"
This is caused due to the fact that ExecProcessReturning() was fed with
a virtual tuple table slot. Thus the resultant expression evaluation
and by extension, call to ExecEvalSysVar(), blows up with the
aforementioned error.
So, we fix this by:
1. Determining if there are system columns (other than tableOid and
ctid) referenced in ExecBuildProjectionInfo() and we store this info
inside the ProjectionInfo struct.
2. If there are such system columns in RETURNING, we explicitly create a
heap tuple table slot, fill in the system column values as we would do
during heap_prepare_insert() and then pass that slot to
ExecProcessReturning(). We use a heap tuple table slot as it is
guaranteed to support these attributes.
---
src/backend/executor/execExpr.c | 19 ++++++++++++++++++-
src/backend/executor/nodeModifyTable.c | 23 +++++++++++++++++++++++
src/include/nodes/execnodes.h | 2 ++
3 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 236413f62a..8cd966d227 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -360,10 +360,12 @@ ExecBuildProjectionInfo(List *targetList,
ExprState *state;
ExprEvalStep scratch = {0};
ListCell *lc;
+ List *tlist_vars;
projInfo->pi_exprContext = econtext;
/* We embed ExprState into ProjectionInfo instead of doing extra palloc */
projInfo->pi_state.tag = T_ExprState;
+ projInfo->has_non_slot_system_cols = false;
state = &projInfo->pi_state;
state->expr = (Expr *) targetList;
state->parent = parent;
@@ -455,7 +457,6 @@ ExecBuildProjectionInfo(List *targetList,
*/
ExecInitExprRec(tle->expr, state,
&state->resvalue, &state->resnull);
-
/*
* Column might be referenced multiple times in upper nodes, so
* force value to R/O - but only if it could be an expanded datum.
@@ -469,6 +470,22 @@ ExecBuildProjectionInfo(List *targetList,
}
}
+ /* Record system columns that are part of this projection */
+ tlist_vars = pull_var_clause((Node *) targetList,
+ PVC_RECURSE_AGGREGATES |
+ PVC_RECURSE_WINDOWFUNCS |
+ PVC_INCLUDE_PLACEHOLDERS);
+ foreach(lc, tlist_vars)
+ {
+ Var *var = (Var *) lfirst(lc);
+ if (var->varattno < 0 && (var->varattno != TableOidAttributeNumber ||
+ var->varattno != SelfItemPointerAttributeNumber))
+ {
+ projInfo->has_non_slot_system_cols = true;
+ break;
+ }
+ }
+
scratch.opcode = EEOP_DONE;
ExprEvalPushStep(state, &scratch);
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 20a4c474cc..956bdf0afd 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -678,7 +678,30 @@ ExecInsert(ModifyTableState *mtstate,
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
+ {
+ /*
+ * If the RETURNING list contains system columns other than ctid and
+ * tableOid, we should make sure that the system columns are available
+ * in a slot that supports system columns.
+ */
+ if (TTS_IS_VIRTUAL(slot) && resultRelInfo->ri_projectReturning->has_non_slot_system_cols)
+ {
+ /*
+ * Explicitly supply the system columns that are not ctid and
+ * tableOid. So, supply the system columns as we do when we insert.
+ * See heap_prepare_insert().
+ */
+ TupleTableSlot *returningSlot = ExecInitExtraTupleSlot(estate,
+ RelationGetDescr(resultRelationDesc),
+ &TTSOpsHeapTuple);
+ HeapTuple heapTuple = ExecFetchSlotHeapTuple(slot, true, NULL);
+ HeapTupleHeaderSetXmin(heapTuple->t_data, GetCurrentTransactionId());
+ HeapTupleHeaderSetXmax(heapTuple->t_data, 0);
+ HeapTupleHeaderSetCmin(heapTuple->t_data, estate->es_output_cid);
+ slot = ExecStoreHeapTuple(heapTuple, returningSlot, true);
+ }
result = ExecProcessReturning(resultRelInfo, slot, planSlot);
+ }
return result;
}
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index f5dfa32d55..f8047d2089 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -333,6 +333,8 @@ typedef struct ProjectionInfo
ExprState pi_state;
/* expression context in which to evaluate expression */
ExprContext *pi_exprContext;
+ /* projection contains system columns other than ctid and tableOid */
+ bool has_non_slot_system_cols;
} ProjectionInfo;
/* ----------------
--
2.24.1
v1-0001-Use-table_tuple_fetch_row_version-to-supply-INSER.patch
Description: Binary data
