From 8cd6ef09a740454743d481304d626ed86c3f2c71 Mon Sep 17 00:00:00 2001 From: Dean Rasheed Date: Wed, 19 Nov 2025 13:45:12 +0000 Subject: [PATCH v12 2/2] More suggested review comments. - Fix a few code comments. - Reduce code duplication in executor. - Rename "lockingStrength" to "lockStrength" (feels slightly better to me). - Remove unneeded "if" from rewriteTargetView() (should reduce final patch size). --- contrib/postgres_fdw/postgres_fdw.c | 2 +- src/backend/commands/explain.c | 6 +- src/backend/executor/execPartition.c | 204 +++++++++--------------- src/backend/executor/nodeModifyTable.c | 96 +++++------ src/backend/optimizer/plan/createplan.c | 8 +- src/backend/optimizer/util/plancat.c | 14 +- src/backend/parser/analyze.c | 8 +- src/backend/parser/gram.y | 6 +- src/backend/parser/parse_clause.c | 21 +-- src/backend/rewrite/rewriteHandler.c | 29 ++-- src/backend/utils/adt/ruleutils.c | 4 +- src/include/nodes/execnodes.h | 5 +- src/include/nodes/parsenodes.h | 9 +- src/include/nodes/plannodes.h | 2 +- src/include/nodes/primnodes.h | 15 +- src/test/regress/expected/rules.out | 2 +- 16 files changed, 179 insertions(+), 252 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 06b52c65300..b793669d97f 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -1856,7 +1856,7 @@ postgresPlanForeignModify(PlannerInfo *root, returningList = (List *) list_nth(plan->returningLists, subplan_index); /* - * ON CONFLICT DO UPDATE and DO NOTHING case with inference specification + * ON CONFLICT DO NOTHING/UPDATE/SELECT with inference specification * should have already been rejected in the optimizer, as presently there * is no way to recognize an arbiter index on a foreign table. Only DO * NOTHING is supported without an inference specification. diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index 1a575cc96e8..10e636d465e 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -4679,7 +4679,7 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors, else { Assert(node->onConflictAction == ONCONFLICT_SELECT); - switch (node->onConflictLockingStrength) + switch (node->onConflictLockStrength) { case LCS_NONE: resolution = "SELECT"; @@ -4696,10 +4696,6 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors, case LCS_FORUPDATE: resolution = "SELECT FOR UPDATE"; break; - default: - elog(ERROR, "unrecognized LockClauseStrength %d", - (int) node->onConflictLockingStrength); - break; } } diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index a8f7d1dc5bd..0868229328b 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -731,20 +731,27 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, leaf_part_rri->ri_onConflictArbiterIndexes = arbiterIndexes; /* - * In the DO UPDATE case, we have some more state to initialize. + * In the DO UPDATE and DO SELECT cases, we have some more state to + * initialize. */ - if (node->onConflictAction == ONCONFLICT_UPDATE) + if (node->onConflictAction == ONCONFLICT_UPDATE || + node->onConflictAction == ONCONFLICT_SELECT) { OnConflictActionState *onconfl = makeNode(OnConflictActionState); TupleConversionMap *map; map = ExecGetRootToChildMap(leaf_part_rri, estate); - Assert(node->onConflictSet != NIL); + Assert(node->onConflictSet != NIL || + node->onConflictAction == ONCONFLICT_SELECT); Assert(rootResultRelInfo->ri_onConflict != NULL); leaf_part_rri->ri_onConflict = onconfl; + /* Lock strength for DO SELECT [FOR UPDATE/SHARE] */ + onconfl->oc_LockStrength = + rootResultRelInfo->ri_onConflict->oc_LockStrength; + /* * Need a separate existing slot for each partition, as the * partition could be of a different AM, even if the tuple @@ -757,7 +764,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, /* * If the partition's tuple descriptor matches exactly the root * parent (the common case), we can re-use most of the parent's ON - * CONFLICT SET state, skipping a bunch of work. Otherwise, we + * CONFLICT action state, skipping a bunch of work. Otherwise, we * need to create state specific to this partition. */ if (map == NULL) @@ -765,7 +772,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, /* * It's safe to reuse these from the partition root, as we * only process one tuple at a time (therefore we won't - * overwrite needed data in slots), and the results of + * overwrite needed data in slots), and the results of any * projections are independent of the underlying storage. * Projections and where clauses themselves don't store state * / are independent of the underlying storage. @@ -779,66 +786,81 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, } else { - List *onconflset; - List *onconflcols; - /* - * Translate expressions in onConflictSet to account for - * different attribute numbers. For that, map partition - * varattnos twice: first to catch the EXCLUDED - * pseudo-relation (INNER_VAR), and second to handle the main - * target relation (firstVarno). + * For ON CONFLICT DO UPDATE, translate expressions in + * onConflictSet to account for different attribute numbers. + * For that, map partition varattnos twice: first to catch the + * EXCLUDED pseudo-relation (INNER_VAR), and second to handle + * the main target relation (firstVarno). */ - onconflset = copyObject(node->onConflictSet); - if (part_attmap == NULL) - part_attmap = - build_attrmap_by_name(RelationGetDescr(partrel), - RelationGetDescr(firstResultRel), - false); - onconflset = (List *) - map_variable_attnos((Node *) onconflset, - INNER_VAR, 0, - part_attmap, - RelationGetForm(partrel)->reltype, - &found_whole_row); - /* We ignore the value of found_whole_row. */ - onconflset = (List *) - map_variable_attnos((Node *) onconflset, - firstVarno, 0, - part_attmap, - RelationGetForm(partrel)->reltype, - &found_whole_row); - /* We ignore the value of found_whole_row. */ - - /* Finally, adjust the target colnos to match the partition. */ - onconflcols = adjust_partition_colnos(node->onConflictCols, - leaf_part_rri); - - /* create the tuple slot for the UPDATE SET projection */ - onconfl->oc_ProjSlot = - table_slot_create(partrel, - &mtstate->ps.state->es_tupleTable); + if (node->onConflictAction == ONCONFLICT_UPDATE) + { + List *onconflset; + List *onconflcols; + + onconflset = copyObject(node->onConflictSet); + if (part_attmap == NULL) + part_attmap = + build_attrmap_by_name(RelationGetDescr(partrel), + RelationGetDescr(firstResultRel), + false); + onconflset = (List *) + map_variable_attnos((Node *) onconflset, + INNER_VAR, 0, + part_attmap, + RelationGetForm(partrel)->reltype, + &found_whole_row); + /* We ignore the value of found_whole_row. */ + onconflset = (List *) + map_variable_attnos((Node *) onconflset, + firstVarno, 0, + part_attmap, + RelationGetForm(partrel)->reltype, + &found_whole_row); + /* We ignore the value of found_whole_row. */ - /* build UPDATE SET projection state */ - onconfl->oc_ProjInfo = - ExecBuildUpdateProjection(onconflset, - true, - onconflcols, - partrelDesc, - econtext, - onconfl->oc_ProjSlot, - &mtstate->ps); + /* + * Finally, adjust the target colnos to match the + * partition. + */ + onconflcols = adjust_partition_colnos(node->onConflictCols, + leaf_part_rri); + + /* create the tuple slot for the UPDATE SET projection */ + onconfl->oc_ProjSlot = + table_slot_create(partrel, + &mtstate->ps.state->es_tupleTable); + + /* build UPDATE SET projection state */ + onconfl->oc_ProjInfo = + ExecBuildUpdateProjection(onconflset, + true, + onconflcols, + partrelDesc, + econtext, + onconfl->oc_ProjSlot, + &mtstate->ps); + } /* - * If there is a WHERE clause, initialize state where it will - * be evaluated, mapping the attribute numbers appropriately. - * As with onConflictSet, we need to map partition varattnos - * to the partition's tupdesc. + * For both ON CONFLICT DO UPDATE and ON CONFLICT DO SELECT, + * there may be a WHERE clause. If so, initialize state where + * it will be evaluated, mapping the attribute numbers + * appropriately. As with onConflictSet, we need to map + * partition varattnos twice, to catch both the EXCLUDED + * pseudo-relation (INNER_VAR), and the main target relation + * (firstVarno). */ if (node->onConflictWhere) { List *clause; + if (part_attmap == NULL) + part_attmap = + build_attrmap_by_name(RelationGetDescr(partrel), + RelationGetDescr(firstResultRel), + false); + clause = copyObject((List *) node->onConflictWhere); clause = (List *) map_variable_attnos((Node *) clause, @@ -859,78 +881,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate, } } } - else if (node->onConflictAction == ONCONFLICT_SELECT) - { - OnConflictActionState *onconfl = makeNode(OnConflictActionState); - TupleConversionMap *map; - - map = ExecGetRootToChildMap(leaf_part_rri, estate); - Assert(rootResultRelInfo->ri_onConflict != NULL); - - leaf_part_rri->ri_onConflict = onconfl; - - onconfl->oc_LockingStrength = - rootResultRelInfo->ri_onConflict->oc_LockingStrength; - - /* - * Need a separate existing slot for each partition, as the - * partition could be of a different AM, even if the tuple - * descriptors match. - */ - onconfl->oc_Existing = - table_slot_create(leaf_part_rri->ri_RelationDesc, - &mtstate->ps.state->es_tupleTable); - - /* - * If the partition's tuple descriptor matches exactly the root - * parent (the common case), we can re-use the parent's ON - * CONFLICT DO SELECT state. Otherwise, we need to remap the - * WHERE clause for this partition's layout. - */ - if (map == NULL) - { - /* - * It's safe to reuse these from the partition root, as we - * only process one tuple at a time (therefore we won't - * overwrite needed data in slots), and the WHERE clause - * doesn't store state / is independent of the underlying - * storage. - */ - onconfl->oc_WhereClause = - rootResultRelInfo->ri_onConflict->oc_WhereClause; - } - else if (node->onConflictWhere) - { - /* - * Map the WHERE clause, if it exists. - */ - List *clause; - - if (part_attmap == NULL) - part_attmap = - build_attrmap_by_name(RelationGetDescr(partrel), - RelationGetDescr(firstResultRel), - false); - - clause = copyObject((List *) node->onConflictWhere); - clause = (List *) - map_variable_attnos((Node *) clause, - INNER_VAR, 0, - part_attmap, - RelationGetForm(partrel)->reltype, - &found_whole_row); - /* We ignore the value of found_whole_row. */ - clause = (List *) - map_variable_attnos((Node *) clause, - firstVarno, 0, - part_attmap, - RelationGetForm(partrel)->reltype, - &found_whole_row); - /* We ignore the value of found_whole_row. */ - onconfl->oc_WhereClause = - ExecInitQual(clause, &mtstate->ps); - } - } } /* diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 2939ab32c84..51d0d0a217c 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -2994,7 +2994,7 @@ ExecOnConflictUpdate(ModifyTableContext *context, * ExecOnConflictSelect --- execute SELECT of INSERT ON CONFLICT DO SELECT * * If SELECT FOR UPDATE/SHARE is specified, try to lock tuple as part of - * speculative insertion. If a qual originating from ON CONFLICT DO UPDATE is + * speculative insertion. If a qual originating from ON CONFLICT DO SELECT is * satisfied, select the row. * * Returns true if we're done (with or without a select), or false if the @@ -3013,7 +3013,7 @@ ExecOnConflictSelect(ModifyTableContext *context, Relation relation = resultRelInfo->ri_RelationDesc; ExprState *onConflictSelectWhere = resultRelInfo->ri_onConflict->oc_WhereClause; TupleTableSlot *existing = resultRelInfo->ri_onConflict->oc_Existing; - LockClauseStrength lockstrength = resultRelInfo->ri_onConflict->oc_LockingStrength; + LockClauseStrength lockStrength = resultRelInfo->ri_onConflict->oc_LockStrength; /* * Parse analysis should have blocked ON CONFLICT for all system @@ -3023,11 +3023,12 @@ ExecOnConflictSelect(ModifyTableContext *context, */ Assert(!resultRelInfo->ri_needLockTagTuple); - if (lockstrength != LCS_NONE) + /* Lock or fetch the existing tuple to select */ + if (lockStrength != LCS_NONE) { LockTupleMode lockmode; - switch (lockstrength) + switch (lockStrength) { case LCS_FORKEYSHARE: lockmode = LockTupleKeyShare; @@ -3042,7 +3043,7 @@ ExecOnConflictSelect(ModifyTableContext *context, lockmode = LockTupleExclusive; break; default: - elog(ERROR, "unexpected lock strength %d", lockstrength); + elog(ERROR, "unexpected lock strength %d", lockStrength); } if (!ExecOnConflictLockRow(context, existing, conflictTid, @@ -5217,75 +5218,60 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) } /* - * If needed, Initialize target list, projection and qual for ON CONFLICT - * DO UPDATE. + * For ON CONFLICT DO UPDATE/SELECT, initialize the ON CONFLICT action + * state. */ - if (node->onConflictAction == ONCONFLICT_UPDATE) + if (node->onConflictAction == ONCONFLICT_UPDATE || + node->onConflictAction == ONCONFLICT_SELECT) { OnConflictActionState *onconfl = makeNode(OnConflictActionState); - ExprContext *econtext; - TupleDesc relationDesc; /* already exists if created by RETURNING processing above */ if (mtstate->ps.ps_ExprContext == NULL) ExecAssignExprContext(estate, &mtstate->ps); - econtext = mtstate->ps.ps_ExprContext; - relationDesc = resultRelInfo->ri_RelationDesc->rd_att; - - /* create state for DO UPDATE SET operation */ + /* action state for DO UPDATE/SELECT */ resultRelInfo->ri_onConflict = onconfl; + /* lock strength for DO SELECT [FOR UPDATE/SHARE] */ + onconfl->oc_LockStrength = node->onConflictLockStrength; + /* initialize slot for the existing tuple */ onconfl->oc_Existing = table_slot_create(resultRelInfo->ri_RelationDesc, &mtstate->ps.state->es_tupleTable); /* - * Create the tuple slot for the UPDATE SET projection. We want a slot - * of the table's type here, because the slot will be used to insert - * into the table, and for RETURNING processing - which may access - * system attributes. + * For ON CONFLICT DO UPDATE, initialize target list and projection. */ - onconfl->oc_ProjSlot = - table_slot_create(resultRelInfo->ri_RelationDesc, - &mtstate->ps.state->es_tupleTable); - - /* build UPDATE SET projection state */ - onconfl->oc_ProjInfo = - ExecBuildUpdateProjection(node->onConflictSet, - true, - node->onConflictCols, - relationDesc, - econtext, - onconfl->oc_ProjSlot, - &mtstate->ps); - - /* initialize state to evaluate the WHERE clause, if any */ - if (node->onConflictWhere) + if (node->onConflictAction == ONCONFLICT_UPDATE) { - ExprState *qualexpr; + ExprContext *econtext; + TupleDesc relationDesc; - qualexpr = ExecInitQual((List *) node->onConflictWhere, - &mtstate->ps); - onconfl->oc_WhereClause = qualexpr; - } - } - else if (node->onConflictAction == ONCONFLICT_SELECT) - { - OnConflictActionState *onconfl = makeNode(OnConflictActionState); - - /* already exists if created by RETURNING processing above */ - if (mtstate->ps.ps_ExprContext == NULL) - ExecAssignExprContext(estate, &mtstate->ps); - - /* create state for DO SELECT operation */ - resultRelInfo->ri_onConflict = onconfl; + econtext = mtstate->ps.ps_ExprContext; + relationDesc = resultRelInfo->ri_RelationDesc->rd_att; - /* initialize slot for the existing tuple */ - onconfl->oc_Existing = - table_slot_create(resultRelInfo->ri_RelationDesc, - &mtstate->ps.state->es_tupleTable); + /* + * Create the tuple slot for the UPDATE SET projection. We want a + * slot of the table's type here, because the slot will be used to + * insert into the table, and for RETURNING processing - which may + * access system attributes. + */ + onconfl->oc_ProjSlot = + table_slot_create(resultRelInfo->ri_RelationDesc, + &mtstate->ps.state->es_tupleTable); + + /* build UPDATE SET projection state */ + onconfl->oc_ProjInfo = + ExecBuildUpdateProjection(node->onConflictSet, + true, + node->onConflictCols, + relationDesc, + econtext, + onconfl->oc_ProjSlot, + &mtstate->ps); + } /* initialize state to evaluate the WHERE clause, if any */ if (node->onConflictWhere) @@ -5296,8 +5282,6 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) &mtstate->ps); onconfl->oc_WhereClause = qualexpr; } - - onconfl->oc_LockingStrength = node->onConflictLockingStrength; } /* diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 52839dbbf2d..8f2586eeda1 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -7036,11 +7036,11 @@ make_modifytable(PlannerInfo *root, Plan *subplan, if (!onconflict) { node->onConflictAction = ONCONFLICT_NONE; + node->arbiterIndexes = NIL; + node->onConflictLockStrength = LCS_NONE; node->onConflictSet = NIL; node->onConflictCols = NIL; node->onConflictWhere = NULL; - node->onConflictLockingStrength = LCS_NONE; - node->arbiterIndexes = NIL; node->exclRelRTI = 0; node->exclRelTlist = NIL; } @@ -7048,6 +7048,9 @@ make_modifytable(PlannerInfo *root, Plan *subplan, { node->onConflictAction = onconflict->action; + /* Lock strength for ON CONFLICT SELECT [FOR UPDATE/SHARE] */ + node->onConflictLockStrength = onconflict->lockStrength; + /* * Here we convert the ON CONFLICT UPDATE tlist, if any, to the * executor's convention of having consecutive resno's. The actual @@ -7058,7 +7061,6 @@ make_modifytable(PlannerInfo *root, Plan *subplan, node->onConflictCols = extract_update_targetlist_colnos(node->onConflictSet); node->onConflictWhere = onconflict->onConflictWhere; - node->onConflictLockingStrength = onconflict->lockingStrength; /* * If a set of unique index inference elements was provided (an diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 0a0335fedb7..120c98b4cfa 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -923,16 +923,18 @@ infer_arbiter_indexes(PlannerInfo *root) */ if (indexOidFromConstraint == idxForm->indexrelid) { + /* + * ON CONFLICT DO UPDATE/SELECT are not supported with exclusion + * constraints (they require a unique index, to ensure that there + * is only one conflicting row to update/select). + */ if (idxForm->indisexclusion && (onconflict->action == ONCONFLICT_UPDATE || onconflict->action == ONCONFLICT_SELECT)) - /* INSERT into an exclusion constraint can conflict with multiple rows. - * So ON CONFLICT UPDATE OR SELECT would have to update/select mutliple rows - * in those cases. Which seems weird - so block it with an error. */ ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("ON CONFLICT DO %s not supported with exclusion constraints", - onconflict->action == ONCONFLICT_UPDATE ? "UPDATE" : "SELECT"))); + errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("ON CONFLICT DO %s not supported with exclusion constraints", + onconflict->action == ONCONFLICT_UPDATE ? "UPDATE" : "SELECT")); results = lappend_oid(results, idxForm->indexrelid); list_free(indexList); diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index a41516ee962..6ef3b9a4cf4 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -668,10 +668,14 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt) qry->override = stmt->override; + /* + * ON CONFLICT DO UPDATE and ON CONFLICT DO SELECT FOR UPDATE/SHARE + * require UPDATE permission on the target relation. + */ requiresUpdatePerm = (stmt->onConflictClause && (stmt->onConflictClause->action == ONCONFLICT_UPDATE || (stmt->onConflictClause->action == ONCONFLICT_SELECT && - stmt->onConflictClause->lockingStrength != LCS_NONE))); + stmt->onConflictClause->lockStrength != LCS_NONE))); /* * We have three cases to deal with: DEFAULT VALUES (selectStmt == NULL), @@ -1273,8 +1277,8 @@ transformOnConflictClause(ParseState *pstate, result->arbiterElems = arbiterElems; result->arbiterWhere = arbiterWhere; result->constraint = arbiterConstraint; + result->lockStrength = onConflictClause->lockStrength; result->onConflictSet = onConflictSet; - result->lockingStrength = onConflictClause->lockingStrength; result->onConflictWhere = onConflictWhere; result->exclRelIndex = exclRelIndex; result->exclRelTlist = exclRelTlist; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 316587a8420..13e6c0de342 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -12445,7 +12445,7 @@ opt_on_conflict: $$->action = ONCONFLICT_SELECT; $$->infer = $3; $$->targetList = NIL; - $$->lockingStrength = $6; + $$->lockStrength = $6; $$->whereClause = $7; $$->location = @1; } @@ -12456,7 +12456,7 @@ opt_on_conflict: $$->action = ONCONFLICT_UPDATE; $$->infer = $3; $$->targetList = $7; - $$->lockingStrength = LCS_NONE; + $$->lockStrength = LCS_NONE; $$->whereClause = $8; $$->location = @1; } @@ -12467,7 +12467,7 @@ opt_on_conflict: $$->action = ONCONFLICT_NOTHING; $$->infer = $3; $$->targetList = NIL; - $$->lockingStrength = LCS_NONE; + $$->lockStrength = LCS_NONE; $$->whereClause = NULL; $$->location = @1; } diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index c5c4273208a..e8d1e01732e 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -3368,20 +3368,15 @@ transformOnConflictArbiter(ParseState *pstate, *arbiterWhere = NULL; *constraint = InvalidOid; - if (onConflictClause->action == ONCONFLICT_UPDATE && !infer) + if ((onConflictClause->action == ONCONFLICT_UPDATE || + onConflictClause->action == ONCONFLICT_SELECT) && !infer) ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("ON CONFLICT DO UPDATE requires inference specification or constraint name"), - errhint("For example, ON CONFLICT (column_name)."), - parser_errposition(pstate, - exprLocation((Node *) onConflictClause)))); - else if (onConflictClause->action == ONCONFLICT_SELECT && !infer) - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("ON CONFLICT DO SELECT requires inference specification or constraint name"), - errhint("For example, ON CONFLICT (column_name)."), - parser_errposition(pstate, - exprLocation((Node *) onConflictClause)))); + errcode(ERRCODE_SYNTAX_ERROR), + errmsg("ON CONFLICT DO %s requires inference specification or constraint name", + onConflictClause->action == ONCONFLICT_UPDATE ? "UPDATE" : "SELECT"), + errhint("For example, ON CONFLICT (column_name)."), + parser_errposition(pstate, + exprLocation((Node *) onConflictClause))); /* * To simplify certain aspects of its design, speculative insertion into diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index cf91c72d40b..aa377022df1 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -666,7 +666,7 @@ rewriteRuleAction(Query *parsetree, ereport(ERROR, errcode(ERRCODE_SYNTAX_ERROR), errmsg("ON CONFLICT DO SELECT requires a RETURNING clause"), - errdetail("A rule action is INSERT ... ON CONFLICT DO SELECT, which requires a RETURNING clause")); + errdetail("A rule action is INSERT ... ON CONFLICT DO SELECT, which requires a RETURNING clause.")); /* * If rule_action has a RETURNING clause, then either throw it away if the @@ -3653,7 +3653,7 @@ rewriteTargetView(Query *parsetree, Relation view) } /* - * For INSERT .. ON CONFLICT .. DO UPDATE/SELECT, we must also update + * For INSERT .. ON CONFLICT .. DO UPDATE/SELECT, we must also update * assorted stuff in the onConflict data structure. */ if (parsetree->onConflict && @@ -3670,23 +3670,20 @@ rewriteTargetView(Query *parsetree, Relation view) * For ON CONFLICT DO UPDATE, update the resnos in the auxiliary * UPDATE targetlist to refer to columns of the base relation. */ - if (parsetree->onConflict->action == ONCONFLICT_UPDATE) + foreach(lc, parsetree->onConflict->onConflictSet) { - foreach(lc, parsetree->onConflict->onConflictSet) - { - TargetEntry *tle = (TargetEntry *) lfirst(lc); - TargetEntry *view_tle; + TargetEntry *tle = (TargetEntry *) lfirst(lc); + TargetEntry *view_tle; - if (tle->resjunk) - continue; + if (tle->resjunk) + continue; - view_tle = get_tle_by_resno(view_targetlist, tle->resno); - if (view_tle != NULL && !view_tle->resjunk && IsA(view_tle->expr, Var)) - tle->resno = ((Var *) view_tle->expr)->varattno; - else - elog(ERROR, "attribute number %d not found in view targetlist", - tle->resno); - } + view_tle = get_tle_by_resno(view_targetlist, tle->resno); + if (view_tle != NULL && !view_tle->resjunk && IsA(view_tle->expr, Var)) + tle->resno = ((Var *) view_tle->expr)->varattno; + else + elog(ERROR, "attribute number %d not found in view targetlist", + tle->resno); } /* diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 82e467a0b2f..5da4b0ff296 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -7148,8 +7148,8 @@ get_insert_query_def(Query *query, deparse_context *context) appendStringInfoString(buf, " DO SELECT"); /* Add FOR [KEY] UPDATE/SHARE clause if present */ - if (confl->lockingStrength != LCS_NONE) - appendStringInfoString(buf, get_lock_clause_strength(confl->lockingStrength)); + if (confl->lockStrength != LCS_NONE) + appendStringInfoString(buf, get_lock_clause_strength(confl->lockStrength)); /* Add a WHERE clause if given */ if (confl->onConflictWhere != NULL) diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 297969efad3..cd5582f2485 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -433,8 +433,7 @@ typedef struct OnConflictActionState TupleTableSlot *oc_Existing; /* slot to store existing target tuple in */ TupleTableSlot *oc_ProjSlot; /* CONFLICT ... SET ... projection target */ ProjectionInfo *oc_ProjInfo; /* for ON CONFLICT DO UPDATE SET */ - LockClauseStrength oc_LockingStrength; /* strength of lock for ON - * CONFLICT DO SELECT, or LCS_NONE */ + LockClauseStrength oc_LockStrength; /* lock strength for DO SELECT */ ExprState *oc_WhereClause; /* state for the WHERE clause */ } OnConflictActionState; @@ -581,7 +580,7 @@ typedef struct ResultRelInfo /* list of arbiter indexes to use to check conflicts */ List *ri_onConflictArbiterIndexes; - /* ON CONFLICT evaluation state */ + /* ON CONFLICT evaluation state for DO UPDATE/SELECT */ OnConflictActionState *ri_onConflict; /* for MERGE, lists of MergeActionState (one per MergeMatchKind) */ diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 31c73abe87b..4db404f5429 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -200,7 +200,7 @@ typedef struct Query /* OVERRIDING clause */ OverridingKind override pg_node_attr(query_jumble_ignore); - OnConflictExpr *onConflict; /* ON CONFLICT DO [NOTHING | UPDATE] */ + OnConflictExpr *onConflict; /* ON CONFLICT DO NOTHING/UPDATE/SELECT */ /* * The following three fields describe the contents of the RETURNING list @@ -1652,11 +1652,10 @@ typedef struct InferClause typedef struct OnConflictClause { NodeTag type; - OnConflictAction action; /* DO NOTHING, SELECT or UPDATE? */ + OnConflictAction action; /* DO NOTHING, SELECT, or UPDATE */ InferClause *infer; /* Optional index inference clause */ - List *targetList; /* the target list (of ResTarget) */ - LockClauseStrength lockingStrength; /* strength of lock for DO SELECT, or - * LCS_NONE */ + LockClauseStrength lockStrength; /* lock strength for DO SELECT */ + List *targetList; /* target list (of ResTarget) for DO UPDATE */ Node *whereClause; /* qualifications */ ParseLoc location; /* token location, or -1 if unknown */ } OnConflictClause; diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index bdbbebd49fd..7b9debccb0f 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -363,7 +363,7 @@ typedef struct ModifyTable /* List of ON CONFLICT arbiter index OIDs */ List *arbiterIndexes; /* lock strength for ON CONFLICT SELECT */ - LockClauseStrength onConflictLockingStrength; + LockClauseStrength onConflictLockStrength; /* INSERT ON CONFLICT DO UPDATE targetlist */ List *onConflictSet; /* target column numbers for onConflictSet */ diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index fe9677bdf3c..34302b42205 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -2371,7 +2371,7 @@ typedef struct FromExpr typedef struct OnConflictExpr { NodeTag type; - OnConflictAction action; /* NONE, DO NOTHING, DO UPDATE, DO SELECT ? */ + OnConflictAction action; /* DO NOTHING, SELECT, or UPDATE */ /* Arbiter */ List *arbiterElems; /* unique index arbiter list (of @@ -2379,15 +2379,14 @@ typedef struct OnConflictExpr Node *arbiterWhere; /* unique index arbiter WHERE clause */ Oid constraint; /* pg_constraint OID for arbiter */ - /* both ON CONFLICT SELECT and UPDATE */ - Node *onConflictWhere; /* qualifiers to restrict SELECT/UPDATE to */ + /* ON CONFLICT DO SELECT */ + LockClauseStrength lockStrength; /* strength of lock for DO SELECT */ - /* ON CONFLICT SELECT */ - LockClauseStrength lockingStrength; /* strength of lock for DO SELECT, or - * LCS_NONE */ - - /* ON CONFLICT UPDATE */ + /* ON CONFLICT DO UPDATE */ List *onConflictSet; /* List of ON CONFLICT SET TargetEntrys */ + + /* both ON CONFLICT DO SELECT and UPDATE */ + Node *onConflictWhere; /* qualifiers to restrict SELECT/UPDATE */ int exclRelIndex; /* RT index of 'excluded' relation */ List *exclRelTlist; /* tlist of the EXCLUDED pseudo relation */ } OnConflictExpr; diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index d760a7c8797..76e2355af20 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -3586,7 +3586,7 @@ SELECT definition FROM pg_rules WHERE tablename = 'hats' ORDER BY rulename; -- fails without RETURNING INSERT INTO hats VALUES ('h7', 'blue'); ERROR: ON CONFLICT DO SELECT requires a RETURNING clause -DETAIL: A rule action is INSERT ... ON CONFLICT DO SELECT, which requires a RETURNING clause +DETAIL: A rule action is INSERT ... ON CONFLICT DO SELECT, which requires a RETURNING clause. -- works (returns conflicts) EXPLAIN (costs off) INSERT INTO hats VALUES ('h7', 'blue') RETURNING *; -- 2.51.0