From 2f3c9f8a26ed86f9f3a16a2bb1765e244963aa5e Mon Sep 17 00:00:00 2001
From: Dean Rasheed <dean.a.rasheed@gmail.com>
Date: Mon, 31 Mar 2025 15:20:47 +0100
Subject: [PATCH v10 2/3] 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 42d43ad7bf4..6e05808e110 100644
--- a/doc/src/sgml/ref/create_policy.sgml
+++ b/doc/src/sgml/ref/create_policy.sgml
@@ -606,6 +606,22 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable
        <entry>&mdash;</entry>
        <entry>Check existing row</entry>
       </row>
+      <row>
+       <entry><command>ON CONFLICT DO SELECT</command></entry>
+       <entry>Existing &amp; new rows</entry>
+       <entry>&mdash;</entry>
+       <entry>&mdash;</entry>
+       <entry>&mdash;</entry>
+       <entry>&mdash;</entry>
+      </row>
+      <row>
+       <entry><command>ON CONFLICT DO SELECT FOR UPDATE/SHARE</command></entry>
+       <entry>Existing &amp; new rows</entry>
+       <entry>&mdash;</entry>
+       <entry>Existing row</entry>
+       <entry>&mdash;</entry>
+       <entry>&mdash;</entry>
+      </row>
      </tbody>
     </tgroup>
    </table>
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 9f1b90a0eb2..1a575cc96e8 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -4670,7 +4670,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";
@@ -4678,6 +4678,7 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors,
 			resolution = "UPDATE";
 		else
 		{
+			Assert(node->onConflictAction == ONCONFLICT_SELECT);
 			switch (node->onConflictLockingStrength)
 			{
 				case LCS_NONE:
@@ -4692,9 +4693,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;
 			}
 		}
 
@@ -4707,7 +4712,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 6b9f17366bd..80e2650366c 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -161,6 +161,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,
@@ -1175,13 +1176,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);
@@ -2731,6 +2732,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,
@@ -2888,7 +2895,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;
@@ -2933,11 +2940,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
@@ -2985,13 +2993,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)
 {
@@ -3049,11 +3062,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))
@@ -3066,19 +3081,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,
@@ -3088,8 +3099,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++;
@@ -3106,6 +3118,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 ccdc9bc264a..b4d9a998e07 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1140,7 +1140,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 af50d705091..a41516ee962 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1028,11 +1028,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)
@@ -1192,12 +1195,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;
@@ -1226,27 +1230,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,
@@ -1260,13 +1265,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 adc9e7600e1..f3cd32b7222 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 556ab057e5a..82e467a0b2f 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);
@@ -5997,30 +5998,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,
@@ -6033,6 +6013,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.
@@ -7125,7 +7127,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 */
@@ -7140,6 +7142,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 43c4b62838e..bdbbebd49fd 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -368,7 +368,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 f3d2e1da802..d226c472340 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 7c52181cbcb..be5d59b08ca 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3562,6 +3562,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 3f240bec7b0..40f5c16e540 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.48.1

