Skip to content

Commit 8cd190e

Browse files
tglsfdctglsfdc
committed
Fix calculation of which GENERATED columns need to be updated.
We were identifying the updatable generated columns of inheritance children by transposing the calculation made for their parent. However, there's nothing that says a traditional-inheritance child can't have generated columns that aren't there in its parent, or that have different dependencies than are in the parent's expression. (At present it seems that we don't enforce that for partitioning either, which is likely wrong to some degree or other; but the case clearly needs to be handled with traditional inheritance.) Hence, drop the very-klugy-anyway "extraUpdatedCols" RTE field in favor of identifying which generated columns depend on updated columns during executor startup. In HEAD we can remove extraUpdatedCols altogether; in back branches, it's still there but always empty. Another difference between the HEAD and back-branch versions of this patch is that in HEAD we can add the new bitmap field to ResultRelInfo, but that would cause an ABI break in back branches. Like 4b3e379, add a List field at the end of struct EState instead. Back-patch to v13. The bogus calculation is also being made in v12, but it doesn't have the same visible effect because we don't use it to decide which generated columns to recalculate; as a consequence of which the patch doesn't apply easily. I think that there might still be a demonstrable bug associated with trigger firing conditions, but that's such a weird corner-case usage that I'm content to leave it unfixed in v12. Amit Langote and Tom Lane Discussion: https://postgr.es/m/CA+HiwqFshLKNvQUd1DgwJ-7tsTp=dwv7KZqXC4j2wYBV1aCDUA@mail.gmail.com Discussion: https://postgr.es/m/2793383.1672944799@sss.pgh.pa.us
1 parent 5136c3f commit 8cd190e

File tree

14 files changed

+330
-126
lines changed

14 files changed

+330
-126
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
#include "optimizer/appendinfo.h"
3232
#include "optimizer/clauses.h"
3333
#include "optimizer/cost.h"
34+
#include "optimizer/inherit.h"
3435
#include "optimizer/optimizer.h"
3536
#include "optimizer/pathnode.h"
3637
#include "optimizer/paths.h"
@@ -1813,7 +1814,8 @@ postgresPlanForeignModify(PlannerInfo *root,
18131814
else if (operation == CMD_UPDATE)
18141815
{
18151816
int col;
1816-
Bitmapset *allUpdatedCols = bms_union(rte->updatedCols, rte->extraUpdatedCols);
1817+
RelOptInfo *rel = find_base_rel(root, resultRelation);
1818+
Bitmapset *allUpdatedCols = get_rel_all_updated_cols(root, rel);
18171819

18181820
col = -1;
18191821
while ((col = bms_next_member(allUpdatedCols, col)) >= 0)

‎src/backend/executor/execUtils.c

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,8 @@ CreateExecutorState(void)
132132
estate->es_insert_pending_result_relations = NIL;
133133
estate->es_insert_pending_modifytables = NIL;
134134

135+
estate->es_resultrelinfo_extra = NIL;
136+
135137
estate->es_param_list_info = NULL;
136138
estate->es_param_exec_vals = NULL;
137139

@@ -1323,26 +1325,25 @@ ExecGetUpdatedCols(ResultRelInfo *relinfo, EState *estate)
13231325
Bitmapset *
13241326
ExecGetExtraUpdatedCols(ResultRelInfo *relinfo, EState *estate)
13251327
{
1326-
/* see ExecGetInsertedCols() */
1327-
if (relinfo->ri_RangeTableIndex != 0)
1328-
{
1329-
RangeTblEntry *rte = exec_rt_fetch(relinfo->ri_RangeTableIndex, estate);
1328+
Relation rel = relinfo->ri_RelationDesc;
1329+
TupleDesc tupdesc = RelationGetDescr(rel);
13301330

1331-
return rte->extraUpdatedCols;
1332-
}
1333-
else if (relinfo->ri_RootResultRelInfo)
1331+
if (tupdesc->constr && tupdesc->constr->has_generated_stored)
13341332
{
1335-
ResultRelInfo *rootRelInfo = relinfo->ri_RootResultRelInfo;
1336-
RangeTblEntry *rte = exec_rt_fetch(rootRelInfo->ri_RangeTableIndex, estate);
1333+
ListCell *lc;
13371334

1338-
if (relinfo->ri_RootToPartitionMap != NULL)
1339-
return execute_attr_map_cols(relinfo->ri_RootToPartitionMap->attrMap,
1340-
rte->extraUpdatedCols);
1341-
else
1342-
return rte->extraUpdatedCols;
1335+
/* Assert that ExecInitStoredGenerated has been called. */
1336+
Assert(relinfo->ri_GeneratedExprs != NULL);
1337+
foreach(lc, estate->es_resultrelinfo_extra)
1338+
{
1339+
ResultRelInfoExtra *rextra = (ResultRelInfoExtra *) lfirst(lc);
1340+
1341+
if (rextra->rinfo == relinfo)
1342+
return rextra->ri_extraUpdatedCols;
1343+
}
1344+
Assert(false); /* shouldn't get here */
13431345
}
1344-
else
1345-
return NULL;
1346+
return NULL;
13461347
}
13471348

13481349
/* Return columns being updated, including generated columns */

‎src/backend/executor/nodeModifyTable.c

Lines changed: 115 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
#include "foreign/fdwapi.h"
4646
#include "miscadmin.h"
4747
#include "nodes/nodeFuncs.h"
48+
#include "optimizer/optimizer.h"
4849
#include "rewrite/rewriteHandler.h"
4950
#include "storage/bufmgr.h"
5051
#include "storage/lmgr.h"
@@ -252,69 +253,128 @@ ExecCheckTIDVisible(EState *estate,
252253
}
253254

254255
/*
255-
* Compute stored generated columns for a tuple
256+
* Initialize to compute stored generated columns for a tuple
257+
*
258+
* This fills the resultRelInfo's ri_GeneratedExprs field and makes an
259+
* associated ResultRelInfoExtra struct to hold ri_extraUpdatedCols.
260+
* (Currently, ri_extraUpdatedCols is consulted only in UPDATE, but we might
261+
* as well fill it for INSERT too.)
256262
*/
257-
void
258-
ExecComputeStoredGenerated(ResultRelInfo *resultRelInfo,
259-
EState *estate, TupleTableSlot *slot,
260-
CmdType cmdtype)
263+
static void
264+
ExecInitStoredGenerated(ResultRelInfo *resultRelInfo,
265+
EState *estate,
266+
CmdType cmdtype)
261267
{
262268
Relation rel = resultRelInfo->ri_RelationDesc;
263269
TupleDesc tupdesc = RelationGetDescr(rel);
264270
int natts = tupdesc->natts;
271+
Bitmapset *updatedCols;
272+
ResultRelInfoExtra *rextra;
265273
MemoryContext oldContext;
266-
Datum *values;
267-
bool *nulls;
268274

269-
Assert(tupdesc->constr && tupdesc->constr->has_generated_stored);
275+
/* Don't call twice */
276+
Assert(resultRelInfo->ri_GeneratedExprs == NULL);
277+
278+
/* Nothing to do if no generated columns */
279+
if (!(tupdesc->constr && tupdesc->constr->has_generated_stored))
280+
return;
270281

271282
/*
272-
* If first time through for this result relation, build expression
273-
* nodetrees for rel's stored generation expressions. Keep them in the
274-
* per-query memory context so they'll survive throughout the query.
283+
* In an UPDATE, we can skip computing any generated columns that do not
284+
* depend on any UPDATE target column. But if there is a BEFORE ROW
285+
* UPDATE trigger, we cannot skip because the trigger might change more
286+
* columns.
275287
*/
276-
if (resultRelInfo->ri_GeneratedExprs == NULL)
277-
{
278-
oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
288+
if (cmdtype == CMD_UPDATE &&
289+
!(rel->trigdesc && rel->trigdesc->trig_update_before_row))
290+
updatedCols = ExecGetUpdatedCols(resultRelInfo, estate);
291+
else
292+
updatedCols = NULL;
279293

280-
resultRelInfo->ri_GeneratedExprs =
281-
(ExprState **) palloc(natts * sizeof(ExprState *));
282-
resultRelInfo->ri_NumGeneratedNeeded = 0;
294+
/*
295+
* Make sure these data structures are built in the per-query memory
296+
* context so they'll survive throughout the query.
297+
*/
298+
oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
299+
300+
resultRelInfo->ri_GeneratedExprs =
301+
(ExprState **) palloc0(natts * sizeof(ExprState *));
302+
resultRelInfo->ri_NumGeneratedNeeded = 0;
303+
304+
rextra = palloc_object(ResultRelInfoExtra);
305+
rextra->rinfo = resultRelInfo;
306+
rextra->ri_extraUpdatedCols = NULL;
307+
estate->es_resultrelinfo_extra = lappend(estate->es_resultrelinfo_extra,
308+
rextra);
283309

284-
for (int i = 0; i < natts; i++)
310+
for (int i = 0; i < natts; i++)
311+
{
312+
if (TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_STORED)
285313
{
286-
if (TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_STORED)
287-
{
288-
Expr *expr;
314+
Expr *expr;
289315

290-
/*
291-
* If it's an update and the current column was not marked as
292-
* being updated, then we can skip the computation. But if
293-
* there is a BEFORE ROW UPDATE trigger, we cannot skip
294-
* because the trigger might affect additional columns.
295-
*/
296-
if (cmdtype == CMD_UPDATE &&
297-
!(rel->trigdesc && rel->trigdesc->trig_update_before_row) &&
298-
!bms_is_member(i + 1 - FirstLowInvalidHeapAttributeNumber,
299-
ExecGetExtraUpdatedCols(resultRelInfo, estate)))
300-
{
301-
resultRelInfo->ri_GeneratedExprs[i] = NULL;
302-
continue;
303-
}
316+
/* Fetch the GENERATED AS expression tree */
317+
expr = (Expr *) build_column_default(rel, i + 1);
318+
if (expr == NULL)
319+
elog(ERROR, "no generation expression found for column number %d of table \"%s\"",
320+
i + 1, RelationGetRelationName(rel));
321+
322+
/*
323+
* If it's an update with a known set of update target columns,
324+
* see if we can skip the computation.
325+
*/
326+
if (updatedCols)
327+
{
328+
Bitmapset *attrs_used = NULL;
304329

305-
expr = (Expr *) build_column_default(rel, i + 1);
306-
if (expr == NULL)
307-
elog(ERROR, "no generation expression found for column number %d of table \"%s\"",
308-
i + 1, RelationGetRelationName(rel));
330+
pull_varattnos((Node *) expr, 1, &attrs_used);
309331

310-
resultRelInfo->ri_GeneratedExprs[i] = ExecPrepareExpr(expr, estate);
311-
resultRelInfo->ri_NumGeneratedNeeded++;
332+
if (!bms_overlap(updatedCols, attrs_used))
333+
continue; /* need not update this column */
312334
}
313-
}
314335

315-
MemoryContextSwitchTo(oldContext);
336+
/* No luck, so prepare the expression for execution */
337+
resultRelInfo->ri_GeneratedExprs[i] = ExecPrepareExpr(expr, estate);
338+
resultRelInfo->ri_NumGeneratedNeeded++;
339+
340+
/* And mark this column in rextra->ri_extraUpdatedCols */
341+
rextra->ri_extraUpdatedCols =
342+
bms_add_member(rextra->ri_extraUpdatedCols,
343+
i + 1 - FirstLowInvalidHeapAttributeNumber);
344+
}
316345
}
317346

347+
MemoryContextSwitchTo(oldContext);
348+
}
349+
350+
/*
351+
* Compute stored generated columns for a tuple
352+
*/
353+
void
354+
ExecComputeStoredGenerated(ResultRelInfo *resultRelInfo,
355+
EState *estate, TupleTableSlot *slot,
356+
CmdType cmdtype)
357+
{
358+
Relation rel = resultRelInfo->ri_RelationDesc;
359+
TupleDesc tupdesc = RelationGetDescr(rel);
360+
int natts = tupdesc->natts;
361+
ExprContext *econtext = GetPerTupleExprContext(estate);
362+
MemoryContext oldContext;
363+
Datum *values;
364+
bool *nulls;
365+
366+
/* We should not be called unless this is true */
367+
Assert(tupdesc->constr && tupdesc->constr->has_generated_stored);
368+
369+
/*
370+
* For relations named directly in the query, ExecInitStoredGenerated
371+
* should have been called already; but this might not have happened yet
372+
* for a partition child rel. Also, it's convenient for outside callers
373+
* to not have to call ExecInitStoredGenerated explicitly.
374+
*/
375+
if (resultRelInfo->ri_GeneratedExprs == NULL)
376+
ExecInitStoredGenerated(resultRelInfo, estate, cmdtype);
377+
318378
/*
319379
* If no generated columns have been affected by this change, then skip
320380
* the rest.
@@ -334,14 +394,13 @@ ExecComputeStoredGenerated(ResultRelInfo *resultRelInfo,
334394
{
335395
Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
336396

337-
if (attr->attgenerated == ATTRIBUTE_GENERATED_STORED &&
338-
resultRelInfo->ri_GeneratedExprs[i])
397+
if (resultRelInfo->ri_GeneratedExprs[i])
339398
{
340-
ExprContext *econtext;
341399
Datum val;
342400
bool isnull;
343401

344-
econtext = GetPerTupleExprContext(estate);
402+
Assert(attr->attgenerated == ATTRIBUTE_GENERATED_STORED);
403+
345404
econtext->ecxt_scantuple = slot;
346405

347406
val = ExecEvalExpr(resultRelInfo->ri_GeneratedExprs[i], econtext, &isnull);
@@ -2934,6 +2993,15 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
29342993
elog(ERROR, "could not find junk wholerow column");
29352994
}
29362995
}
2996+
2997+
/*
2998+
* For INSERT and UPDATE, prepare to evaluate any generated columns.
2999+
* We must do this now, even if we never insert or update any rows,
3000+
* because we have to fill resultRelInfo->ri_extraUpdatedCols for
3001+
* possible use by the trigger machinery.
3002+
*/
3003+
if (operation == CMD_INSERT || operation == CMD_UPDATE)
3004+
ExecInitStoredGenerated(resultRelInfo, estate, operation);
29373005
}
29383006

29393007
/*

0 commit comments

Comments
 (0)