Re: BUG #16958: "Invalid reference to FROM-clause entry for table" when qualifying columns in "on conflict .. where" - Mailing list pgsql-bugs

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

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);
     }

-    /* 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.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;