Fix incorrect handling of CTEs and ENRs as DML target relations.

setTargetTable threw an error if the proposed target RangeVar's relname
matched any visible CTE or ENR.  This breaks backwards compatibility in
the CTE case, since pre-v10 we never looked for a CTE here at all, so that
CTE names did not mask regular tables.  It does seem like a good idea to
throw an error for the ENR case, though, thus causing ENRs to mask tables
for this purpose; ENRs are new in v10 so we're not breaking existing code,
and we may someday want to allow them to be the targets of DML.

To fix that, replace use of getRTEForSpecialRelationTypes, which was
overkill anyway, with use of scanNameSpaceForENR.

A second problem was that the check neglected to verify null schemaname,
so that a CTE or ENR could incorrectly be thought to match a qualified
RangeVar.  That happened because getRTEForSpecialRelationTypes relied
on its caller to have checked for null schemaname.  Even though the one
remaining caller got it right, this is obviously bug-prone, so move
the check inside getRTEForSpecialRelationTypes.

Also, revert commit 18ce3a4ab's extremely poorly thought out decision to
add a NULL return case to parserOpenTable --- without either documenting
that or adjusting any of the callers to check for it.  The current bug
seems to have arisen in part due to working around that bad idea.

In passing, remove the one-line shim functions transformCTEReference and
transformENRReference --- they don't seem to be adding any clarity or
functionality.

Per report from Hugo Mercier (via Julien Rouhaud).  Back-patch to v10
where the bug was introduced.

Thomas Munro, with minor editing by me

Discussion: https://postgr.es/m/CAOBaU_YdPVH+PTtiKSSLOiiW3mVDYsnNUekK+XPbHXiP=wrFLA@mail.gmail.com

diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 9ff80b8b403552e1de5182fe199de0ee66703a4b..af99e65aa7df2eaa43bd29e5db6336dab6c1d181 100644 (file)
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -62,9 +62,6 @@ static Node *transformJoinOnClause(ParseState *pstate, JoinExpr *j,
 static RangeTblEntry *getRTEForSpecialRelationTypes(ParseState *pstate,
                                                          RangeVar *rv);
 static RangeTblEntry *transformTableEntry(ParseState *pstate, RangeVar *r);
-static RangeTblEntry *transformCTEReference(ParseState *pstate, RangeVar *r,
-                                         CommonTableExpr *cte, Index levelsup);
-static RangeTblEntry *transformENRReference(ParseState *pstate, RangeVar *r);
 static RangeTblEntry *transformRangeSubselect(ParseState *pstate,
                                                RangeSubselect *r);
 static RangeTblEntry *transformRangeFunction(ParseState *pstate,
@@ -184,9 +181,12 @@ setTargetTable(ParseState *pstate, RangeVar *relation,
        RangeTblEntry *rte;
        int                     rtindex;
 
-       /* So far special relations are immutable; so they cannot be targets. */
-       rte = getRTEForSpecialRelationTypes(pstate, relation);
-       if (rte != NULL)
+       /*
+        * ENRs hide tables of the same name, so we need to check for them first.
+        * In contrast, CTEs don't hide tables (for this purpose).
+        */
+       if (relation->schemaname == NULL &&
+               scanNameSpaceForENR(pstate, relation->relname))
                ereport(ERROR,
                                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                                 errmsg("relation \"%s\" cannot be the target of a modifying statement",
@@ -430,35 +430,6 @@ transformTableEntry(ParseState *pstate, RangeVar *r)
        return rte;
 }
 
-/*
- * transformCTEReference --- transform a RangeVar that references a common
- * table expression (ie, a sub-SELECT defined in a WITH clause)
- */
-static RangeTblEntry *
-transformCTEReference(ParseState *pstate, RangeVar *r,
-                                         CommonTableExpr *cte, Index levelsup)
-{
-       RangeTblEntry *rte;
-
-       rte = addRangeTableEntryForCTE(pstate, cte, levelsup, r, true);
-
-       return rte;
-}
-
-/*
- * transformENRReference --- transform a RangeVar that references an ephemeral
- * named relation
- */
-static RangeTblEntry *
-transformENRReference(ParseState *pstate, RangeVar *r)
-{
-       RangeTblEntry *rte;
-
-       rte = addRangeTableEntryForENR(pstate, r, true);
-
-       return rte;
-}
-
 /*
  * transformRangeSubselect --- transform a sub-SELECT appearing in FROM
  */
@@ -1071,19 +1042,32 @@ transformRangeTableSample(ParseState *pstate, RangeTableSample *rts)
        return tablesample;
 }
 
-
+/*
+ * getRTEForSpecialRelationTypes
+ *
+ * If given RangeVar refers to a CTE or an EphemeralNamedRelation,
+ * build and return an appropriate RTE, otherwise return NULL
+ */
 static RangeTblEntry *
 getRTEForSpecialRelationTypes(ParseState *pstate, RangeVar *rv)
 {
        CommonTableExpr *cte;
        Index           levelsup;
-       RangeTblEntry *rte = NULL;
+       RangeTblEntry *rte;
+
+       /*
+        * if it is a qualified name, it can't be a CTE or tuplestore reference
+        */
+       if (rv->schemaname)
+               return NULL;
 
        cte = scanNameSpaceForCTE(pstate, rv->relname, &levelsup);
        if (cte)
-               rte = transformCTEReference(pstate, rv, cte, levelsup);
-       if (!rte && scanNameSpaceForENR(pstate, rv->relname))
-               rte = transformENRReference(pstate, rv);
+               rte = addRangeTableEntryForCTE(pstate, cte, levelsup, rv, true);
+       else if (scanNameSpaceForENR(pstate, rv->relname))
+               rte = addRangeTableEntryForENR(pstate, rv, true);
+       else
+               rte = NULL;
 
        return rte;
 }
@@ -1119,15 +1103,11 @@ transformFromClauseItem(ParseState *pstate, Node *n,
                /* Plain relation reference, or perhaps a CTE reference */
                RangeVar   *rv = (RangeVar *) n;
                RangeTblRef *rtr;
-               RangeTblEntry *rte = NULL;
+               RangeTblEntry *rte;
                int                     rtindex;
 
-               /*
-                * if it is an unqualified name, it might be a CTE or tuplestore
-                * reference
-                */
-               if (!rv->schemaname)
-                       rte = getRTEForSpecialRelationTypes(pstate, rv);
+               /* Check if it's a CTE or tuplestore reference */
+               rte = getRTEForSpecialRelationTypes(pstate, rv);
 
                /* if not found above, must be a table reference */
                if (!rte)

diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 4c5c684b44149d8b487fb0e87aa3f8c6669263a0..a9273affb233fd3bfdacd28856fdbb01a53f274f 100644 (file)
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1159,19 +1159,13 @@ parserOpenTable(ParseState *pstate, const RangeVar *relation, int lockmode)
                                                        relation->schemaname, relation->relname)));
                else
                {
-                       /*
-                        * An unqualified name might be a named ephemeral relation.
-                        */
-                       if (get_visible_ENR_metadata(pstate->p_queryEnv, relation->relname))
-                               rel = NULL;
-
                        /*
                         * An unqualified name might have been meant as a reference to
                         * some not-yet-in-scope CTE.  The bare "does not exist" message
                         * has proven remarkably unhelpful for figuring out such problems,
                         * so we take pains to offer a specific hint.
                         */
-                       else if (isFutureCTE(pstate, relation->relname))
+                       if (isFutureCTE(pstate, relation->relname))
                                ereport(ERROR,
                                                (errcode(ERRCODE_UNDEFINED_TABLE),
                                                 errmsg("relation \"%s\" does not exist",

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 7d3e9225bb24df8cb8360069790849ce28b79bd6..bb3532676bd25bbdf1bd025ccffa041d68a37d0c 100644 (file)
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -5879,19 +5879,19 @@ CREATE FUNCTION transition_table_level2_bad_usage_func()
   LANGUAGE plpgsql
 AS $$
   BEGIN
-    INSERT INTO d VALUES (1000000, 1000000, 'x');
+    INSERT INTO dx VALUES (1000000, 1000000, 'x');
     RETURN NULL;
   END;
 $$;
 CREATE TRIGGER transition_table_level2_bad_usage_trigger
   AFTER DELETE ON transition_table_level2
-  REFERENCING OLD TABLE AS d
+  REFERENCING OLD TABLE AS dx
   FOR EACH STATEMENT EXECUTE PROCEDURE
     transition_table_level2_bad_usage_func();
 DELETE FROM transition_table_level2
   WHERE level2_no BETWEEN 301 AND 305;
-ERROR:  relation "d" cannot be the target of a modifying statement
-CONTEXT:  SQL statement "INSERT INTO d VALUES (1000000, 1000000, 'x')"
+ERROR:  relation "dx" cannot be the target of a modifying statement
+CONTEXT:  SQL statement "INSERT INTO dx VALUES (1000000, 1000000, 'x')"
 PL/pgSQL function transition_table_level2_bad_usage_func() line 3 at SQL statement
 DROP TRIGGER transition_table_level2_bad_usage_trigger
   ON transition_table_level2;

diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
index c32a4905806862932df9e87f5add67a011c97e91..b4e0a1e83d88b2e49daf5aed5892ad13037472f3 100644 (file)
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -2273,5 +2273,19 @@ with ordinality as (select 1 as x) select * from ordinality;
 (1 row)
 
 -- check sane response to attempt to modify CTE relation
-WITH d AS (SELECT 42) INSERT INTO d VALUES (1);
-ERROR:  relation "d" cannot be the target of a modifying statement
+WITH test AS (SELECT 42) INSERT INTO test VALUES (1);
+ERROR:  relation "test" does not exist
+LINE 1: WITH test AS (SELECT 42) INSERT INTO test VALUES (1);
+                                             ^
+-- check response to attempt to modify table with same name as a CTE (perhaps
+-- surprisingly it works, because CTEs don't hide tables from data-modifying
+-- statements)
+create table test (i int);
+with test as (select 42) insert into test select * from test;
+select * from test;
+ i  
+----
+ 42
+(1 row)
+
+drop table test;

diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 6c9399696bf796456470ba68d36252907b9fcee4..6620ea61729920edd414ef79651731c82d3e4f70 100644 (file)
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4678,14 +4678,14 @@ CREATE FUNCTION transition_table_level2_bad_usage_func()
   LANGUAGE plpgsql
 AS $$
   BEGIN
-    INSERT INTO d VALUES (1000000, 1000000, 'x');
+    INSERT INTO dx VALUES (1000000, 1000000, 'x');
     RETURN NULL;
   END;
 $$;
 
 CREATE TRIGGER transition_table_level2_bad_usage_trigger
   AFTER DELETE ON transition_table_level2
-  REFERENCING OLD TABLE AS d
+  REFERENCING OLD TABLE AS dx
   FOR EACH STATEMENT EXECUTE PROCEDURE
     transition_table_level2_bad_usage_func();
 

diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql
index 8ae5184d0f17cc4c2e805aad0280ae90134fe41f..baf65488a8edd9efd078f9422cd2cd8987b6f5a1 100644 (file)
--- a/src/test/regress/sql/with.sql
+++ b/src/test/regress/sql/with.sql
@@ -1030,4 +1030,12 @@ create table foo (with ordinality);  -- fail, WITH is a reserved word
 with ordinality as (select 1 as x) select * from ordinality;
 
 -- check sane response to attempt to modify CTE relation
-WITH d AS (SELECT 42) INSERT INTO d VALUES (1);
+WITH test AS (SELECT 42) INSERT INTO test VALUES (1);
+
+-- check response to attempt to modify table with same name as a CTE (perhaps
+-- surprisingly it works, because CTEs don't hide tables from data-modifying
+-- statements)
+create table test (i int);
+with test as (select 42) insert into test select * from test;
+select * from test;
+drop table test;