On Sun, Oct 2, 2016 at 11:20 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Sat, Sep 10, 2016 at 7:28 AM, Kevin Grittner <kgri...@gmail.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
transition-v7.tgz
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
