Review comments for ON CONFLICT DO SELECT.

Author: deanrasheed

doc/src/sgml/ref/create_policy.sgml

@@ -491,6 +491,22 @@ CREATE POLICY <replaceable class="parameter">name</replaceable> ON <replaceable
        <entry>New row</entry>
        <entry>&mdash;</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>

src/backend/commands/explain.c

@@ -4651,14 +4651,15 @@ 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";
 		else if (node->onConflictAction == ONCONFLICT_UPDATE)
 			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",

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 @@ ExecUpdate(ModifyTableContext *context, ResultRelInfo *resultRelInfo,
 
 /*
  * 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;
 }
 

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;
 

src/backend/parser/analyze.c

@@ -1102,11 +1102,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)
@@ -1266,12 +1269,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;
@@ -1300,27 +1304,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,
@@ -1334,13 +1339,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);

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