pgsql: Add infrastructure to support EphemeralNamedRelation references.

Add infrastructure to support EphemeralNamedRelation references.

A QueryEnvironment concept is added, which allows new types of
objects to be passed into queries from parsing on through
execution. At this point, the only thing implemented is a
collection of EphemeralNamedRelation objects -- relations which
can be referenced by name in queries, but do not exist in the
catalogs. The only type of ENR implemented is NamedTuplestore, but
provision is made to add more types fairly easily.

An ENR can carry its own TupleDesc or reference a relation in the
catalogs by relid.

Although these features can be used without SPI, convenience
functions are added to SPI so that ENRs can easily be used by code
run through SPI.

The initial use of all this is going to be transition tables in
AFTER triggers, but that will be added to each PL as a separate
commit.

An incidental effect of this patch is to produce a more informative
error message if an attempt is made to modify the contents of a CTE
from a referencing DML statement. No tests previously covered that
possibility, so one is added.

Kevin Grittner and Thomas Munro
Reviewed by Heikki Linnakangas, David Fetter, and Thomas Munro
with valuable comments and suggestions from many others

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/18ce3a4ab22d2984f8540ab480979c851dae5338

Modified Files
--------------
contrib/pg_stat_statements/pg_stat_statements.c | 15 +-
doc/src/sgml/spi.sgml | 204 ++++++++++++++++++++++++
src/backend/catalog/pg_proc.c | 3 +-
src/backend/commands/copy.c | 5 +-
src/backend/commands/createas.c | 5 +-
src/backend/commands/explain.c | 40 +++--
src/backend/commands/extension.c | 6 +-
src/backend/commands/foreigncmds.c | 2 +-
src/backend/commands/matview.c | 2 +-
src/backend/commands/prepare.c | 17 +-
src/backend/commands/schemacmds.c | 1 +
src/backend/commands/trigger.c | 9 +-
src/backend/commands/view.c | 2 +-
src/backend/executor/Makefile | 3 +-
src/backend/executor/execAmi.c | 6 +
src/backend/executor/execMain.c | 5 +
src/backend/executor/execParallel.c | 2 +-
src/backend/executor/execProcnode.c | 14 ++
src/backend/executor/execUtils.c | 2 +
src/backend/executor/functions.c | 8 +-
src/backend/executor/nodeNamedtuplestorescan.c | 198 +++++++++++++++++++++++
src/backend/executor/spi.c | 116 ++++++++++++--
src/backend/nodes/copyfuncs.c | 25 +++
src/backend/nodes/nodeFuncs.c | 2 +
src/backend/nodes/outfuncs.c | 20 +++
src/backend/nodes/print.c | 4 +
src/backend/nodes/readfuncs.c | 7 +
src/backend/optimizer/path/allpaths.c | 45 ++++++
src/backend/optimizer/path/costsize.c | 70 ++++++++
src/backend/optimizer/plan/createplan.c | 71 +++++++++
src/backend/optimizer/plan/setrefs.c | 16 ++
src/backend/optimizer/plan/subselect.c | 4 +
src/backend/optimizer/prep/prepjointree.c | 2 +
src/backend/optimizer/util/clauses.c | 2 +-
src/backend/optimizer/util/pathnode.c | 26 +++
src/backend/optimizer/util/plancat.c | 7 +-
src/backend/optimizer/util/relnode.c | 1 +
src/backend/parser/Makefile | 5 +-
src/backend/parser/analyze.c | 14 +-
src/backend/parser/parse_clause.c | 59 +++++--
src/backend/parser/parse_enr.c | 29 ++++
src/backend/parser/parse_node.c | 2 +
src/backend/parser/parse_relation.c | 143 ++++++++++++++++-
src/backend/parser/parse_target.c | 2 +
src/backend/tcop/postgres.c | 22 ++-
src/backend/tcop/pquery.c | 10 +-
src/backend/tcop/utility.c | 34 ++--
src/backend/utils/adt/ruleutils.c | 1 +
src/backend/utils/cache/plancache.c | 34 ++--
src/backend/utils/misc/Makefile | 4 +-
src/backend/utils/misc/queryenvironment.c | 144 +++++++++++++++++
src/backend/utils/sort/tuplestore.c | 17 ++
src/include/catalog/catversion.h | 2 +-
src/include/commands/createas.h | 3 +-
src/include/commands/explain.h | 9 +-
src/include/commands/prepare.h | 4 +-
src/include/executor/execdesc.h | 2 +
src/include/executor/nodeNamedtuplestorescan.h | 24 +++
src/include/executor/spi.h | 7 +
src/include/executor/spi_priv.h | 2 +
src/include/nodes/execnodes.h | 21 +++
src/include/nodes/nodes.h | 2 +
src/include/nodes/parsenodes.h | 6 +-
src/include/nodes/plannodes.h | 10 ++
src/include/optimizer/cost.h | 3 +
src/include/optimizer/pathnode.h | 2 +
src/include/parser/analyze.h | 2 +-
src/include/parser/parse_enr.h | 22 +++
src/include/parser/parse_node.h | 3 +
src/include/parser/parse_relation.h | 4 +
src/include/tcop/tcopprot.h | 7 +-
src/include/tcop/utility.h | 5 +-
src/include/utils/plancache.h | 10 +-
src/include/utils/portal.h | 1 +
src/include/utils/queryenvironment.h | 74 +++++++++
src/include/utils/tuplestore.h | 2 +
src/test/regress/expected/with.out | 3 +
src/test/regress/sql/with.sql | 3 +
78 files changed, 1598 insertions(+), 122 deletions(-)

rhinoceros says you missed contrib/sepgsql.

More generally, if you hack the API of some globally-referenced function,
you ought to grep for references to it rather than just assume your
compiler will find them all for you. For one thing, that approach is a
great way to fail to update relevant comments.

(And while I'm bitching, you definitely failed to update ProcessUtility's
header comment, which like most significant functions takes some pains
to describe all the arguments.)

regards, tom lane

On Sat, Apr 1, 2017 at 12:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> rhinoceros says you missed contrib/sepgsql.

Yeah, I saw that and have pushed an attempt to fix.

> More generally, if you hack the API of some globally-referenced function,
> you ought to grep for references to it rather than just assume your
> compiler will find them all for you. For one thing, that approach is a
> great way to fail to update relevant comments.

That's what I normally do, and thought that I had here, but clearly
I messed up. The log shows that even in 2014, when this was
written, those calls were there.

> (And while I'm bitching, you definitely failed to update ProcessUtility's
> header comment, which like most significant functions takes some pains
> to describe all the arguments.)

Will fix.

--
Kevin Grittner

On Sat, Apr 1, 2017 at 12:21 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Sat, Apr 1, 2017 at 12:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> rhinoceros says you missed contrib/sepgsql.
>
> Yeah, I saw that and have pushed an attempt to fix.

That blind fix seemed to work.

>> (And while I'm bitching, you definitely failed to update ProcessUtility's
>> header comment, which like most significant functions takes some pains
>> to describe all the arguments.)
>
> Will fix.

I pushed a fix for that and then went looking to see what else I
missed. I found CreateCachedPlan, but then saw that the parameter
was never referenced within the function. That seems odd. I want
to go over that carefully, but I'm too tired now. Will investigate
tomorrow.

--
Kevin Grittner

Hi,

On 2017-04-01 04:24:25 +0000, Kevin Grittner wrote:
> Add infrastructure to support EphemeralNamedRelation references.

My compiler, quite justifiedly, complains:

/home/andres/src/postgresql/src/backend/parser/parse_relation.c: In function ‘get_rte_attribute_is_dropped’:
/home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:43: warning: comparison between pointer and zero character constant [-Wpointer-compare]
(list_nth(rte->coltypes, attnum - 1) != InvalidOid);
^~
/home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:7: note: did you mean to dereference the pointer?
(list_nth(rte->coltypes, attnum - 1) != InvalidOid);
^

- Andres

On Thu, Apr 6, 2017 at 4:19 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> My compiler, quite justifiedly, complains:
>
> /home/andres/src/postgresql/src/backend/parser/parse_relation.c: In function ‘get_rte_attribute_is_dropped’:
> /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:43: warning: comparison between pointer and zero character constant [-Wpointer-compare]
> (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
> ^~
> /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:7: note: did you mean to dereference the pointer?
> (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
> ^

Good catch. Will push a change from list_nth() to list_nth_oid()
for the benefit of stricter compilers. While I'm at it, I'll throw
on another layer of parentheses to ensure people read that
correctly. Out of curiosity, what compiler or setting catches this?

--
Kevin Grittner

On Fri, Apr 7, 2017 at 10:03 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Thu, Apr 6, 2017 at 4:19 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
>> My compiler, quite justifiedly, complains:
>>
>> /home/andres/src/postgresql/src/backend/parser/parse_relation.c: In function ‘get_rte_attribute_is_dropped’:
>> /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:43: warning: comparison between pointer and zero character constant [-Wpointer-compare]
>> (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
>> ^~
>> /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:7: note: did you mean to dereference the pointer?
>> (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
>> ^
>
> Good catch. Will push a change from list_nth() to list_nth_oid()
> for the benefit of stricter compilers. While I'm at it, I'll throw
> on another layer of parentheses to ensure people read that
> correctly. Out of curiosity, what compiler or setting catches this?

Doesn't it also have the logic backwards? According to the comment,
the attribute is dropped if the type *is* InvalidOid, so we want
result == true in that case. But I don't actually know how to reach
this code to test it.

/*
- * We checked when we loaded ctecoltypes for the tuplestore
+ * We checked when we loaded coltypes for the tuplestore
* that InvalidOid was only used for dropped columns, so it is
* safe to count on that here.
*/
result =
- (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
+ (list_nth_oid(rte->coltypes, attnum - 1) == InvalidOid);
}

--
Thomas Munro
http://www.enterprisedb.com

On 2017-04-06 17:03:20 -0500, Kevin Grittner wrote:
> On Thu, Apr 6, 2017 at 4:19 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > My compiler, quite justifiedly, complains:
> >
> > /home/andres/src/postgresql/src/backend/parser/parse_relation.c: In function ‘get_rte_attribute_is_dropped’:
> > /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:43: warning: comparison between pointer and zero character constant [-Wpointer-compare]
> > (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
> > ^~
> > /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:7: note: did you mean to dereference the pointer?
> > (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
> > ^
>
> Good catch. Will push a change from list_nth() to list_nth_oid()
> for the benefit of stricter compilers. While I'm at it, I'll throw
> on another layer of parentheses to ensure people read that
> correctly. Out of curiosity, what compiler or setting catches this?

gcc-7 here, and the specific warning is -Wpointer-compare.

- Andres

On Thu, Apr 6, 2017 at 5:07 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:

> Doesn't it also have the logic backwards? According to the comment,
> the attribute is dropped if the type *is* InvalidOid, so we want
> result == true in that case. But I don't actually know how to reach
> this code to test it.

You're right. So on this one line I had a reverse logic bug,
something that strict compilers would not accept, and code that
assumed that readers and compilers would always know that tests for
equality or inequality bind tighter than assignment.

I'll commit this fix first so I don't hold up Andres or break any
picky buildfarm critters and then see whether I can't manage to get
the tests to cover this code.

Thanks!

On Thu, Apr 6, 2017 at 5:16 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2017-04-06 17:03:20 -0500, Kevin Grittner wrote:
>> Out of curiosity, what compiler or setting catches this?
>
> gcc-7 here, and the specific warning is -Wpointer-compare.

Thanks! I'll add that to my builds.

--
Kevin Grittner

Kevin Grittner <kgrittn(at)gmail(dot)com> writes:
> On Thu, Apr 6, 2017 at 4:19 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> My compiler, quite justifiedly, complains:
>> /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:43: warning: comparison between pointer and zero character constant [-Wpointer-compare]
>> (list_nth(rte->coltypes, attnum - 1) != InvalidOid);

> Good catch. Will push a change from list_nth() to list_nth_oid()
> for the benefit of stricter compilers.

If the problem is that the list is an OID list, then why didn't the
"Assert(IsPointerList(list))" in list_nth fire? Either this is the
wrong fix, or this code has never been exercised (at least not in
an assert-enabled build).

rte->coltypes certainly ought to be an OID list, so I lean to the
inadequate-testing theory ...

regards, tom lane

On Thu, Apr 6, 2017 at 5:20 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:

> I'll commit this fix first so I don't hold up Andres or break any
> picky buildfarm critters

Done.

> and then see whether I can't manage to get
> the tests to cover this code.

The function in question is only called from rewrite, and here's the
relevant comment:

* About JOINs and dropped columns: although the parser never includes an
* already-dropped column in a JOIN RTE's alias var list, it is possible for
* such a list in a stored rule to include references to dropped columns.
* (If the column is not explicitly referenced anywhere else in the query,
* the dependency mechanism won't consider it used by the rule and so won't
* prevent the column drop.) To support get_rte_attribute_is_dropped(), we
* replace join alias vars that reference dropped columns with null pointers.

So, to test this I guess I need to create a view that does SELECT *
on a table, write a plpgsql trigger function and use it as an AFTER
EACH STATEMENT trigger on that table -- referencing the view and
explicitly using a specific column from the transition table in a
join qual, modify the table so the trigger gets fired and the
function gets cached, ALTER the table to drop the column so
referenced without doing anything that might cause the function plan
to be discarded from cache, and then modify the table again to fire
the cached trigger. Does that seem like the right test to add? Or
would even that fail to reach this code because the transition table
is not on the view?

Oh well, I guess I'll write the code and find out -- seems easier
than reverse-engineering that code path.

--
Kevin Grittner

On Thu, Apr 6, 2017 at 6:10 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:

> * About JOINs and dropped columns: although the parser never includes an
> * already-dropped column in a JOIN RTE's alias var list, it is possible for
> * such a list in a stored rule to include references to dropped columns.
> * (If the column is not explicitly referenced anywhere else in the query,
> * the dependency mechanism won't consider it used by the rule and so won't
> * prevent the column drop.) To support get_rte_attribute_is_dropped(), we
> * replace join alias vars that reference dropped columns with null pointers.

test=# create table t1 (t1id int not null, t1desc text);
CREATE TABLE
test=# create table t2 (t1id int not null, t2id int not null, t2desc text);
CREATE TABLE
test=# create view v1 as select t2desc from t1 join t2 on t1.t1id = t2.t1id;
CREATE VIEW
test=# alter table t1 drop column t1id;
ERROR: cannot drop table t1 column t1id because other objects depend on it
DETAIL: view v1 depends on table t1 column t1id
HINT: Use DROP ... CASCADE to drop the dependent objects too.

Is that comment wrong?

--
Kevin Grittner

2017-04-07 13:06 GMT-03:00 Kevin Grittner <kgrittn(at)gmail(dot)com>:

> ERROR: cannot drop table t1 column t1id because other objects depend on it
> DETAIL: view v1 depends on table t1 column t1id
> HINT: Use DROP ... CASCADE to drop the dependent objects too.
>
> Is that comment wrong?
>

Sort of. If you consider ALTER TABLE ... DROP COLUMN ... CASCADE, it is not
that wrong. However, if you want to be strict, there should be a check to
identify a table column and then hint a specific message (ALTER instead of
DROP).

--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
<http://www.timbira.com.br>

On Fri, Apr 7, 2017 at 8:26 PM, Euler Taveira <euler(at)timbira(dot)com(dot)br> wrote:
> 2017-04-07 13:06 GMT-03:00 Kevin Grittner <kgrittn(at)gmail(dot)com>:
>>
>> ERROR: cannot drop table t1 column t1id because other objects depend on
>> it
>> DETAIL: view v1 depends on table t1 column t1id
>> HINT: Use DROP ... CASCADE to drop the dependent objects too.
>>
>> Is that comment wrong?
>
> Sort of. If you consider ALTER TABLE ... DROP COLUMN ... CASCADE, it is not
> that wrong. However, if you want to be strict, there should be a check to
> identify a table column and then hint a specific message (ALTER instead of
> DROP).

I think you missed the point -- I wasn't talking about the error
message or its associated DETAIL or HINT. I was talking about this
C comment:

> * About JOINs and dropped columns: although the parser never includes an
> * already-dropped column in a JOIN RTE's alias var list, it is possible for
> * such a list in a stored rule to include references to dropped columns.
> * (If the column is not explicitly referenced anywhere else in the query,
> * the dependency mechanism won't consider it used by the rule and so won't
> * prevent the column drop.) To support get_rte_attribute_is_dropped(), we
> * replace join alias vars that reference dropped columns with null pointers.

"If the column is not explicitly referenced anywhere else in the
query [other than JOIN conditions, as I read it], the dependency
mechanism won't consider it used by the rule and so won't prevent
the column drop."

In my example the only place the column was explicitly referenced
was in a join condition, yet the dependency was recognized and the
column drop was prevented. Am I missing something or did someone
invalidate that comment without changing it to reflect the new
reality. It would appear that, under current conditions, there is
no way to reach the code which went untested. At least, I can't see
it.

--
Kevin Grittner