From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Geoghegan <pg@bowt.ie>
Cc: Vik Fearing <vik@postgresfriends.org>
Cc: Pantelis Theodosiou <ypercube@gmail.com>
Cc: Lukas Eder <lukas.eder@gmail.com>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #16958: "Invalid reference to FROM-clause entry for table" when qualifying columns in "on conflict .. where"
Date: Sat, 10 Apr 2021 20:20:43 -0400
Message-ID: <2556685.1618100443@sss.pgh.pa.us> (raw)
In-Reply-To: <CAH2-Wz=uNJvUkbMhhwKZEOf=9vU=G7wSe6b0R_6SsrjkmMyQnw@mail.gmail.com>
References: <16958-963f638020de271c@postgresql.org>
<CAE3TBxxXQ2+7EaBvb+zEFL6iTw90xq0+faHEqK2gDS0tTgiNmQ@mail.gmail.com>
<8a396e1e-c2a0-5ba3-180e-697082ae397f@postgresfriends.org>
<2364236.1618072118@sss.pgh.pa.us>
<CAH2-Wz=uNJvUkbMhhwKZEOf=9vU=G7wSe6b0R_6SsrjkmMyQnw@mail.gmail.com>
Peter Geoghegan <pg@bowt.ie> writes:
> On Sat, Apr 10, 2021 at 9:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It does seem like a pointless prohibition, but the comment about it in
>> the source code implies it was intentional. Peter, do you remember
>> why?
> No. The intention was to make it like CREATE INDEX. Apparently CREATE
> INDEX allows the columns to be qualified, though, so that explanation
> doesn't justify it.
OK. Here's a more fleshed-out patch that makes an effort to get rid of
duplicate namespace-munging.
I found that creating the EXCLUDED RTE soon enough to draw the complaint
I wanted about bogus references also caused this interesting change in
the regression test results:
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:
This seems to me like an independent bug: why is the misspelled-column-
name hint machinery including inaccessible columns in its results?
That seems much more likely to be confusing than helpful. But if there
is actually a defensible reason for doing that, then this seems like
an okay change.
regards, tom lane
Attachments:allow-table-qual-in-ON-CONFLICT-1.patch (text/x-diff, 9.0K) downloaddiff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.cindex 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);
}
- /* 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+ * 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->returningList)+ 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);++ /* Process RETURNING, if any. */+ if (stmt->returningList)
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.cindex 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.outindex 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.sqlindex 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;
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, pg@bowt.ie, vik@postgresfriends.org, ypercube@gmail.com, lukas.eder@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16958: "Invalid reference to FROM-clause entry for table" when qualifying columns in "on conflict .. where"
In-Reply-To: <2556685.1618100443@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox