Skip to content

Commit 7fee787

Browse files
tglsfdctglsfdc
committed
Fix some more cases of missed GENERATED-column updates.
If UPDATE is forced to retry after an EvalPlanQual check, it neglected to repeat GENERATED-column computations, even though those might well have changed since we're dealing with a different tuple than before. Fixing this is mostly a matter of looping back a bit further when we retry. In v15 and HEAD that's most easily done by altering the API of ExecUpdateAct so that it includes computing GENERATED expressions. Also, if an UPDATE in a partitioned table turns into a cross-partition INSERT operation, we failed to recompute GENERATED columns. That's a bug since 8bf6ec3 allowed partitions to have different generation expressions; although it seems to have no ill effects before that. Fixing this is messier because we can now have situations where the same query needs both the UPDATE-aligned set of GENERATED columns and the INSERT-aligned set, and it's unclear which set will be generated first (else we could hack things by forcing the INSERT-aligned set to be generated, which is indeed how fe9e658 made it work for MERGE). The best fix seems to be to build and store separate sets of expressions for the INSERT and UPDATE cases. That would create ABI issues in the back branches, but so far it seems we can leave this alone in the back branches. Per bug #17823 from Hisahiro Kauchi. The first part of this affects all branches back to v12 where GENERATED columns were added. Discussion: https://postgr.es/m/17823-b64909cf7d63de84@postgresql.org
1 parent d937904 commit 7fee787

File tree

8 files changed

+280
-207
lines changed

8 files changed

+280
-207
lines changed

‎src/backend/executor/execMain.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1232,7 +1232,8 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
12321232
resultRelInfo->ri_FdwState = NULL;
12331233
resultRelInfo->ri_usesFdwDirectModify = false;
12341234
resultRelInfo->ri_ConstraintExprs = NULL;
1235-
resultRelInfo->ri_GeneratedExprs = NULL;
1235+
resultRelInfo->ri_GeneratedExprsI = NULL;
1236+
resultRelInfo->ri_GeneratedExprsU = NULL;
12361237
resultRelInfo->ri_projectReturning = NULL;
12371238
resultRelInfo->ri_onConflictArbiterIndexes = NIL;
12381239
resultRelInfo->ri_onConflict = NULL;

‎src/backend/executor/execUtils.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1339,8 +1339,8 @@ ExecGetUpdatedCols(ResultRelInfo *relinfo, EState *estate)
13391339
Bitmapset *
13401340
ExecGetExtraUpdatedCols(ResultRelInfo *relinfo, EState *estate)
13411341
{
1342-
/* In some code paths we can reach here before initializing the info */
1343-
if (relinfo->ri_GeneratedExprs == NULL)
1342+
/* Compute the info if we didn't already */
1343+
if (relinfo->ri_GeneratedExprsU == NULL)
13441344
ExecInitStoredGenerated(relinfo, estate, CMD_UPDATE);
13451345
return relinfo->ri_extraUpdatedCols;
13461346
}

‎src/backend/executor/nodeModifyTable.c

Lines changed: 75 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -356,9 +356,14 @@ ExecCheckTIDVisible(EState *estate,
356356
/*
357357
* Initialize to compute stored generated columns for a tuple
358358
*
359-
* This fills the resultRelInfo's ri_GeneratedExprs and ri_extraUpdatedCols
360-
* fields. (Currently, ri_extraUpdatedCols is consulted only in UPDATE,
361-
* but we might as well fill it for INSERT too.)
359+
* This fills the resultRelInfo's ri_GeneratedExprsI/ri_NumGeneratedNeededI
360+
* or ri_GeneratedExprsU/ri_NumGeneratedNeededU fields, depending on cmdtype.
361+
* If cmdType == CMD_UPDATE, the ri_extraUpdatedCols field is filled too.
362+
*
363+
* Note: usually, a given query would need only one of ri_GeneratedExprsI and
364+
* ri_GeneratedExprsU per result rel; but MERGE can need both, and so can
365+
* cross-partition UPDATEs, since a partition might be the target of both
366+
* UPDATE and INSERT actions.
362367
*/
363368
void
364369
ExecInitStoredGenerated(ResultRelInfo *resultRelInfo,
@@ -368,12 +373,11 @@ ExecInitStoredGenerated(ResultRelInfo *resultRelInfo,
368373
Relation rel = resultRelInfo->ri_RelationDesc;
369374
TupleDesc tupdesc = RelationGetDescr(rel);
370375
int natts = tupdesc->natts;
376+
ExprState **ri_GeneratedExprs;
377+
int ri_NumGeneratedNeeded;
371378
Bitmapset *updatedCols;
372379
MemoryContext oldContext;
373380

374-
/* Don't call twice */
375-
Assert(resultRelInfo->ri_GeneratedExprs == NULL);
376-
377381
/* Nothing to do if no generated columns */
378382
if (!(tupdesc->constr && tupdesc->constr->has_generated_stored))
379383
return;
@@ -396,9 +400,8 @@ ExecInitStoredGenerated(ResultRelInfo *resultRelInfo,
396400
*/
397401
oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
398402

399-
resultRelInfo->ri_GeneratedExprs =
400-
(ExprState **) palloc0(natts * sizeof(ExprState *));
401-
resultRelInfo->ri_NumGeneratedNeeded = 0;
403+
ri_GeneratedExprs = (ExprState **) palloc0(natts * sizeof(ExprState *));
404+
ri_NumGeneratedNeeded = 0;
402405

403406
for (int i = 0; i < natts; i++)
404407
{
@@ -427,16 +430,35 @@ ExecInitStoredGenerated(ResultRelInfo *resultRelInfo,
427430
}
428431

429432
/* No luck, so prepare the expression for execution */
430-
resultRelInfo->ri_GeneratedExprs[i] = ExecPrepareExpr(expr, estate);
431-
resultRelInfo->ri_NumGeneratedNeeded++;
432-
433-
/* And mark this column in resultRelInfo->ri_extraUpdatedCols */
434-
resultRelInfo->ri_extraUpdatedCols =
435-
bms_add_member(resultRelInfo->ri_extraUpdatedCols,
436-
i + 1 - FirstLowInvalidHeapAttributeNumber);
433+
ri_GeneratedExprs[i] = ExecPrepareExpr(expr, estate);
434+
ri_NumGeneratedNeeded++;
435+
436+
/* If UPDATE, mark column in resultRelInfo->ri_extraUpdatedCols */
437+
if (cmdtype == CMD_UPDATE)
438+
resultRelInfo->ri_extraUpdatedCols =
439+
bms_add_member(resultRelInfo->ri_extraUpdatedCols,
440+
i + 1 - FirstLowInvalidHeapAttributeNumber);
437441
}
438442
}
439443

444+
/* Save in appropriate set of fields */
445+
if (cmdtype == CMD_UPDATE)
446+
{
447+
/* Don't call twice */
448+
Assert(resultRelInfo->ri_GeneratedExprsU == NULL);
449+
450+
resultRelInfo->ri_GeneratedExprsU = ri_GeneratedExprs;
451+
resultRelInfo->ri_NumGeneratedNeededU = ri_NumGeneratedNeeded;
452+
}
453+
else
454+
{
455+
/* Don't call twice */
456+
Assert(resultRelInfo->ri_GeneratedExprsI == NULL);
457+
458+
resultRelInfo->ri_GeneratedExprsI = ri_GeneratedExprs;
459+
resultRelInfo->ri_NumGeneratedNeededI = ri_NumGeneratedNeeded;
460+
}
461+
440462
MemoryContextSwitchTo(oldContext);
441463
}
442464

@@ -452,6 +474,7 @@ ExecComputeStoredGenerated(ResultRelInfo *resultRelInfo,
452474
TupleDesc tupdesc = RelationGetDescr(rel);
453475
int natts = tupdesc->natts;
454476
ExprContext *econtext = GetPerTupleExprContext(estate);
477+
ExprState **ri_GeneratedExprs;
455478
MemoryContext oldContext;
456479
Datum *values;
457480
bool *nulls;
@@ -460,20 +483,25 @@ ExecComputeStoredGenerated(ResultRelInfo *resultRelInfo,
460483
Assert(tupdesc->constr && tupdesc->constr->has_generated_stored);
461484

462485
/*
463-
* For relations named directly in the query, ExecInitStoredGenerated
464-
* should have been called already; but this might not have happened yet
465-
* for a partition child rel. Also, it's convenient for outside callers
466-
* to not have to call ExecInitStoredGenerated explicitly.
467-
*/
468-
if (resultRelInfo->ri_GeneratedExprs == NULL)
469-
ExecInitStoredGenerated(resultRelInfo, estate, cmdtype);
470-
471-
/*
472-
* If no generated columns have been affected by this change, then skip
473-
* the rest.
486+
* Initialize the expressions if we didn't already, and check whether we
487+
* can exit early because nothing needs to be computed.
474488
*/
475-
if (resultRelInfo->ri_NumGeneratedNeeded == 0)
476-
return;
489+
if (cmdtype == CMD_UPDATE)
490+
{
491+
if (resultRelInfo->ri_GeneratedExprsU == NULL)
492+
ExecInitStoredGenerated(resultRelInfo, estate, cmdtype);
493+
if (resultRelInfo->ri_NumGeneratedNeededU == 0)
494+
return;
495+
ri_GeneratedExprs = resultRelInfo->ri_GeneratedExprsU;
496+
}
497+
else
498+
{
499+
if (resultRelInfo->ri_GeneratedExprsI == NULL)
500+
ExecInitStoredGenerated(resultRelInfo, estate, cmdtype);
501+
/* Early exit is impossible given the prior Assert */
502+
Assert(resultRelInfo->ri_NumGeneratedNeededI > 0);
503+
ri_GeneratedExprs = resultRelInfo->ri_GeneratedExprsI;
504+
}
477505

478506
oldContext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
479507

@@ -487,7 +515,7 @@ ExecComputeStoredGenerated(ResultRelInfo *resultRelInfo,
487515
{
488516
Form_pg_attribute attr = TupleDescAttr(tupdesc, i);
489517

490-
if (resultRelInfo->ri_GeneratedExprs[i])
518+
if (ri_GeneratedExprs[i])
491519
{
492520
Datum val;
493521
bool isnull;
@@ -496,7 +524,7 @@ ExecComputeStoredGenerated(ResultRelInfo *resultRelInfo,
496524

497525
econtext->ecxt_scantuple = slot;
498526

499-
val = ExecEvalExpr(resultRelInfo->ri_GeneratedExprs[i], econtext, &isnull);
527+
val = ExecEvalExpr(ri_GeneratedExprs[i], econtext, &isnull);
500528

501529
/*
502530
* We must make a copy of val as we have no guarantees about where
@@ -1910,9 +1938,10 @@ ExecUpdatePrologue(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
19101938
}
19111939

19121940
/*
1913-
* ExecUpdatePrepareSlot -- subroutine for ExecUpdate
1941+
* ExecUpdatePrepareSlot -- subroutine for ExecUpdateAct
19141942
*
19151943
* Apply the final modifications to the tuple slot before the update.
1944+
* (This is split out because we also need it in the foreign-table code path.)
19161945
*/
19171946
static void
19181947
ExecUpdatePrepareSlot(ResultRelInfo *resultRelInfo,
@@ -1962,13 +1991,14 @@ ExecUpdateAct(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
19621991
updateCxt->crossPartUpdate = false;
19631992

19641993
/*
1965-
* If we generate a new candidate tuple after EvalPlanQual testing, we
1966-
* must loop back here and recheck any RLS policies and constraints. (We
1967-
* don't need to redo triggers, however. If there are any BEFORE triggers
1968-
* then trigger.c will have done table_tuple_lock to lock the correct
1969-
* tuple, so there's no need to do them again.)
1994+
* If we move the tuple to a new partition, we loop back here to recompute
1995+
* GENERATED values (which are allowed to be different across partitions)
1996+
* and recheck any RLS policies and constraints. We do not fire any
1997+
* BEFORE triggers of the new partition, however.
19701998
*/
19711999
lreplace:
2000+
/* Fill in GENERATEd columns */
2001+
ExecUpdatePrepareSlot(resultRelInfo, slot, estate);
19722002

19732003
/* ensure slot is independent, consider e.g. EPQ */
19742004
ExecMaterializeSlot(slot);
@@ -2268,6 +2298,7 @@ ExecUpdate(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
22682298
}
22692299
else if (resultRelInfo->ri_FdwRoutine)
22702300
{
2301+
/* Fill in GENERATEd columns */
22712302
ExecUpdatePrepareSlot(resultRelInfo, slot, estate);
22722303

22732304
/*
@@ -2290,9 +2321,13 @@ ExecUpdate(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
22902321
}
22912322
else
22922323
{
2293-
/* Fill in the slot appropriately */
2294-
ExecUpdatePrepareSlot(resultRelInfo, slot, estate);
2295-
2324+
/*
2325+
* If we generate a new candidate tuple after EvalPlanQual testing, we
2326+
* must loop back here to try again. (We don't need to redo triggers,
2327+
* however. If there are any BEFORE triggers then trigger.c will have
2328+
* done table_tuple_lock to lock the correct tuple, so there's no need
2329+
* to do them again.)
2330+
*/
22962331
redo_act:
22972332
result = ExecUpdateAct(context, resultRelInfo, tupleid, oldtuple, slot,
22982333
canSetTag, &updateCxt);
@@ -2876,7 +2911,6 @@ ExecMergeMatched(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
28762911
result = TM_Ok;
28772912
break;
28782913
}
2879-
ExecUpdatePrepareSlot(resultRelInfo, newslot, context->estate);
28802914
result = ExecUpdateAct(context, resultRelInfo, tupleid, NULL,
28812915
newslot, false, &updateCxt);
28822916
if (result == TM_Ok && updateCxt.updated)
@@ -4135,15 +4169,6 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
41354169
elog(ERROR, "could not find junk wholerow column");
41364170
}
41374171
}
4138-
4139-
/*
4140-
* For INSERT/UPDATE/MERGE, prepare to evaluate any generated columns.
4141-
* We must do this now, even if we never insert or update any rows,
4142-
* because we have to fill resultRelInfo->ri_extraUpdatedCols for
4143-
* possible use by the trigger machinery.
4144-
*/
4145-
if (operation == CMD_INSERT || operation == CMD_UPDATE || operation == CMD_MERGE)
4146-
ExecInitStoredGenerated(resultRelInfo, estate, operation);
41474172
}
41484173

41494174
/*

‎src/include/nodes/execnodes.h

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -462,7 +462,7 @@ typedef struct ResultRelInfo
462462
*/
463463
AttrNumber ri_RowIdAttNo;
464464

465-
/* For INSERT/UPDATE, attnums of generated columns to be computed */
465+
/* For UPDATE, attnums of generated columns to be computed */
466466
Bitmapset *ri_extraUpdatedCols;
467467

468468
/* Projection to generate new tuple in an INSERT/UPDATE */
@@ -516,11 +516,13 @@ typedef struct ResultRelInfo
516516
/* array of constraint-checking expr states */
517517
ExprState **ri_ConstraintExprs;
518518

519-
/* array of stored generated columns expr states */
520-
ExprState **ri_GeneratedExprs;
519+
/* arrays of stored generated columns expr states, for INSERT and UPDATE */
520+
ExprState **ri_GeneratedExprsI;
521+
ExprState **ri_GeneratedExprsU;
521522

522523
/* number of stored generated columns we need to compute */
523-
int ri_NumGeneratedNeeded;
524+
int ri_NumGeneratedNeededI;
525+
int ri_NumGeneratedNeededU;
524526

525527
/* list of RETURNING expressions */
526528
List *ri_returningList;

0 commit comments

Comments
 (0)