v14-0002-More-suggested-review-comments.patch

From 8432d99ce4612b74db2c55852ba7421ae550a2ae Mon Sep 17 00:00:00 2001
From: Dean Rasheed <dean.a.rasheed@gmail.com>
Date: Wed, 19 Nov 2025 13:45:12 +0000
Subject: [PATCH v14 2/4] 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.48.1