On Fri, Oct 13, 2017 at 12:46 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.mu...@enterprisedb.com> writes:
>> On Fri, Oct 13, 2017 at 10:01 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>> The CTE was simply not part of the available namespace for the INSERT's
>>> target, so it found the regular table instead. v10 has thus broken
>>> cases that used to work. I think that's a bug.
>
>> Hmm. Yeah. I have to say it's a bit surprising that the following
>> refers to two different objects:
>> with mytable as (select 1 as x) insert into mytable select * from mytable
>
> Yeah, I agree --- personally I'd never write a query like that. But
> the fact that somebody ran into it when v10 has been out for barely
> a week suggests that people are doing it.
Not exactly -- Julien's bug report was about a *qualified* reference
being incorrectly rejected.
>>> I think we need to either remove that call from setTargetTable entirely,
>>> or maybe adjust it so it can only find ENRs not CTEs.
>
>> I think it'd be better to find and reject ENRs only. The alternative
>> would be to make ENRs invisible to DML, which seems arbitrary and
>> weird (even though it might be more consistent with our traditional
>> treatment of CTEs). One handwavy reason I'd like them to remain
>> visible to DML (and be rejected) is that I think they're a bit like
>> table variables (see SQL Server), and someone might eventually want to
>> teach them, or something like them, how to be writable.
>
> Yeah, that would be the argument for making them visible. I'm not
> sure how likely it is that we'll ever actually have writable ENRs,
> but I won't stand in the way if you want to do it like that.
I hope so :-) I might be a nice way to get cheap locally scoped
temporary tables, among other things.
Okay, here's Julien's patch tweaked to reject just the ENR case. This
takes us back to the 9.6 behaviour where CTEs don't hide tables in
this context. I also removed the schema qualification in his
regression test so we don't break that again. This way, his query
from the first message in the thread works with the schema
qualification (the bug he reported) and without it (the bug or at
least incompatible change from 9.6 you discovered).
I considered testing for a NULL return from parserOpenTable() instead
of the way the patch has it, since parserOpenTable() already had an
explicit test for ENRs, but its coding would give preference to an
unqualified table of the same name. I considered moving the test for
an ENR match higher up in parserOpenTable(), and that might have been
OK, but then I realised no code in the tree actually tests for its
undocumented NULL return value anyway. I think that NULL-returning
branch is dead code, and all tests pass without it. Shouldn't we just
remove it, as in the attached?
I renamed the ENR used in plpgsql.sql's
transition_table_level2_bad_usage_func() and with.sql's "sane
response" test, because they both confused matters by using an ENR
with the name "d" which is also the name of an existing table. For
example, if you start with unpatched master, rename
transition_table_level2_bad_usage_func()'s ENR to "dx" and simply
remove the check for ENRs from setTargetTable() as you originally
suggested, you'll get a segfault because the NULL return from
parserOpenTable() wasn't checked. If you leave
transition_table_level2_bad_usage_func()'s ENR name as "d" it'll
quietly access the wrong table instead, which is misleading.
--
Thomas Munro
http://www.enterprisedb.com
diff --git a/src/backend/parser/parse_clause.c
b/src/backend/parser/parse_clause.c
index 9ff80b8b40..2d56a07774 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -184,9 +184,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.
+ */
+ 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",
@@ -1072,6 +1075,12 @@ transformRangeTableSample(ParseState *pstate,
RangeTableSample *rts)
}
+/*
+ * getRTEForSpecialRelationTypes
+ *
+ * If given RangeVar if a CTE reference or an EphemeralNamedRelation, return
+ * the transformed RangeVar otherwise return NULL
+ */
static RangeTblEntry *
getRTEForSpecialRelationTypes(ParseState *pstate, RangeVar *rv)
{
@@ -1079,6 +1088,13 @@ getRTEForSpecialRelationTypes(ParseState *pstate,
RangeVar *rv)
Index levelsup;
RangeTblEntry *rte = NULL;
+ /*
+ * 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);
@@ -1119,15 +1135,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 4c5c684b44..a9273affb2 100644
--- 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 7d3e9225bb..bb3532676b 100644
--- 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 c32a490580..b4e0a1e83d 100644
--- 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 6c9399696b..6620ea6172 100644
--- 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 8ae5184d0f..baf65488a8 100644
--- 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;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
