Обсуждение: BUG #16958: "Invalid reference to FROM-clause entry for table" when qualifying columns in "on conflict .. where"

The following bug has been logged on the website:

Bug reference:      16958
Logged by:          Lukas Eder
Email address:      lukas.eder@gmail.com
PostgreSQL version: 13.2
Operating system:   Docker on Windows
Description:

I'm using Docker on Windows:

-------------------------------------------------------------------
select version();
-------------------------------------------------------------------
PostgreSQL 13.2 (Debian 13.2-1.pgdg100+1) on x86_64-pc-linux-gnu, compiled
by gcc (Debian 8.3.0-6) 8.3.0, 64-bit

Consider the following SQL script:
-------------------------------------------------------------------
create table t (
  a int,
  b int,
  c int
);

create unique index i on t(a)
where t.b is null;

insert into t (a, b)
values (
  1, 
  1
)
on conflict (a)
where t.b is null -- Error here
do update
set
  c = 1
returning t.a, t.b, t.c;

drop table t;
-------------------------------------------------------------------

There's an error reported:

SQL Error [42P01]: ERROR: invalid reference to FROM-clause entry for table
"t"
  Hint: There is an entry for table "t", but it cannot be referenced from
this part of the query.
  Position: 71

Notice the qualification of the t.b column in the "on conflict .. where"
clause. I don't understand why b cannot be qualified at this location. It
can be qualified in the index definition, looks like a bug to me.

On 4/10/21 9:57 AM, Pantelis Theodosiou wrote:
> On Fri, Apr 9, 2021 at 11:00 PM PG Bug reporting form <
> noreply@postgresql.org> wrote:
> 
>> insert into t (a, b)
>> values (
>>   1,
>>   1
>> )
>> on conflict (a)
>> where t.b is null -- Error here
>> do update
>> set
>>   c = 1
>> returning t.a, t.b, t.c;

>> There's an error reported:
>>
>> SQL Error [42P01]: ERROR: invalid reference to FROM-clause entry for table
>> "t"
>>   Hint: There is an entry for table "t", but it cannot be referenced from
>> this part of the query.
>>   Position: 71
>>
>> Notice the qualification of the t.b column in the "on conflict .. where"
>> clause. I don't understand why b cannot be qualified at this location. It
>> can be qualified in the index definition, looks like a bug to me.
>>
> 
>  You don't need and shouldn't prefix the column with the table name in that
> part. This should work:
> 
> ...
> on conflict (a)
> where b is null
> do update
> ...

Not needing to and not being able to are two completely different
things.  I believe tablename qualification should be allowed here even
if it isn't necessary.
-- 
Vik Fearing

Vik Fearing <vik@postgresfriends.org> writes:
> On 4/10/21 9:57 AM, Pantelis Theodosiou wrote:
>> On Fri, Apr 9, 2021 at 11:00 PM PG Bug reporting form <
>> noreply@postgresql.org> wrote:
>>> SQL Error [42P01]: ERROR: invalid reference to FROM-clause entry for table "t"
>>> Hint: There is an entry for table "t", but it cannot be referenced from
>>> this part of the query.
>>>
>>> Notice the qualification of the t.b column in the "on conflict .. where"
>>> clause. I don't understand why b cannot be qualified at this location. It
>>> can be qualified in the index definition, looks like a bug to me.

>> You don't need and shouldn't prefix the column with the table name in that
>> part. This should work:

> Not needing to and not being able to are two completely different
> things.  I believe tablename qualification should be allowed here even
> if it isn't necessary.

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?

Anyway, if the prohibition isn't necessary, the attached one-liner
gets most of the way to removing it.  I'm not quite satisfied with
this though:

regression=# create table t(a int primary key, b int, c int);
CREATE TABLE
regression=# insert into t (a,b) values(1,1) on conflict(a)
where excluded.b is null do update set c=excluded.c+1;
ERROR:  missing FROM-clause entry for table "excluded"
LINE 2: where excluded.b is null do update set c=excluded.c+1;
              ^

Seems like we want the same cannot-be-referenced HINT here as in
the complaint, so just nuking the whole namespace as the code
currently does ought to be revisited.

            regards, tom lane

diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index af80aa4593..4402be6845 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -3225,13 +3225,13 @@ transformOnConflictArbiter(ParseState *pstate,
         List       *save_namespace;

         /*
-         * While we process the arbiter expressions, accept only non-qualified
-         * references to the target table. Hide any other relations.
+         * While we process the arbiter expressions, accept only 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);
+                         false, true, true);

         if (infer->indexElems)
             *arbiterExpr = resolve_unique_index_expr(pstate, infer,

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.

There might have been a concern about users being confused about the
difference between what the INSERT docs call 'index_predicate' and
what they call 'condition' in the synopsis.

-- 
Peter Geoghegan

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;