Re: [HACKERS] oversight in EphemeralNamedRelation support
Thomas Munro writes: > On Fri, Oct 13, 2017 at 10:01 AM, Tom Lane wrote: >> But I see very >> little case for allowing CTEs to capture such references, because surely >> we are never going to allow that to do anything useful, and we have >> several years of precedent now that they don't capture. > For what it's worth, SQL Server allows DML in CTEs like us but went > the other way on this. Not only are its CTEs in scope as DML targets, > it actually lets you update them in cases where a view would be > updatable, rewriting as base table updates. I'm not suggesting that > we should do that too (unless of course it shows up in a future > standard), just pointing it out as a curiosity. Interesting. Still, given that we have quite a few years of precedent that CTEs aren't in scope as DML targets, I'm disinclined to change our semantics unless the point does show up in the standard. I've not heard anyone speaking against the choices you made in your prior message, so I'll go review your v3 patch, and push unless I find problems. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
On Fri, Oct 13, 2017 at 10:01 AM, Tom Lane wrote: > But I see very > little case for allowing CTEs to capture such references, because surely > we are never going to allow that to do anything useful, and we have > several years of precedent now that they don't capture. For what it's worth, SQL Server allows DML in CTEs like us but went the other way on this. Not only are its CTEs in scope as DML targets, it actually lets you update them in cases where a view would be updatable, rewriting as base table updates. I'm not suggesting that we should do that too (unless of course it shows up in a future standard), just pointing it out as a curiosity. -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
On Fri, Oct 13, 2017 at 5:22 AM, Tom Lane wrote: > Thomas Munro writes: >> On Fri, Oct 13, 2017 at 12:46 PM, Tom Lane wrote: >>> 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. > > Nonetheless, he was using a CTE name equivalent to the name of the > query's target table. That's already confusing IMV ... and it does > not seem unreasonable to guess that he only qualified the target > because it stopped working unqualified. FWIW, the original (and much more complex) query Hugo sent me was inserting data in a qualified table name (the schema wasn't public, and I assume not in his search_path). -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
Thomas Munro writes: > On Fri, Oct 13, 2017 at 12:46 PM, Tom Lane wrote: >> 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. Nonetheless, he was using a CTE name equivalent to the name of the query's target table. That's already confusing IMV ... and it does not seem unreasonable to guess that he only qualified the target because it stopped working unqualified. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
On Fri, Oct 13, 2017 at 12:46 PM, Tom Lane wrote: > Thomas Munro writes: >> On Fri, Oct 13, 2017 at 10:01 AM, Tom Lane 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; + /* +
Re: [HACKERS] oversight in EphemeralNamedRelation support
Thomas Munro writes: > On Fri, Oct 13, 2017 at 10:01 AM, Tom Lane 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. >> 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. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
On Fri, Oct 13, 2017 at 10:01 AM, Tom Lane wrote: > Thomas Munro writes: >> Before that, CTE used as modify targets produced a different error message: > >> postgres=# WITH d AS (SELECT 42) INSERT INTO d VALUES (1); >> ERROR: relation "d" does not exist >> LINE 1: WITH d AS (SELECT 42) INSERT INTO d VALUES (1); >> ^ > > Well, I think that is a poorly chosen example. Consider this instead: > pre-v10, you could do this: > > regression=# create table mytable (f1 int); > CREATE TABLE > regression=# with mytable as (select 1 as x) insert into mytable values(1); > INSERT 0 1 > regression=# select * from mytable; > f1 > > 1 > (1 row) > > 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 Obviously the spec is useless here since this is non-standard (at a guess they'd probably require a qualifier there to avoid parsing as a if they allowed DML after ). As you said it's worked like that for several releases, so whatever I might think about someone who deliberately writes such queries, the precedent probably trumps naive notions about WITH creating a single consistent lexical scope. > There may or may not be a case for allowing ENRs to capture names that > would otherwise refer to ordinary tables; I'm not sure. But I see very > little case for allowing CTEs to capture such references, because surely > we are never going to allow that to do anything useful, and we have > several years of precedent now that they don't capture. > > 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. -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
Thomas Munro writes: > On Fri, Oct 13, 2017 at 8:50 AM, Tom Lane wrote: >> Hm. I actually think the bug here is that 18ce3a4ab introduced >> anything into setTargetTable at all. There was never previously >> any assumption that the target could be anything but a regular >> table, so we just ignored CTEs there, and I do not think the >> new behavior is an improvement. > Before that, CTE used as modify targets produced a different error message: > postgres=# WITH d AS (SELECT 42) INSERT INTO d VALUES (1); > ERROR: relation "d" does not exist > LINE 1: WITH d AS (SELECT 42) INSERT INTO d VALUES (1); > ^ Well, I think that is a poorly chosen example. Consider this instead: pre-v10, you could do this: regression=# create table mytable (f1 int); CREATE TABLE regression=# with mytable as (select 1 as x) insert into mytable values(1); INSERT 0 1 regression=# select * from mytable; f1 1 (1 row) 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. There may or may not be a case for allowing ENRs to capture names that would otherwise refer to ordinary tables; I'm not sure. But I see very little case for allowing CTEs to capture such references, because surely we are never going to allow that to do anything useful, and we have several years of precedent now that they don't capture. I think we need to either remove that call from setTargetTable entirely, or maybe adjust it so it can only find ENRs not CTEs. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
On Fri, Oct 13, 2017 at 8:50 AM, Tom Lane wrote: > Julien Rouhaud writes: >> On Mon, Oct 9, 2017 at 10:43 PM, Thomas Munro >> wrote: >>> I suppose we could consider moving the schemaname check into >>> getRTEForSpecialRelationType(), since otherwise both callers need to >>> do that (and as you discovered, one forgot). > >> Thanks for the feedback. That was my first idea, but I assumed there >> could be future use for this function on qualified RangeVar if it >> wasn't done this way. > >> I agree it'd be much safer, so v2 attached, check moved in >> getRTEForSpecialRelationType(). > > Hm. I actually think the bug here is that 18ce3a4ab introduced > anything into setTargetTable at all. There was never previously > any assumption that the target could be anything but a regular > table, so we just ignored CTEs there, and I do not think the > new behavior is an improvement. > > So my proposal is to rip out the getRTEForSpecialRelationTypes > check there. I tend to agree that getRTEForSpecialRelationTypes > should probably contain an explicit check for unqualified name > rather than relying on its caller ... but that's a matter of > future-proofing not a bug fix. That check arrived in v11 revision of the patch: https://www.postgresql.org/message-id/CACjxUsPfUUa813oDvJRx2wuiqHXO3VsCLQzcuy0r%3DUEyS-xOjQ%40mail.gmail.com Before that, CTE used as modify targets produced a different error message: postgres=# WITH d AS (SELECT 42) INSERT INTO d VALUES (1); ERROR: relation "d" does not exist LINE 1: WITH d AS (SELECT 42) INSERT INTO d VALUES (1); ^ ... but ENRs used like that caused a crash. The change to setTargetTable() went in to prevent that (and improved the CTE case's error message semi-incidentally). To take out we'll need a new check somewhere else to prevent that. Where? -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
Julien Rouhaud writes: > On Mon, Oct 9, 2017 at 10:43 PM, Thomas Munro > wrote: >> I suppose we could consider moving the schemaname check into >> getRTEForSpecialRelationType(), since otherwise both callers need to >> do that (and as you discovered, one forgot). > Thanks for the feedback. That was my first idea, but I assumed there > could be future use for this function on qualified RangeVar if it > wasn't done this way. > I agree it'd be much safer, so v2 attached, check moved in > getRTEForSpecialRelationType(). Hm. I actually think the bug here is that 18ce3a4ab introduced anything into setTargetTable at all. There was never previously any assumption that the target could be anything but a regular table, so we just ignored CTEs there, and I do not think the new behavior is an improvement. So my proposal is to rip out the getRTEForSpecialRelationTypes check there. I tend to agree that getRTEForSpecialRelationTypes should probably contain an explicit check for unqualified name rather than relying on its caller ... but that's a matter of future-proofing not a bug fix. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
On Mon, Oct 9, 2017 at 10:43 PM, Thomas Munro wrote: > > I suppose we could consider moving the schemaname check into > getRTEForSpecialRelationType(), since otherwise both callers need to > do that (and as you discovered, one forgot). Thanks for the feedback. That was my first idea, but I assumed there could be future use for this function on qualified RangeVar if it wasn't done this way. I agree it'd be much safer, so v2 attached, check moved in getRTEForSpecialRelationType(). diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index 9ff80b8b40..255f485494 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -184,8 +184,10 @@ setTargetTable(ParseState *pstate, RangeVar *relation, RangeTblEntry *rte; int rtindex; - /* So far special relations are immutable; so they cannot be targets. */ + /* Check if it's a CTE or tuplestore reference */ rte = getRTEForSpecialRelationTypes(pstate, relation); + + /* So far special relations are immutable; so they cannot be targets. */ if (rte != NULL) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), @@ -1072,6 +1074,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 +1087,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 +1134,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/test/regress/expected/with.out b/src/test/regress/expected/with.out index c32a490580..53ea9991b2 100644 --- a/src/test/regress/expected/with.out +++ b/src/test/regress/expected/with.out @@ -2275,3 +2275,7 @@ 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); ERROR: relation "d" cannot be the target of a modifying statement +-- check qualified relation name doesn't conflict with CTE name +CREATE TABLE public.self (id integer); +WITH self AS (SELECT 42) INSERT INTO public.self SELECT * from self; +DROP TABLE public.self; diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql index 8ae5184d0f..17f32c3c87 100644 --- a/src/test/regress/sql/with.sql +++ b/src/test/regress/sql/with.sql @@ -1031,3 +1031,8 @@ 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); + +-- check qualified relation name doesn't conflict with CTE name +CREATE TABLE public.self (id integer); +WITH self AS (SELECT 42) INSERT INTO public.self SELECT * from self; +DROP TABLE public.self; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
On Tue, Oct 10, 2017 at 2:35 AM, Julien Rouhaud wrote: > Hugo Mercier (in Cc) reported me an error in a query, failing since pg10. > > Simple test case to reproduce: > > CREATE TABLE public.test (id integer); > WITH test AS (select 42) INSERT INTO public.test SELECT * FROM test; > > which will fail with "relation "test" cannot be the target of a > modifying statement". > > IIUC, that's an oversight in 18ce3a4ab22, where setTargetTable() > doesn't exclude qualified relation when searching for special > relation. I agree. > PFA a simple patch to fix this issue, with updated regression test. Thanks! I suppose we could consider moving the schemaname check into getRTEForSpecialRelationType(), since otherwise both callers need to do that (and as you discovered, one forgot). -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers