From 6c0373ab77359c94b279c4e67c91aa623841af65 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 13 Apr 2021 15:39:33 -0400 Subject: [PATCH] Allow table-qualified variable names in ON CONFLICT ... WHERE. Previously you could only use unqualified variable names here. While that's not a functional deficiency, since only the target table can be referenced, it's a surprising inconsistency with the rules for partial-index predicates, on which this syntax is supposedly modeled. The fix for that is no harder than passing addToRelNameSpace = true to addNSItemToQuery. However, it's really pretty bogus for transformOnConflictArbiter and transformOnConflictClause to be messing with the namespace item for the target table at all. It's not theirs to manage, it results in duplicative creations of namespace items, and transformOnConflictClause wasn't even doing it quite correctly (that coding resulted in two nsitems for the target table, since it hadn't cleaned out the existing one). Hence, make transformInsertStmt responsible for setting up the target nsitem once for both these clauses and RETURNING. Also, arrange for ON CONFLICT ... UPDATE's "excluded" pseudo-relation to be added to the rangetable before we run transformOnConflictArbiter. This produces a more helpful HINT if someone writes "excluded.col" in the arbiter expression. Per bug #16958 from Lukas Eder. Although I agree this is a bug, the consequences are hardly severe, so no back-patch. Discussion: https://postgr.es/m/16958-963f638020de271c@postgresql.org --- src/backend/parser/analyze.c | 83 +++++++++++-------- src/backend/parser/parse_clause.c | 13 --- src/test/regress/expected/insert_conflict.out | 4 +- src/test/regress/sql/insert_conflict.sql | 2 +- 4 files changed, 53 insertions(+), 49 deletions(-) diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index 9f13880d19..862f18a92f 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -869,25 +869,27 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt) attr_num - FirstLowInvalidHeapAttributeNumber); } + /* + * If we have any clauses yet to process, set the query namespace to + * contain only the target relation, removing any entries added in a + * sub-SELECT or VALUES list. + */ + if (stmt->onConflictClause || stmt->returningList) + { + pstate->p_namespace = NIL; + addNSItemToQuery(pstate, pstate->p_target_nsitem, + false, true, true); + } + /* Process ON CONFLICT, if any. */ if (stmt->onConflictClause) qry->onConflict = transformOnConflictClause(pstate, stmt->onConflictClause); - /* - * If we have a RETURNING clause, we need to add the target relation to - * the query namespace before processing it, so that Var references in - * RETURNING will work. Also, remove any namespace entries added in a - * sub-SELECT or VALUES list. - */ + /* Process RETURNING, if any. */ if (stmt->returningList) - { - pstate->p_namespace = NIL; - addNSItemToQuery(pstate, pstate->p_target_nsitem, - false, true, true); qry->returningList = transformReturningList(pstate, stmt->returningList); - } /* done building the range table and jointree */ qry->rtable = pstate->p_rtable; @@ -1014,6 +1016,7 @@ static OnConflictExpr * transformOnConflictClause(ParseState *pstate, OnConflictClause *onConflictClause) { + ParseNamespaceItem *exclNSItem = NULL; List *arbiterElems; Node *arbiterWhere; Oid arbiterConstraint; @@ -1023,29 +1026,17 @@ transformOnConflictClause(ParseState *pstate, List *exclRelTlist = NIL; OnConflictExpr *result; - /* Process the arbiter clause, ON CONFLICT ON (...) */ - transformOnConflictArbiter(pstate, onConflictClause, &arbiterElems, - &arbiterWhere, &arbiterConstraint); - - /* Process DO UPDATE */ + /* + * 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 (onConflictClause->action == ONCONFLICT_UPDATE) { Relation targetrel = pstate->p_target_relation; - ParseNamespaceItem *exclNSItem; RangeTblEntry *exclRte; - /* - * All INSERT expressions have been parsed, get ready for potentially - * existing SET statements that need to be processed like an UPDATE. - */ - pstate->p_is_insert = false; - - /* - * Add range table entry for the EXCLUDED pseudo relation. relkind is - * set to composite to signal that we're not dealing with an actual - * relation, and no permission checks are required on it. (We'll - * check the actual target relation, instead.) - */ exclNSItem = addRangeTableEntryForRelation(pstate, targetrel, RowExclusiveLock, @@ -1054,6 +1045,11 @@ transformOnConflictClause(ParseState *pstate, exclRte = exclNSItem->p_rte; exclRelIndex = exclNSItem->p_rtindex; + /* + * relkind is set to composite to signal that we're not dealing with + * an actual relation, and no permission checks are required on it. + * (We'll check the actual target relation, instead.) + */ exclRte->relkind = RELKIND_COMPOSITE_TYPE; exclRte->requiredPerms = 0; /* other permissions fields in exclRte are already empty */ @@ -1061,14 +1057,27 @@ transformOnConflictClause(ParseState *pstate, /* Create EXCLUDED rel's targetlist for use by EXPLAIN */ exclRelTlist = BuildOnConflictExcludedTargetlist(targetrel, exclRelIndex); + } + + /* Process the arbiter clause, ON CONFLICT ON (...) */ + transformOnConflictArbiter(pstate, onConflictClause, &arbiterElems, + &arbiterWhere, &arbiterConstraint); + + /* Process DO UPDATE */ + 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; /* - * Add EXCLUDED and the target RTE to the namespace, so that they can - * be used in the UPDATE subexpressions. + * Add the EXCLUDED pseudo relation to the query namespace, making it + * available in the UPDATE subexpressions. */ addNSItemToQuery(pstate, exclNSItem, false, true, true); - addNSItemToQuery(pstate, pstate->p_target_nsitem, - false, true, true); /* * Now transform the UPDATE subexpressions. @@ -1079,6 +1088,14 @@ transformOnConflictClause(ParseState *pstate, onConflictWhere = transformWhereClause(pstate, onConflictClause->whereClause, EXPR_KIND_WHERE, "WHERE"); + + /* + * Remove the EXCLUDED pseudo relation from the query namespace, since + * it's not supposed to be available in RETURNING. (Maybe someday we + * could allow that, and drop this step.) + */ + Assert((ParseNamespaceItem *) llast(pstate->p_namespace) == exclNSItem); + pstate->p_namespace = list_delete_last(pstate->p_namespace); } /* Finally, build ON CONFLICT DO [NOTHING | UPDATE] expression */ diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index af80aa4593..89d95d3e94 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -3222,17 +3222,6 @@ transformOnConflictArbiter(ParseState *pstate, /* ON CONFLICT DO NOTHING does not require an inference clause */ if (infer) { - List *save_namespace; - - /* - * While we process the arbiter expressions, accept only non-qualified - * references to the target table. Hide any other relations. - */ - save_namespace = pstate->p_namespace; - pstate->p_namespace = NIL; - addNSItemToQuery(pstate, pstate->p_target_nsitem, - false, false, true); - if (infer->indexElems) *arbiterExpr = resolve_unique_index_expr(pstate, infer, pstate->p_target_relation); @@ -3245,8 +3234,6 @@ transformOnConflictArbiter(ParseState *pstate, *arbiterWhere = transformExpr(pstate, infer->whereClause, EXPR_KIND_INDEX_PREDICATE); - pstate->p_namespace = save_namespace; - /* * If the arbiter is specified by constraint name, get the constraint * OID and mark the constrained columns as requiring SELECT privilege, diff --git a/src/test/regress/expected/insert_conflict.out b/src/test/regress/expected/insert_conflict.out index a54a51e5c7..66d8633e3e 100644 --- a/src/test/regress/expected/insert_conflict.out +++ b/src/test/regress/expected/insert_conflict.out @@ -248,7 +248,7 @@ insert into insertconflicttest values (1, 'Apple') on conflict (keyy) do update ERROR: column "keyy" does not exist LINE 1: ...nsertconflicttest values (1, 'Apple') on conflict (keyy) do ... ^ -HINT: Perhaps you meant to reference the column "insertconflicttest.key". +HINT: Perhaps you meant to reference the column "insertconflicttest.key" or the column "excluded.key". -- Have useful HINT for EXCLUDED.* RTE within UPDATE: insert into insertconflicttest values (1, 'Apple') on conflict (key) do update set fruit = excluded.fruitt; ERROR: column excluded.fruitt does not exist @@ -373,7 +373,7 @@ drop index fruit_index; create unique index partial_key_index on insertconflicttest(key) where fruit like '%berry'; -- Succeeds insert into insertconflicttest values (23, 'Blackberry') on conflict (key) where fruit like '%berry' do update set fruit = excluded.fruit; -insert into insertconflicttest values (23, 'Blackberry') on conflict (key) where fruit like '%berry' and fruit = 'inconsequential' do nothing; +insert into insertconflicttest as t values (23, 'Blackberry') on conflict (key) where fruit like '%berry' and t.fruit = 'inconsequential' do nothing; -- fails insert into insertconflicttest values (23, 'Blackberry') on conflict (key) do update set fruit = excluded.fruit; ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification diff --git a/src/test/regress/sql/insert_conflict.sql b/src/test/regress/sql/insert_conflict.sql index 43691cd335..23d5778b82 100644 --- a/src/test/regress/sql/insert_conflict.sql +++ b/src/test/regress/sql/insert_conflict.sql @@ -214,7 +214,7 @@ create unique index partial_key_index on insertconflicttest(key) where fruit lik -- Succeeds insert into insertconflicttest values (23, 'Blackberry') on conflict (key) where fruit like '%berry' do update set fruit = excluded.fruit; -insert into insertconflicttest values (23, 'Blackberry') on conflict (key) where fruit like '%berry' and fruit = 'inconsequential' do nothing; +insert into insertconflicttest as t values (23, 'Blackberry') on conflict (key) where fruit like '%berry' and t.fruit = 'inconsequential' do nothing; -- fails insert into insertconflicttest values (23, 'Blackberry') on conflict (key) do update set fruit = excluded.fruit;