Re: delta relations in AFTER triggers

On Sun, Oct 2, 2016 at 11:20 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Sat, Sep 10, 2016 at 7:28 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>> v6 fixes recently-introduced bit-rot.
>
> Not as big as I thought, only 2k when both patches are combined... The
> patch without noapi in its name needs to be applied first, and after
> the patch with noapi can be applied.
> 60 files changed, 2073 insertions(+), 63 deletions(-)
> Moved to next CF.

In an attempt to make this patch more digestible for reviewers, I
have split it up as follows:

transition-c-triggers-only-v7.diff
contrib/pgstattuple/pgstattuple.c | 2 +
doc/src/sgml/catalogs.sgml | 16 ++
doc/src/sgml/ref/create_trigger.sgml | 94 +++++--
src/backend/commands/tablecmds.c | 5 +-
src/backend/commands/trigger.c | 327 ++++++++++++++++++++++++-
src/backend/nodes/copyfuncs.c | 16 ++
src/backend/nodes/equalfuncs.c | 14 ++
src/backend/nodes/outfuncs.c | 13 +
src/backend/parser/gram.y | 70 +++++-
src/backend/utils/adt/ruleutils.c | 23 ++
src/bin/pg_basebackup/walmethods.c | 5 -
src/include/catalog/pg_trigger.h | 13 +-
src/include/commands/trigger.h | 2 +
src/include/nodes/nodes.h | 1 +
src/include/nodes/parsenodes.h | 17 ++
src/include/parser/kwlist.h | 3 +
src/include/utils/reltrigger.h | 7 +
17 files changed, 581 insertions(+), 47 deletions(-)

This part is virtually unchanged (just curing bit-rot) since
August, 2014, when I believe I had addressed all issues raised by
reviewers. It does provide a barely usable feature, since the
syntax for transition tables is added and tuplestores are created
when needed (and only when needed), with references stored in the
TriggerData structure. No new execution nodes are provided, so
only C triggers can use these relations, and must explicitly and
directly access the tuplestores from within the C code -- there is
no support for referencing these tuplestores from within queries.

This is basic infrastructure needed for the more complete feature.
As far as I know there are no objections to what is implemented
here. I have pulled it out to make the review of the more
controversial portions easier. Since it had quite a bit of review
two years ago, I will do some testing to make sure that nothing has
broken and then push this part in a few days if there are no
objections.

transition-noapi.diff
contrib/pgstattuple/pgstattuple.c | 2 -
doc/src/sgml/spi.sgml | 279 ++++++++++++++++++++
src/backend/commands/explain.c | 10 +
src/backend/executor/Makefile | 3 +-
src/backend/executor/execAmi.c | 6 +
src/backend/executor/execProcnode.c | 14 +
src/backend/executor/nodeTuplestorescan.c | 200 ++++++++++++++
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 | 44 +++
src/backend/optimizer/path/costsize.c | 66 +++++
src/backend/optimizer/plan/createplan.c | 71 +++++
src/backend/optimizer/plan/setrefs.c | 11 +
src/backend/optimizer/plan/subselect.c | 5 +
src/backend/optimizer/prep/prepjointree.c | 2 +
src/backend/optimizer/util/pathnode.c | 25 ++
src/backend/optimizer/util/plancat.c | 4 +-
src/backend/optimizer/util/relnode.c | 1 +
src/backend/parser/Makefile | 3 +-
src/backend/parser/analyze.c | 11 +
src/backend/parser/parse_clause.c | 23 +-
src/backend/parser/parse_node.c | 2 +
src/backend/parser/parse_relation.c | 115 +++++++-
src/backend/parser/parse_target.c | 2 +
src/backend/parser/parse_tuplestore.c | 34 +++
src/backend/tcop/utility.c | 3 +-
src/backend/utils/adt/ruleutils.c | 1 +
src/backend/utils/cache/Makefile | 2 +-
src/backend/utils/cache/tsrcache.c | 111 ++++++++
src/bin/pg_basebackup/walmethods.c | 5 +
src/include/executor/nodeTuplestorescan.h | 24 ++
src/include/nodes/execnodes.h | 18 ++
src/include/nodes/nodes.h | 2 +
src/include/nodes/parsenodes.h | 11 +-
src/include/nodes/plannodes.h | 10 +
src/include/optimizer/cost.h | 3 +
src/include/optimizer/pathnode.h | 2 +
src/include/parser/parse_node.h | 2 +
src/include/parser/parse_relation.h | 4 +
src/include/parser/parse_tuplestore.h | 24 ++
src/include/utils/tsrcache.h | 27 ++
src/include/utils/tsrmd.h | 29 ++
src/include/utils/tsrmdcache.h | 31 +++
src/include/utils/tuplestore.h | 14 +
src/pl/plpgsql/src/pl_comp.c | 13 +-
src/pl/plpgsql/src/pl_exec.c | 26 ++
src/pl/plpgsql/src/plpgsql.h | 11 +-
src/test/regress/expected/plpgsql.out | 85 ++++++
src/test/regress/sql/plpgsql.sql | 73 +++++
52 files changed, 1499 insertions(+), 23 deletions(-)

This is the meat of the feature, with the API more-or-less ripped
out. It depends on the transition-c-triggers-only patch. This
part is, as far as I know, OK with others based on past review, but
is useless without some API to glue the pieces together --
specifically, passing the tuplestore name and its related TupleDesc
structure to the parse and plan phases, and passing those plus the
actual tuplestore to the execution phase. I will not commit this
until it is integrated with a working API that has consensus
support. If that doesn't happen before release we will have the
choice of documenting this feature as C trigger only for now, or
reverting transition-c-triggers-only-v7.diff.

transition-tsr
.../pg_stat_statements/pg_stat_statements.c | 15 +-
src/backend/catalog/pg_proc.c | 3 +-
src/backend/commands/copy.c | 5 +-
src/backend/commands/createas.c | 4 +-
src/backend/commands/explain.c | 23 +--
src/backend/commands/extension.c | 6 +-
src/backend/commands/foreigncmds.c | 2 +-
src/backend/commands/matview.c | 2 +-
src/backend/commands/prepare.c | 7 +-
src/backend/commands/schemacmds.c | 1 +
src/backend/commands/trigger.c | 2 +-
src/backend/commands/view.c | 2 +-
src/backend/executor/execMain.c | 5 +
src/backend/executor/execParallel.c | 2 +-
src/backend/executor/execUtils.c | 2 +
src/backend/executor/functions.c | 8 +-
src/backend/executor/nodeTuplestorescan.c | 4 +-
src/backend/executor/spi.c | 135 ++++++++++++++++-
src/backend/optimizer/util/clauses.c | 2 +-
src/backend/parser/analyze.c | 4 +-
src/backend/tcop/postgres.c | 11 +-
src/backend/tcop/pquery.c | 10 +-
src/backend/tcop/utility.c | 34 +++--
src/backend/utils/cache/plancache.c | 6 +-
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/spi.h | 8 +
src/include/executor/spi_priv.h | 2 +
src/include/nodes/execnodes.h | 4 +
src/include/nodes/parsenodes.h | 2 +-
src/include/parser/analyze.h | 2 +-
src/include/tcop/tcopprot.h | 6 +-
src/include/tcop/utility.h | 7 +-
src/include/utils/portal.h | 1 +
src/pl/plpgsql/src/pl_exec.c | 13 +-
src/pl/plpgsql/src/plpgsql.h | 3 +
38 files changed, 281 insertions(+), 80 deletions(-)

This is the API that has been working and tested for two years, but
that Heikki doesn't like. It depends on the transition-noapi
patch.

Note that about half of it is in SPI support, which could be split
out. There is a complete and usable API without it, consisting of
new fields in some structures and an additional parameter in 7
existing functions. The SPI is there to make using the feature
easy from code using SPI. For example, the total of lines added
and deleted to implement support in plpgsql is 16. Each other PL
should need about the same for this API. SPI support would also
allow us to consider using set logic for validating foreign keys,
instead of the one-row-at-a-time approach currently used. My guess
is that even if we go with some other API, we will choose to add
SPI support more-or-less like this to avoid duplicating complex
alternative code everywhere.

transition-via-params
.../pg_stat_statements/pg_stat_statements.c | 4 +
src/backend/executor/nodeTuplestorescan.c | 30 +++---
src/backend/nodes/copyfuncs.c | 4 +-
src/backend/nodes/outfuncs.c | 10 +-
src/backend/nodes/readfuncs.c | 2 +-
src/backend/optimizer/path/costsize.c | 37 +++++++
src/backend/optimizer/plan/createplan.c | 45 ++++++++-
src/backend/parser/parse_clause.c | 4 +
src/backend/parser/parse_node.c | 1 +
src/backend/parser/parse_relation.c | 23 ++---
src/include/nodes/parsenodes.h | 3 +-
src/include/nodes/plannodes.h | 2 +-
src/include/parser/parse_node.h | 4 +-
src/include/parser/parse_relation.h | 2 +
src/pl/plpgsql/src/pl_comp.c | 95 +++++++++++++++++-
src/pl/plpgsql/src/pl_exec.c | 65 ++++++++----
src/pl/plpgsql/src/pl_funcs.c | 2 +
src/pl/plpgsql/src/plpgsql.h | 30 ++++--
src/test/regress/expected/plpgsql.out | 1 -
19 files changed, 291 insertions(+), 73 deletions(-)

This is my attempt to get an API working along the lines suggested
by Heikki. It depends on the transition-noapi patch. Significant
assistance has been provided by Thomas Munro, although please blame
me for the problems that remain. It does not yet work. Note that
plpgsql so far has close to 200 lines inserted and deleted in an
attempt to make it work, and other PLs would either need the SPI
support or would need more lines of support per PL, because their
implementations are not as close to this technique as plpgsql is.

I'm sure there is some wet paint in this one from repeated attempts
to modify it one way or another to try to get it to actually work.
Apologies for that. Any suggestions on how to wrangle this into
something actually functional would be welcome.

As with the last CF, I think that it would be useful for anyone
with an interest in this feature itself or in the incremental
maintenance of materialized views (which this is an incremental
step toward) to look at the documentation and to play with the
transition-tsr variation, which should be fully functional and
should match the behavior we will see with any alternative API.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company