From fd18867d91b2fedce631b0f6574d13216016264e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= Date: Wed, 19 Mar 2025 16:46:58 +0100 Subject: [PATCH 3/3] Some code review --- src/backend/commands/tablecmds.c | 57 +++++++---------- src/backend/executor/execMain.c | 105 ++++++++++++++++++------------- src/include/executor/executor.h | 8 +-- 3 files changed, 89 insertions(+), 81 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index e49a4a4ad7d..32890fa5453 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -6092,7 +6092,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap) TupleDesc newTupDesc; bool needscan = false; List *notnull_attrs; - List *all_virtual_nns = NIL; + List *notnull_virtual_attrs; int i; ListCell *l; EState *estate; @@ -6177,35 +6177,33 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap) ex->exprstate = ExecInitExpr((Expr *) ex->expr, NULL); } - notnull_attrs = NIL; + notnull_attrs = notnull_virtual_attrs = NIL; if (newrel || tab->verify_new_notnull) { /* * If we are rebuilding the tuples OR if we added any new but not * verified not-null constraints, check all not-null constraints. This * is a bit of overkill but it minimizes risk of bugs. + * + * Collect attribute numbers on virtual generated columns separately. */ for (i = 0; i < newTupDesc->natts; i++) { Form_pg_attribute attr = TupleDescAttr(newTupDesc, i); if (attr->attnotnull && !attr->attisdropped) - notnull_attrs = lappend_int(notnull_attrs, i); + { + if (attr->attgenerated != ATTRIBUTE_GENERATED_VIRTUAL) + notnull_attrs = lappend_int(notnull_attrs, i); + else + notnull_virtual_attrs = lappend_int(notnull_virtual_attrs, + attr->attnum); + } } - if (notnull_attrs) + if (notnull_attrs || notnull_virtual_attrs) needscan = true; } - foreach_int(attn, notnull_attrs) - { - Form_pg_attribute attr = TupleDescAttr(newTupDesc, attn); - - Assert(attr->attnotnull); - Assert(!attr->attisdropped); - if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) - all_virtual_nns = lappend_int(all_virtual_nns, attr->attnum); - } - if (newrel || needscan) { ExprContext *econtext; @@ -6217,10 +6215,15 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap) ListCell *lc; Snapshot snapshot; ResultRelInfo *rInfo = NULL; - MemoryContext oldcontext; - if (list_length(all_virtual_nns) > 0) + /* + * XXX this deserves an explanation. Also, is rInfo a good variable + * name? + */ + if (notnull_virtual_attrs != NIL) { + MemoryContext oldcontext; + Assert(newTupDesc->constr->has_generated_virtual); Assert(newTupDesc->constr->has_not_null); oldcontext = MemoryContextSwitchTo(estate->es_query_cxt); @@ -6314,7 +6317,6 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap) while (table_scan_getnextslot(scan, ForwardScanDirection, oldslot)) { TupleTableSlot *insertslot; - bool virtual_notnull_check = false; if (tab->rewrite > 0) { @@ -6406,17 +6408,6 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap) { Form_pg_attribute attr = TupleDescAttr(newTupDesc, attn); - /* - * virtual generated column not null constraint handled - * below - */ - - if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) - { - if (!virtual_notnull_check) - virtual_notnull_check = true; - continue; - } ereport(ERROR, (errcode(ERRCODE_NOT_NULL_VIOLATION), errmsg("column \"%s\" of relation \"%s\" contains null values", @@ -6426,16 +6417,14 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap) } } - if (virtual_notnull_check) + if (notnull_virtual_attrs != NIL) { - int attnum = -1; - - Assert(list_length(all_virtual_nns) > 0); + AttrNumber attnum; attnum = ExecRelCheckGenVirtualNotNull(rInfo, insertslot, estate, - all_virtual_nns); - if (attnum > 0) + notnull_virtual_attrs); + if (attnum != InvalidAttrNumber) { Form_pg_attribute attr = TupleDescAttr(newTupDesc, attnum - 1); diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 32e1978a724..0cefb88aaba 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1842,44 +1842,54 @@ ExecutePlan(QueryDesc *queryDesc, ExitParallelMode(); } - /* - * we warp "virtual_generated col IS NOT NULL" to a NullTest node. - * NullTest->arg is the virtual generated expression. return value of -1 means - * ok. return value > 0 means not null violation happended for that attribute - * number. - * all_virtual_nns is *all* the virtual generated column not-null attribute numbers. + * Verify not-null constraints on virtual generated columns of the given + * tuple slot. * -*/ -int + * Return value of InvalidAttrNumber means all not-null constraints on virtual + * generated columns are satisfied. A return value > 0 means a not-null + * violation happened for that attribute. + * + * all_virtual_nns is the list of the attnums of virtual generated column with + * not-null constraints. + */ +AttrNumber ExecRelCheckGenVirtualNotNull(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate, List *all_virtual_nns) { Relation rel = resultRelInfo->ri_RelationDesc; ExprContext *econtext; MemoryContext oldContext; - int i = 0; - int cnt = list_length(all_virtual_nns); + + /* + * We implement this by consing up a NullTest node for each virtual + * generated column, which we cache in resultRelInfo, and running those + * through ExecCheck. + * + * XXX are there cases where we need ->argisrow = true? + */ if (resultRelInfo->ri_GeneratedNotNullExprs == NULL) { + int cnt = list_length(all_virtual_nns); + oldContext = MemoryContextSwitchTo(estate->es_query_cxt); resultRelInfo->ri_GeneratedNotNullExprs = (ExprState **) palloc0(cnt * sizeof(ExprState *)); - foreach_int(attidx, all_virtual_nns) + foreach_int(attnum, all_virtual_nns) { - Expr *expr; + int i = foreach_current_index(attnum); NullTest *nnulltest; nnulltest = makeNode(NullTest); - nnulltest->arg = (Expr *) build_generation_expression(rel, attidx); + nnulltest->arg = (Expr *) build_generation_expression(rel, attnum); nnulltest->nulltesttype = IS_NOT_NULL; nnulltest->argisrow = false; nnulltest->location = -1; - expr = (Expr *) nnulltest; - resultRelInfo->ri_GeneratedNotNullExprs[i++] = ExecPrepareExpr(expr, estate); + resultRelInfo->ri_GeneratedNotNullExprs[i] = + ExecPrepareExpr((Expr *) nnulltest, estate); } MemoryContextSwitchTo(oldContext); } @@ -1895,17 +1905,18 @@ ExecRelCheckGenVirtualNotNull(ResultRelInfo *resultRelInfo, TupleTableSlot *slot econtext->ecxt_scantuple = slot; /* And evaluate the check constraints for virtual generated column */ - i = 0; foreach_int(attnum, all_virtual_nns) { - ExprState *exprstate = resultRelInfo->ri_GeneratedNotNullExprs[i++]; + int i = foreach_current_index(attnum); + ExprState *exprstate = resultRelInfo->ri_GeneratedNotNullExprs[i]; - if (exprstate && !ExecCheck(exprstate, econtext)) + Assert(exprstate != NULL); + if (!ExecCheck(exprstate, econtext)) return attnum; } - /* -1 result means no error */ - return -1; + /* InvalidAttrNumber result means no error */ + return InvalidAttrNumber; } /* @@ -2106,9 +2117,11 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo, errtable(resultRelInfo->ri_RelationDesc))); } -/* not null constraint violation happened, now format the error message */ +/* + * Report a violation of a not-null constraint that was already detected. + */ static void -NotNullViolationErrorReport(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, +ReportNotNullViolationError(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate, int attrChk) { Bitmapset *modifiedCols; @@ -2188,20 +2201,22 @@ ExecConstraints(ResultRelInfo *resultRelInfo, TupleDesc tupdesc = RelationGetDescr(rel); TupleConstr *constr = tupdesc->constr; Bitmapset *modifiedCols; - Form_pg_attribute att; - int natts; - int attnum; List *all_virtual_nns = NIL; bool virtual_notnull_check = false; Assert(constr); /* we should not be called otherwise */ - natts = tupdesc->natts; + /* + * First, go over all the not-null constraints and verify and possibly + * throw errors for all of those that aren't on virtual generated columns. + * (The latter need executor support and a precise list of columns to + * handle, so we accumulate that here and initialize separately.) + */ if (constr->has_not_null) { - for (attnum = 1; attnum <= natts; attnum++) + for (AttrNumber attnum = 1; attnum <= tupdesc->natts; attnum++) { - att = TupleDescAttr(tupdesc, attnum - 1); + Form_pg_attribute att = TupleDescAttr(tupdesc, attnum - 1); if (att->attnotnull && att->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) all_virtual_nns = lappend_int(all_virtual_nns, att->attnum); @@ -2214,34 +2229,33 @@ ExecConstraints(ResultRelInfo *resultRelInfo, virtual_notnull_check = true; continue; } - NotNullViolationErrorReport(resultRelInfo, slot, estate, attnum); + + ReportNotNullViolationError(resultRelInfo, slot, estate, attnum); } } } - /* check virtual generated column not null constraint */ + /* Check not-null constraints on virtual generated column, if any */ if (virtual_notnull_check) { + AttrNumber attnum; + Assert(constr->has_not_null); Assert(constr->has_generated_virtual); - Assert(list_length(all_virtual_nns) > 0); + Assert(all_virtual_nns != NIL); - attnum = -1; - attnum = ExecRelCheckGenVirtualNotNull(resultRelInfo, slot, estate, all_virtual_nns); - - /* - * constraint evaulation return falsem do error report, also make an - * Assert - */ - if (attnum > 0) + attnum = ExecRelCheckGenVirtualNotNull(resultRelInfo, slot, estate, + all_virtual_nns); + if (attnum != InvalidAttrNumber) { - att = TupleDescAttr(tupdesc, attnum - 1); - Assert(att->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL); + Form_pg_attribute att = TupleDescAttr(tupdesc, attnum - 1); - NotNullViolationErrorReport(resultRelInfo, slot, estate, att->attnum); + Assert(att->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL); + ReportNotNullViolationError(resultRelInfo, slot, estate, att->attnum); } } + /* Last, verify any CHECK constraints */ if (rel->rd_rel->relchecks > 0) { const char *failed; @@ -2251,7 +2265,12 @@ ExecConstraints(ResultRelInfo *resultRelInfo, char *val_desc; Relation orig_rel = rel; - /* See the comment above. */ + /* + * If the tuple has been routed, it's been converted to the + * partition's rowtype, which might differ from the root table's. + * We must convert it back to the root table's rowtype so that + * val_desc shown error message matches the input tuple. + */ if (resultRelInfo->ri_RootResultRelInfo) { ResultRelInfo *rootrel = resultRelInfo->ri_RootResultRelInfo; diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 7cfedd14c9f..cd7f2266af7 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -220,10 +220,10 @@ extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid, extern List *ExecGetAncestorResultRels(EState *estate, ResultRelInfo *resultRelInfo); extern void ExecConstraints(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate); -extern int ExecRelCheckGenVirtualNotNull(ResultRelInfo *resultRelInfo, - TupleTableSlot *slot, - EState *estate, - List *all_virtual_nns); +extern AttrNumber ExecRelCheckGenVirtualNotNull(ResultRelInfo *resultRelInfo, + TupleTableSlot *slot, + EState *estate, + List *all_virtual_nns); extern bool ExecPartitionCheck(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate, bool emitError); extern void ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo, -- 2.39.5