INSERT .. ON CONFLICT DO SELECT [FOR ..]

Started by Marko Tiikkajaover 8 years ago11 messages
#1Marko Tiikkaja
marko@joh.to
1 attachment(s)

Hi,

Attached is a patch for $SUBJECT. It might still be a bit rough around the
edges and probably light on docs and testing, but I thought I'd post it
anyway so I won't forget.

.m

Attachments:

insert_conflict_select_v1.patchapplication/octet-stream; name=insert_conflict_select_v1.patchDownload
diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
index 94dad00..8ca6bcf 100644
*** a/doc/src/sgml/ref/insert.sgml
--- b/doc/src/sgml/ref/insert.sgml
***************
*** 36,41 **** INSERT INTO <replaceable class="PARAMETER">table_name</replaceable> [ AS <replac
--- 36,42 ----
  <phrase>and <replaceable class="parameter">conflict_action</replaceable> is one of:</phrase>
  
      DO NOTHING
+     DO SELECT [ FOR { UPDATE | NO KEY UPDATE | SHARE | KEY SHARE } ]
      DO UPDATE SET { <replaceable class="PARAMETER">column_name</replaceable> = { <replaceable class="PARAMETER">expression</replaceable> | DEFAULT } |
                      ( <replaceable class="PARAMETER">column_name</replaceable> [, ...] ) = [ ROW ] ( { <replaceable class="PARAMETER">expression</replaceable> | DEFAULT } [, ...] ) |
                      ( <replaceable class="PARAMETER">column_name</replaceable> [, ...] ) = ( <replaceable class="PARAMETER">sub-SELECT</replaceable> )
***************
*** 83,108 **** INSERT INTO <replaceable class="PARAMETER">table_name</replaceable> [ AS <replac
  
    <para>
     The optional <literal>RETURNING</> clause causes <command>INSERT</>
!    to compute and return value(s) based on each row actually inserted
!    (or updated, if an <literal>ON CONFLICT DO UPDATE</> clause was
!    used).  This is primarily useful for obtaining values that were
     supplied by defaults, such as a serial sequence number.  However,
     any expression using the table's columns is allowed.  The syntax of
     the <literal>RETURNING</> list is identical to that of the output
!    list of <command>SELECT</>.  Only rows that were successfully
     inserted or updated will be returned.  For example, if a row was
     locked but not updated because an <literal>ON CONFLICT DO UPDATE
     ... WHERE</literal> clause <replaceable
     class="PARAMETER">condition</replaceable> was not satisfied, the
!    row will not be returned.
    </para>
  
    <para>
     You must have <literal>INSERT</literal> privilege on a table in
     order to insert into it.  If <literal>ON CONFLICT DO UPDATE</> is
     present, <literal>UPDATE</literal> privilege on the table is also
!    required.
!   </para>
  
    <para>
     If a column list is specified, you only need
--- 84,114 ----
  
    <para>
     The optional <literal>RETURNING</> clause causes <command>INSERT</>
!    to compute and return value(s) based on each row actually inserted.
!    If an <literal>ON CONFLICT DO UPDATE</> clause was used,
!    <literal>RETURNING</> also returns tuples which were updated, and
!    in the presence of an <literal>ON CONFLICT DO SELECT</> clause all
!    input rows are returned.  With a traditional <command>INSERT</>,
!    the <literal>RETURNING</> clause is primarily useful for obtaining
!    values that were
     supplied by defaults, such as a serial sequence number.  However,
     any expression using the table's columns is allowed.  The syntax of
     the <literal>RETURNING</> list is identical to that of the output
!    list of <command>SELECT</>.  If an <literal>ON CONFLICT DO SELECT</>
!    clause is not present, only rows that were successfully
     inserted or updated will be returned.  For example, if a row was
     locked but not updated because an <literal>ON CONFLICT DO UPDATE
     ... WHERE</literal> clause <replaceable
     class="PARAMETER">condition</replaceable> was not satisfied, the
!    row will not be returned.  <literal>ON CONFLICT DO SELECT</>
!    works similarly, except no update takes place.
    </para>
  
    <para>
     You must have <literal>INSERT</literal> privilege on a table in
     order to insert into it.  If <literal>ON CONFLICT DO UPDATE</> is
     present, <literal>UPDATE</literal> privilege on the table is also
!    required.  </para>
  
    <para>
     If a column list is specified, you only need
diff --git a/src/backend/executor/nindex 30add8e..bcb719a 100644
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 54,59 ****
--- 54,66 ----
  #include "utils/tqual.h"
  
  
+ static bool
+ ExecOnConflictLockRow(ModifyTableState *mtstate,
+ 					 Relation relation,
+ 					 LockTupleMode lockmode,
+ 					 HeapTuple tuple,
+ 					 Buffer *buffer,
+ 					 EState *estate);
  static bool ExecOnConflictUpdate(ModifyTableState *mtstate,
  					 ResultRelInfo *resultRelInfo,
  					 ItemPointer conflictTid,
***************
*** 62,67 **** static bool ExecOnConflictUpdate(ModifyTableState *mtstate,
--- 69,81 ----
  					 EState *estate,
  					 bool canSetTag,
  					 TupleTableSlot **returning);
+ static bool ExecOnConflictSelect(ModifyTableState *mtstate,
+ 					 ResultRelInfo *resultRelInfo,
+ 					 ItemPointer conflictTid,
+ 					 TupleTableSlot *planSlot,
+ 					 EState *estate,
+ 					 TupleTableSlot **returning);
+ 
  
  /*
   * Verify that the tuples to be produced by INSERT or UPDATE match the
***************
*** 528,533 **** ExecInsert(ModifyTableState *mtstate,
--- 542,566 ----
  					else
  						goto vlock;
  				}
+ 				else if (onconflict == ONCONFLICT_SELECT)
+ 				{
+ 					/*
+ 					 * In case of ON CONFLICT DO SELECT, simply fetch the
+ 					 * conflicting tuple and project RETURNING on it.
+ 					 */
+ 					TupleTableSlot *returning = NULL;
+ 
+ 					if (ExecOnConflictSelect(mtstate, resultRelInfo,
+ 											 &conflictTid, planSlot,
+ 											 estate, &returning))
+ 					{
+ 						InstrCountFiltered2(&mtstate->ps, 1);
+ 						return returning;
+ 					}
+ 					else
+ 						goto vlock;
+ 				}
+ 
  				else
  				{
  					/*
***************
*** 1181,1227 **** lreplace:;
  }
  
  /*
!  * ExecOnConflictUpdate --- execute UPDATE of INSERT ON CONFLICT DO UPDATE
!  *
!  * Try to lock tuple for update as part of speculative insertion.  If
!  * a qual originating from ON CONFLICT DO UPDATE is satisfied, update
!  * (but still lock row, even though it may not satisfy estate's
!  * snapshot).
!  *
!  * Returns true if if we're done (with or without an update), or false if
!  * the caller must retry the INSERT from scratch.
   */
  static bool
! ExecOnConflictUpdate(ModifyTableState *mtstate,
! 					 ResultRelInfo *resultRelInfo,
! 					 ItemPointer conflictTid,
! 					 TupleTableSlot *planSlot,
! 					 TupleTableSlot *excludedSlot,
! 					 EState *estate,
! 					 bool canSetTag,
! 					 TupleTableSlot **returning)
  {
- 	ExprContext *econtext = mtstate->ps.ps_ExprContext;
- 	Relation	relation = resultRelInfo->ri_RelationDesc;
- 	ExprState  *onConflictSetWhere = resultRelInfo->ri_onConflictSetWhere;
- 	HeapTupleData tuple;
  	HeapUpdateFailureData hufd;
- 	LockTupleMode lockmode;
  	HTSU_Result test;
- 	Buffer		buffer;
- 
- 	/* Determine lock mode to use */
- 	lockmode = ExecUpdateLockMode(estate, resultRelInfo);
  
  	/*
! 	 * Lock tuple for update.  Don't follow updates when tuple cannot be
! 	 * locked without doing so.  A row locking conflict here means our
! 	 * previous conclusion that the tuple is conclusively committed is not
! 	 * true anymore.
  	 */
! 	tuple.t_self = *conflictTid;
! 	test = heap_lock_tuple(relation, &tuple, estate->es_output_cid,
! 						   lockmode, LockWaitBlock, false, &buffer,
  						   &hufd);
  	switch (test)
  	{
--- 1214,1239 ----
  }
  
  /*
!  * ExecOnConflictLockRow --- lock the row for ON CONFLICT DO UPDATE/SELECT
   */
  static bool
! ExecOnConflictLockRow(ModifyTableState *mtstate,
! 					 Relation relation,
! 					 LockTupleMode lockmode,
! 					 HeapTuple tuple,
! 					 Buffer *buffer,
! 					 EState *estate)
  {
  	HeapUpdateFailureData hufd;
  	HTSU_Result test;
  
  	/*
! 	 * Don't follow updates when tuple cannot be locked without doing so.  A
! 	 * row locking conflict here means our previous conclusion that the tuple
! 	 * is conclusively committed is not true anymore.
  	 */
! 	test = heap_lock_tuple(relation, tuple, estate->es_output_cid,
! 						   lockmode, LockWaitBlock, false, buffer,
  						   &hufd);
  	switch (test)
  	{
***************
*** 1247,1256 **** ExecOnConflictUpdate(ModifyTableState *mtstate,
  			 * that for SQL MERGE, an exception must be raised in the event of
  			 * an attempt to update the same row twice.
  			 */
! 			if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmin(tuple.t_data)))
  				ereport(ERROR,
  						(errcode(ERRCODE_CARDINALITY_VIOLATION),
! 						 errmsg("ON CONFLICT DO UPDATE command cannot affect row a second time"),
  						 errhint("Ensure that no rows proposed for insertion within the same command have duplicate constrained values.")));
  
  			/* This shouldn't happen */
--- 1259,1268 ----
  			 * that for SQL MERGE, an exception must be raised in the event of
  			 * an attempt to update the same row twice.
  			 */
! 			if (TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmin(tuple->t_data)))
  				ereport(ERROR,
  						(errcode(ERRCODE_CARDINALITY_VIOLATION),
! 						 errmsg("ON CONFLICT command cannot affect row a second time"),
  						 errhint("Ensure that no rows proposed for insertion within the same command have duplicate constrained values.")));
  
  			/* This shouldn't happen */
***************
*** 1278,1290 **** ExecOnConflictUpdate(ModifyTableState *mtstate,
  			 * loop here, as the new version of the row might not conflict
  			 * anymore, or the conflicting tuple has actually been deleted.
  			 */
! 			ReleaseBuffer(buffer);
  			return false;
  
  		default:
  			elog(ERROR, "unrecognized heap_lock_tuple status: %u", test);
  	}
  
  	/*
  	 * Success, the tuple is locked.
  	 *
--- 1290,1346 ----
  			 * loop here, as the new version of the row might not conflict
  			 * anymore, or the conflicting tuple has actually been deleted.
  			 */
! 			ReleaseBuffer(*buffer);
  			return false;
  
  		default:
  			elog(ERROR, "unrecognized heap_lock_tuple status: %u", test);
  	}
  
+ 	return true;
+ }
+ 
+ 
+ /*
+  * ExecOnConflictUpdate --- execute UPDATE of INSERT ON CONFLICT DO UPDATE
+  *
+  * Try to lock tuple for update as part of speculative insertion.  If
+  * a qual originating from ON CONFLICT DO UPDATE is satisfied, update
+  * (but still lock row, even though it may not satisfy estate's
+  * snapshot).
+  *
+  * Returns true if if we're done (with or without an update), or false if
+  * the caller must retry the INSERT from scratch.
+  */
+ static bool
+ ExecOnConflictUpdate(ModifyTableState *mtstate,
+ 					 ResultRelInfo *resultRelInfo,
+ 					 ItemPointer conflictTid,
+ 					 TupleTableSlot *planSlot,
+ 					 TupleTableSlot *excludedSlot,
+ 					 EState *estate,
+ 					 bool canSetTag,
+ 					 TupleTableSlot **returning)
+ {
+ 	ExprContext *econtext = mtstate->ps.ps_ExprContext;
+ 	ExprState  *onConflictSetWhere = resultRelInfo->ri_onConflictActionWhere;
+ 	HeapTupleData tuple;
+ 	LockTupleMode lockmode;
+ 	Buffer		buffer;
+ 
+ 	/* Determine lock mode to use */
+ 	lockmode = ExecUpdateLockMode(estate, resultRelInfo);
+ 
+ 	/*
+ 	 * Lock tuple for update.  Don't follow updates when tuple cannot be
+ 	 * locked without doing so.  A row locking conflict here means our
+ 	 * previous conclusion that the tuple is conclusively committed is not
+ 	 * true anymore.
+ 	 */
+ 	tuple.t_self = *conflictTid;
+ 	if (!ExecOnConflictLockRow(mtstate, resultRelInfo->ri_RelationDesc, lockmode, &tuple, &buffer, estate))
+ 		return false;
+ 
  	/*
  	 * Success, the tuple is locked.
  	 *
***************
*** 1373,1378 **** ExecOnConflictUpdate(ModifyTableState *mtstate,
--- 1429,1522 ----
  	return true;
  }
  
+ /*
+  * ExecOnConflictSelect --- execute SELECT of INSERT ON CONFLICT DO UPDATE
+  *
+  * Returns true if if we're done (with or without an update), or false if the
+  * caller must retry the INSERT from scratch.
+  */
+ static bool
+ ExecOnConflictSelect(ModifyTableState *mtstate,
+ 					 ResultRelInfo *resultRelInfo,
+ 					 ItemPointer conflictTid,
+ 					 TupleTableSlot *planSlot,
+ 					 EState *estate,
+ 					 TupleTableSlot **returning)
+ {
+ 	ExprContext *econtext = mtstate->ps.ps_ExprContext;
+ 	Relation	relation = resultRelInfo->ri_RelationDesc;
+ 	ExprState  *onConflictSelectWhere = resultRelInfo->ri_onConflictActionWhere;
+ 	LockClauseStrength lockstrength = resultRelInfo->ri_onConflictLockingStrength;
+ 	HeapTupleData tuple;
+ 	Buffer		buffer;
+ 
+ 	tuple.t_self = *conflictTid;
+ 	if (lockstrength != LCS_NONE)
+ 	{
+ 		LockTupleMode lockmode;
+ 
+ 		switch (lockstrength)
+ 		{
+ 			case LCS_FORKEYSHARE:
+ 				lockmode = LockTupleKeyShare;
+ 				break;
+ 			case LCS_FORSHARE:
+ 				lockmode = LockTupleShare;
+ 				break;
+ 			case LCS_FORNOKEYUPDATE:
+ 				lockmode = LockTupleNoKeyExclusive;
+ 				break;
+ 			case LCS_FORUPDATE:
+ 				lockmode = LockTupleExclusive;
+ 				break;
+ 			default:
+ 				elog(ERROR, "unexpected lock strength %d", lockstrength);
+ 		}
+ 		if (!ExecOnConflictLockRow(mtstate, resultRelInfo->ri_RelationDesc, lockmode, &tuple, &buffer, estate))
+ 			return false;
+ 	}
+ 	else
+ 	{
+ 		if (!heap_fetch(relation, SnapshotAny, &tuple, &buffer, false, relation))
+ 			return false;
+ 	}
+ 
+ 	/*
+ 	 * Reset per-tuple memory context to free any expression evaluation
+ 	 * storage allocated in the previous cycle.
+ 	 */
+ 	ResetExprContext(econtext);
+ 
+ 	/*
+ 	 * For the same reasons as ExecOnConflictUpdate, we must verify that the
+ 	 * tuple is visible to our snapshot.
+ 	 */
+ 	ExecCheckHeapTupleVisible(estate, &tuple, buffer);
+ 
+ 	/* Store target's existing tuple in the state's dedicated slot */
+ 	ExecStoreTuple(&tuple, mtstate->mt_existing, buffer, false);
+ 
+ 	/*
+ 	 * Make the tuple available to ExecQual and ExecProject.  EXCLUDED is not
+ 	 * used at all.
+ 	 */
+ 	econtext->ecxt_scantuple = mtstate->mt_existing;
+ 	econtext->ecxt_innertuple = NULL;
+ 	econtext->ecxt_outertuple = NULL;
+ 
+ 	if (!ExecQual(onConflictSelectWhere, econtext))
+ 	{
+ 		ReleaseBuffer(buffer);
+ 		InstrCountFiltered1(&mtstate->ps, 1);
+ 		return true;			/* done with the tuple */
+ 	}
+ 
+ 	*returning = ExecProcessReturning(resultRelInfo, mtstate->mt_existing, planSlot);
+ 
+ 	ReleaseBuffer(buffer);
+ 	return true;
+ }
+ 
  
  /*
   * Process BEFORE EACH STATEMENT triggers
***************
*** 2140,2148 **** ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
  			qualexpr = ExecInitQual((List *) node->onConflictWhere,
  									&mtstate->ps);
  
! 			resultRelInfo->ri_onConflictSetWhere = qualexpr;
  		}
  	}
  
  	/*
  	 * If we have any secondary relations in an UPDATE or DELETE, they need to
--- 2284,2316 ----
  			qualexpr = ExecInitQual((List *) node->onConflictWhere,
  									&mtstate->ps);
  
! 			resultRelInfo->ri_onConflictActionWhere = qualexpr;
  		}
  	}
+ 	else if (node->onConflictAction == ONCONFLICT_SELECT)
+ 	{
+ 		/* already exists if created by RETURNING processing above */
+ 		if (mtstate->ps.ps_ExprContext == NULL)
+ 			ExecAssignExprContext(estate, &mtstate->ps);
+ 
+ 		/* initialize slot for the existing tuple */
+ 		mtstate->mt_existing = ExecInitExtraTupleSlot(mtstate->ps.state);
+ 		ExecSetSlotDescriptor(mtstate->mt_existing,
+ 							  resultRelInfo->ri_RelationDesc->rd_att);
+ 
+ 		/* build DO SELECT WHERE clause expression */
+ 		if (node->onConflictWhere)
+ 		{
+ 			ExprState  *qualexpr;
+ 
+ 			qualexpr = ExecInitQual((List *) node->onConflictWhere,
+ 									&mtstate->ps);
+ 
+ 			resultRelInfo->ri_onConflictActionWhere = qualexpr;
+ 		}
+ 
+ 		resultRelInfo->ri_onConflictLockingStrength = node->onConflictLockingStrength;
+ 	}
  
  	/*
  	 * If we have any secondary relations in an UPDATE or DELETE, they need to
diff --git a/src/backend/nodes/copyfuncs.c b/index 45a04b0..7b75993 100644
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 2455,2460 **** _copyOnConflictClause(const OnConflictClause *from)
--- 2455,2461 ----
  	COPY_SCALAR_FIELD(action);
  	COPY_NODE_FIELD(infer);
  	COPY_NODE_FIELD(targetList);
+ 	COPY_SCALAR_FIELD(lockingStrength);
  	COPY_NODE_FIELD(whereClause);
  	COPY_LOCATION_FIELD(location);
  
diff --git a/src/backend/nodes/equalindex 8d92c03..c332d6a 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 2758,2763 **** _equalOnConflictClause(const OnConflictClause *a, const OnConflictClause *b)
--- 2758,2764 ----
  	COMPARE_SCALAR_FIELD(action);
  	COMPARE_NODE_FIELD(infer);
  	COMPARE_NODE_FIELD(targetList);
+ 	COMPARE_SCALAR_FIELD(lockingStrength);
  	COMPARE_NODE_FIELD(whereClause);
  	COMPARE_LOCATION_FIELD(location);
  
diff --git a/src/backend/optimizer/plindex 5c934f2..73dacde 100644
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***************
*** 6434,6439 **** make_modifytable(PlannerInfo *root,
--- 6434,6440 ----
  		node->onConflictAction = ONCONFLICT_NONE;
  		node->onConflictSet = NIL;
  		node->onConflictWhere = NULL;
+ 		node->onConflictLockingStrength = LCS_NONE;
  		node->arbiterIndexes = NIL;
  		node->exclRelRTI = 0;
  		node->exclRelTlist = NIL;
***************
*** 6443,6448 **** make_modifytable(PlannerInfo *root,
--- 6444,6450 ----
  		node->onConflictAction = onconflict->action;
  		node->onConflictSet = onconflict->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/parser/analyze.c b/srindex 4fb793c..628e431 100644
*** a/src/backend/parser/analyze.c
--- b/src/backend/parser/analyze.c
***************
*** 1108,1123 **** transformOnConflictClause(ParseState *pstate,
  											   onConflictClause->whereClause,
  											   EXPR_KIND_WHERE, "WHERE");
  	}
  
! 	/* Finally, build ON CONFLICT DO [NOTHING | UPDATE] expression */
  	result = makeNode(OnConflictExpr);
  
  	result->action = onConflictClause->action;
  	result->arbiterElems = arbiterElems;
  	result->arbiterWhere = arbiterWhere;
  	result->constraint = arbiterConstraint;
- 	result->onConflictSet = onConflictSet;
  	result->onConflictWhere = onConflictWhere;
  	result->exclRelIndex = exclRelIndex;
  	result->exclRelTlist = exclRelTlist;
  
--- 1108,1137 ----
  											   onConflictClause->whereClause,
  											   EXPR_KIND_WHERE, "WHERE");
  	}
+ 	else if (onConflictClause->action == ONCONFLICT_SELECT)
+ 	{
+ 		/*
+ 		 * References to EXCLUDED are not allowed, but we need the main
+ 		 * relation to be visible to the WHERE clause.
+ 		 */
+ 		addRTEtoQuery(pstate, pstate->p_target_rangetblentry,
+ 					  false, true, true);
+ 
+ 		onConflictWhere = transformWhereClause(pstate,
+ 											   onConflictClause->whereClause,
+ 											   EXPR_KIND_WHERE, "WHERE");
+ 	}
  
! 	/* Finally, build ON CONFLICT DO [NOTHING | SELECT | UPDATE] expression */
  	result = makeNode(OnConflictExpr);
  
  	result->action = onConflictClause->action;
  	result->arbiterElems = arbiterElems;
  	result->arbiterWhere = arbiterWhere;
  	result->constraint = arbiterConstraint;
  	result->onConflictWhere = onConflictWhere;
+ 	result->lockingStrength = onConflictClause->lockingStrength;
+ 	result->onConflictSet = onConflictSet;
  	result->exclRelIndex = exclRelIndex;
  	result->exclRelTlist = exclRelTlist;
  
diff --git a/src/backend/parser/graindex 7d0de99..9751a13 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***************
*** 419,425 **** static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
  %type <ival>	 OptNoLog
  %type <oncommit> OnCommitOption
  
! %type <ival>	for_locking_strength
  %type <node>	for_locking_item
  %type <list>	for_locking_clause opt_for_locking_clause for_locking_items
  %type <list>	locked_rels_list
--- 419,425 ----
  %type <ival>	 OptNoLog
  %type <oncommit> OnCommitOption
  
! %type <ival>	for_locking_strength opt_for_locking_strength
  %type <node>	for_locking_item
  %type <list>	for_locking_clause opt_for_locking_clause for_locking_items
  %type <list>	locked_rels_list
***************
*** 10523,10534 **** insert_column_item:
--- 10523,10546 ----
  		;
  
  opt_on_conflict:
+ 			ON CONFLICT opt_conf_expr DO SELECT opt_for_locking_strength where_clause
+ 				{
+ 					$$ = makeNode(OnConflictClause);
+ 					$$->action = ONCONFLICT_SELECT;
+ 					$$->infer = $3;
+ 					$$->targetList = NIL;
+ 					$$->lockingStrength = $6;
+ 					$$->whereClause = $7;
+ 					$$->location = @1;
+ 				}
+ 			|
  			ON CONFLICT opt_conf_expr DO UPDATE SET set_clause_list	where_clause
  				{
  					$$ = makeNode(OnConflictClause);
  					$$->action = ONCONFLICT_UPDATE;
  					$$->infer = $3;
  					$$->targetList = $7;
+ 					$$->lockingStrength = LCS_NONE;
  					$$->whereClause = $8;
  					$$->location = @1;
  				}
***************
*** 10539,10544 **** opt_on_conflict:
--- 10551,10557 ----
  					$$->action = ONCONFLICT_NOTHING;
  					$$->infer = $3;
  					$$->targetList = NIL;
+ 					$$->lockingStrength = LCS_NONE;
  					$$->whereClause = NULL;
  					$$->location = @1;
  				}
***************
*** 11347,11352 **** for_locking_strength:
--- 11360,11370 ----
  			| FOR KEY SHARE 					{ $$ = LCS_FORKEYSHARE; }
  		;
  
+ opt_for_locking_strength:
+ 			for_locking_strength				{ $$ = $1; }
+ 			| /* EMPTY */						{ $$ = LCS_NONE; }
+ 		;
+ 
  locked_rels_list:
  			OF qualified_name_list					{ $$ = $2; }
  			| /* EMPTY */							{ $$ = NIL; }
diff --git a/src/backend/parser/index 9ff80b8..dee1231 100644
*** a/src/backend/parser/parse_clause.c
--- b/src/backend/parser/parse_clause.c
***************
*** 3135,3140 **** transformOnConflictArbiter(ParseState *pstate,
--- 3135,3147 ----
  				 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))));
  
  	/*
  	 * To simplify certain aspects of its design, speculative insertion into
diff --git a/src/include/nodes/execnodesindex 35c28a6..827c1a7 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
***************
*** 401,408 **** typedef struct ResultRelInfo
  	/* for computing ON CONFLICT DO UPDATE SET */
  	ProjectionInfo *ri_onConflictSetProj;
  
! 	/* list of ON CONFLICT DO UPDATE exprs (qual) */
! 	ExprState  *ri_onConflictSetWhere;
  
  	/* partition check expression */
  	List	   *ri_PartitionCheck;
--- 401,411 ----
  	/* for computing ON CONFLICT DO UPDATE SET */
  	ProjectionInfo *ri_onConflictSetProj;
  
! 	/* list of ON CONFLICT DO SELECT/UPDATE exprs (qual) */
! 	ExprState  *ri_onConflictActionWhere;
! 
! 	/* strengh of lock for ON CONFLICT DO SELECT, or LCS_NONE */
! 	LockClauseStrength ri_onConflictLockingStrength;
  
  	/* partition check expression */
  	List	   *ri_PartitionCheck;
diff --git a/src/include/nodes/lockoindex e0981da..eae4210 100644
*** a/src/include/nodes/lockoptions.h
--- b/src/include/nodes/lockoptions.h
***************
*** 20,26 ****
   */
  typedef enum LockClauseStrength
  {
! 	LCS_NONE,					/* no such clause - only used in PlanRowMark */
  	LCS_FORKEYSHARE,			/* FOR KEY SHARE */
  	LCS_FORSHARE,				/* FOR SHARE */
  	LCS_FORNOKEYUPDATE,			/* FOR NO KEY UPDATE */
--- 20,26 ----
   */
  typedef enum LockClauseStrength
  {
! 	LCS_NONE,					/* no such clause - only used in PlanRowMark and ON CONFLICT SELECT */
  	LCS_FORKEYSHARE,			/* FOR KEY SHARE */
  	LCS_FORSHARE,				/* FOR SHARE */
  	LCS_FORNOKEYUPDATE,			/* FOR NO KEY UPDATE */
diff --git a/src/include/nodes/nodes.hindex 27bd4f3..6c80bb7 100644
*** a/src/include/nodes/nodes.h
--- b/src/include/nodes/nodes.h
***************
*** 800,806 **** typedef enum OnConflictAction
  {
  	ONCONFLICT_NONE,			/* No "ON CONFLICT" clause */
  	ONCONFLICT_NOTHING,			/* ON CONFLICT ... DO NOTHING */
! 	ONCONFLICT_UPDATE			/* ON CONFLICT ... DO UPDATE */
  } OnConflictAction;
  
  #endif							/* NODES_H */
--- 800,807 ----
  {
  	ONCONFLICT_NONE,			/* No "ON CONFLICT" clause */
  	ONCONFLICT_NOTHING,			/* ON CONFLICT ... DO NOTHING */
! 	ONCONFLICT_UPDATE,			/* ON CONFLICT ... DO UPDATE */
! 	ONCONFLICT_SELECT			/* ON CONFLICT ... DO SELECT */
  } OnConflictAction;
  
  #endif							/* NODES_H */
diff --git a/src/include/nodes/pindex 5f2a4a7..9afc693 100644
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 1343,1351 **** typedef struct InferClause
  typedef struct OnConflictClause
  {
  	NodeTag		type;
! 	OnConflictAction action;	/* DO NOTHING or UPDATE? */
  	InferClause *infer;			/* Optional index inference clause */
  	List	   *targetList;		/* the target list (of ResTarget) */
  	Node	   *whereClause;	/* qualifications */
  	int			location;		/* token location, or -1 if unknown */
  } OnConflictClause;
--- 1343,1352 ----
  typedef struct OnConflictClause
  {
  	NodeTag		type;
! 	OnConflictAction action;	/* DO NOTHING, SELECT or UPDATE? */
  	InferClause *infer;			/* Optional index inference clause */
  	List	   *targetList;		/* the target list (of ResTarget) */
+ 	LockClauseStrength lockingStrength; /* strengh of lock for DO SELECT, or LCS_NONE */
  	Node	   *whereClause;	/* qualifications */
  	int			location;		/* token location, or -1 if unknown */
  } OnConflictClause;
diff --git a/src/include/nodes/plannoindex 7c51e7f..9975680 100644
*** a/src/include/nodes/plannodes.h
--- b/src/include/nodes/plannodes.h
***************
*** 232,238 **** typedef struct ModifyTable
  	OnConflictAction onConflictAction;	/* ON CONFLICT action */
  	List	   *arbiterIndexes; /* List of ON CONFLICT arbiter index OIDs  */
  	List	   *onConflictSet;	/* SET for INSERT ON CONFLICT DO UPDATE */
! 	Node	   *onConflictWhere;	/* WHERE for ON CONFLICT UPDATE */
  	Index		exclRelRTI;		/* RTI of the EXCLUDED pseudo relation */
  	List	   *exclRelTlist;	/* tlist of the EXCLUDED pseudo relation */
  } ModifyTable;
--- 232,239 ----
  	OnConflictAction onConflictAction;	/* ON CONFLICT action */
  	List	   *arbiterIndexes; /* List of ON CONFLICT arbiter index OIDs  */
  	List	   *onConflictSet;	/* SET for INSERT ON CONFLICT DO UPDATE */
! 	LockClauseStrength onConflictLockingStrength; /* lock strength for ON CONFLICT SELECT */
! 	Node	   *onConflictWhere;	/* WHERE for ON CONFLICT SELECT and UPDATE */
  	Index		exclRelRTI;		/* RTI of the EXCLUDED pseudo relation */
  	List	   *exclRelTlist;	/* tlist of the EXCLUDED pseudo relation */
  } ModifyTable;
diff --git a/src/include/nodes/primnindex 8c536a8..5d28ff1 100644
*** a/src/include/nodes/primnodes.h
--- b/src/include/nodes/primnodes.h
***************
*** 20,25 ****
--- 20,26 ----
  #include "access/attnum.h"
  #include "nodes/bitmapset.h"
  #include "nodes/pg_list.h"
+ #include "nodes/lockoptions.h"
  
  
  /* ----------------------------------------------------------------
***************
*** 1492,1500 **** typedef struct OnConflictExpr
  	Node	   *arbiterWhere;	/* unique index arbiter WHERE clause */
  	Oid			constraint;		/* pg_constraint OID for arbiter */
  
  	/* ON CONFLICT UPDATE */
  	List	   *onConflictSet;	/* List of ON CONFLICT SET TargetEntrys */
- 	Node	   *onConflictWhere;	/* qualifiers to restrict UPDATE to */
  	int			exclRelIndex;	/* RT index of 'excluded' relation */
  	List	   *exclRelTlist;	/* tlist of the EXCLUDED pseudo relation */
  } OnConflictExpr;
--- 1493,1506 ----
  	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 SELECT */
+ 	LockClauseStrength lockingStrength; /* strengh of lock for DO SELECT, or LCS_NONE */
+ 
  	/* ON CONFLICT UPDATE */
  	List	   *onConflictSet;	/* List of ON CONFLICT SET TargetEntrys */
  	int			exclRelIndex;	/* RT index of 'excluded' relation */
  	List	   *exclRelTlist;	/* tlist of the EXCLUDED pseudo relation */
  } OnConflictExpr;
diff --git a/src/test/regress/expectindex 8d005fd..258ab77 100644
*** a/src/test/regress/expected/insert_conflict.out
--- b/src/test/regress/expected/insert_conflict.out
***************
*** 236,241 **** insert into insertconflicttest values (2, 'Orange') on conflict (key, key, key)
--- 236,278 ----
  insert into insertconflicttest
  values (1, 'Apple'), (2, 'Orange')
  on conflict (key) do update set (fruit, key) = (excluded.fruit, excluded.key);
+ -- DO SELECT
+ delete from insertconflicttest where fruit = 'Apple';
+ insert into insertconflicttest values (1, 'Apple') on conflict (key) do select returning *;
+  key | fruit 
+ -----+-------
+    1 | Apple
+ (1 row)
+ 
+ insert into insertconflicttest values (1, 'Apple') on conflict (key) do select returning *;
+  key | fruit 
+ -----+-------
+    1 | Apple
+ (1 row)
+ 
+ insert into insertconflicttest values (1, 'Apple') on conflict (key) do select where fruit <> 'Apple' returning *;
+  key | fruit 
+ -----+-------
+ (0 rows)
+ 
+ delete from insertconflicttest where fruit = 'Apple';
+ insert into insertconflicttest values (1, 'Apple') on conflict (key) do select for update returning *;
+  key | fruit 
+ -----+-------
+    1 | Apple
+ (1 row)
+ 
+ insert into insertconflicttest values (1, 'Apple') on conflict (key) do select for update returning *;
+  key | fruit 
+ -----+-------
+    1 | Apple
+ (1 row)
+ 
+ insert into insertconflicttest values (1, 'Apple') on conflict (key) do select for update where fruit <> 'Apple' returning *;
+  key | fruit 
+ -----+-------
+ (0 rows)
+ 
  -- Give good diagnostic message when EXCLUDED.* spuriously referenced from
  -- RETURNING:
  insert into insertconflicttest values (1, 'Apple') on conflict (key) do update set fruit = excluded.fruit RETURNING excluded.fruit;
***************
*** 764,780 **** insert into selfconflict values (3,1), (3,2) on conflict do nothing;
  commit;
  begin transaction isolation level read committed;
  insert into selfconflict values (4,1), (4,2) on conflict(f1) do update set f2 = 0;
! ERROR:  ON CONFLICT DO UPDATE command cannot affect row a second time
  HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
  commit;
  begin transaction isolation level repeatable read;
  insert into selfconflict values (5,1), (5,2) on conflict(f1) do update set f2 = 0;
! ERROR:  ON CONFLICT DO UPDATE command cannot affect row a second time
  HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
  commit;
  begin transaction isolation level serializable;
  insert into selfconflict values (6,1), (6,2) on conflict(f1) do update set f2 = 0;
! ERROR:  ON CONFLICT DO UPDATE command cannot affect row a second time
  HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
  commit;
  select * from selfconflict;
--- 801,859 ----
  commit;
  begin transaction isolation level read committed;
  insert into selfconflict values (4,1), (4,2) on conflict(f1) do update set f2 = 0;
! ERROR:  ON CONFLICT command cannot affect row a second time
  HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
  commit;
  begin transaction isolation level repeatable read;
  insert into selfconflict values (5,1), (5,2) on conflict(f1) do update set f2 = 0;
! ERROR:  ON CONFLICT command cannot affect row a second time
  HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
  commit;
  begin transaction isolation level serializable;
  insert into selfconflict values (6,1), (6,2) on conflict(f1) do update set f2 = 0;
! ERROR:  ON CONFLICT command cannot affect row a second time
! HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
! commit;
! begin transaction isolation level read committed;
! insert into selfconflict values (7,1), (7,2) on conflict(f1) do select returning *;
!  f1 | f2 
! ----+----
!   7 |  1
!   7 |  1
! (2 rows)
! 
! commit;
! begin transaction isolation level repeatable read;
! insert into selfconflict values (8,1), (8,2) on conflict(f1) do select returning *;
!  f1 | f2 
! ----+----
!   8 |  1
!   8 |  1
! (2 rows)
! 
! commit;
! begin transaction isolation level serializable;
! insert into selfconflict values (9,1), (9,2) on conflict(f1) do select returning *;
!  f1 | f2 
! ----+----
!   9 |  1
!   9 |  1
! (2 rows)
! 
! commit;
! begin transaction isolation level read committed;
! insert into selfconflict values (10,1), (10,2) on conflict(f1) do select for update returning *;
! ERROR:  ON CONFLICT command cannot affect row a second time
! HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
! commit;
! begin transaction isolation level repeatable read;
! insert into selfconflict values (11,1), (11,2) on conflict(f1) do select for update returning *;
! ERROR:  ON CONFLICT command cannot affect row a second time
! HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
! commit;
! begin transaction isolation level serializable;
! insert into selfconflict values (12,1), (12,2) on conflict(f1) do select for update returning *;
! ERROR:  ON CONFLICT command cannot affect row a second time
  HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
  commit;
  select * from selfconflict;
***************
*** 783,788 **** select * from selfconflict;
    1 |  1
    2 |  1
    3 |  1
! (3 rows)
  
  drop table selfconflict;
--- 862,870 ----
    1 |  1
    2 |  1
    3 |  1
!   7 |  1
!   8 |  1
!   9 |  1
! (6 rows)
  
  drop table selfconflict;
diff --git a/src/test/regress/output/constraints.souindex bb75165..b39e39b 100644
*** a/src/test/regress/output/constraints.source
--- b/src/test/regress/output/constraints.source
***************
*** 425,431 **** INSERT INTO UNIQUE_TBL VALUES (5, 'five-upsert-insert') ON CONFLICT (i) DO UPDAT
  INSERT INTO UNIQUE_TBL VALUES (6, 'six-upsert-insert') ON CONFLICT (i) DO UPDATE SET t = 'six-upsert-update';
  -- should fail
  INSERT INTO UNIQUE_TBL VALUES (1, 'a'), (2, 'b'), (2, 'b') ON CONFLICT (i) DO UPDATE SET t = 'fails';
! ERROR:  ON CONFLICT DO UPDATE command cannot affect row a second time
  HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
  SELECT '' AS five, * FROM UNIQUE_TBL;
   five | i |         t          
--- 425,431 ----
  INSERT INTO UNIQUE_TBL VALUES (6, 'six-upsert-insert') ON CONFLICT (i) DO UPDATE SET t = 'six-upsert-update';
  -- should fail
  INSERT INTO UNIQUE_TBL VALUES (1, 'a'), (2, 'b'), (2, 'b') ON CONFLICT (i) DO UPDATE SET t = 'fails';
! ERROR:  ON CONFLICT command cannot affect row a second time
  HINT:  Ensure that no rows proposed for insertion within the same command have duplicate constrained values.
  SELECT '' AS five, * FROM UNIQUE_TBL;
   five | i |         t          
diff --git a/src/test/regress/sql/insert_conflictindex df3a9b5..ae553d8 100644
*** a/src/test/regress/sql/insert_conflict.sql
--- b/src/test/regress/sql/insert_conflict.sql
***************
*** 97,102 **** insert into insertconflicttest
--- 97,112 ----
  values (1, 'Apple'), (2, 'Orange')
  on conflict (key) do update set (fruit, key) = (excluded.fruit, excluded.key);
  
+ -- DO SELECT
+ delete from insertconflicttest where fruit = 'Apple';
+ 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 *;
+ 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 *;
+ 
  -- Give good diagnostic message when EXCLUDED.* spuriously referenced from
  -- RETURNING:
  insert into insertconflicttest values (1, 'Apple') on conflict (key) do update set fruit = excluded.fruit RETURNING excluded.fruit;
***************
*** 468,473 **** begin transaction isolation level serializable;
--- 478,507 ----
  insert into selfconflict values (6,1), (6,2) on conflict(f1) do update set f2 = 0;
  commit;
  
+ begin transaction isolation level read committed;
+ insert into selfconflict values (7,1), (7,2) on conflict(f1) do select returning *;
+ commit;
+ 
+ begin transaction isolation level repeatable read;
+ insert into selfconflict values (8,1), (8,2) on conflict(f1) do select returning *;
+ commit;
+ 
+ begin transaction isolation level serializable;
+ insert into selfconflict values (9,1), (9,2) on conflict(f1) do select returning *;
+ commit;
+ 
+ begin transaction isolation level read committed;
+ insert into selfconflict values (10,1), (10,2) on conflict(f1) do select for update returning *;
+ commit;
+ 
+ begin transaction isolation level repeatable read;
+ insert into selfconflict values (11,1), (11,2) on conflict(f1) do select for update returning *;
+ commit;
+ 
+ begin transaction isolation level serializable;
+ insert into selfconflict values (12,1), (12,2) on conflict(f1) do select for update returning *;
+ commit;
+ 
  select * from selfconflict;
  
  drop table selfconflict;
In reply to: Marko Tiikkaja (#1)
Re: INSERT .. ON CONFLICT DO SELECT [FOR ..]

On Mon, Aug 14, 2017 at 6:23 PM, Marko Tiikkaja <marko@joh.to> wrote:

Attached is a patch for $SUBJECT. It might still be a bit rough around the
edges and probably light on docs and testing, but I thought I'd post it
anyway so I won't forget.

Is it possible for ON CONFLICT DO SELECT to raise a cardinality
violation error? Why or why not?

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Marko Tiikkaja
marko@joh.to
In reply to: Peter Geoghegan (#2)
Re: INSERT .. ON CONFLICT DO SELECT [FOR ..]

On Tue, Aug 15, 2017 at 7:43 AM, Peter Geoghegan <pg@bowt.ie> wrote:

On Mon, Aug 14, 2017 at 6:23 PM, Marko Tiikkaja <marko@joh.to> wrote:

Attached is a patch for $SUBJECT. It might still be a bit rough around

the

edges and probably light on docs and testing, but I thought I'd post it
anyway so I won't forget.

Is it possible for ON CONFLICT DO SELECT to raise a cardinality
violation error? Why or why not?

No. I don't see when that would need to happen. But I'm guessing you have
a case in mind?

.m

In reply to: Marko Tiikkaja (#3)
Re: INSERT .. ON CONFLICT DO SELECT [FOR ..]

On Tue, Aug 15, 2017 at 12:17 AM, Marko Tiikkaja <marko@joh.to> wrote:

On Tue, Aug 15, 2017 at 7:43 AM, Peter Geoghegan <pg@bowt.ie> wrote:

On Mon, Aug 14, 2017 at 6:23 PM, Marko Tiikkaja <marko@joh.to> wrote:

Attached is a patch for $SUBJECT. It might still be a bit rough around
the
edges and probably light on docs and testing, but I thought I'd post it
anyway so I won't forget.

Is it possible for ON CONFLICT DO SELECT to raise a cardinality
violation error? Why or why not?

No. I don't see when that would need to happen. But I'm guessing you have
a case in mind?

Actually, no, I didn't. But I wondered if you did. I think that it
makes some sense not to, now that I think about it. ON CONFLICT DO
NOTHING doesn't have cardinality violations, because it cannot affect
a row twice if there are duplicates proposed for insertion (at least,
not through any ON CONFLICT related codepath). But, this opinion only
applies to ON CONFLICT DO SELECT, not ON CONFLICT DO SELECT FOR
UPDATE. And I have other reservations, which I'll go in to
momentarily, about ON CONFLICT DO SELECT in general. So, the upshot is
that I think we need cardinality violations in all cases for this
feature. Why would a user ever not want to know that the row was
locked twice?

On to the subject of my more general reservation: Why did you include
ON CONFLICT DO SELECT at all? Why not just ON CONFLICT DO SELECT FOR
UPDATE (and FOR SHARE, ...) ?

I think I know what you're going to say about it: ON CONFLICT DO
NOTHING doesn't lock the conflicting row, so why should I insist on it
here (why not have an ON CONFLICT DO SELECT variant, too)? That's not
a bad argument. However, I think that there is still a difference.
Namely, ON CONFLICT DO NOTHING doesn't let you project the rows with
RETURNING (that may not even be visible to the command's MVCC snapshot
-- the rows that we also don't lock), because those are simply not the
semantics it has. DO NOTHING is more or less about ETL use-cases, some
of which involve data from very unclean sources. It seems questionable
to cite that as precedent for not locking a row (that is, for having a
plain ON CONFLICT DO SELECT variant at all).

In other words, while ON CONFLICT DO NOTHING may have set a precedent
here, it at least has semantics that limit the consequences of not
locking the row; and it *feels* a little bit dirty to use it
indifferently, even where that makes sense. ON CONFLICT DO SELECT is
probably going to be used within wCTEs some of the time. I'm not sure
that a plain ON CONFLICT DO SELECT variant won't allow unpredictable,
complicated problems when composed within a more complicated query.
(This brings me back!)

In other other words, plain SELECT FOR UPDATE has to do all the EPQ
stuff, in order to confront many of the same issues that ON CONFLICT
must confront in its own way [1]https://wiki.postgresql.org/wiki/UPSERT#Visibility_issues_and_the_proposed_syntax_.28WHERE_clause.2Fpredicate_stuff.29 -- Peter Geoghegan, but a plain SELECT never does any
EPQ stuff at all. It seems to follow that a plain ON CONFLICT DO
SELECT is an oxymoron. If I've missed the point of ON CONFLICT DO
SELECT, please let me know how.

[1]: https://wiki.postgresql.org/wiki/UPSERT#Visibility_issues_and_the_proposed_syntax_.28WHERE_clause.2Fpredicate_stuff.29 -- Peter Geoghegan
--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Marko Tiikkaja
marko@joh.to
In reply to: Peter Geoghegan (#4)
Re: INSERT .. ON CONFLICT DO SELECT [FOR ..]

On Mon, Sep 4, 2017 at 4:09 AM, Peter Geoghegan <pg@bowt.ie> wrote:

On Tue, Aug 15, 2017 at 12:17 AM, Marko Tiikkaja <marko@joh.to> wrote:

On Tue, Aug 15, 2017 at 7:43 AM, Peter Geoghegan <pg@bowt.ie> wrote:

On Mon, Aug 14, 2017 at 6:23 PM, Marko Tiikkaja <marko@joh.to> wrote:

Attached is a patch for $SUBJECT. It might still be a bit rough

around

the
edges and probably light on docs and testing, but I thought I'd post

it

anyway so I won't forget.

Is it possible for ON CONFLICT DO SELECT to raise a cardinality
violation error? Why or why not?

No. I don't see when that would need to happen. But I'm guessing you

have

a case in mind?

Actually, no, I didn't. But I wondered if you did. I think that it
makes some sense not to, now that I think about it. ON CONFLICT DO
NOTHING doesn't have cardinality violations, because it cannot affect
a row twice if there are duplicates proposed for insertion (at least,
not through any ON CONFLICT related codepath). But, this opinion only
applies to ON CONFLICT DO SELECT, not ON CONFLICT DO SELECT FOR
UPDATE. And I have other reservations, which I'll go in to
momentarily, about ON CONFLICT DO SELECT in general. So, the upshot is
that I think we need cardinality violations in all cases for this
feature. Why would a user ever not want to know that the row was
locked twice?

I had to look at the patch to see what I'd done, and the tests suggest that
we already complain about this with if a FOR [lock level] clause is present:

   +begin transaction isolation level read committed;
   +insert into selfconflict values (10,1), (10,2) on conflict(f1) do
select for update returning *;
   +ERROR:  ON CONFLICT command cannot affect row a second time
   +HINT:  Ensure that no rows proposed for insertion within the same
command have duplicate constrained values.
   +commit;

(in expected/insert_conflict.out.)

On to the subject of my more general reservation: Why did you include
ON CONFLICT DO SELECT at all? Why not just ON CONFLICT DO SELECT FOR
UPDATE (and FOR SHARE, ...) ?

If you don't intend to actually do anything with the row in the same
database transaction, locking seems unnecessary. For example, you might
want to provide an idempotent method in your API which inserts the data and
returns the ID to the caller. The transaction is committed by the time the
client sees the data, so locking is just extra overhead.

I think I know what you're going to say about it: ON CONFLICT DO
NOTHING doesn't lock the conflicting row, so why should I insist on it
here (why not have an ON CONFLICT DO SELECT variant, too)?

I wasn't going to say that, no.

In other words, while ON CONFLICT DO NOTHING may have set a precedent
here, it at least has semantics that limit the consequences of not
locking the row; and it *feels* a little bit dirty to use it
indifferently, even where that makes sense. ON CONFLICT DO SELECT is
probably going to be used within wCTEs some of the time. I'm not sure
that a plain ON CONFLICT DO SELECT variant won't allow unpredictable,
complicated problems when composed within a more complicated query.

Yeah, in most cases you'd probably do a SELECT FOR KEY SHARE. And I
wouldn't mind that being the default, but it would mean inventing special
syntax with no previous precedent (as far as I know) for not locking the
row in cases where the user doesn't want that. And that doesn't seem too
attractive, either.

Maybe it would be better to make the default sensible for people who are
not super familiar with postgres. I don't know. And the more I think
about the use case above, the less I care about it. But I'm generally
against interfaces which put arbitrary restrictions on what power users can
do on the basis that less experienced people might misuse the interface.

.m

In reply to: Marko Tiikkaja (#5)
Re: INSERT .. ON CONFLICT DO SELECT [FOR ..]

On Mon, Sep 4, 2017 at 10:05 AM, Marko Tiikkaja <marko@joh.to> wrote:

I had to look at the patch to see what I'd done, and the tests suggest that
we already complain about this with if a FOR [lock level] clause is present:

+begin transaction isolation level read committed;
+insert into selfconflict values (10,1), (10,2) on conflict(f1) do select
for update returning *;
+ERROR:  ON CONFLICT command cannot affect row a second time
+HINT:  Ensure that no rows proposed for insertion within the same
command have duplicate constrained values.
+commit;

(in expected/insert_conflict.out.)

Right. I saw that you do it for ON CONFLICT DO SELECT FOR UPDATE, and so on.

On to the subject of my more general reservation: Why did you include
ON CONFLICT DO SELECT at all? Why not just ON CONFLICT DO SELECT FOR
UPDATE (and FOR SHARE, ...) ?

If you don't intend to actually do anything with the row in the same
database transaction, locking seems unnecessary. For example, you might
want to provide an idempotent method in your API which inserts the data and
returns the ID to the caller. The transaction is committed by the time the
client sees the data, so locking is just extra overhead.

That makes sense, but I personally feel that the plain ON CONFLICT DO
SELECT variant isn't worth the trouble. I welcome other people's
opinions on that.

I think I know what you're going to say about it: ON CONFLICT DO
NOTHING doesn't lock the conflicting row, so why should I insist on it
here (why not have an ON CONFLICT DO SELECT variant, too)?

I wasn't going to say that, no.

Well, it was a foundation for the ON CONFLICT DO SELECT variant that I
actually agree with, in any case.

In other words, while ON CONFLICT DO NOTHING may have set a precedent
here, it at least has semantics that limit the consequences of not
locking the row; and it *feels* a little bit dirty to use it
indifferently, even where that makes sense. ON CONFLICT DO SELECT is
probably going to be used within wCTEs some of the time. I'm not sure
that a plain ON CONFLICT DO SELECT variant won't allow unpredictable,
complicated problems when composed within a more complicated query.

Yeah, in most cases you'd probably do a SELECT FOR KEY SHARE. And I
wouldn't mind that being the default, but it would mean inventing special
syntax with no previous precedent (as far as I know) for not locking the row
in cases where the user doesn't want that. And that doesn't seem too
attractive, either.

I'm not in favor of spellings that are inconsistent with SELECT FOR ... .

Maybe it would be better to make the default sensible for people who are not
super familiar with postgres. I don't know. And the more I think about the
use case above, the less I care about it.

I was going to ask you to offer a real-world example of where the
plain ON CONFLICT DO SELECT variant is compelling. If you care about
it (if you're not going to concede the point), then I still suggest
doing so.

But I'm generally against
interfaces which put arbitrary restrictions on what power users can do on
the basis that less experienced people might misuse the interface.

I agree with that as a broad principle, but disagree that it applies
here. Or if it does, then you have yet to convince me of it. In all
sincerity, if you think that I just don't understand your perspective,
then please try to make me understand it. Would a power user actually
miss ON CONFLICT DO SELECT? And if that's the case, would it really be
something they'd remember 5 minutes later?

I actually think that ON CONFLICT DO NOTHING does have semantics that
are, shall we say, questionable. That's the cost of having it not lock
conflicting rows during big ETL operations. That's a huge practical
benefit for ETL use-cases. Whereas here, with ON CONFLICT DO SELECT,
I see a somewhat greater risk, and a much, much smaller benefit. A
benefit that might actually be indistinguishable from zero.

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Marko Tiikkaja
marko@joh.to
In reply to: Peter Geoghegan (#6)
Re: INSERT .. ON CONFLICT DO SELECT [FOR ..]

On Mon, Sep 4, 2017 at 7:46 PM, Peter Geoghegan <pg@bowt.ie> wrote:

On Mon, Sep 4, 2017 at 10:05 AM, Marko Tiikkaja <marko@joh.to> wrote:

But I'm generally against

interfaces which put arbitrary restrictions on what power users can do on
the basis that less experienced people might misuse the interface.

I agree with that as a broad principle, but disagree that it applies
here. Or if it does, then you have yet to convince me of it. In all
sincerity, if you think that I just don't understand your perspective,
then please try to make me understand it. Would a power user actually
miss ON CONFLICT DO SELECT? And if that's the case, would it really be
something they'd remember 5 minutes later?

I don't know, but I don't want to be limiting what people can do just
because I can't come up with a use case.

In any case, others, feel free to chime in.

.m

#8Michael Paquier
michael.paquier@gmail.com
In reply to: Marko Tiikkaja (#7)
Re: [HACKERS] INSERT .. ON CONFLICT DO SELECT [FOR ..]

On Tue, Sep 5, 2017 at 3:28 AM, Marko Tiikkaja <marko@joh.to> wrote:

On Mon, Sep 4, 2017 at 7:46 PM, Peter Geoghegan <pg@bowt.ie> wrote:

On Mon, Sep 4, 2017 at 10:05 AM, Marko Tiikkaja <marko@joh.to> wrote:

But I'm generally against
interfaces which put arbitrary restrictions on what power users can do
on
the basis that less experienced people might misuse the interface.

I agree with that as a broad principle, but disagree that it applies
here. Or if it does, then you have yet to convince me of it. In all
sincerity, if you think that I just don't understand your perspective,
then please try to make me understand it. Would a power user actually
miss ON CONFLICT DO SELECT? And if that's the case, would it really be
something they'd remember 5 minutes later?

I don't know, but I don't want to be limiting what people can do just
because I can't come up with a use case.

In any case, others, feel free to chime in.

The patch does not currently apply. I am noticing as well that Peter
Geoghegan has registered himself as a reviewer a couple of hours back,
so moved to next CF with waiting on author as status.
--
Michael

In reply to: Michael Paquier (#8)
Re: [HACKERS] INSERT .. ON CONFLICT DO SELECT [FOR ..]

On Wed, Nov 29, 2017 at 8:34 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

The patch does not currently apply. I am noticing as well that Peter
Geoghegan has registered himself as a reviewer a couple of hours back,
so moved to next CF with waiting on author as status.

Marko unregistered me, so I promptly reregistered. You'll have to ask him why.

--
Peter Geoghegan

#10Marko Tiikkaja
marko@joh.to
In reply to: Peter Geoghegan (#9)
Re: [HACKERS] INSERT .. ON CONFLICT DO SELECT [FOR ..]

On Thu, Nov 30, 2017 at 6:39 AM, Peter Geoghegan <pg@bowt.ie> wrote:

On Wed, Nov 29, 2017 at 8:34 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

The patch does not currently apply. I am noticing as well that Peter
Geoghegan has registered himself as a reviewer a couple of hours back,
so moved to next CF with waiting on author as status.

Marko unregistered me, so I promptly reregistered. You'll have to ask him
why.

Oops. I didn't mean to do what I did, and I apologize. I had my months
mixed up and I thought this commit fest had ended without you having looked
at the patch, so I assumed you would register in the next CF if you were
still interested, or someone else could pick it up.

In any case, *I* don't have any interest in pursuing this patch at this
point. I think this is a really good feature, and as far as I know the
patch is technically solid, or at least was when it still applied. Can
someone else take it from here?

.m

#11Stephen Frost
sfrost@snowman.net
In reply to: Marko Tiikkaja (#10)
Re: [HACKERS] INSERT .. ON CONFLICT DO SELECT [FOR ..]

Greetings,

* Marko Tiikkaja (marko@joh.to) wrote:

On Thu, Nov 30, 2017 at 6:39 AM, Peter Geoghegan <pg@bowt.ie> wrote:

On Wed, Nov 29, 2017 at 8:34 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

The patch does not currently apply. I am noticing as well that Peter
Geoghegan has registered himself as a reviewer a couple of hours back,
so moved to next CF with waiting on author as status.

Marko unregistered me, so I promptly reregistered. You'll have to ask him
why.

Oops. I didn't mean to do what I did, and I apologize. I had my months
mixed up and I thought this commit fest had ended without you having looked
at the patch, so I assumed you would register in the next CF if you were
still interested, or someone else could pick it up.

In any case, *I* don't have any interest in pursuing this patch at this
point. I think this is a really good feature, and as far as I know the
patch is technically solid, or at least was when it still applied. Can
someone else take it from here?

Given that there hasn't been any further progress on this, I'm going to
go ahead and mark it as returned with feedback. It'll be in the
archives and in the CF app if someone decides to pick it up but there
doesn't seem much point leaving it as Waiting for Author in this CF when
the author doesn't have further time/interest in it.

Thanks!

Stephen