weird ON CONFLICT clauses

Started by Álvaro Herreraabout 2 months ago3 messages
#1Álvaro Herrera
alvherre@kurilemu.de
1 attachment(s)

Hi,

While reviewing a patch I noticed that we allow some extraneous items in
ON CONFLICT clauses -- for instance,

create table tab (a int unique, b int);
insert into tab values (1, 1) on conflict (a int4_ops (fillfactor=10)) do nothing;

Why do we accept reloptions there without complaint? Should we tighten
this up a little bit, or maybe it makes sense to accept this for some
reason? I suspect the reloptions were added to index_elems after the ON
CONFLICT clause was made to use that production, but I didn't check the
git history.

So what about the attached patch? I ran all tests and everything seems
to work correctly. (Maybe I'd add some tests to verify that this
new error is covered, as the ones just above.) It would complain to the
above:

ERROR: operator class options are not allowed in ON CONFLICT clause
LÍNEA 1: insert into tab values (1, 1) on conflict (a int4_ops (fillf...
^

This is certainly not very critical.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"El Maquinismo fue proscrito so pena de cosquilleo hasta la muerte"
(Ijon Tichy en Viajes, Stanislaw Lem)

Attachments:

0001-Reject-opclass-options-in-ON-CONFLICT-clause.patchtext/x-diff; charset=utf-8Download
From 332db7b70a395da7e8628350285dd81505d81c20 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Thu, 27 Nov 2025 16:51:31 +0100
Subject: [PATCH] Reject opclass options in ON CONFLICT clause

---
 src/backend/parser/parse_clause.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index ca26f6f61f2..bee9860c513 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -3277,11 +3277,11 @@ resolve_unique_index_expr(ParseState *pstate, InferClause *infer,
 		 * Raw grammar re-uses CREATE INDEX infrastructure for unique index
 		 * inference clause, and so will accept opclasses by name and so on.
 		 *
-		 * Make no attempt to match ASC or DESC ordering or NULLS FIRST/NULLS
-		 * LAST ordering, since those are not significant for inference
-		 * purposes (any unique index matching the inference specification in
-		 * other regards is accepted indifferently).  Actively reject this as
-		 * wrong-headed.
+		 * Make no attempt to match ASC or DESC ordering, NULLS FIRST/NULLS
+		 * LAST ordering or opclass options, since those are not significant
+		 * for inference purposes (any unique index matching the inference
+		 * specification in other regards is accepted indifferently). Actively
+		 * reject this as wrong-headed.
 		 */
 		if (ielem->ordering != SORTBY_DEFAULT)
 			ereport(ERROR,
@@ -3295,6 +3295,12 @@ resolve_unique_index_expr(ParseState *pstate, InferClause *infer,
 					 errmsg("NULLS FIRST/LAST is not allowed in ON CONFLICT clause"),
 					 parser_errposition(pstate,
 										exprLocation((Node *) infer))));
+		if (ielem->opclassopts)
+			ereport(ERROR,
+					errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+					errmsg("operator class options are not allowed in ON CONFLICT clause"),
+					parser_errposition(pstate,
+									   exprLocation((Node *) infer)));
 
 		if (!ielem->expr)
 		{
-- 
2.47.3

In reply to: Álvaro Herrera (#1)
Re: weird ON CONFLICT clauses

On Thu, Nov 27, 2025 at 11:00 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

Why do we accept reloptions there without complaint? Should we tighten
this up a little bit, or maybe it makes sense to accept this for some
reason? I suspect the reloptions were added to index_elems after the ON
CONFLICT clause was made to use that production, but I didn't check the
git history.

index_elems is needed by ON CONFLICT so that the user can specify an
operator class and/or a collation. This is probably hardly ever used,
but it does have its place.

So what about the attached patch? I ran all tests and everything seems
to work correctly. (Maybe I'd add some tests to verify that this
new error is covered, as the ones just above.) It would complain to the
above:

Seems reasonable to me.

--
Peter Geoghegan

#3Álvaro Herrera
alvherre@kurilemu.de
In reply to: Peter Geoghegan (#2)
1 attachment(s)
Re: weird ON CONFLICT clauses

On 2025-Nov-27, Peter Geoghegan wrote:

On Thu, Nov 27, 2025 at 11:00 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

index_elems is needed by ON CONFLICT so that the user can specify an
operator class and/or a collation. This is probably hardly ever used,
but it does have its place.

Right.

So what about the attached patch? I ran all tests and everything seems
to work correctly. (Maybe I'd add some tests to verify that this
new error is covered, as the ones just above.) It would complain to the
above:

Seems reasonable to me.

Pushed, thanks for looking.

While looking at the test output, I wondered if it would be useful to
make the error cursor point to the bogus element itself rather than to
the overall InferClause. At the moment it doesn't look terribly useful,
so I'm parking this patch here; but if somebody were to be motivated to,
say, patch ComputeIndexAttrs to have a ParseState, we could add error
location to the ereports there.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Before you were born your parents weren't as boring as they are now. They
got that way paying your bills, cleaning up your room and listening to you
tell them how idealistic you are." -- Charles J. Sykes' advice to teenagers

Attachments:

0001-add-Location-to-IndexElem.patchtext/x-diff; charset=utf-8Download
From 0a4a5ae8830dd411237e0221b58aaa554aa3b97b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Fri, 12 Dec 2025 14:22:00 +0100
Subject: [PATCH] add Location to IndexElem

---
 src/backend/nodes/nodeFuncs.c     | 3 +++
 src/backend/parser/gram.y         | 5 +++++
 src/backend/parser/parse_clause.c | 8 ++++----
 src/include/nodes/parsenodes.h    | 1 +
 4 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 024a2b2fd84..89b7d80c0aa 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -1726,6 +1726,9 @@ exprLocation(const Node *expr)
 		case T_ColumnDef:
 			loc = ((const ColumnDef *) expr)->location;
 			break;
+		case T_IndexElem:
+			loc = ((const IndexElem *) expr)->location;
+			break;
 		case T_Constraint:
 			loc = ((const Constraint *) expr)->location;
 			break;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7856ce9d78f..a9bad1bff04 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -8402,6 +8402,7 @@ index_elem_options:
 			$$->opclassopts = NIL;
 			$$->ordering = $3;
 			$$->nulls_ordering = $4;
+			$$->location = @1;
 		}
 	| opt_collate any_name reloptions opt_asc_desc opt_nulls_order
 		{
@@ -8414,6 +8415,7 @@ index_elem_options:
 			$$->opclassopts = $3;
 			$$->ordering = $4;
 			$$->nulls_ordering = $5;
+			$$->location = @1;
 		}
 	;
 
@@ -8426,16 +8428,19 @@ index_elem: ColId index_elem_options
 				{
 					$$ = $2;
 					$$->name = $1;
+					$$->location = @1;
 				}
 			| func_expr_windowless index_elem_options
 				{
 					$$ = $2;
 					$$->expr = $1;
+					$$->location = @1;
 				}
 			| '(' a_expr ')' index_elem_options
 				{
 					$$ = $4;
 					$$->expr = $2;
+					$$->location = @1;
 				}
 		;
 
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 57609e2d55c..34693c8a9e6 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -3289,20 +3289,20 @@ resolve_unique_index_expr(ParseState *pstate, InferClause *infer,
 					 errmsg("%s is not allowed in ON CONFLICT clause",
 							"ASC/DESC"),
 					 parser_errposition(pstate,
-										exprLocation((Node *) infer))));
+										exprLocation((Node *) ielem))));
 		if (ielem->nulls_ordering != SORTBY_NULLS_DEFAULT)
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
 					 errmsg("%s is not allowed in ON CONFLICT clause",
 							"NULLS FIRST/LAST"),
 					 parser_errposition(pstate,
-										exprLocation((Node *) infer))));
+										exprLocation((Node *) ielem))));
 		if (ielem->opclassopts)
 			ereport(ERROR,
 					errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
 					errmsg("operator class options are not allowed in ON CONFLICT clause"),
 					parser_errposition(pstate,
-									   exprLocation((Node *) infer)));
+									   exprLocation((Node *) ielem)));
 
 		if (!ielem->expr)
 		{
@@ -3342,7 +3342,7 @@ resolve_unique_index_expr(ParseState *pstate, InferClause *infer,
 			pInfer->infercollid = InvalidOid;
 		else
 			pInfer->infercollid = LookupCollation(pstate, ielem->collation,
-												  exprLocation(pInfer->expr));
+												  exprLocation((Node *) ielem));
 
 		if (!ielem->opclass)
 			pInfer->inferopclass = InvalidOid;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index d14294a4ece..48738aa26f0 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -816,6 +816,7 @@ typedef struct IndexElem
 	List	   *opclassopts;	/* opclass-specific options, or NIL */
 	SortByDir	ordering;		/* ASC/DESC/default */
 	SortByNulls nulls_ordering; /* FIRST/LAST/default */
+	ParseLoc	location;		/* token location, or -1 if unknown */
 } IndexElem;
 
 /*
-- 
2.47.3