From 1df023e0e190d111a119f206e1f017cda163267c Mon Sep 17 00:00:00 2001 From: Dean Rasheed Date: Mon, 31 Mar 2025 15:20:47 +0100 Subject: [PATCH v7 2/2] Review comments for ON CONFLICT DO SELECT. --- doc/src/sgml/ref/create_policy.sgml | 16 +++ src/backend/commands/explain.c | 11 +- src/backend/executor/nodeModifyTable.c | 61 ++++---- src/backend/optimizer/plan/setrefs.c | 3 +- src/backend/parser/analyze.c | 60 ++++---- src/backend/rewrite/rewriteHandler.c | 13 ++ src/backend/rewrite/rowsecurity.c | 131 ++++++++---------- src/backend/utils/adt/ruleutils.c | 69 +++++---- src/include/nodes/plannodes.h | 2 +- src/test/regress/expected/insert_conflict.out | 40 +++++- src/test/regress/expected/rules.out | 55 ++++++++ src/test/regress/sql/insert_conflict.sql | 10 +- src/test/regress/sql/rules.sql | 26 ++++ 13 files changed, 336 insertions(+), 161 deletions(-) diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml index e76c342d3da..abbf1f23168 100644 --- a/doc/src/sgml/ref/create_policy.sgml +++ b/doc/src/sgml/ref/create_policy.sgml @@ -491,6 +491,22 @@ CREATE POLICY name ON New row — + + ON CONFLICT DO SELECT + Existing & new rows + — + — + — + — + + + ON CONFLICT DO SELECT FOR UPDATE/SHARE + Existing & new rows + — + Existing row + — + — + diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index ecb0bbeaa9c..8f9e63888b2 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -4651,7 +4651,7 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors, if (node->onConflictAction != ONCONFLICT_NONE) { - const char *resolution; + const char *resolution = NULL; if (node->onConflictAction == ONCONFLICT_NOTHING) resolution = "NOTHING"; @@ -4659,6 +4659,7 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors, resolution = "UPDATE"; else { + Assert(node->onConflictAction == ONCONFLICT_SELECT); switch (node->onConflictLockingStrength) { case LCS_NONE: @@ -4673,9 +4674,13 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors, case LCS_FORNOKEYUPDATE: resolution = "SELECT FOR NO KEY UPDATE"; break; - default: /* LCS_FORUPDATE */ + case LCS_FORUPDATE: resolution = "SELECT FOR UPDATE"; break; + default: + elog(ERROR, "unrecognized LockClauseStrength %d", + (int) node->onConflictLockingStrength); + break; } } @@ -4688,7 +4693,7 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors, if (idxNames) ExplainPropertyList("Conflict Arbiter Indexes", idxNames, es); - /* ON CONFLICT DO UPDATE WHERE qual is specially displayed */ + /* ON CONFLICT DO UPDATE/SELECT WHERE qual is specially displayed */ if (node->onConflictWhere) { show_upper_qual((List *) node->onConflictWhere, "Conflict Filter", diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 250685e2eda..ba5b0b4c363 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -160,6 +160,7 @@ static bool ExecOnConflictUpdate(ModifyTableContext *context, static bool ExecOnConflictSelect(ModifyTableContext *context, ResultRelInfo *resultRelInfo, ItemPointer conflictTid, + TupleTableSlot *excludedSlot, bool canSetTag, TupleTableSlot **returning); static TupleTableSlot *ExecPrepareTupleRouting(ModifyTableState *mtstate, @@ -1154,13 +1155,13 @@ ExecInsert(ModifyTableContext *context, /* * In case of ON CONFLICT DO SELECT, optionally lock the * conflicting tuple, fetch it and project RETURNING on - * it. Be prepared to retry if fetching fails because of a + * it. Be prepared to retry if locking fails because of a * concurrent UPDATE/DELETE to the conflict tuple. */ TupleTableSlot *returning = NULL; if (ExecOnConflictSelect(context, resultRelInfo, - &conflictTid, canSetTag, + &conflictTid, slot, canSetTag, &returning)) { InstrCountTuples2(&mtstate->ps, 1); @@ -2708,6 +2709,12 @@ redo_act: /* * ExecOnConflictLockRow --- lock the row for ON CONFLICT DO UPDATE/SELECT + * + * Try to lock tuple for update as part of speculative insertion for ON + * CONFLICT DO UPDATE or ON CONFLICT DO SELECT FOR UPDATE/SHARE. + * + * Returns true if the row is successfully locked, or false if the caller must + * retry the INSERT from scratch. */ static bool ExecOnConflictLockRow(ModifyTableContext *context, @@ -2865,7 +2872,7 @@ ExecOnConflictUpdate(ModifyTableContext *context, /* Determine lock mode to use */ lockmode = ExecUpdateLockMode(context->estate, resultRelInfo); - /* Lock tuple for update. */ + /* Lock tuple for update */ if (!ExecOnConflictLockRow(context, existing, conflictTid, resultRelInfo->ri_RelationDesc, lockmode, true)) return false; @@ -2910,11 +2917,12 @@ ExecOnConflictUpdate(ModifyTableContext *context, * security barrier quals (if any), enforced here as RLS checks/WCOs. * * The rewriter creates UPDATE RLS checks/WCOs for UPDATE security - * quals, and stores them as WCOs of "kind" WCO_RLS_CONFLICT_CHECK, - * but that's almost the extent of its special handling for ON - * CONFLICT DO UPDATE. + * quals, and stores them as WCOs of "kind" WCO_RLS_CONFLICT_CHECK. If + * SELECT rights are required on the target table, the rewriter also + * adds SELECT RLS checks/WCOs for SELECT security quals, using WCOs + * of the same kind, so this check enforces them too. * - * The rewriter will also have associated UPDATE applicable straight + * The rewriter will also have associated UPDATE-applicable straight * RLS checks/WCOs for the benefit of the ExecUpdate() call that * follows. INSERTs and UPDATEs naturally have mutually exclusive WCO * kinds, so there is no danger of spurious over-enforcement in the @@ -2962,13 +2970,18 @@ ExecOnConflictUpdate(ModifyTableContext *context, /* * ExecOnConflictSelect --- execute SELECT of INSERT ON CONFLICT DO SELECT * - * Returns true if if we're done (with or without an update), or false if the + * 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 + * satisfied, select the row. + * + * Returns true if if we're done (with or without a select), or false if the * caller must retry the INSERT from scratch. */ static bool ExecOnConflictSelect(ModifyTableContext *context, ResultRelInfo *resultRelInfo, ItemPointer conflictTid, + TupleTableSlot *excludedSlot, bool canSetTag, TupleTableSlot **rslot) { @@ -3026,11 +3039,13 @@ ExecOnConflictSelect(ModifyTableContext *context, ExecCheckTupleVisible(context->estate, relation, existing); /* - * Make the tuple available to ExecQual and ExecProject. EXCLUDED is not - * used at all. + * Make tuple and any needed join variables available to ExecQual. The + * EXCLUDED tuple is installed in ecxt_innertuple, while the target's + * existing tuple is installed in the scantuple. EXCLUDED has been made + * to reference INNER_VAR in setrefs.c, but there is no other redirection. */ econtext->ecxt_scantuple = existing; - econtext->ecxt_innertuple = NULL; + econtext->ecxt_innertuple = excludedSlot; econtext->ecxt_outertuple = NULL; if (!ExecQual(onConflictSelectWhere, econtext)) @@ -3043,19 +3058,15 @@ ExecOnConflictSelect(ModifyTableContext *context, if (resultRelInfo->ri_WithCheckOptions != NIL) { /* - * Check target's existing tuple against UPDATE-applicable USING + * Check target's existing tuple against SELECT-applicable USING * security barrier quals (if any), enforced here as RLS checks/WCOs. * - * The rewriter creates UPDATE RLS checks/WCOs for UPDATE security - * quals, and stores them as WCOs of "kind" WCO_RLS_CONFLICT_CHECK, - * but that's almost the extent of its special handling for ON - * CONFLICT DO UPDATE. - * - * The rewriter will also have associated UPDATE applicable straight - * RLS checks/WCOs for the benefit of the ExecUpdate() call that - * follows. INSERTs and UPDATEs naturally have mutually exclusive WCO - * kinds, so there is no danger of spurious over-enforcement in the - * INSERT or UPDATE path. + * The rewriter creates SELECT RLS checks/WCOs for SELECT security + * quals, and stores them as WCOs of "kind" WCO_RLS_CONFLICT_CHECK. If + * FOR UPDATE/SHARE was specified, UPDATE rights are required on the + * target table, and the rewriter also adds UPDATE RLS checks/WCOs for + * UPDATE security quals, using WCOs of the same kind, so this check + * enforces them too. */ ExecWithCheckOptions(WCO_RLS_CONFLICT_CHECK, resultRelInfo, existing, @@ -3065,8 +3076,9 @@ ExecOnConflictSelect(ModifyTableContext *context, /* Parse analysis should already have disallowed this */ Assert(resultRelInfo->ri_projectReturning); - *rslot = ExecProcessReturning(context, resultRelInfo, CMD_INSERT, - existing, NULL, context->planSlot); + /* Process RETURNING like an UPDATE that didn't change anything */ + *rslot = ExecProcessReturning(context, resultRelInfo, CMD_UPDATE, + existing, existing, context->planSlot); if (canSetTag) context->estate->es_processed++; @@ -3083,6 +3095,7 @@ ExecOnConflictSelect(ModifyTableContext *context, * query. */ ExecClearTuple(existing); + return true; } diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 150e9f060ee..9f87a0e7148 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -1114,7 +1114,8 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset) * those are already used by RETURNING and it seems better to * be non-conflicting. */ - if (splan->onConflictSet) + if (splan->onConflictAction == ONCONFLICT_UPDATE || + splan->onConflictAction == ONCONFLICT_SELECT) { indexed_tlist *itlist; diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index 806d2689f20..21a2aed1f08 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -1063,11 +1063,14 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt) false, true, true); } - if (stmt->onConflictClause && stmt->onConflictClause->action == ONCONFLICT_SELECT && !stmt->returningClause) + /* ON CONFLICT DO SELECT requires a RETURNING clause */ + if (stmt->onConflictClause && + stmt->onConflictClause->action == ONCONFLICT_SELECT && + !stmt->returningClause) ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("ON CONFLICT DO SELECT requires a RETURNING clause"), - parser_errposition(pstate, stmt->onConflictClause->location))); + errcode(ERRCODE_SYNTAX_ERROR), + errmsg("ON CONFLICT DO SELECT requires a RETURNING clause"), + parser_errposition(pstate, stmt->onConflictClause->location)); /* Process ON CONFLICT, if any. */ if (stmt->onConflictClause) @@ -1227,12 +1230,13 @@ transformOnConflictClause(ParseState *pstate, OnConflictExpr *result; /* - * If this is ON CONFLICT ... UPDATE, first create the range table entry - * for the EXCLUDED pseudo relation, so that that will be present while - * processing arbiter expressions. (You can't actually reference it from - * there, but this provides a useful error message if you try.) + * If this is ON CONFLICT ... UPDATE/SELECT, first create the range table + * entry for the EXCLUDED pseudo relation, so that that will be present + * while processing arbiter expressions. (You can't actually reference it + * from there, but this provides a useful error message if you try.) */ - if (onConflictClause->action == ONCONFLICT_UPDATE) + if (onConflictClause->action == ONCONFLICT_UPDATE || + onConflictClause->action == ONCONFLICT_SELECT) { Relation targetrel = pstate->p_target_relation; RangeTblEntry *exclRte; @@ -1261,27 +1265,28 @@ transformOnConflictClause(ParseState *pstate, transformOnConflictArbiter(pstate, onConflictClause, &arbiterElems, &arbiterWhere, &arbiterConstraint); - /* Process DO UPDATE */ - if (onConflictClause->action == ONCONFLICT_UPDATE) + /* Process DO UPDATE/SELECT */ + if (onConflictClause->action == ONCONFLICT_UPDATE || + onConflictClause->action == ONCONFLICT_SELECT) { - /* - * Expressions in the UPDATE targetlist need to be handled like UPDATE - * not INSERT. We don't need to save/restore this because all INSERT - * expressions have been parsed already. - */ - pstate->p_is_insert = false; - /* * Add the EXCLUDED pseudo relation to the query namespace, making it - * available in the UPDATE subexpressions. + * available in the UPDATE/SELECT subexpressions. */ addNSItemToQuery(pstate, exclNSItem, false, true, true); - /* - * Now transform the UPDATE subexpressions. - */ - onConflictSet = - transformUpdateTargetList(pstate, onConflictClause->targetList); + if (onConflictClause->action == ONCONFLICT_UPDATE) + { + /* + * Expressions in the UPDATE targetlist need to be handled like + * UPDATE not INSERT. We don't need to save/restore this because + * all INSERT expressions have been parsed already. + */ + pstate->p_is_insert = false; + + onConflictSet = + transformUpdateTargetList(pstate, onConflictClause->targetList); + } onConflictWhere = transformWhereClause(pstate, onConflictClause->whereClause, @@ -1295,13 +1300,6 @@ transformOnConflictClause(ParseState *pstate, Assert((ParseNamespaceItem *) llast(pstate->p_namespace) == exclNSItem); pstate->p_namespace = list_delete_last(pstate->p_namespace); } - else if (onConflictClause->action == ONCONFLICT_SELECT) - { - onConflictWhere = transformWhereClause(pstate, - onConflictClause->whereClause, - EXPR_KIND_WHERE, "WHERE"); - - } /* Finally, build ON CONFLICT DO [NOTHING | SELECT | UPDATE] expression */ result = makeNode(OnConflictExpr); diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index f0bce5f9ed9..a0fa66eaada 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -655,6 +655,19 @@ rewriteRuleAction(Query *parsetree, rule_action = sub_action; } + /* + * If rule_action is INSERT .. ON CONFLICT DO SELECT, the parser should + * have verified that it has a RETURNING clause, but we must also check + * that the triggering query has a RETURNING clause. + */ + if (rule_action->onConflict && + rule_action->onConflict->action == ONCONFLICT_SELECT && + (!rule_action->returningList || !parsetree->returningList)) + 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")); + /* * If rule_action has a RETURNING clause, then either throw it away if the * triggering query has no RETURNING clause, or rewrite it to emit what diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c index e2877faca91..c9bdff6f8f5 100644 --- a/src/backend/rewrite/rowsecurity.c +++ b/src/backend/rewrite/rowsecurity.c @@ -301,45 +301,50 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index, } /* - * For INSERT ... ON CONFLICT DO UPDATE and DO SELECT FOR ... we need - * additional policy checks for the UPDATE or locking which may be - * applied to the same RTE. + * For INSERT ... ON CONFLICT DO UPDATE/SELECT we need additional + * policy checks for the UPDATE/SELECT which may be applied to the + * same RTE. */ - if (commandType == CMD_INSERT && - root->onConflict && (root->onConflict->action == ONCONFLICT_UPDATE || - (root->onConflict->action == ONCONFLICT_SELECT && - root->onConflict->lockingStrength != LCS_NONE))) + if (commandType == CMD_INSERT && root->onConflict && + (root->onConflict->action == ONCONFLICT_UPDATE || + root->onConflict->action == ONCONFLICT_SELECT)) { - List *conflict_permissive_policies; - List *conflict_restrictive_policies; + List *conflict_permissive_policies = NIL; + List *conflict_restrictive_policies = NIL; List *conflict_select_permissive_policies = NIL; List *conflict_select_restrictive_policies = NIL; - /* Get the policies that apply to the auxiliary UPDATE */ - get_policies_for_relation(rel, CMD_UPDATE, user_id, - &conflict_permissive_policies, - &conflict_restrictive_policies); - - /* - * Enforce the USING clauses of the UPDATE policies using WCOs - * rather than security quals. This ensures that an error is - * raised if the conflicting row cannot be updated due to RLS, - * rather than the change being silently dropped. - */ - add_with_check_options(rel, rt_index, - WCO_RLS_CONFLICT_CHECK, - conflict_permissive_policies, - conflict_restrictive_policies, - withCheckOptions, - hasSubLinks, - true); + if (perminfo->requiredPerms & ACL_UPDATE) + { + /* + * Get the policies that apply to the auxiliary UPDATE or + * SELECT FOR SHARE/UDPATE. + */ + get_policies_for_relation(rel, CMD_UPDATE, user_id, + &conflict_permissive_policies, + &conflict_restrictive_policies); + + /* + * Enforce the USING clauses of the UPDATE policies using WCOs + * rather than security quals. This ensures that an error is + * raised if the conflicting row cannot be updated/locked due + * to RLS, rather than the change being silently dropped. + */ + add_with_check_options(rel, rt_index, + WCO_RLS_CONFLICT_CHECK, + conflict_permissive_policies, + conflict_restrictive_policies, + withCheckOptions, + hasSubLinks, + true); + } /* * Get and add ALL/SELECT policies, as WCO_RLS_CONFLICT_CHECK WCOs - * to ensure they are considered when taking the UPDATE path of an - * INSERT .. ON CONFLICT, if SELECT rights are required for this - * relation, also as WCO policies, again, to avoid silently - * dropping data. See above. + * to ensure they are considered when taking the UPDATE/SELECT + * path of an INSERT .. ON CONFLICT, if SELECT rights are required + * for this relation, also as WCO policies, again, to avoid + * silently dropping data. See above. */ if (perminfo->requiredPerms & ACL_SELECT) { @@ -355,52 +360,36 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index, true); } - /* Enforce the WITH CHECK clauses of the UPDATE policies */ - add_with_check_options(rel, rt_index, - WCO_RLS_UPDATE_CHECK, - conflict_permissive_policies, - conflict_restrictive_policies, - withCheckOptions, - hasSubLinks, - false); - /* - * Add ALL/SELECT policies as WCO_RLS_UPDATE_CHECK WCOs, to ensure - * that the final updated row is visible when taking the UPDATE - * path of an INSERT .. ON CONFLICT, if SELECT rights are required - * for this relation. + * For INSERT .. ON CONFLICT DO UPDATE, add additional policies to + * be checked when the auxiliary UPDATE is executed. */ - if (perminfo->requiredPerms & ACL_SELECT) + if (root->onConflict->action == ONCONFLICT_UPDATE) + { + /* Enforce the WITH CHECK clauses of the UPDATE policies */ add_with_check_options(rel, rt_index, WCO_RLS_UPDATE_CHECK, - conflict_select_permissive_policies, - conflict_select_restrictive_policies, + conflict_permissive_policies, + conflict_restrictive_policies, withCheckOptions, hasSubLinks, - true); - } - - /* - * For INSERT ... ON CONFLICT DO SELELT we need additional policy - * checks for the SELECT which may be applied to the same RTE. - */ - if (commandType == CMD_INSERT && - root->onConflict && root->onConflict->action == ONCONFLICT_SELECT && - root->onConflict->lockingStrength == LCS_NONE) - { - List *conflict_permissive_policies; - List *conflict_restrictive_policies; - - get_policies_for_relation(rel, CMD_SELECT, user_id, - &conflict_permissive_policies, - &conflict_restrictive_policies); - add_with_check_options(rel, rt_index, - WCO_RLS_CONFLICT_CHECK, - conflict_permissive_policies, - conflict_restrictive_policies, - withCheckOptions, - hasSubLinks, - true); + false); + + /* + * Add ALL/SELECT policies as WCO_RLS_UPDATE_CHECK WCOs, to + * ensure that the final updated row is visible when taking + * the UPDATE path of an INSERT .. ON CONFLICT, if SELECT + * rights are required for this relation. + */ + if (perminfo->requiredPerms & ACL_SELECT) + add_with_check_options(rel, rt_index, + WCO_RLS_UPDATE_CHECK, + conflict_select_permissive_policies, + conflict_select_restrictive_policies, + withCheckOptions, + hasSubLinks, + true); + } } } diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 9e90acedb91..0e95c750fa3 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -426,6 +426,7 @@ static void get_update_query_targetlist_def(Query *query, List *targetList, static void get_delete_query_def(Query *query, deparse_context *context); static void get_merge_query_def(Query *query, deparse_context *context); static void get_utility_query_def(Query *query, deparse_context *context); +static char *get_lock_clause_strength(LockClauseStrength strength); static void get_basic_select_query(Query *query, deparse_context *context); static void get_target_list(List *targetList, deparse_context *context); static void get_returning_clause(Query *query, deparse_context *context); @@ -5984,30 +5985,9 @@ get_select_query_def(Query *query, deparse_context *context) if (rc->pushedDown) continue; - switch (rc->strength) - { - case LCS_NONE: - /* we intentionally throw an error for LCS_NONE */ - elog(ERROR, "unrecognized LockClauseStrength %d", - (int) rc->strength); - break; - case LCS_FORKEYSHARE: - appendContextKeyword(context, " FOR KEY SHARE", - -PRETTYINDENT_STD, PRETTYINDENT_STD, 0); - break; - case LCS_FORSHARE: - appendContextKeyword(context, " FOR SHARE", - -PRETTYINDENT_STD, PRETTYINDENT_STD, 0); - break; - case LCS_FORNOKEYUPDATE: - appendContextKeyword(context, " FOR NO KEY UPDATE", - -PRETTYINDENT_STD, PRETTYINDENT_STD, 0); - break; - case LCS_FORUPDATE: - appendContextKeyword(context, " FOR UPDATE", - -PRETTYINDENT_STD, PRETTYINDENT_STD, 0); - break; - } + appendContextKeyword(context, + get_lock_clause_strength(rc->strength), + -PRETTYINDENT_STD, PRETTYINDENT_STD, 0); appendStringInfo(buf, " OF %s", quote_identifier(get_rtable_name(rc->rti, @@ -6020,6 +6000,28 @@ get_select_query_def(Query *query, deparse_context *context) } } +static char * +get_lock_clause_strength(LockClauseStrength strength) +{ + switch (strength) + { + case LCS_NONE: + /* we intentionally throw an error for LCS_NONE */ + elog(ERROR, "unrecognized LockClauseStrength %d", + (int) strength); + break; + case LCS_FORKEYSHARE: + return " FOR KEY SHARE"; + case LCS_FORSHARE: + return " FOR SHARE"; + case LCS_FORNOKEYUPDATE: + return " FOR NO KEY UPDATE"; + case LCS_FORUPDATE: + return " FOR UPDATE"; + } + return NULL; /* keep compiler quiet */ +} + /* * Detect whether query looks like SELECT ... FROM VALUES(), * with no need to rename the output columns of the VALUES RTE. @@ -7110,7 +7112,7 @@ get_insert_query_def(Query *query, deparse_context *context) { appendStringInfoString(buf, " DO NOTHING"); } - else + else if (confl->action == ONCONFLICT_UPDATE) { appendStringInfoString(buf, " DO UPDATE SET "); /* Deparse targetlist */ @@ -7125,6 +7127,23 @@ get_insert_query_def(Query *query, deparse_context *context) get_rule_expr(confl->onConflictWhere, context, false); } } + else + { + Assert(confl->action == ONCONFLICT_SELECT); + 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)); + + /* Add a WHERE clause if given */ + if (confl->onConflictWhere != NULL) + { + appendContextKeyword(context, " WHERE ", + -PRETTYINDENT_STD, PRETTYINDENT_STD, 1); + get_rule_expr(confl->onConflictWhere, context, false); + } + } } /* Add RETURNING if present */ diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 1ad0a240b38..1743bc22e08 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -328,7 +328,7 @@ typedef struct ModifyTable List *onConflictSet; /* target column numbers for onConflictSet */ List *onConflictCols; - /* WHERE for ON CONFLICT UPDATE */ + /* WHERE for ON CONFLICT UPDATE/SELECT */ Node *onConflictWhere; /* RTI of the EXCLUDED pseudo relation */ Index exclRelRTI; diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out index 2edf04c78f3..9f84e2aa05a 100644 --- a/src/test/regress/expected/insert_conflict.out +++ b/src/test/regress/expected/insert_conflict.out @@ -267,11 +267,28 @@ insert into insertconflicttest values (1, 'Apple') on conflict (key) do select r 1 | Apple (1 row) -insert into insertconflicttest values (1, 'Apple') on conflict (key) do select where fruit <> 'Apple' returning *; +insert into insertconflicttest as i values (1, 'Apple') on conflict (key) do select where i.fruit = 'Apple' returning *; + key | fruit +-----+------- + 1 | Apple +(1 row) + +insert into insertconflicttest as i values (1, 'Apple') on conflict (key) do select where i.fruit = 'Orange' returning *; + key | fruit +-----+------- +(0 rows) + +insert into insertconflicttest as i values (1, 'Orange') on conflict (key) do select where excluded.fruit = 'Apple' returning *; key | fruit -----+------- (0 rows) +insert into insertconflicttest as i values (1, 'Orange') on conflict (key) do select where excluded.fruit = 'Orange' returning *; + key | fruit +-----+------- + 1 | Apple +(1 row) + delete from insertconflicttest where fruit = 'Apple'; insert into insertconflicttest values (1, 'Apple') on conflict (key) do select for update returning *; key | fruit @@ -285,11 +302,28 @@ insert into insertconflicttest values (1, 'Apple') on conflict (key) do select f 1 | Apple (1 row) -insert into insertconflicttest values (1, 'Apple') on conflict (key) do select for update where fruit <> 'Apple' returning *; +insert into insertconflicttest as i values (1, 'Apple') on conflict (key) do select for update where i.fruit = 'Apple' returning *; + key | fruit +-----+------- + 1 | Apple +(1 row) + +insert into insertconflicttest as i values (1, 'Apple') on conflict (key) do select for update where i.fruit = 'Orange' returning *; + key | fruit +-----+------- +(0 rows) + +insert into insertconflicttest as i values (1, 'Orange') on conflict (key) do select for update where excluded.fruit = 'Apple' returning *; key | fruit -----+------- (0 rows) +insert into insertconflicttest as i values (1, 'Orange') on conflict (key) do select for update where excluded.fruit = 'Orange' returning *; + key | fruit +-----+------- + 1 | Apple +(1 row) + insert into insertconflicttest as ict values (3, 'Pear') on conflict (key) do select for update returning old.*, new.*, ict.*; key | fruit | key | fruit | key | fruit -----+-------+-----+-------+-----+------- @@ -299,7 +333,7 @@ insert into insertconflicttest as ict values (3, 'Pear') on conflict (key) do se insert into insertconflicttest as ict values (3, 'Banana') on conflict (key) do select for update returning old.*, new.*, ict.*; key | fruit | key | fruit | key | fruit -----+-------+-----+-------+-----+------- - 3 | Pear | | | 3 | Pear + 3 | Pear | 3 | Pear | 3 | Pear (1 row) explain (costs off) insert into insertconflicttest values (1, 'Apple') on conflict (key) do select for key share returning *; diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 47478969135..0e49eda2551 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -3507,6 +3507,61 @@ SELECT * FROM hat_data WHERE hat_name IN ('h8', 'h9', 'h7') ORDER BY hat_name; (3 rows) DROP RULE hat_upsert ON hats; +-- DO SELECT with a WHERE clause +CREATE RULE hat_confsel AS ON INSERT TO hats + DO INSTEAD + INSERT INTO hat_data VALUES ( + NEW.hat_name, + NEW.hat_color) + ON CONFLICT (hat_name) + DO SELECT FOR UPDATE + WHERE excluded.hat_color <> 'forbidden' AND hat_data.* != excluded.* + RETURNING *; +SELECT definition FROM pg_rules WHERE tablename = 'hats' ORDER BY rulename; + definition +-------------------------------------------------------------------------------------- + CREATE RULE hat_confsel AS + + ON INSERT TO public.hats DO INSTEAD INSERT INTO hat_data (hat_name, hat_color) + + VALUES (new.hat_name, new.hat_color) ON CONFLICT(hat_name) DO SELECT FOR UPDATE + + WHERE ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*))+ + RETURNING hat_data.hat_name, + + hat_data.hat_color; +(1 row) + +-- 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 +-- works (returns conflicts) +EXPLAIN (costs off) +INSERT INTO hats VALUES ('h7', 'blue') RETURNING *; + QUERY PLAN +------------------------------------------------------------------------------------------------- + Insert on hat_data + Conflict Resolution: SELECT FOR UPDATE + Conflict Arbiter Indexes: hat_data_unique_idx + Conflict Filter: ((excluded.hat_color <> 'forbidden'::bpchar) AND (hat_data.* <> excluded.*)) + -> Result +(5 rows) + +INSERT INTO hats VALUES ('h7', 'blue') RETURNING *; + hat_name | hat_color +------------+------------ + h7 | black +(1 row) + +-- conflicts excluded by WHERE clause +INSERT INTO hats VALUES ('h7', 'forbidden') RETURNING *; + hat_name | hat_color +----------+----------- +(0 rows) + +INSERT INTO hats VALUES ('h7', 'black') RETURNING *; + hat_name | hat_color +----------+----------- +(0 rows) + +DROP RULE hat_confsel ON hats; drop table hats; drop table hat_data; -- test for pg_get_functiondef properly regurgitating SET parameters diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql index b80b7dae91a..72b8147f849 100644 --- a/src/test/regress/sql/insert_conflict.sql +++ b/src/test/regress/sql/insert_conflict.sql @@ -106,11 +106,17 @@ delete from insertconflicttest where fruit = 'Apple'; insert into insertconflicttest values (1, 'Apple') on conflict (key) do select; -- fails insert into insertconflicttest values (1, 'Apple') on conflict (key) do select returning *; insert into insertconflicttest values (1, 'Apple') on conflict (key) do select returning *; -insert into insertconflicttest values (1, 'Apple') on conflict (key) do select where fruit <> 'Apple' returning *; +insert into insertconflicttest as i values (1, 'Apple') on conflict (key) do select where i.fruit = 'Apple' returning *; +insert into insertconflicttest as i values (1, 'Apple') on conflict (key) do select where i.fruit = 'Orange' returning *; +insert into insertconflicttest as i values (1, 'Orange') on conflict (key) do select where excluded.fruit = 'Apple' returning *; +insert into insertconflicttest as i values (1, 'Orange') on conflict (key) do select where excluded.fruit = 'Orange' returning *; delete from insertconflicttest where fruit = 'Apple'; insert into insertconflicttest values (1, 'Apple') on conflict (key) do select for update returning *; insert into insertconflicttest values (1, 'Apple') on conflict (key) do select for update returning *; -insert into insertconflicttest values (1, 'Apple') on conflict (key) do select for update where fruit <> 'Apple' returning *; +insert into insertconflicttest as i values (1, 'Apple') on conflict (key) do select for update where i.fruit = 'Apple' returning *; +insert into insertconflicttest as i values (1, 'Apple') on conflict (key) do select for update where i.fruit = 'Orange' returning *; +insert into insertconflicttest as i values (1, 'Orange') on conflict (key) do select for update where excluded.fruit = 'Apple' returning *; +insert into insertconflicttest as i values (1, 'Orange') on conflict (key) do select for update where excluded.fruit = 'Orange' returning *; insert into insertconflicttest as ict values (3, 'Pear') on conflict (key) do select for update returning old.*, new.*, ict.*; insert into insertconflicttest as ict values (3, 'Banana') on conflict (key) do select for update returning old.*, new.*, ict.*; diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql index fdd3ff1d161..9206a7f8887 100644 --- a/src/test/regress/sql/rules.sql +++ b/src/test/regress/sql/rules.sql @@ -1205,6 +1205,32 @@ SELECT * FROM hat_data WHERE hat_name IN ('h8', 'h9', 'h7') ORDER BY hat_name; DROP RULE hat_upsert ON hats; +-- DO SELECT with a WHERE clause +CREATE RULE hat_confsel AS ON INSERT TO hats + DO INSTEAD + INSERT INTO hat_data VALUES ( + NEW.hat_name, + NEW.hat_color) + ON CONFLICT (hat_name) + DO SELECT FOR UPDATE + WHERE excluded.hat_color <> 'forbidden' AND hat_data.* != excluded.* + RETURNING *; +SELECT definition FROM pg_rules WHERE tablename = 'hats' ORDER BY rulename; + +-- fails without RETURNING +INSERT INTO hats VALUES ('h7', 'blue'); + +-- works (returns conflicts) +EXPLAIN (costs off) +INSERT INTO hats VALUES ('h7', 'blue') RETURNING *; +INSERT INTO hats VALUES ('h7', 'blue') RETURNING *; + +-- conflicts excluded by WHERE clause +INSERT INTO hats VALUES ('h7', 'forbidden') RETURNING *; +INSERT INTO hats VALUES ('h7', 'black') RETURNING *; + +DROP RULE hat_confsel ON hats; + drop table hats; drop table hat_data; -- 2.43.0