delta relations in AFTER triggers
Attached is a WIP patch for implementing the capture of delta
relations for a DML statement, in the form of two tuplestores --
one for the old versions and one for the new versions. In the
short term it is intended to make these relations available in
trigger functions, although the patch so far doesn't touch any PLs
-- it just takes things as far as providing the relations as
tuplestores in the TriggerData structure when appropriate, for the
PLs to pick up from there. It seemed best to get agreement on the
overall approach before digging into all the PLs. This is
implemented only for INSERT, UPDATE, and DELETE since it wasn't
totally clear what the use cases and proper behavior was for other
triggers. Opinions on whether we should try to provide deltas for
other cases, and if so what the semantics are, are welcome.
Once triggers can access this delta data, it will also be used for
incremental maintenance of materialized views, although I don't
want get too sidetracked on any details of that until we have
proven delta data available in triggers. (One step at a time or
we'll never get there.)
I looked at the standard, and initially tried to implement the
standard syntax for this; however, it appeared that the reasons
given for not using standard syntax for the row variables also
apply to the transition relations (the term used by the standard).
There isn't an obvious way to tie that in to all the PLs we
support. It could be done, but it seems like it would intolerably
ugly, and more fragile than what we have done so far.
Some things which I *did* follow from the standard: these new
relations are only allowed within AFTER triggers, but are available
in both AFTER STATEMENT and AFTER ROW triggers. That is, an AFTER
UPDATE ... FOR EACH ROW trigger could use both the OLD and NEW row
variables as well as the delta relations (under whatever names we
pick). That probably won't be used very often, but I can imagine
some cases where it might be useful. I expect that these will
normally be used in FOR EACH STATEMENT triggers.
There are a couple things I would really like to get settled in
this round of review, so things don't need to be refactored in
major ways later:
(1) My first impulse was to capture this delta data in the form of
tuplestores of just TIDs, and fetching the tuples themselves from
the heap on reference. In earlier discussions others have argued
for using tuplestores of the actual rows themselves. I have taken
that advice here, but still don't feel 100% convinced. What I am
convinced of is that I don't want to write a lot of code based on
that decision and only then have people weigh in on the side of how
I had planned to do it in the first place. I hate it when that
happens.
(2) Do we want to just pick names for these in the PLs rather than
using the standard syntax? Implementing the standard syntax seemed
to require three new (unreserved) keywords, changes to the catalogs
to store the chosen relations names, and some way to tie the
specified relation names in to the various PLs. The way I have
gone here just adds two new fields to the TriggerData structure and
leaves it to each PL how to deal with that. Failure to do anything
in a PL just leaves it at the status quo with no real harm done --
it just won't have the new delta relations available.
Of course, any other comments on the approach taken or how it can
be improved are welcome.
At this point the only testing is that make check-world completes
without problems. If we can agree on this part of it I will look
at the PLs, and create regression tests. I would probably submit
each PL implementation as a separate patch.
I was surprised that the patch to this point was so small:
5 files changed, 170 insertions(+), 19 deletions(-)
Hopefully that's not due to having missed something.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
after-trigger-delta-relations-v1.patchtext/x-diff; name=after-trigger-delta-relations-v1.patchDownload
*** a/doc/src/sgml/ref/create_table.sgml
--- b/doc/src/sgml/ref/create_table.sgml
***************
*** 1062,1067 **** CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
--- 1062,1079 ----
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>generate_deltas</literal> (<type>boolean</type>)</term>
+ <listitem>
+ <para>
+ Declare that a table generates delta relations when modified. This
+ allows <literal>AFTER</> triggers to reference the set of rows modified
+ by a statement. See
+ <xref linkend="sql-createtrigger"> for details.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</refsect2>
*** a/src/backend/access/common/reloptions.c
--- b/src/backend/access/common/reloptions.c
***************
*** 85,90 **** static relopt_bool boolRelOpts[] =
--- 85,98 ----
},
false
},
+ {
+ {
+ "generate_deltas",
+ "Relation generates delta relations for use by AFTER triggers",
+ RELOPT_KIND_HEAP
+ },
+ false
+ },
/* list terminator */
{{NULL}}
};
***************
*** 1205,1211 **** default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
{"check_option", RELOPT_TYPE_STRING,
offsetof(StdRdOptions, check_option_offset)},
{"user_catalog_table", RELOPT_TYPE_BOOL,
! offsetof(StdRdOptions, user_catalog_table)}
};
options = parseRelOptions(reloptions, validate, kind, &numoptions);
--- 1213,1221 ----
{"check_option", RELOPT_TYPE_STRING,
offsetof(StdRdOptions, check_option_offset)},
{"user_catalog_table", RELOPT_TYPE_BOOL,
! offsetof(StdRdOptions, user_catalog_table)},
! {"generate_deltas", RELOPT_TYPE_BOOL,
! offsetof(StdRdOptions, generate_deltas)}
};
options = parseRelOptions(reloptions, validate, kind, &numoptions);
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
***************
*** 2060,2066 **** ExecARInsertTriggers(EState *estate, ResultRelInfo *relinfo,
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc && trigdesc->trig_insert_after_row)
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_INSERT,
true, NULL, trigtuple, recheckIndexes, NULL);
}
--- 2060,2069 ----
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc &&
! (trigdesc->trig_insert_after_row ||
! (trigdesc->trig_insert_after_statement &&
! RelationGeneratesDeltas(relinfo->ri_RelationDesc))))
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_INSERT,
true, NULL, trigtuple, recheckIndexes, NULL);
}
***************
*** 2263,2269 **** ExecARDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc && trigdesc->trig_delete_after_row)
{
HeapTuple trigtuple;
--- 2266,2275 ----
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc &&
! (trigdesc->trig_delete_after_row ||
! (trigdesc->trig_delete_after_statement &&
! RelationGeneratesDeltas(relinfo->ri_RelationDesc))))
{
HeapTuple trigtuple;
***************
*** 2528,2534 **** ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc && trigdesc->trig_update_after_row)
{
HeapTuple trigtuple;
--- 2534,2543 ----
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc &&
! (trigdesc->trig_update_after_row ||
! (trigdesc->trig_update_after_statement &&
! RelationGeneratesDeltas(relinfo->ri_RelationDesc))))
{
HeapTuple trigtuple;
***************
*** 3160,3167 **** typedef struct AfterTriggerEventList
* fdw_tuplestores[query_depth] is a tuplestore containing the foreign tuples
* needed for the current query.
*
! * maxquerydepth is just the allocated length of query_stack and
! * fdw_tuplestores.
*
* state_stack is a stack of pointers to saved copies of the SET CONSTRAINTS
* state data; each subtransaction level that modifies that state first
--- 3169,3179 ----
* fdw_tuplestores[query_depth] is a tuplestore containing the foreign tuples
* needed for the current query.
*
! * old_tuplestores[query_depth] and new_tuplestores[query_depth] hold the
! * delta relations for the current query.
! *
! * maxquerydepth is just the allocated length of query_stack and the
! * tuplestores.
*
* state_stack is a stack of pointers to saved copies of the SET CONSTRAINTS
* state data; each subtransaction level that modifies that state first
***************
*** 3190,3196 **** typedef struct AfterTriggersData
AfterTriggerEventList events; /* deferred-event list */
int query_depth; /* current query list index */
AfterTriggerEventList *query_stack; /* events pending from each query */
! Tuplestorestate **fdw_tuplestores; /* foreign tuples from each query */
int maxquerydepth; /* allocated len of above array */
MemoryContext event_cxt; /* memory context for events, if any */
--- 3202,3210 ----
AfterTriggerEventList events; /* deferred-event list */
int query_depth; /* current query list index */
AfterTriggerEventList *query_stack; /* events pending from each query */
! Tuplestorestate **fdw_tuplestores; /* foreign tuples for one row from each query */
! Tuplestorestate **old_tuplestores; /* all old tuples from each query */
! Tuplestorestate **new_tuplestores; /* all new tuples from each query */
int maxquerydepth; /* allocated len of above array */
MemoryContext event_cxt; /* memory context for events, if any */
***************
*** 3224,3234 **** static SetConstraintState SetConstraintStateAddItem(SetConstraintState state,
* Gets the current query fdw tuplestore and initializes it if necessary
*/
static Tuplestorestate *
! GetCurrentFDWTuplestore()
{
Tuplestorestate *ret;
! ret = afterTriggers->fdw_tuplestores[afterTriggers->query_depth];
if (ret == NULL)
{
MemoryContext oldcxt;
--- 3238,3248 ----
* Gets the current query fdw tuplestore and initializes it if necessary
*/
static Tuplestorestate *
! GetCurrentTuplestore(Tuplestorestate **tss)
{
Tuplestorestate *ret;
! ret = tss[afterTriggers->query_depth];
if (ret == NULL)
{
MemoryContext oldcxt;
***************
*** 3255,3261 **** GetCurrentFDWTuplestore()
CurrentResourceOwner = saveResourceOwner;
MemoryContextSwitchTo(oldcxt);
! afterTriggers->fdw_tuplestores[afterTriggers->query_depth] = ret;
}
return ret;
--- 3269,3275 ----
CurrentResourceOwner = saveResourceOwner;
MemoryContextSwitchTo(oldcxt);
! tss[afterTriggers->query_depth] = ret;
}
return ret;
***************
*** 3515,3520 **** AfterTriggerExecute(AfterTriggerEvent event,
--- 3529,3535 ----
{
AfterTriggerShared evtshared = GetTriggerSharedData(event);
Oid tgoid = evtshared->ats_tgoid;
+ int event_op = evtshared->ats_event & TRIGGER_EVENT_OPMASK;
TriggerData LocTriggerData;
HeapTupleData tuple1;
HeapTupleData tuple2;
***************
*** 3552,3565 **** AfterTriggerExecute(AfterTriggerEvent event,
{
case AFTER_TRIGGER_FDW_FETCH:
{
! Tuplestorestate *fdw_tuplestore = GetCurrentFDWTuplestore();
if (!tuplestore_gettupleslot(fdw_tuplestore, true, false,
trig_tuple_slot1))
elog(ERROR, "failed to fetch tuple1 for AFTER trigger");
! if ((evtshared->ats_event & TRIGGER_EVENT_OPMASK) ==
! TRIGGER_EVENT_UPDATE &&
!tuplestore_gettupleslot(fdw_tuplestore, true, false,
trig_tuple_slot2))
elog(ERROR, "failed to fetch tuple2 for AFTER trigger");
--- 3567,3580 ----
{
case AFTER_TRIGGER_FDW_FETCH:
{
! Tuplestorestate *fdw_tuplestore =
! GetCurrentTuplestore(afterTriggers->fdw_tuplestores);
if (!tuplestore_gettupleslot(fdw_tuplestore, true, false,
trig_tuple_slot1))
elog(ERROR, "failed to fetch tuple1 for AFTER trigger");
! if (event_op == TRIGGER_EVENT_UPDATE &&
!tuplestore_gettupleslot(fdw_tuplestore, true, false,
trig_tuple_slot2))
elog(ERROR, "failed to fetch tuple2 for AFTER trigger");
***************
*** 3580,3588 **** AfterTriggerExecute(AfterTriggerEvent event,
ExecMaterializeSlot(trig_tuple_slot1);
LocTriggerData.tg_trigtuplebuf = InvalidBuffer;
! LocTriggerData.tg_newtuple =
! ((evtshared->ats_event & TRIGGER_EVENT_OPMASK) ==
! TRIGGER_EVENT_UPDATE) ?
ExecMaterializeSlot(trig_tuple_slot2) : NULL;
LocTriggerData.tg_newtuplebuf = InvalidBuffer;
--- 3595,3601 ----
ExecMaterializeSlot(trig_tuple_slot1);
LocTriggerData.tg_trigtuplebuf = InvalidBuffer;
! LocTriggerData.tg_newtuple = (event_op == TRIGGER_EVENT_UPDATE) ?
ExecMaterializeSlot(trig_tuple_slot2) : NULL;
LocTriggerData.tg_newtuplebuf = InvalidBuffer;
***************
*** 3622,3627 **** AfterTriggerExecute(AfterTriggerEvent event,
--- 3635,3659 ----
}
/*
+ * Set up the tuplestore information.
+ */
+ if (RelationGeneratesDeltas(rel))
+ {
+ if (event_op == TRIGGER_EVENT_DELETE ||
+ event_op == TRIGGER_EVENT_UPDATE)
+ LocTriggerData.tg_olddelta =
+ GetCurrentTuplestore(afterTriggers->old_tuplestores);
+ else
+ LocTriggerData.tg_olddelta = NULL;
+ if (event_op == TRIGGER_EVENT_INSERT ||
+ event_op == TRIGGER_EVENT_UPDATE)
+ LocTriggerData.tg_newdelta =
+ GetCurrentTuplestore(afterTriggers->new_tuplestores);
+ else
+ LocTriggerData.tg_newdelta = NULL;
+ }
+
+ /*
* Setup the remaining trigger information
*/
LocTriggerData.type = T_TriggerData;
***************
*** 3921,3926 **** AfterTriggerBeginXact(void)
--- 3953,3964 ----
afterTriggers->fdw_tuplestores = (Tuplestorestate **)
MemoryContextAllocZero(TopTransactionContext,
8 * sizeof(Tuplestorestate *));
+ afterTriggers->old_tuplestores = (Tuplestorestate **)
+ MemoryContextAllocZero(TopTransactionContext,
+ 8 * sizeof(Tuplestorestate *));
+ afterTriggers->new_tuplestores = (Tuplestorestate **)
+ MemoryContextAllocZero(TopTransactionContext,
+ 8 * sizeof(Tuplestorestate *));
afterTriggers->maxquerydepth = 8;
/* Context for events is created only when needed */
***************
*** 3970,3978 **** AfterTriggerBeginQuery(void)
--- 4008,4026 ----
afterTriggers->fdw_tuplestores = (Tuplestorestate **)
repalloc(afterTriggers->fdw_tuplestores,
new_alloc * sizeof(Tuplestorestate *));
+ afterTriggers->old_tuplestores = (Tuplestorestate **)
+ repalloc(afterTriggers->old_tuplestores,
+ new_alloc * sizeof(Tuplestorestate *));
+ afterTriggers->new_tuplestores = (Tuplestorestate **)
+ repalloc(afterTriggers->new_tuplestores,
+ new_alloc * sizeof(Tuplestorestate *));
/* Clear newly-allocated slots for subsequent lazy initialization. */
memset(afterTriggers->fdw_tuplestores + old_alloc,
0, (new_alloc - old_alloc) * sizeof(Tuplestorestate *));
+ memset(afterTriggers->old_tuplestores + old_alloc,
+ 0, (new_alloc - old_alloc) * sizeof(Tuplestorestate *));
+ memset(afterTriggers->new_tuplestores + old_alloc,
+ 0, (new_alloc - old_alloc) * sizeof(Tuplestorestate *));
afterTriggers->maxquerydepth = new_alloc;
}
***************
*** 4001,4006 **** AfterTriggerEndQuery(EState *estate)
--- 4049,4056 ----
{
AfterTriggerEventList *events;
Tuplestorestate *fdw_tuplestore;
+ Tuplestorestate *old_tuplestore;
+ Tuplestorestate *new_tuplestore;
/* Must be inside a transaction */
Assert(afterTriggers != NULL);
***************
*** 4045,4057 **** AfterTriggerEndQuery(EState *estate)
break;
}
! /* Release query-local storage for events, including tuplestore if any */
fdw_tuplestore = afterTriggers->fdw_tuplestores[afterTriggers->query_depth];
if (fdw_tuplestore)
{
tuplestore_end(fdw_tuplestore);
afterTriggers->fdw_tuplestores[afterTriggers->query_depth] = NULL;
}
afterTriggerFreeEventList(&afterTriggers->query_stack[afterTriggers->query_depth]);
afterTriggers->query_depth--;
--- 4095,4119 ----
break;
}
! /* Release query-local storage for events, including tuplestores, if any */
fdw_tuplestore = afterTriggers->fdw_tuplestores[afterTriggers->query_depth];
if (fdw_tuplestore)
{
tuplestore_end(fdw_tuplestore);
afterTriggers->fdw_tuplestores[afterTriggers->query_depth] = NULL;
}
+ old_tuplestore = afterTriggers->old_tuplestores[afterTriggers->query_depth];
+ if (old_tuplestore)
+ {
+ tuplestore_end(old_tuplestore);
+ afterTriggers->old_tuplestores[afterTriggers->query_depth] = NULL;
+ }
+ new_tuplestore = afterTriggers->new_tuplestores[afterTriggers->query_depth];
+ if (new_tuplestore)
+ {
+ tuplestore_end(new_tuplestore);
+ afterTriggers->new_tuplestores[afterTriggers->query_depth] = NULL;
+ }
afterTriggerFreeEventList(&afterTriggers->query_stack[afterTriggers->query_depth]);
afterTriggers->query_depth--;
***************
*** 4283,4288 **** AfterTriggerEndSubXact(bool isCommit)
--- 4345,4362 ----
tuplestore_end(ts);
afterTriggers->fdw_tuplestores[afterTriggers->query_depth] = NULL;
}
+ ts = afterTriggers->old_tuplestores[afterTriggers->query_depth];
+ if (ts)
+ {
+ tuplestore_end(ts);
+ afterTriggers->old_tuplestores[afterTriggers->query_depth] = NULL;
+ }
+ ts = afterTriggers->new_tuplestores[afterTriggers->query_depth];
+ if (ts)
+ {
+ tuplestore_end(ts);
+ afterTriggers->new_tuplestores[afterTriggers->query_depth] = NULL;
+ }
afterTriggerFreeEventList(&afterTriggers->query_stack[afterTriggers->query_depth]);
afterTriggers->query_depth--;
***************
*** 4767,4773 **** AfterTriggerPendingOnRel(Oid relid)
*
* NOTE: this is called whenever there are any triggers associated with
* the event (even if they are disabled). This function decides which
! * triggers actually need to be queued.
* ----------
*/
static void
--- 4841,4854 ----
*
* NOTE: this is called whenever there are any triggers associated with
* the event (even if they are disabled). This function decides which
! * triggers actually need to be queued. It is also called after each row,
! * even if there are no triggers for that event, if the table has the
! * generate_deltas storage property set and there are any AFTER STATEMENT
! * triggers, so that the delta relations can be built.
! *
! * Delta tuplestores are built now, rather than when events are pulled off
! * of the queue because AFTER ROW triggers are allowed to select from the
! * delta relations for the statement.
* ----------
*/
static void
***************
*** 4777,4782 **** AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
--- 4858,4864 ----
List *recheckIndexes, Bitmapset *modifiedCols)
{
Relation rel = relinfo->ri_RelationDesc;
+ bool generate_deltas = RelationGeneratesDeltas(rel);
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
AfterTriggerEventData new_event;
AfterTriggerSharedData new_shared;
***************
*** 4797,4807 **** AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
--- 4879,4924 ----
elog(ERROR, "AfterTriggerSaveEvent() called outside of query");
/*
+ * If the relation has enabled generate_deltas, capture rows into delta
+ * tuplestores for this depth.
+ */
+ if (generate_deltas && row_trigger)
+ {
+ if (event == TRIGGER_EVENT_DELETE || event == TRIGGER_EVENT_UPDATE)
+ {
+ Tuplestorestate *old_tuplestore;
+
+ Assert(oldtup != NULL);
+ old_tuplestore =
+ GetCurrentTuplestore(afterTriggers->old_tuplestores);
+ tuplestore_puttuple(old_tuplestore, oldtup);
+ }
+ if (event == TRIGGER_EVENT_INSERT || event == TRIGGER_EVENT_UPDATE)
+ {
+ Tuplestorestate *new_tuplestore;
+
+ Assert(newtup != NULL);
+ new_tuplestore =
+ GetCurrentTuplestore(afterTriggers->new_tuplestores);
+ tuplestore_puttuple(new_tuplestore, newtup);
+ }
+
+ /* If deltas were the only reason we're here, return. */
+ if ((event == TRIGGER_EVENT_DELETE && !trigdesc->trig_delete_after_row) ||
+ (event == TRIGGER_EVENT_INSERT && !trigdesc->trig_insert_after_row) ||
+ (event == TRIGGER_EVENT_UPDATE && !trigdesc->trig_update_after_row))
+ return;
+ }
+
+ /*
* Validate the event code and collect the associated tuple CTIDs.
*
* The event code will be used both as a bitmask and an array offset, so
* validation is important to make sure we don't walk off the edge of our
* arrays.
+ *
+ * If we are only saving a row for statement level delta relations, return
+ * early.
*/
switch (event)
{
***************
*** 4893,4899 **** AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
{
if (fdw_tuplestore == NULL)
{
! fdw_tuplestore = GetCurrentFDWTuplestore();
new_event.ate_flags = AFTER_TRIGGER_FDW_FETCH;
}
else
--- 5010,5017 ----
{
if (fdw_tuplestore == NULL)
{
! fdw_tuplestore =
! GetCurrentTuplestore(afterTriggers->fdw_tuplestores);
new_event.ate_flags = AFTER_TRIGGER_FDW_FETCH;
}
else
*** a/src/include/commands/trigger.h
--- b/src/include/commands/trigger.h
***************
*** 36,41 **** typedef struct TriggerData
--- 36,43 ----
Trigger *tg_trigger;
Buffer tg_trigtuplebuf;
Buffer tg_newtuplebuf;
+ Tuplestorestate *tg_olddelta;
+ Tuplestorestate *tg_newdelta;
} TriggerData;
/*
*** a/src/include/utils/rel.h
--- b/src/include/utils/rel.h
***************
*** 220,225 **** typedef struct StdRdOptions
--- 220,226 ----
int check_option_offset; /* for views */
bool user_catalog_table; /* use as an additional catalog
* relation */
+ bool generate_deltas; /* generate delta relations */
} StdRdOptions;
#define HEAP_MIN_FILLFACTOR 10
***************
*** 298,303 **** typedef struct StdRdOptions
--- 299,312 ----
((StdRdOptions *) (relation)->rd_options)->user_catalog_table : false)
/*
+ * RelationGeneratesDeltas
+ * Returns whether the relation captures delta information when changed.
+ */
+ #define RelationGeneratesDeltas(relation) \
+ ((relation)->rd_options ? \
+ ((StdRdOptions *) (relation)->rd_options)->generate_deltas : false)
+
+ /*
* RelationIsValid
* True iff relation descriptor is valid.
*/
On Sat, Jun 14, 2014 at 04:56:44PM -0700, Kevin Grittner wrote:
Attached is a WIP patch for implementing the capture of delta
relations for a DML statement, in the form of two tuplestores --
one for the old versions and one for the new versions.
Thanks!
Any chance we might be able to surface the old version for the case of
UPDATE ... RETURNING?
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Fetter <david@fetter.org> wrote:
Any chance we might be able to surface the old version for the
case of UPDATE ... RETURNING?
Not as part of this patch.
Of course, once delta relations are available, who knows what
people might do with them. I have a hard time imagining exactly
how you would expose what you're talking about, but a column to
distinguish before and after images might work. Incremental
maintenance of materialized views will require that in the form of
a count column with -1 for deleted and +1 for inserted, so there
might be some common ground when we get to that.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner wrote:
Attached is a WIP patch for implementing the capture of delta
relations for a DML statement, in the form of two tuplestores --
one for the old versions and one for the new versions.� In the
short term it is intended to make these relations available in
trigger functions, although the patch so far doesn't touch any PLs
-- it just takes things as far as providing the relations as
tuplestores in the TriggerData structure when appropriate, for the
PLs to pick up from there.� It seemed best to get agreement on the
overall approach before digging into all the PLs.� This is
implemented only for INSERT, UPDATE, and DELETE since it wasn't
totally clear what the use cases and proper behavior was for other
triggers.� Opinions on whether we should try to provide deltas for
other cases, and if so what the semantics are, are welcome.
TRUNCATE triggers shouldn't have delta relations, that's for sure, or
it'd completely negate the point of TRUNCATE triggers. I don't think we
have any other event, do we? People who get delta relations for
deleting all rows should be using DELETE, I think.
I am not sure about providing delta relations in the FOR EACH ROW case.
For what cases are them useful? In all cases I think NEW and OLD are
sufficient. I didn't read the standard, but if it's saying that in FOR
EACH ROW the new/deleted/changed row should be accessible by way of a
delta relation, then perhaps we should look into making NEW and OLD be
accesible via that relation rather than adding delta relations. It
might be more code, though.
Now, if you only have delta relations in FOR EACH STATEMENT, then it
seems to me you can optimize obtaining them only when such triggers
are defined; this lets you do away with the reloption entirely, doesn't
it?
I noticed that GetCurrentFDWTuplestore() changed its identity without
getting its comment updated. The new name seems awfully generic, and I
don't think it really describes what the function does. I think it
needs more renaminguu
(1)� My first impulse was to capture this delta data in the form of
tuplestores of just TIDs, and fetching the tuples themselves from
the heap on reference.� In earlier discussions others have argued
for using tuplestores of the actual rows themselves.
Can you please supply pointers to such discussion? I don't see any
point in not just storing TIDs, but perhaps I'm missing something.
(2)� Do we want to just pick names for these in the PLs rather than
using the standard syntax?� Implementing the standard syntax seemed
to require three new (unreserved) keywords, changes to the catalogs
to store the chosen relations names, and some way to tie the
specified relation names in to the various PLs.
I think the only one for which we have a compulsion to follow someone
in this area would be PL/pgSQL, which probably needs to follow PL/SQL's
lead if there is one. Other than that I don't think we need to do
anything standard. We don't (yet) have PL/PSM which would need to have
the standard-mandated syntax.
The way I have
gone here just adds two new fields to the TriggerData structure and
leaves it to each PL how to deal with that.� Failure to do anything
in a PL just leaves it at the status quo with no real harm done --
it just won't have the new delta relations available.
It seems to me that plpgsql support is mandatory, but other PLs can add
support as interested parties weigh in with patches. As with event
triggers, I don't feel the author of the main feature is responsible for
patching all PLs.
+ <varlistentry> + <term><literal>generate_deltas</literal> (<type>boolean</type>)</term> + <listitem> + <para> + Declare that a table generates delta relations when modified. This + allows <literal>AFTER</> triggers to reference the set of rows modified + by a statement. See + <xref linkend="sql-createtrigger"> for details. + </para> + </listitem> + </varlistentry>
Are you intentionally omitting the corresponding sql-createtrigger patch?
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jun 14, 2014 at 7:56 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
I looked at the standard, and initially tried to implement the
standard syntax for this; however, it appeared that the reasons
given for not using standard syntax for the row variables also
apply to the transition relations (the term used by the standard).
There isn't an obvious way to tie that in to all the PLs we
support. It could be done, but it seems like it would intolerably
ugly, and more fragile than what we have done so far.
I'm not too familiar with this area. Can you describe what the
standard syntax for the row variables is, as opposed to our syntax?
Also, what's the standard syntax for the the transition relations?
Some things which I *did* follow from the standard: these new
relations are only allowed within AFTER triggers, but are available
in both AFTER STATEMENT and AFTER ROW triggers. That is, an AFTER
UPDATE ... FOR EACH ROW trigger could use both the OLD and NEW row
variables as well as the delta relations (under whatever names we
pick). That probably won't be used very often, but I can imagine
some cases where it might be useful. I expect that these will
normally be used in FOR EACH STATEMENT triggers.
I'm concerned about the performance implications of capturing the
delta relations unconditionally. If a particular trigger actually
needs the delta relations, then the time spent capturing that
information is well spent; but if it doesn't, then it's a waste.
There are lots of people using FOR EACH STATEMENT triggers right now
who won't be happy if those start requiring O(n) additional temporary
storage, where n is the number of rows modified by the statement.
This is likely an even bigger issue for per-row triggers, of course,
where as you say, it probably won't be used often.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 17, 2014 at 04:07:55PM -0400, Robert Haas wrote:
On Sat, Jun 14, 2014 at 7:56 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
I looked at the standard, and initially tried to implement the
standard syntax for this; however, it appeared that the reasons
given for not using standard syntax for the row variables also
apply to the transition relations (the term used by the standard).
There isn't an obvious way to tie that in to all the PLs we
support. It could be done, but it seems like it would intolerably
ugly, and more fragile than what we have done so far.I'm not too familiar with this area. Can you describe what the
standard syntax for the row variables is, as opposed to our syntax?
Also, what's the standard syntax for the the transition relations?
The good:
- Generating the tuplestores. Yay!
The bad:
- Generating them exactly and only for AFTER triggers
- Requiring that the tuplestores both be generated or not at all.
There are real use cases described below where only one would be relevant.
- Generating the tuplestores unconditionally.
The ugly:
- Attaching tuplestore generation to tables rather than callers (triggers, DML, etc.)
The SQL standard says:
<trigger definition> ::=
CREATE TRIGGER <trigger name> <trigger action time> <trigger event>
ON <table name> [ REFERENCING <transition table or variable list> ]
<triggered action>
<trigger action time> ::=
BEFORE
| AFTER
| INSTEAD OF
<trigger event> ::=
INSERT
| DELETE
| UPDATE [ OF <trigger column list> ]
<trigger column list> ::=
<column name list>
<triggered action> ::=
[ FOR EACH { ROW | STATEMENT } ]
[ <triggered when clause> ]
<triggered SQL statement>
<triggered when clause> ::=
WHEN <left paren> <search condition> <right paren>
<triggered SQL statement> ::=
<SQL procedure statement>
| BEGIN ATOMIC { <SQL procedure statement> <semicolon> }... END
<transition table or variable list> ::=
<transition table or variable>...
<transition table or variable> ::=
OLD [ ROW ] [ AS ] <old transition variable name>
| NEW [ ROW ] [ AS ] <new transition variable name>
| OLD TABLE [ AS ] <old transition table name>
| NEW TABLE [ AS ] <new transition table name>
<old transition table name> ::=
<transition table name>
<new transition table name> ::=
<transition table name>
<transition table name> ::=
<identifier>
<old transition variable name> ::=
<correlation name>
<new transition variable name> ::=
<correlation name>
Sorry that was a little verbose, but what it does do is give us what we need at
trigger definition time. I'd say it's pilot error if a trigger
definition says "make these tuplestores" and the trigger body then
does nothing with them, which goes to Robert's point below re:
unconditional overhead.
Some things which I *did* follow from the standard: these new
relations are only allowed within AFTER triggers, but are available
in both AFTER STATEMENT and AFTER ROW triggers. That is, an AFTER
UPDATE ... FOR EACH ROW trigger could use both the OLD and NEW row
variables as well as the delta relations (under whatever names we
pick). That probably won't be used very often, but I can imagine
some cases where it might be useful. I expect that these will
normally be used in FOR EACH STATEMENT triggers.I'm concerned about the performance implications of capturing the
delta relations unconditionally.
Along that same line, we don't always need to capture both the before
tuplestores and the after ones. Two examples of this come to mind:
- BEFORE STATEMENT triggers accessing rows, where there is no after part to use, and
- DML (RETURNING BEFORE, e.g.) which only touches one of them. This
applies both to extant use cases of RETURNING and to planned ones.
I'm sure if I can think of two, there are more.
In summary, I'd like to propose that the tuplestores be generated
separately in general and attached to callers. We can optimize this by
not generating redundant tuplestores.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Kevin Grittner wrote:
TRUNCATE triggers shouldn't have delta relations, that's for
sure, or it'd completely negate the point of TRUNCATE triggers.
I don't think we have any other event, do we? People who get
delta relations for deleting all rows should be using DELETE, I
think.
That sounds reasonable. The only other issue was INSTEAD OF
triggers, but I don't think we need them there, either.
I am not sure about providing delta relations in the FOR EACH ROW
case. For what cases are them useful?
In an accounting application for courts I have seen a case where
each receivable (asset) account had a contra (liability) account of
"uncollected receivables", because until and unless the Clerk of
Courts collected the judgment there was no obligation to pay the
money back out. Any financial transaction had to be in a database
transaction, and not only did all the transaction detail need to
balance to zero, but any receivable detail row needed to be
immediately followed by a balancing contra account row (and vice
versa). A FOR EACH ROW trigger, on seeing one of these rows, could
check for the required balancing row. Now this would only be
solved by the standard feature if both rows were always inserted by
a single statement, which might not always have been the case; so
even with this example I am stretching a bit. But it's close
enough to suggest that there might be legitimate uses.
And there is the fact that the standard seems to want this to be
supported.
In all cases I think NEW and OLD are sufficient. I didn't read
the standard, but if it's saying that in FOR EACH ROW the
new/deleted/changed row should be accessible by way of a delta
relation, [...]
No, it says that you can specify *both* the row variables for the
row under consideration and the tables for the full set of rows
affected by the statement *for the same FOR EACH ROW trigger*.
Now, if you only have delta relations in FOR EACH STATEMENT, then
it seems to me you can optimize obtaining them only when such
triggers are defined; this lets you do away with the reloption
entirely, doesn't it?
That was intended to minimize the situations under which there was
a performance hit where the new delta relations were not needed in
an AFTER trigger. If we only generate these for FOR EACH STATEMENT
triggers, that certainly reduces the need for that, but I'm not
sure it eliminates it -- especially if we're generating tuplestores
for the full rows rather than their TIDs.
It is already generating the tuplestores only if there is an AFTER
trigger for the type of statement being executed, but I agree that
it would be a very rare FOR EACH ROW trigger that actually used it.
Of course, that is one argument for the standard syntax -- we
could test whether any of the trigger definitions for that
operation on that table specified each transition table, and only
generate them if needed based on that.
I noticed that GetCurrentFDWTuplestore() changed its identity
without getting its comment updated. The new name seems awfully
generic, and I don't think it really describes what the function
does. I think it needs more renaminguu
Good point. Will fix that in the next version.
(1) My first impulse was to capture this delta data in the form
of tuplestores of just TIDs, and fetching the tuples themselves
from the heap on reference. In earlier discussions others have
argued for using tuplestores of the actual rows themselves.Can you please supply pointers to such discussion?
That was in a matview-related discussion, so perhaps we should
ignore that for now and discuss what's best for triggers here. If
matviews need something different, the rows could always be
materialized from the TIDs at a later point.
I don't see any point in not just storing TIDs, but perhaps I'm
missing something.
I think there was some question about performance, but I really
have a hard time seeing the performance being significantly worse
with a TID tuplestore for most reasonable uses; and I think the TID
tuplestore could be a lot faster if (for example) you have a table
with a large, TOASTed text or bytea column which is not referenced
(in selection criteria or the SET clause) and is not used in the
FOR EACH STATEMENT trigger.
I think there might also have been some question about visibility.
A TID tuplestore would need to use a different snapshot (like maybe
SnapshotAny) in the same query where it joined to other tables with
a normal MVCC snapshot.
(2) Do we want to just pick names for these in the PLs rather
than using the standard syntax? Implementing the standard
syntax seemed to require three new (unreserved) keywords,
changes to the catalogs to store the chosen relations names, and
some way to tie the specified relation names in to the various
PLs.I think the only one for which we have a compulsion to follow
someone in this area would be PL/pgSQL,
... which currently hard-codes the row variable names rather than
using standard syntax to specify them.
which probably needs to follow PL/SQL's lead if there is one.
I don't believe we can create PL/SQL trigger functions, can we?
Other than that I don't think we need to do anything standard.
We don't (yet) have PL/PSM which would need to have the
standard-mandated syntax.
The stated reason for not specifying the row variable names in the
CREATE TRIGGER statement is the difficulty of tying that in to all
the supported PLs. I couldn't see any reason for that to be easier
for the tables than the row variables. Of course, if we intend to
support PL/PSM and we want to use standard CREATE TRIGGER syntax
for that (rather than having trigger functions in that language act
as though some particular name was specified) it seems better to me
that we implement it now. But I don't really see why we would need
to do that to have a conforming PL/PSM implementation.
The way I have gone here just adds two new fields to the
TriggerData structure and leaves it to each PL how to deal with
that. Failure to do anything in a PL just leaves it at the
status quo with no real harm done -- it just won't have the new
delta relations available.It seems to me that plpgsql support is mandatory, but other PLs
can add support as interested parties weigh in with patches. As
with event triggers, I don't feel the author of the main feature
is responsible for patching all PLs.
I like that point of view!
Are you intentionally omitting the corresponding
sql-createtrigger patch?
Yes, because that depends on decisions made about whether to use
standard syntax and (if not) a round of bikeshedding about what
fixed names to use in plpgsql.
Thanks for the feedback!
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> wrote:
Kevin Grittner <kgrittn@ymail.com> wrote:
Can you describe what the standard syntax for the row variables
is, as opposed to our syntax? Also, what's the standard syntax
for the the transition relations?
If you want either (or both), the standard has you using a
REFERENCING clause right before the FOR EACH part of the statement.
You can specify one or more sections in the format:
{ OLD | NEW } [ ROW | TABLE ] [ AS ] name
If you have more than one, they are separated by whitespace. (No
commas or other visible delimiters.) If you omit ROW/TABLE it
defaults to ROW. You are only allowed to specify TABLE on an AFTER
trigger. You are only allowed to specify ROW on a FOR EACH ROW
trigger. (There is no prohibition against specifying TABLE on a
FOR EACH ROW trigger.) You are only allowed to specify OLD for a
DELETE or UPDATE trigger. (The ability for one trigger definition
to specify multiple operations is a PostgreSQL extension.) You are
only allowed to specify NEW for an UPDATE or INSERT trigger. You
may not repeat an entry.
Essentially, we have an implied clause on every FOR EACH ROW
trigger like:
REFERENCING OLD ROW AS OLD NEW ROW AS NEW
Some things which I *did* follow from the standard: these new
relations are only allowed within AFTER triggers, but are
available in both AFTER STATEMENT and AFTER ROW triggers. That
is, an AFTER UPDATE ... FOR EACH ROW trigger could use both the
OLD and NEW row variables as well as the delta relations (under
whatever names we pick). That probably won't be used very
often, but I can imagine some cases where it might be useful. I
expect that these will normally be used in FOR EACH STATEMENT
triggers.I'm concerned about the performance implications of capturing the
delta relations unconditionally. If a particular trigger
actually needs the delta relations, then the time spent capturing
that information is well spent; but if it doesn't, then it's a
waste. There are lots of people using FOR EACH STATEMENT
triggers right now who won't be happy if those start requiring
O(n) additional temporary storage, where n is the number of rows
modified by the statement. This is likely an even bigger issue
for per-row triggers, of course, where as you say, it probably
won't be used often.
That is why I added a reloption which must be specifically enabled
for a table in order to generate these deltas. That would be an
inconvenience for those wanting to use the new feature, but should
prevent a performance regression for any tables where it is not
specifically turned on. That's not perfect, of course, because if
you turn it on for an UPDATE ... AFTER EACH STATEMENT trigger where
you want it, you do suffer the overhead on every AFTER trigger on
that table. Perhaps this is sufficient reason to use the standard
syntax for the new delta tables -- the would then be generated only
in the specific cases where they were needed. And I think I could
lose the reloption.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Fetter <david@fetter.org> wrote:
Robert Haas wrote:
Kevin Grittner <kgrittn@ymail.com> wrote:
The good:
- Generating the tuplestores. Yay!
Thanks for that. ;-)
The bad:
- Generating them exactly and only for AFTER triggers
The standard only allows them for AFTER triggers, and I'm not sure
what the semantics would be for any others.
- Requiring that the tuplestores both be generated or not at
all. There are real use cases described below where only
one would be relevant.
Yeah.
- Generating the tuplestores unconditionally.
Well, there are conditions. Only when the reloption allows and
only if there is an AFTER trigger for the type of operation in
progress.
The ugly:
- Attaching tuplestore generation to tables rather than
callers (triggers, DML, etc.)
I'm not sure what you're getting at here. This patch is
specifically only concerned with generating delta relations for DML
AFTER triggers, although my hope is that it will be a basis for
delta relations used for other purposes. This seems to me like the
right place to initially capture the data for incremental
maintenance of materialized views, and might be of value for other
purposes, too.
[formal definition of standard CREATE TRIGGER statement]
Sorry that was a little verbose, but what it does do is give us
what we need at trigger definition time. I'd say it's pilot
error if a trigger definition says "make these tuplestores" and
the trigger body then does nothing with them, which goes to
Robert's point below re: unconditional overhead.
Yeah, the more I think about it (and discuss it) the more I'm
inclined to suffer the additional complexity of the standard syntax
for specifying transition relations in order to avoid unnecessary
overhead creating them when not needed. I'm also leaning toward
just storing TIDs in the tuplestores, even though it requires mixed
snapshots in executing queries in the triggers. It just seems like
there will otherwise be to much overhead in copying around big,
unreferenced columns for some situations.
Along that same line, we don't always need to capture both the
before tuplestores and the after ones. Two examples of this come
to mind:- BEFORE STATEMENT triggers accessing rows, where there is no
after part to use,
Are you talking about an UPDATE for which the AFTER trigger(s) only
reference the before transition table, and don't look at AFTER? If
so, using the standard syntax would cover that just fine. If not,
can you elaborate?
and
- DML (RETURNING BEFORE, e.g.) which only touches one of them.
This applies both to extant use cases of RETURNING and to planned
ones.
I think that can be sorted out by a patch which implements that, if
these deltas even turn out to be the appropriate way to get that
data (which is not clear to me at this time). Assuming standard
syntax, the first thing would be for the statement to somehow
communicate to the trigger layer the need to capture a tuplestore
it might otherwise not generate, and there would need to be a way
for the statement to access the needed tuplestore(s). The
statement would also need to project the right set of columns.
None of that seems to me to be relevant to this patch. If this
patch turns out to provide infrastructure that helps, great. If
you have a specific suggestion about how to make the tuplestores
more accessible to other layers, I'm listening.
In summary, I'd like to propose that the tuplestores be generated
separately in general and attached to callers. We can optimize
this by not generating redundant tuplestores.
Well, if we use the standard syntax for CREATE TRIGGER and store
the transition table names (if any) in pg_trigger, the code can
generate one relation if any AFTER triggers which are going to fire
need it. I don't see any point in generating exactly the same
tuplestore contents for each trigger. And suspect that we can wire
in any other uses later when we have something to connect them to.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 18, 2014 at 03:30:34PM -0700, Kevin Grittner wrote:
David Fetter <david@fetter.org> wrote:
Robert Haas wrote:
Kevin Grittner <kgrittn@ymail.com> wrote:
The good:
���� - Generating the tuplestores.� Yay!Thanks for that.� ;-)
Sorry, I just can't resist references to Spaghetti Westerns.
https://en.wikipedia.org/wiki/The_Good,_the_Bad_and_the_Ugly
The bad:
���� - Generating them exactly and only for AFTER triggersThe standard only allows them for AFTER triggers, and I'm not sure
what the semantics would be for any others.
As, so here's where we differ. You're looking at deltas, a very nice
capability to have. I'm looking at the before and after tuplestores
as components of which deltas, among many other things, could be
composed.
���� - Requiring that the tuplestores both be generated or not at
������ all.� There are real use cases described below where only
������ one would be relevant.Yeah.
���� - Generating the tuplestores unconditionally.
Well, there are conditions.� Only when the reloption allows and
only if there is an AFTER trigger for the type of operation in
progress.
For deltas, this is just the thing.
I'm vaguely picturing the following as infrastructure:
- Instead of modifying Rel, we modify Query to contain two more bools
default false: hasBeforeTuplestore and hasAfterTuplestore
- Each use case we implement would set 0 or more of these to true.
For the delta use case, appropriate trigger definitions would set
both.
This is vague because I haven't really gotten hacking on it, just
exploring what I hope are the relevant parts of the code.
The ugly:
���� - Attaching tuplestore generation to tables rather than������� callers (triggers, DML, etc.)
I'm not sure what you're getting at here.� This patch is
specifically only concerned with generating delta relations for DML
AFTER triggers, although my hope is that it will be a basis for
delta relations used for other purposes.� This seems to me like the
right place to initially capture the data for incremental
maintenance of materialized views, and might be of value for other
purposes, too.
Hrm. I don't really see this stuff as table properties. The
materialized view case is an obvious example where the matview, not
the relations underneath, wants this information. The relations
underneath may have their own concerns, but it's the matview whose
existence should ensure that the tuplestores are being generated.
Once the last depending-on-one-of-the-tuplestores things is gone, and
this could simply be the end of a RETURNING query, the tuplestores go
away.
[formal definition of standard CREATE TRIGGER statement]
Sorry that was a little verbose, but what it does do is give us
what we need at trigger definition time.� I'd say it's pilot
error if a trigger definition says "make these tuplestores" and
the trigger body then does nothing with them, which goes to
Robert's point below re: unconditional overhead.Yeah, the more I think about it (and discuss it) the more I'm
inclined to suffer the additional complexity of the standard syntax
for specifying transition relations in order to avoid unnecessary
overhead creating them when not needed.� I'm also leaning toward
just storing TIDs in the tuplestores, even though it requires mixed
snapshots in executing queries in the triggers.
So in this case one tuplestore with two TIDs, either of which might be
NULL?
just seems like there will otherwise be to much overhead in copying
around big, unreferenced columns for some situations.
Yeah, it'd be nice to have the minimal part be as slim as possible.
Along that same line, we don't always need to capture both the
before tuplestores and the after ones.� Two examples of this come
to mind:- BEFORE STATEMENT triggers accessing rows, where there is no
after part to use,Are you talking about an UPDATE for which the AFTER trigger(s) only
reference the before transition table, and don't look at AFTER?If
so, using the standard syntax would cover that just fine.� If not,
can you elaborate?
Sorry I was unclear. I was looking at one of the many things having
these tuplestores around could enable. As things stand now, there is
no access of any kind to rows with any per-statement trigger, modulo
user-space hacks like this one:
Having the "before" tuplestore available to a BEFORE STATEMENT trigger
would make it possible to do things with the before transition table
that are fragile and hacky now.
and
- DML (RETURNING BEFORE, e.g.) which only touches one of them.
This applies both to extant use cases of RETURNING and to planned
ones.I think that can be sorted out by a patch which implements that, if
these deltas even turn out to be the appropriate way to get that
data (which is not clear to me at this time).
Again, I see the tuplestores as infrastructure both deltas and many
other things, so long as they're attached to the right objects. In my
opinion, the right objects would include materialized views, triggers,
and certain very specific kinds of DML of which all the RETURNING ones
are one example. They would not include the underlying tables.
standard
syntax, the first thing would be for the statement to somehow
communicate to the trigger layer the need to capture a tuplestore
it might otherwise not generate, and there would need to be a way
for the statement to access the needed tuplestore(s).
Right. Hence my proposal to make the existence of the tuplestores
part of Query, writeable by the types of triggers which specify that
they'll be needed.
The statement would also need to project the right set of columns.
None of that seems to me to be relevant to this patch.� If this
patch turns out to provide infrastructure that helps, great.� If you
have a specific suggestion about how to make the tuplestores more
accessible to other layers, I'm listening.
See above :)
In summary, I'd like to propose that the tuplestores be generated
separately in general and attached to callers. We can optimize
this by not generating redundant tuplestores.Well, if we use the standard syntax for CREATE TRIGGER and store
the transition table names (if any) in pg_trigger, the code can
generate one relation if any AFTER triggers which are going to fire
need it.� I don't see any point in generating exactly the same
tuplestore contents for each trigger.� And suspect that we can wire
in any other uses later when we have something to connect them to.
Yes. I just don't think that Rel is the place to connect them.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Fetter <david@fetter.org> wrote:
On Wed, Jun 18, 2014 at 03:30:34PM -0700, Kevin Grittner wrote:
the more I think about it (and discuss it) the more I'm
inclined to suffer the additional complexity of the standard
syntax for specifying transition relations in order to avoid
unnecessary overhead creating them when not needed. I'm also
leaning toward just storing TIDs in the tuplestores, even though
it requires mixed snapshots in executing queries in the
triggers.So in this case one tuplestore with two TIDs, either of which
might be NULL?
No, one or two tuplestores, depending on need, each with TIDs of
either the "before" set or the "after" set of all tuples affected
by the DML statement, however many that may be. More or less like
this first draft patch, except with TIDs instead of copies of the
rows, and with better selectivity about when the tuplestores are
generated.
Having the "before" tuplestore available to a BEFORE STATEMENT
trigger would make it possible to do things with the before
transition table that are fragile and hacky now.
How do you propose to have an accurate "before" tuplestore of
affected rows before the scan runs and before the BEFORE ... FOR
EACH ROW triggers fire? That would be particularly interesting to
try to generate if the scan involves evaluating any VOLATILE
functions.
Again, I see the tuplestores as infrastructure both deltas and
many other things, so long as they're attached to the right
objects. In my opinion, the right objects would include
materialized views, triggers, and certain very specific kinds of
DML of which all the RETURNING ones are one example. They would
not include the underlying tables.
Right now I've presented something for capturing the information
and allowing it to be accessed from triggers. I don't think the
means of capturing it precludes passing it along to other
consumers. I would like to get this part working before trying to
wire it up to anything other than triggers. The best way to kill
an effort like this is to allow scope creep. Now, if you think
that something fundamentally belongs at another level, that's
something to address -- but the point where we capture data to pass
to triggers seems like a natural and sound place to capture it for
other purposes. And since almost all the code for this patch is in
trigger.c, this seems like I'm in the right place for a trigger
feature.
standard syntax, the first thing would be for the statement to
somehow communicate to the trigger layer the need to capture a
tuplestore it might otherwise not generate, and there would need
to be a way for the statement to access the needed
tuplestore(s).Right. Hence my proposal to make the existence of the
tuplestores part of Query, writeable by the types of triggers
which specify that they'll be needed.
I just don't think that Rel is the place to connect them.
I don't know what you mean by that. I've already said that I now
think we should use the standard CREATE TRIGGER syntax to specify
the transition tables, and that if we do that we don't need the
reloption that's in the patch. If you ignore the 19 lines of new
code to add that reloption, absolutely 100% of the code changes in
the patch so far are in trigger.c and trigger.h. That reloption
was never anything I would consider as *connecting* the tuplestores
to the Rel anyway -- it was simply an attempt to minimize
unnecessary work. No matter how I try, I'm not seeing what you
mean by references to "connecting to Rel".
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@ymail.com> wrote:
I've already said that I now think we should use the standard
CREATE TRIGGER syntax to specify the transition tables, and that
if we do that we don't need the reloption that's in the patch.
If you ignore the 19 lines of new code to add that reloption,
absolutely 100% of the code changes in the patch so far are in
trigger.c and trigger.h.
Although nobody has actually framed their feedback as a review, I
feel that I have enough to work with to throw the patch into
Waiting on Author status. Since I started with the assumption that
I was going to be using standard syntax and got a ways into that
before convincing myself it was a bad idea, I should have a new
version of the patch working that way in a couple days.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@ymail.com> wrote:
Kevin Grittner <kgrittn@ymail.com> wrote:
I've already said that I now think we should use the standard
CREATE TRIGGER syntax to specify the transition tables, and that
if we do that we don't need the reloption that's in the patch.
If you ignore the 19 lines of new code to add that reloption,
absolutely 100% of the code changes in the patch so far are in
trigger.c and trigger.h.Although nobody has actually framed their feedback as a review, I
feel that I have enough to work with to throw the patch into
Waiting on Author status. Since I started with the assumption
that I was going to be using standard syntax and got a ways into
that before convincing myself it was a bad idea, I should have a
new version of the patch working that way in a couple days.
Here is v2.
This implements the standard syntax for transition tables, but
leaves transition row values alone. (They remain as OLD and NEW in
plpgsql, for example.) I took a first pass at the documentation
changes; I hope they are fairly readable. I didn't create new
regression tests yet, since those will be a lot more interesting
when there is a PL to use with this. That does mean there's not a
lot to test yet. You can create triggers with transition tables
specified, and they should show correctly in psql and behave
correctly in pg_dump.
I think the new columns in pg_trigger should be null capable (which
currently means moving them to the variable part of the record).
This seems like it is better done once plpgsql support is there and
we have regression tests, since techniques for that seem a little
fragile.
I didn't change the tuplestores to TID because it seemed to me that
it would preclude using transition relations with FDW triggers, and
it seemed bad not to support that. Does anyone see a way around
that, or feel that it's OK to not support FDW triggers in this
regard?
Does this look good otherwise, as far as it goes?
Unless people want the tuplestores changed to hold TIDs of the
tuples rather than the tuples themselves, or there are other
problems with the generation of the tuplestores, I think this is
done as far as capturing the data and passing it to the triggers.
I don't think any of the existing execution node types quite fit
this, although there are some which are similar enough to crib most
of the code from. Have I missed a node type that can be bent to
this purpose?
What I'm looking for in this CF is to confirm the approach for
capturing the data, and get any suggestions people want to offer on
the PL implementations to use it -- at which point I think it can
be Returned with Feedback to be finished after this CF. Now is a
great time to tell me what I've done wrong in the work so far, or
make suggestions for the next phase. :-)
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
after-trigger-delta-relations-v2.patchtext/x-diff; name=after-trigger-delta-relations-v2.patchDownload
*** a/doc/src/sgml/ref/create_trigger.sgml
--- b/doc/src/sgml/ref/create_trigger.sgml
***************
*** 25,30 **** CREATE [ CONSTRAINT ] TRIGGER <replaceable class="PARAMETER">name</replaceable>
--- 25,31 ----
ON <replaceable class="PARAMETER">table_name</replaceable>
[ FROM <replaceable class="parameter">referenced_table_name</replaceable> ]
[ NOT DEFERRABLE | [ DEFERRABLE ] { INITIALLY IMMEDIATE | INITIALLY DEFERRED } ]
+ [ REFERENCING { { OLD | NEW } TABLE [ AS ] <replaceable class="PARAMETER">transition_relation_name</replaceable> } [ ... ] ]
[ FOR [ EACH ] { ROW | STATEMENT } ]
[ WHEN ( <replaceable class="parameter">condition</replaceable> ) ]
EXECUTE PROCEDURE <replaceable class="PARAMETER">function_name</replaceable> ( <replaceable class="PARAMETER">arguments</replaceable> )
***************
*** 175,180 **** CREATE [ CONSTRAINT ] TRIGGER <replaceable class="PARAMETER">name</replaceable>
--- 176,190 ----
</para>
<para>
+ The <literal>REFERENCING</> option is only allowed for an <literal>AFTER</>
+ trigger which is not a constraint trigger. <literal>OLD TABLE</> may only
+ be specified once, and only on a trigger which can fire on
+ <literal>UPDATE</> or <literal>DELETE</>. <literal>NEW TABLE</> may only
+ be specified once, and only on a trigger which can fire on
+ <literal>UPDATE</> or <literal>INSERT</>.
+ </para>
+
+ <para>
<command>SELECT</command> does not modify any rows so you cannot
create <command>SELECT</command> triggers. Rules and views are more
appropriate in such cases.
***************
*** 279,284 **** UPDATE OF <replaceable>column_name1</replaceable> [, <replaceable>column_name2</
--- 289,328 ----
</varlistentry>
<varlistentry>
+ <term><literal>REFERENCING</literal></term>
+ <listitem>
+ <para>
+ This immediately preceeds the declaration of one or two relations which
+ can be used to read the before and/or after images of all rows directly
+ affected by the triggering statement. An <literal>AFTER EACH ROW</>
+ trigger is allowed to use both these transition relation names and the
+ row names (<literal>OLD</> and <literal>NEW</>) which reference each
+ individual row for which the trigger fires.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><literal>OLD TABLE</literal></term>
+ <term><literal>NEW TABLE</literal></term>
+ <listitem>
+ <para>
+ This specifies whether the named relation contains the before or after
+ images for rows affected by the statement which fired the triggered.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><replaceable class="PARAMETER">transition_relation_name</replaceable></term>
+ <listitem>
+ <para>
+ The (unqualified) name to be used within the trigger for this relation.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>FOR EACH ROW</literal></term>
<term><literal>FOR EACH STATEMENT</literal></term>
***************
*** 471,476 **** CREATE TRIGGER view_insert
--- 515,544 ----
FOR EACH ROW
EXECUTE PROCEDURE view_insert_row();
</programlisting>
+
+ Execute the function <function>check_transfer_balances_to_zero</> for each
+ statement to confirm that the <literal>transfer</> rows offset to a net of
+ zero:
+
+ <programlisting>
+ CREATE TRIGGER transfer_insert
+ AFTER INSERT ON transfer
+ FOR EACH STATEMENT
+ REFERENCING NEW TABLE AS inserted
+ EXECUTE PROCEDURE check_transfer_balances_to_zero();
+ </programlisting>
+
+ Execute the function <function>check_matching_pairs</> for each row to
+ confirm that changes are made to matching pairs at the same time (by the
+ same statement):
+
+ <programlisting>
+ CREATE TRIGGER paired_items_update
+ AFTER UPDATE ON paired_items
+ FOR EACH ROW
+ REFERENCING NEW TABLE AS newtab OLD TABLE AS oldtab
+ EXECUTE PROCEDURE check_matching_pairs();
+ </programlisting>
</para>
<para>
***************
*** 499,522 **** CREATE TRIGGER view_insert
<itemizedlist>
<listitem>
<para>
! SQL allows you to define aliases for the <quote>old</quote>
! and <quote>new</quote> rows or tables for use in the definition
! of the triggered action (e.g., <literal>CREATE TRIGGER ... ON
! tablename REFERENCING OLD ROW AS somename NEW ROW AS othername
! ...</literal>). Since <productname>PostgreSQL</productname>
! allows trigger procedures to be written in any number of
! user-defined languages, access to the data is handled in a
! language-specific way.
! </para>
! </listitem>
!
! <listitem>
! <para>
! <productname>PostgreSQL</productname> does not allow the old and new
! tables to be referenced in statement-level triggers, i.e., the tables
! that contain all the old and/or new rows, which are referred to by the
! <literal>OLD TABLE</literal> and <literal>NEW TABLE</literal> clauses in
! the <acronym>SQL</> standard.
</para>
</listitem>
--- 567,580 ----
<itemizedlist>
<listitem>
<para>
! While transition tables for <literal>AFTER</> triggers are specified
! using the <literal>REFERENCING</> clause in the standard way, the row
! variables used in <literal>FOR EACH ROW</> triggers may not be
! specified in <literal>REFERENCING</> clause. They are available in a
! manner which is dependent on the language in which the trigger function
! is written. Some languages effectively behave as though there is a
! <literal>REFERENCING</> clause containing <literal>OLD ROW AS OLD NEW
! ROW AS NEW</>.
</para>
</listitem>
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 7020,7026 **** validateForeignKeyConstraint(char *conname,
trig.tgconstraint = constraintOid;
trig.tgdeferrable = FALSE;
trig.tginitdeferred = FALSE;
! /* we needn't fill in tgargs or tgqual */
/*
* See if we can do it with a single LEFT JOIN query. A FALSE result
--- 7020,7026 ----
trig.tgconstraint = constraintOid;
trig.tgdeferrable = FALSE;
trig.tginitdeferred = FALSE;
! /* we needn't fill in remaining fields */
/*
* See if we can do it with a single LEFT JOIN query. A FALSE result
***************
*** 7104,7109 **** CreateFKCheckTrigger(Oid myRelOid, Oid refRelOid, Constraint *fkconstraint,
--- 7104,7110 ----
}
fk_trigger->columns = NIL;
+ fk_trigger->transitionRels = NIL;
fk_trigger->whenClause = NULL;
fk_trigger->isconstraint = true;
fk_trigger->deferrable = fkconstraint->deferrable;
***************
*** 7144,7149 **** createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint,
--- 7145,7151 ----
fk_trigger->timing = TRIGGER_TYPE_AFTER;
fk_trigger->events = TRIGGER_TYPE_DELETE;
fk_trigger->columns = NIL;
+ fk_trigger->transitionRels = NIL;
fk_trigger->whenClause = NULL;
fk_trigger->isconstraint = true;
fk_trigger->constrrel = NULL;
***************
*** 7198,7203 **** createForeignKeyTriggers(Relation rel, Oid refRelOid, Constraint *fkconstraint,
--- 7200,7206 ----
fk_trigger->timing = TRIGGER_TYPE_AFTER;
fk_trigger->events = TRIGGER_TYPE_UPDATE;
fk_trigger->columns = NIL;
+ fk_trigger->transitionRels = NIL;
fk_trigger->whenClause = NULL;
fk_trigger->isconstraint = true;
fk_trigger->constrrel = NULL;
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
***************
*** 155,160 **** CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
--- 155,162 ----
Oid constrrelid = InvalidOid;
ObjectAddress myself,
referenced;
+ char *oldtablename = NULL;
+ char *newtablename = NULL;
if (OidIsValid(relOid))
rel = heap_open(relOid, AccessExclusiveLock);
***************
*** 301,306 **** CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
--- 303,383 ----
}
/*
+ * We don't yet support naming ROW transition variables, but the parser
+ * recognizes the syntax so we can give a nicer message here.
+ *
+ * Per standard, REFERENCING TABLE names are only allowed on AFTER
+ * triggers. Per standard, REFERENCING ROW names are not allowed with FOR
+ * EACH STATEMENT. Per standard, each OLD/NEW, ROW/TABLE permutation is
+ * only allowed once. Per standard, OLD may not be specified when
+ * creating a trigger only for INSERT, and NEW may not be specified when
+ * creating a trigger only for DELETE.
+ *
+ * Notice that the standard allows an AFTER ... FOR EACH ROW trigger to
+ * reference both ROW and TABLE transition data.
+ */
+ if (stmt->transitionRels != NIL)
+ {
+ List *varList = stmt->transitionRels;
+ ListCell *lc;
+
+ foreach(lc, varList)
+ {
+ TriggerTransition *tt = (TriggerTransition *) lfirst(lc);
+
+ Assert(IsA(tt, TriggerTransition));
+
+ if (!(tt->isTable))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ROW variable naming in the REFERENCING clause is not supported"),
+ errhint("Use OLD TABLE or NEW TABLE for naming transition tables.")));
+
+ /*
+ * Because of the above test, we omit further ROW-related testing
+ * below. If we later allow naming OLD and NEW ROW variables,
+ * adjustments will be needed below.
+ */
+
+ if (stmt->timing != TRIGGER_TYPE_AFTER)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("transition table name can only be specified for an AFTER trigger")));
+
+ if (tt->isNew)
+ {
+ if (!(TRIGGER_FOR_INSERT(tgtype) ||
+ TRIGGER_FOR_UPDATE(tgtype)))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("NEW TABLE can only be specified for an INSERT or UPDATE trigger")));
+
+ if (newtablename != NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("NEW TABLE cannot be specified multiple times")));
+
+ newtablename = tt->name;
+ }
+ else
+ {
+ if (!(TRIGGER_FOR_DELETE(tgtype) ||
+ TRIGGER_FOR_UPDATE(tgtype)))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("OLD TABLE can only be specified for a DELETE or UPDATE trigger")));
+
+ if (oldtablename != NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("OLD TABLE cannot be specified multiple times")));
+
+ oldtablename = tt->name;
+ }
+ }
+ }
+
+ /*
* Parse the WHEN clause, if any
*/
if (stmt->whenClause)
***************
*** 566,571 **** CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
--- 643,660 ----
values[Anum_pg_trigger_tgconstraint - 1] = ObjectIdGetDatum(constraintOid);
values[Anum_pg_trigger_tgdeferrable - 1] = BoolGetDatum(stmt->deferrable);
values[Anum_pg_trigger_tginitdeferred - 1] = BoolGetDatum(stmt->initdeferred);
+ if (oldtablename)
+ values[Anum_pg_trigger_tgoldtable - 1] = DirectFunctionCall1(namein,
+ CStringGetDatum(oldtablename));
+ else
+ values[Anum_pg_trigger_tgoldtable - 1] = DirectFunctionCall1(namein,
+ CStringGetDatum(""));
+ if (newtablename)
+ values[Anum_pg_trigger_tgnewtable - 1] = DirectFunctionCall1(namein,
+ CStringGetDatum(newtablename));
+ else
+ values[Anum_pg_trigger_tgnewtable - 1] = DirectFunctionCall1(namein,
+ CStringGetDatum(""));
if (stmt->args)
{
***************
*** 673,678 **** CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
--- 762,769 ----
heap_close(tgrel, RowExclusiveLock);
pfree(DatumGetPointer(values[Anum_pg_trigger_tgname - 1]));
+ pfree(DatumGetPointer(values[Anum_pg_trigger_tgoldtable - 1]));
+ pfree(DatumGetPointer(values[Anum_pg_trigger_tgnewtable - 1]));
pfree(DatumGetPointer(values[Anum_pg_trigger_tgargs - 1]));
pfree(DatumGetPointer(values[Anum_pg_trigger_tgattr - 1]));
***************
*** 1542,1547 **** RelationBuildTriggers(Relation relation)
--- 1633,1642 ----
build->tgconstraint = pg_trigger->tgconstraint;
build->tgdeferrable = pg_trigger->tgdeferrable;
build->tginitdeferred = pg_trigger->tginitdeferred;
+ build->tgoldtable = DatumGetCString(DirectFunctionCall1(nameout,
+ NameGetDatum(&pg_trigger->tgoldtable)));
+ build->tgnewtable = DatumGetCString(DirectFunctionCall1(nameout,
+ NameGetDatum(&pg_trigger->tgnewtable)));
build->tgnargs = pg_trigger->tgnargs;
/* tgattr is first var-width field, so OK to access directly */
build->tgnattr = pg_trigger->tgattr.dim1;
***************
*** 1670,1675 **** SetTriggerFlags(TriggerDesc *trigdesc, Trigger *trigger)
--- 1765,1783 ----
trigdesc->trig_truncate_after_statement |=
TRIGGER_TYPE_MATCHES(tgtype, TRIGGER_TYPE_STATEMENT,
TRIGGER_TYPE_AFTER, TRIGGER_TYPE_TRUNCATE);
+
+ trigdesc->trig_insert_new_table |=
+ (TRIGGER_FOR_INSERT(tgtype) &&
+ TRIGGER_USES_TRANSITION_TABLE(trigger->tgnewtable)) ? true : false;
+ trigdesc->trig_update_old_table |=
+ (TRIGGER_FOR_UPDATE(tgtype) &&
+ TRIGGER_USES_TRANSITION_TABLE(trigger->tgoldtable)) ? true : false;
+ trigdesc->trig_update_new_table |=
+ (TRIGGER_FOR_UPDATE(tgtype) &&
+ TRIGGER_USES_TRANSITION_TABLE(trigger->tgnewtable)) ? true : false;
+ trigdesc->trig_delete_old_table |=
+ (TRIGGER_FOR_DELETE(tgtype) &&
+ TRIGGER_USES_TRANSITION_TABLE(trigger->tgoldtable)) ? true : false;
}
/*
***************
*** 1698,1703 **** CopyTriggerDesc(TriggerDesc *trigdesc)
--- 1806,1813 ----
for (i = 0; i < trigdesc->numtriggers; i++)
{
trigger->tgname = pstrdup(trigger->tgname);
+ trigger->tgoldtable = pstrdup(trigger->tgoldtable);
+ trigger->tgnewtable = pstrdup(trigger->tgnewtable);
if (trigger->tgnattr > 0)
{
int16 *newattr;
***************
*** 1741,1746 **** FreeTriggerDesc(TriggerDesc *trigdesc)
--- 1851,1858 ----
for (i = 0; i < trigdesc->numtriggers; i++)
{
pfree(trigger->tgname);
+ pfree(trigger->tgoldtable);
+ pfree(trigger->tgnewtable);
if (trigger->tgnattr > 0)
pfree(trigger->tgattr);
if (trigger->tgnargs > 0)
***************
*** 1812,1817 **** equalTriggerDescs(TriggerDesc *trigdesc1, TriggerDesc *trigdesc2)
--- 1924,1933 ----
return false;
if (trig1->tginitdeferred != trig2->tginitdeferred)
return false;
+ if (strcmp(trig1->tgoldtable, trig2->tgoldtable) != 0)
+ return false;
+ if (strcmp(trig1->tgnewtable, trig2->tgnewtable) != 0)
+ return false;
if (trig1->tgnargs != trig2->tgnargs)
return false;
if (trig1->tgnattr != trig2->tgnattr)
***************
*** 2060,2066 **** ExecARInsertTriggers(EState *estate, ResultRelInfo *relinfo,
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc && trigdesc->trig_insert_after_row)
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_INSERT,
true, NULL, trigtuple, recheckIndexes, NULL);
}
--- 2176,2183 ----
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc &&
! (trigdesc->trig_insert_after_row || trigdesc->trig_insert_new_table))
AfterTriggerSaveEvent(estate, relinfo, TRIGGER_EVENT_INSERT,
true, NULL, trigtuple, recheckIndexes, NULL);
}
***************
*** 2263,2269 **** ExecARDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc && trigdesc->trig_delete_after_row)
{
HeapTuple trigtuple;
--- 2380,2387 ----
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc &&
! (trigdesc->trig_delete_after_row || trigdesc->trig_delete_old_table))
{
HeapTuple trigtuple;
***************
*** 2528,2534 **** ExecARUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc && trigdesc->trig_update_after_row)
{
HeapTuple trigtuple;
--- 2646,2653 ----
{
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
! if (trigdesc && (trigdesc->trig_update_after_row ||
! trigdesc->trig_update_old_table || trigdesc->trig_update_new_table))
{
HeapTuple trigtuple;
***************
*** 3160,3167 **** typedef struct AfterTriggerEventList
* fdw_tuplestores[query_depth] is a tuplestore containing the foreign tuples
* needed for the current query.
*
! * maxquerydepth is just the allocated length of query_stack and
! * fdw_tuplestores.
*
* state_stack is a stack of pointers to saved copies of the SET CONSTRAINTS
* state data; each subtransaction level that modifies that state first
--- 3279,3289 ----
* fdw_tuplestores[query_depth] is a tuplestore containing the foreign tuples
* needed for the current query.
*
! * old_tuplestores[query_depth] and new_tuplestores[query_depth] hold the
! * delta relations for the current query.
! *
! * maxquerydepth is just the allocated length of query_stack and the
! * tuplestores.
*
* state_stack is a stack of pointers to saved copies of the SET CONSTRAINTS
* state data; each subtransaction level that modifies that state first
***************
*** 3190,3196 **** typedef struct AfterTriggersData
AfterTriggerEventList events; /* deferred-event list */
int query_depth; /* current query list index */
AfterTriggerEventList *query_stack; /* events pending from each query */
! Tuplestorestate **fdw_tuplestores; /* foreign tuples from each query */
int maxquerydepth; /* allocated len of above array */
MemoryContext event_cxt; /* memory context for events, if any */
--- 3312,3320 ----
AfterTriggerEventList events; /* deferred-event list */
int query_depth; /* current query list index */
AfterTriggerEventList *query_stack; /* events pending from each query */
! Tuplestorestate **fdw_tuplestores; /* foreign tuples for one row from each query */
! Tuplestorestate **old_tuplestores; /* all old tuples from each query */
! Tuplestorestate **new_tuplestores; /* all new tuples from each query */
int maxquerydepth; /* allocated len of above array */
MemoryContext event_cxt; /* memory context for events, if any */
***************
*** 3221,3234 **** static SetConstraintState SetConstraintStateAddItem(SetConstraintState state,
/*
! * Gets the current query fdw tuplestore and initializes it if necessary
*/
static Tuplestorestate *
! GetCurrentFDWTuplestore()
{
Tuplestorestate *ret;
! ret = afterTriggers->fdw_tuplestores[afterTriggers->query_depth];
if (ret == NULL)
{
MemoryContext oldcxt;
--- 3345,3358 ----
/*
! * Gets a current query delta tuplestore and initializes it if necessary
*/
static Tuplestorestate *
! GetCurrentTriggerDeltaTuplestore(Tuplestorestate **tss)
{
Tuplestorestate *ret;
! ret = tss[afterTriggers->query_depth];
if (ret == NULL)
{
MemoryContext oldcxt;
***************
*** 3255,3261 **** GetCurrentFDWTuplestore()
CurrentResourceOwner = saveResourceOwner;
MemoryContextSwitchTo(oldcxt);
! afterTriggers->fdw_tuplestores[afterTriggers->query_depth] = ret;
}
return ret;
--- 3379,3385 ----
CurrentResourceOwner = saveResourceOwner;
MemoryContextSwitchTo(oldcxt);
! tss[afterTriggers->query_depth] = ret;
}
return ret;
***************
*** 3552,3558 **** AfterTriggerExecute(AfterTriggerEvent event,
{
case AFTER_TRIGGER_FDW_FETCH:
{
! Tuplestorestate *fdw_tuplestore = GetCurrentFDWTuplestore();
if (!tuplestore_gettupleslot(fdw_tuplestore, true, false,
trig_tuple_slot1))
--- 3676,3684 ----
{
case AFTER_TRIGGER_FDW_FETCH:
{
! Tuplestorestate *fdw_tuplestore =
! GetCurrentTriggerDeltaTuplestore
! (afterTriggers->fdw_tuplestores);
if (!tuplestore_gettupleslot(fdw_tuplestore, true, false,
trig_tuple_slot1))
***************
*** 3622,3627 **** AfterTriggerExecute(AfterTriggerEvent event,
--- 3748,3767 ----
}
/*
+ * Set up the tuplestore information.
+ */
+ if (trigdesc->trig_delete_old_table || trigdesc->trig_update_old_table)
+ LocTriggerData.tg_olddelta =
+ GetCurrentTriggerDeltaTuplestore(afterTriggers->old_tuplestores);
+ else
+ LocTriggerData.tg_olddelta = NULL;
+ if (trigdesc->trig_insert_new_table || trigdesc->trig_update_new_table)
+ LocTriggerData.tg_newdelta =
+ GetCurrentTriggerDeltaTuplestore(afterTriggers->new_tuplestores);
+ else
+ LocTriggerData.tg_newdelta = NULL;
+
+ /*
* Setup the remaining trigger information
*/
LocTriggerData.type = T_TriggerData;
***************
*** 3921,3926 **** AfterTriggerBeginXact(void)
--- 4061,4072 ----
afterTriggers->fdw_tuplestores = (Tuplestorestate **)
MemoryContextAllocZero(TopTransactionContext,
8 * sizeof(Tuplestorestate *));
+ afterTriggers->old_tuplestores = (Tuplestorestate **)
+ MemoryContextAllocZero(TopTransactionContext,
+ 8 * sizeof(Tuplestorestate *));
+ afterTriggers->new_tuplestores = (Tuplestorestate **)
+ MemoryContextAllocZero(TopTransactionContext,
+ 8 * sizeof(Tuplestorestate *));
afterTriggers->maxquerydepth = 8;
/* Context for events is created only when needed */
***************
*** 3970,3978 **** AfterTriggerBeginQuery(void)
--- 4116,4134 ----
afterTriggers->fdw_tuplestores = (Tuplestorestate **)
repalloc(afterTriggers->fdw_tuplestores,
new_alloc * sizeof(Tuplestorestate *));
+ afterTriggers->old_tuplestores = (Tuplestorestate **)
+ repalloc(afterTriggers->old_tuplestores,
+ new_alloc * sizeof(Tuplestorestate *));
+ afterTriggers->new_tuplestores = (Tuplestorestate **)
+ repalloc(afterTriggers->new_tuplestores,
+ new_alloc * sizeof(Tuplestorestate *));
/* Clear newly-allocated slots for subsequent lazy initialization. */
memset(afterTriggers->fdw_tuplestores + old_alloc,
0, (new_alloc - old_alloc) * sizeof(Tuplestorestate *));
+ memset(afterTriggers->old_tuplestores + old_alloc,
+ 0, (new_alloc - old_alloc) * sizeof(Tuplestorestate *));
+ memset(afterTriggers->new_tuplestores + old_alloc,
+ 0, (new_alloc - old_alloc) * sizeof(Tuplestorestate *));
afterTriggers->maxquerydepth = new_alloc;
}
***************
*** 4001,4006 **** AfterTriggerEndQuery(EState *estate)
--- 4157,4164 ----
{
AfterTriggerEventList *events;
Tuplestorestate *fdw_tuplestore;
+ Tuplestorestate *old_tuplestore;
+ Tuplestorestate *new_tuplestore;
/* Must be inside a transaction */
Assert(afterTriggers != NULL);
***************
*** 4045,4057 **** AfterTriggerEndQuery(EState *estate)
break;
}
! /* Release query-local storage for events, including tuplestore if any */
fdw_tuplestore = afterTriggers->fdw_tuplestores[afterTriggers->query_depth];
if (fdw_tuplestore)
{
tuplestore_end(fdw_tuplestore);
afterTriggers->fdw_tuplestores[afterTriggers->query_depth] = NULL;
}
afterTriggerFreeEventList(&afterTriggers->query_stack[afterTriggers->query_depth]);
afterTriggers->query_depth--;
--- 4203,4227 ----
break;
}
! /* Release query-local storage for events, including tuplestores, if any */
fdw_tuplestore = afterTriggers->fdw_tuplestores[afterTriggers->query_depth];
if (fdw_tuplestore)
{
tuplestore_end(fdw_tuplestore);
afterTriggers->fdw_tuplestores[afterTriggers->query_depth] = NULL;
}
+ old_tuplestore = afterTriggers->old_tuplestores[afterTriggers->query_depth];
+ if (old_tuplestore)
+ {
+ tuplestore_end(old_tuplestore);
+ afterTriggers->old_tuplestores[afterTriggers->query_depth] = NULL;
+ }
+ new_tuplestore = afterTriggers->new_tuplestores[afterTriggers->query_depth];
+ if (new_tuplestore)
+ {
+ tuplestore_end(new_tuplestore);
+ afterTriggers->new_tuplestores[afterTriggers->query_depth] = NULL;
+ }
afterTriggerFreeEventList(&afterTriggers->query_stack[afterTriggers->query_depth]);
afterTriggers->query_depth--;
***************
*** 4283,4288 **** AfterTriggerEndSubXact(bool isCommit)
--- 4453,4470 ----
tuplestore_end(ts);
afterTriggers->fdw_tuplestores[afterTriggers->query_depth] = NULL;
}
+ ts = afterTriggers->old_tuplestores[afterTriggers->query_depth];
+ if (ts)
+ {
+ tuplestore_end(ts);
+ afterTriggers->old_tuplestores[afterTriggers->query_depth] = NULL;
+ }
+ ts = afterTriggers->new_tuplestores[afterTriggers->query_depth];
+ if (ts)
+ {
+ tuplestore_end(ts);
+ afterTriggers->new_tuplestores[afterTriggers->query_depth] = NULL;
+ }
afterTriggerFreeEventList(&afterTriggers->query_stack[afterTriggers->query_depth]);
afterTriggers->query_depth--;
***************
*** 4767,4773 **** AfterTriggerPendingOnRel(Oid relid)
*
* NOTE: this is called whenever there are any triggers associated with
* the event (even if they are disabled). This function decides which
! * triggers actually need to be queued.
* ----------
*/
static void
--- 4949,4962 ----
*
* NOTE: this is called whenever there are any triggers associated with
* the event (even if they are disabled). This function decides which
! * triggers actually need to be queued. It is also called after each row,
! * even if there are no triggers for that event, if there are any AFTER
! * STATEMENT triggers for the statement which use transition tables, so that
! * the delta tuplestores can be built.
! *
! * Delta tuplestores are built now, rather than when events are pulled off
! * of the queue because AFTER ROW triggers are allowed to select from the
! * transition tables for the statement.
* ----------
*/
static void
***************
*** 4797,4802 **** AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
--- 4986,5031 ----
elog(ERROR, "AfterTriggerSaveEvent() called outside of query");
/*
+ * If the relation has AFTER ... FOR EACH ROW triggers, capture rows into
+ * delta tuplestores for this depth.
+ */
+ if (row_trigger)
+ {
+ if ((event == TRIGGER_EVENT_DELETE &&
+ trigdesc->trig_delete_old_table) ||
+ (event == TRIGGER_EVENT_UPDATE &&
+ trigdesc->trig_update_old_table))
+ {
+ Tuplestorestate *old_tuplestore;
+
+ Assert(oldtup != NULL);
+ old_tuplestore =
+ GetCurrentTriggerDeltaTuplestore
+ (afterTriggers->old_tuplestores);
+ tuplestore_puttuple(old_tuplestore, oldtup);
+ }
+ if ((event == TRIGGER_EVENT_INSERT &&
+ trigdesc->trig_insert_new_table) ||
+ (event == TRIGGER_EVENT_UPDATE &&
+ trigdesc->trig_update_new_table))
+ {
+ Tuplestorestate *new_tuplestore;
+
+ Assert(newtup != NULL);
+ new_tuplestore =
+ GetCurrentTriggerDeltaTuplestore
+ (afterTriggers->new_tuplestores);
+ tuplestore_puttuple(new_tuplestore, newtup);
+ }
+
+ /* If deltas are the only reason we're here, return. */
+ if ((event == TRIGGER_EVENT_DELETE && !trigdesc->trig_delete_after_row) ||
+ (event == TRIGGER_EVENT_INSERT && !trigdesc->trig_insert_after_row) ||
+ (event == TRIGGER_EVENT_UPDATE && !trigdesc->trig_update_after_row))
+ return;
+ }
+
+ /*
* Validate the event code and collect the associated tuple CTIDs.
*
* The event code will be used both as a bitmask and an array offset, so
***************
*** 4893,4899 **** AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo,
{
if (fdw_tuplestore == NULL)
{
! fdw_tuplestore = GetCurrentFDWTuplestore();
new_event.ate_flags = AFTER_TRIGGER_FDW_FETCH;
}
else
--- 5122,5130 ----
{
if (fdw_tuplestore == NULL)
{
! fdw_tuplestore =
! GetCurrentTriggerDeltaTuplestore
! (afterTriggers->fdw_tuplestores);
new_event.ate_flags = AFTER_TRIGGER_FDW_FETCH;
}
else
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 2470,2475 **** _copyXmlSerialize(const XmlSerialize *from)
--- 2470,2487 ----
return newnode;
}
+ static TriggerTransition *
+ _copyTriggerTransition(const TriggerTransition *from)
+ {
+ TriggerTransition *newnode = makeNode(TriggerTransition);
+
+ COPY_STRING_FIELD(name);
+ COPY_SCALAR_FIELD(isNew);
+ COPY_SCALAR_FIELD(isTable);
+
+ return newnode;
+ }
+
static Query *
_copyQuery(const Query *from)
{
***************
*** 3582,3587 **** _copyCreateTrigStmt(const CreateTrigStmt *from)
--- 3594,3600 ----
COPY_NODE_FIELD(columns);
COPY_NODE_FIELD(whenClause);
COPY_SCALAR_FIELD(isconstraint);
+ COPY_NODE_FIELD(transitionRels);
COPY_SCALAR_FIELD(deferrable);
COPY_SCALAR_FIELD(initdeferred);
COPY_NODE_FIELD(constrrel);
***************
*** 4649,4654 **** copyObject(const void *from)
--- 4662,4670 ----
case T_XmlSerialize:
retval = _copyXmlSerialize(from);
break;
+ case T_TriggerTransition:
+ retval = _copyTriggerTransition(from);
+ break;
default:
elog(ERROR, "unrecognized node type: %d", (int) nodeTag(from));
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 1781,1786 **** _equalCreateTrigStmt(const CreateTrigStmt *a, const CreateTrigStmt *b)
--- 1781,1787 ----
COMPARE_NODE_FIELD(columns);
COMPARE_NODE_FIELD(whenClause);
COMPARE_SCALAR_FIELD(isconstraint);
+ COMPARE_NODE_FIELD(transitionRels);
COMPARE_SCALAR_FIELD(deferrable);
COMPARE_SCALAR_FIELD(initdeferred);
COMPARE_NODE_FIELD(constrrel);
***************
*** 2413,2418 **** _equalXmlSerialize(const XmlSerialize *a, const XmlSerialize *b)
--- 2414,2429 ----
return true;
}
+ static bool
+ _equalTriggerTransition(const TriggerTransition *a, const TriggerTransition *b)
+ {
+ COMPARE_STRING_FIELD(name);
+ COMPARE_SCALAR_FIELD(isNew);
+ COMPARE_SCALAR_FIELD(isTable);
+
+ return true;
+ }
+
/*
* Stuff from pg_list.h
*/
***************
*** 3115,3120 **** equal(const void *a, const void *b)
--- 3126,3134 ----
case T_XmlSerialize:
retval = _equalXmlSerialize(a, b);
break;
+ case T_TriggerTransition:
+ retval = _equalTriggerTransition(a, b);
+ break;
default:
elog(ERROR, "unrecognized node type: %d",
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 2138,2143 **** _outXmlSerialize(StringInfo str, const XmlSerialize *node)
--- 2138,2153 ----
}
static void
+ _outTriggerTransition(StringInfo str, const TriggerTransition *node)
+ {
+ WRITE_NODE_TYPE("TRIGGERTRANSITION");
+
+ WRITE_STRING_FIELD(name);
+ WRITE_BOOL_FIELD(isNew);
+ WRITE_BOOL_FIELD(isTable);
+ }
+
+ static void
_outColumnDef(StringInfo str, const ColumnDef *node)
{
WRITE_NODE_TYPE("COLUMNDEF");
***************
*** 3236,3241 **** _outNode(StringInfo str, const void *obj)
--- 3246,3254 ----
case T_XmlSerialize:
_outXmlSerialize(str, obj);
break;
+ case T_TriggerTransition:
+ _outTriggerTransition(str, obj);
+ break;
default:
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***************
*** 293,298 **** static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
--- 293,301 ----
%type <list> TriggerEvents TriggerOneEvent
%type <value> TriggerFuncArg
%type <node> TriggerWhen
+ %type <str> TransitionRelName
+ %type <boolean> TransitionRowOrTable TransitionOldOrNew
+ %type <node> TriggerTransition
%type <list> event_trigger_when_list event_trigger_value_list
%type <defelt> event_trigger_when_item
***************
*** 350,355 **** static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
--- 353,359 ----
opt_enum_val_list enum_val_list table_func_column_list
create_generic_options alter_generic_options
relation_expr_list dostmt_opt_list
+ TriggerTransitions TriggerReferencing
%type <list> opt_fdw_options fdw_options
%type <defelt> fdw_option
***************
*** 570,580 **** static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
MAPPING MATCH MATERIALIZED MAXVALUE MINUTE_P MINVALUE MODE MONTH_P MOVE
! NAME_P NAMES NATIONAL NATURAL NCHAR NEXT NO NONE
NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
NULLS_P NUMERIC
! OBJECT_P OF OFF OFFSET OIDS ON ONLY OPERATOR OPTION OPTIONS OR
ORDER ORDINALITY OUT_P OUTER_P OVER OVERLAPS OVERLAY OWNED OWNER
PARSER PARTIAL PARTITION PASSING PASSWORD PLACING PLANS POSITION
--- 574,584 ----
MAPPING MATCH MATERIALIZED MAXVALUE MINUTE_P MINVALUE MODE MONTH_P MOVE
! NAME_P NAMES NATIONAL NATURAL NCHAR NEW NEXT NO NONE
NOT NOTHING NOTIFY NOTNULL NOWAIT NULL_P NULLIF
NULLS_P NUMERIC
! OBJECT_P OF OFF OFFSET OIDS OLD ON ONLY OPERATOR OPTION OPTIONS OR
ORDER ORDINALITY OUT_P OUTER_P OVER OVERLAPS OVERLAY OWNED OWNER
PARSER PARTIAL PARTITION PASSING PASSWORD PLACING PLANS POSITION
***************
*** 583,590 **** static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
QUOTE
! RANGE READ REAL REASSIGN RECHECK RECURSIVE REF REFERENCES REFRESH REINDEX
! RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA
RESET RESTART RESTRICT RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK
ROW ROWS RULE
--- 587,594 ----
QUOTE
! RANGE READ REAL REASSIGN RECHECK RECURSIVE REF REFERENCES REFERENCING
! REFRESH REINDEX RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA
RESET RESTART RESTRICT RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK
ROW ROWS RULE
***************
*** 4347,4365 **** AlterUserMappingStmt: ALTER USER MAPPING FOR auth_ident SERVER name alter_generi
CreateTrigStmt:
CREATE TRIGGER name TriggerActionTime TriggerEvents ON
! qualified_name TriggerForSpec TriggerWhen
EXECUTE PROCEDURE func_name '(' TriggerFuncArgs ')'
{
CreateTrigStmt *n = makeNode(CreateTrigStmt);
n->trigname = $3;
n->relation = $7;
! n->funcname = $12;
! n->args = $14;
! n->row = $8;
n->timing = $4;
n->events = intVal(linitial($5));
n->columns = (List *) lsecond($5);
! n->whenClause = $9;
n->isconstraint = FALSE;
n->deferrable = FALSE;
n->initdeferred = FALSE;
--- 4351,4370 ----
CreateTrigStmt:
CREATE TRIGGER name TriggerActionTime TriggerEvents ON
! qualified_name TriggerReferencing TriggerForSpec TriggerWhen
EXECUTE PROCEDURE func_name '(' TriggerFuncArgs ')'
{
CreateTrigStmt *n = makeNode(CreateTrigStmt);
n->trigname = $3;
n->relation = $7;
! n->funcname = $13;
! n->args = $15;
! n->row = $9;
n->timing = $4;
n->events = intVal(linitial($5));
n->columns = (List *) lsecond($5);
! n->whenClause = $10;
! n->transitionRels = $8;
n->isconstraint = FALSE;
n->deferrable = FALSE;
n->initdeferred = FALSE;
***************
*** 4381,4386 **** CreateTrigStmt:
--- 4386,4392 ----
n->events = intVal(linitial($6));
n->columns = (List *) lsecond($6);
n->whenClause = $14;
+ n->transitionRels = NIL;
n->isconstraint = TRUE;
processCASbits($10, @10, "TRIGGER",
&n->deferrable, &n->initdeferred, NULL,
***************
*** 4433,4438 **** TriggerOneEvent:
--- 4439,4484 ----
{ $$ = list_make2(makeInteger(TRIGGER_TYPE_TRUNCATE), NIL); }
;
+ TriggerReferencing:
+ REFERENCING TriggerTransitions { $$ = $2; }
+ | /*EMPTY*/ { $$ = NIL; }
+ ;
+
+ TriggerTransitions:
+ TriggerTransition { $$ = list_make1($1); }
+ | TriggerTransitions TriggerTransition { $$ = lappend($1, $2); }
+ ;
+
+ TriggerTransition:
+ TransitionOldOrNew TransitionRowOrTable opt_as TransitionRelName
+ {
+ TriggerTransition *n = makeNode(TriggerTransition);
+ n->name = $4;
+ n->isNew = $1;
+ n->isTable = $2;
+ $$ = (Node *)n;
+ }
+ ;
+
+ TransitionOldOrNew:
+ NEW { $$ = TRUE; }
+ | OLD { $$ = FALSE; }
+ ;
+
+ TransitionRowOrTable:
+ TABLE { $$ = TRUE; }
+ /*
+ * Explicit ROW specification is not supported to avoid fully
+ * reserving the keyword ROW. According to the standard, lack of
+ * a keyword here means ROW anyway.
+ */
+ | /*EMPTY*/ { $$ = FALSE; }
+ ;
+
+ TransitionRelName:
+ ColId { $$ = $1; }
+ ;
+
TriggerForSpec:
FOR TriggerForOptEach TriggerForType
{
***************
*** 12978,12983 **** unreserved_keyword:
--- 13024,13030 ----
| MOVE
| NAME_P
| NAMES
+ | NEW
| NEXT
| NO
| NOTHING
***************
*** 12988,12993 **** unreserved_keyword:
--- 13035,13041 ----
| OF
| OFF
| OIDS
+ | OLD
| OPERATOR
| OPTION
| OPTIONS
***************
*** 13017,13022 **** unreserved_keyword:
--- 13065,13071 ----
| RECHECK
| RECURSIVE
| REF
+ | REFERENCING
| REFRESH
| REINDEX
| RELATIVE_P
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***************
*** 809,814 **** pg_get_triggerdef_worker(Oid trigid, bool pretty)
--- 809,824 ----
appendStringInfoString(&buf, "IMMEDIATE ");
}
+ if (TRIGGER_USES_TRANSITION_TABLE(NameStr(trigrec->tgoldtable)) ||
+ TRIGGER_USES_TRANSITION_TABLE(NameStr(trigrec->tgnewtable)))
+ {
+ appendStringInfoString(&buf, "REFERENCING ");
+ if (TRIGGER_USES_TRANSITION_TABLE(NameStr(trigrec->tgoldtable)))
+ appendStringInfo(&buf, "OLD TABLE AS %s ", NameStr(trigrec->tgoldtable));
+ if (TRIGGER_USES_TRANSITION_TABLE(NameStr(trigrec->tgnewtable)))
+ appendStringInfo(&buf, "NEW TABLE AS %s ", NameStr(trigrec->tgnewtable));
+ }
+
if (TRIGGER_FOR_ROW(trigrec->tgtype))
appendStringInfoString(&buf, "FOR EACH ROW ");
else
*** a/src/include/catalog/pg_trigger.h
--- b/src/include/catalog/pg_trigger.h
***************
*** 48,53 **** CATALOG(pg_trigger,2620)
--- 48,55 ----
Oid tgconstraint; /* associated pg_constraint entry, if any */
bool tgdeferrable; /* constraint trigger is deferrable */
bool tginitdeferred; /* constraint trigger is deferred initially */
+ NameData tgoldtable; /* Name to use for old delta table for stmt */
+ NameData tgnewtable; /* Name to use for new delta table for stmt */
int16 tgnargs; /* # of extra arguments in tgargs */
/*
***************
*** 73,79 **** typedef FormData_pg_trigger *Form_pg_trigger;
* compiler constants for pg_trigger
* ----------------
*/
! #define Natts_pg_trigger 15
#define Anum_pg_trigger_tgrelid 1
#define Anum_pg_trigger_tgname 2
#define Anum_pg_trigger_tgfoid 3
--- 75,81 ----
* compiler constants for pg_trigger
* ----------------
*/
! #define Natts_pg_trigger 17
#define Anum_pg_trigger_tgrelid 1
#define Anum_pg_trigger_tgname 2
#define Anum_pg_trigger_tgfoid 3
***************
*** 85,94 **** typedef FormData_pg_trigger *Form_pg_trigger;
#define Anum_pg_trigger_tgconstraint 9
#define Anum_pg_trigger_tgdeferrable 10
#define Anum_pg_trigger_tginitdeferred 11
! #define Anum_pg_trigger_tgnargs 12
! #define Anum_pg_trigger_tgattr 13
! #define Anum_pg_trigger_tgargs 14
! #define Anum_pg_trigger_tgqual 15
/* Bits within tgtype */
#define TRIGGER_TYPE_ROW (1 << 0)
--- 87,98 ----
#define Anum_pg_trigger_tgconstraint 9
#define Anum_pg_trigger_tgdeferrable 10
#define Anum_pg_trigger_tginitdeferred 11
! #define Anum_pg_trigger_tgoldtable 12
! #define Anum_pg_trigger_tgnewtable 13
! #define Anum_pg_trigger_tgnargs 14
! #define Anum_pg_trigger_tgattr 15
! #define Anum_pg_trigger_tgargs 16
! #define Anum_pg_trigger_tgqual 17
/* Bits within tgtype */
#define TRIGGER_TYPE_ROW (1 << 0)
***************
*** 142,145 **** typedef FormData_pg_trigger *Form_pg_trigger;
--- 146,160 ----
#define TRIGGER_TYPE_MATCHES(type, level, timing, event) \
(((type) & (TRIGGER_TYPE_LEVEL_MASK | TRIGGER_TYPE_TIMING_MASK | (event))) == ((level) | (timing) | (event)))
+ /*
+ * Macro to determine whether tgnewtable or tgoldtable has been specified for
+ * a trigger.
+ *
+ * TODO: Once the dust settles on development, this can probably be
+ * simplified to test for either a NULL pointer or a zero-length cstring, but
+ * for now we'll do both.
+ */
+ #define TRIGGER_USES_TRANSITION_TABLE(namepointer) \
+ ((namepointer) != (char *) NULL && (*(namepointer)) != '\0')
+
#endif /* PG_TRIGGER_H */
*** a/src/include/commands/trigger.h
--- b/src/include/commands/trigger.h
***************
*** 36,41 **** typedef struct TriggerData
--- 36,43 ----
Trigger *tg_trigger;
Buffer tg_trigtuplebuf;
Buffer tg_newtuplebuf;
+ Tuplestorestate *tg_olddelta;
+ Tuplestorestate *tg_newdelta;
} TriggerData;
/*
*** a/src/include/nodes/nodes.h
--- b/src/include/nodes/nodes.h
***************
*** 407,412 **** typedef enum NodeTag
--- 407,413 ----
T_XmlSerialize,
T_WithClause,
T_CommonTableExpr,
+ T_TriggerTransition,
/*
* TAGS FOR REPLICATION GRAMMAR PARSE NODES (replnodes.h)
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 1018,1023 **** typedef struct CommonTableExpr
--- 1018,1039 ----
List *ctecolcollations; /* OID list of column collation OIDs */
} CommonTableExpr;
+ /*
+ * TriggerTransition -
+ * representation of transition row or table naming clause
+ *
+ * Only tables are initially supported, and only for AFTER EACH STATEMENT
+ * triggers, but other permutations are accepted by the parser so we can give
+ * a meaningful message from C code.
+ */
+ typedef struct TriggerTransition
+ {
+ NodeTag type;
+ char *name;
+ bool isNew;
+ bool isTable;
+ } TriggerTransition;
+
/* Convenience macro to get the output tlist of a CTE's query */
#define GetCTETargetList(cte) \
(AssertMacro(IsA((cte)->ctequery, Query)), \
***************
*** 1850,1855 **** typedef struct CreateTrigStmt
--- 1866,1873 ----
List *columns; /* column names, or NIL for all columns */
Node *whenClause; /* qual expression, or NULL if none */
bool isconstraint; /* This is a constraint trigger */
+ /* explicitly named transition data */
+ List *transitionRels; /* TriggerTransition nodes, or NIL if none */
/* The remaining fields are only used for constraint triggers */
bool deferrable; /* [NOT] DEFERRABLE */
bool initdeferred; /* INITIALLY {DEFERRED|IMMEDIATE} */
*** a/src/include/parser/kwlist.h
--- b/src/include/parser/kwlist.h
***************
*** 245,250 **** PG_KEYWORD("names", NAMES, UNRESERVED_KEYWORD)
--- 245,251 ----
PG_KEYWORD("national", NATIONAL, COL_NAME_KEYWORD)
PG_KEYWORD("natural", NATURAL, TYPE_FUNC_NAME_KEYWORD)
PG_KEYWORD("nchar", NCHAR, COL_NAME_KEYWORD)
+ PG_KEYWORD("new", NEW, UNRESERVED_KEYWORD)
PG_KEYWORD("next", NEXT, UNRESERVED_KEYWORD)
PG_KEYWORD("no", NO, UNRESERVED_KEYWORD)
PG_KEYWORD("none", NONE, COL_NAME_KEYWORD)
***************
*** 262,267 **** PG_KEYWORD("of", OF, UNRESERVED_KEYWORD)
--- 263,269 ----
PG_KEYWORD("off", OFF, UNRESERVED_KEYWORD)
PG_KEYWORD("offset", OFFSET, RESERVED_KEYWORD)
PG_KEYWORD("oids", OIDS, UNRESERVED_KEYWORD)
+ PG_KEYWORD("old", OLD, UNRESERVED_KEYWORD)
PG_KEYWORD("on", ON, RESERVED_KEYWORD)
PG_KEYWORD("only", ONLY, RESERVED_KEYWORD)
PG_KEYWORD("operator", OPERATOR, UNRESERVED_KEYWORD)
***************
*** 305,310 **** PG_KEYWORD("recheck", RECHECK, UNRESERVED_KEYWORD)
--- 307,313 ----
PG_KEYWORD("recursive", RECURSIVE, UNRESERVED_KEYWORD)
PG_KEYWORD("ref", REF, UNRESERVED_KEYWORD)
PG_KEYWORD("references", REFERENCES, RESERVED_KEYWORD)
+ PG_KEYWORD("referencing", REFERENCING, UNRESERVED_KEYWORD)
PG_KEYWORD("refresh", REFRESH, UNRESERVED_KEYWORD)
PG_KEYWORD("reindex", REINDEX, UNRESERVED_KEYWORD)
PG_KEYWORD("relative", RELATIVE_P, UNRESERVED_KEYWORD)
*** a/src/include/utils/reltrigger.h
--- b/src/include/utils/reltrigger.h
***************
*** 34,39 **** typedef struct Trigger
--- 34,41 ----
Oid tgconstraint;
bool tgdeferrable;
bool tginitdeferred;
+ char *tgoldtable;
+ char *tgnewtable;
int16 tgnargs;
int16 tgnattr;
int16 *tgattr;
***************
*** 68,73 **** typedef struct TriggerDesc
--- 70,80 ----
/* there are no row-level truncate triggers */
bool trig_truncate_before_statement;
bool trig_truncate_after_statement;
+ /* Is there at least one trigger specifying each transition relation? */
+ bool trig_insert_new_table;
+ bool trig_update_old_table;
+ bool trig_update_new_table;
+ bool trig_delete_old_table;
} TriggerDesc;
#endif /* RELTRIGGER_H */
On Sat, Jun 21, 2014 at 11:06:26AM -0700, Kevin Grittner wrote:
Kevin Grittner <kgrittn@ymail.com> wrote:
Kevin Grittner <kgrittn@ymail.com> wrote:
I've already said that I now think we should use the standard
CREATE TRIGGER syntax to specify the transition tables, and that
if we do that we don't need the reloption that's in the patch.
If you ignore the 19 lines of new code to add that reloption,
absolutely 100% of the code changes in the patch so far are in
trigger.c and trigger.h.Although nobody has actually framed their feedback as a review, I
feel that I have enough to work with to throw the patch into
Waiting on Author status.� Since I started with the assumption
that I was going to be using standard syntax and got a ways into
that before convincing myself it was a bad idea, I should have a
new version of the patch working that way in a couple days.Here is v2.
Thanks!
I've taken the liberty of making an extension that uses this.
Preliminary tests indicate a 10x performance improvement over the
user-space hack I did that's similar in functionality.
Please find attached the extension, etc., which I've published to
https://github.com/davidfetter/postgresql_projects/tree/test_delta_v2
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
test_delta_v2_001.difftext/plain; charset=us-asciiDownload
diff --git a/contrib/statement_trigger_row/Makefile b/contrib/statement_trigger_row/Makefile
new file mode 100644
index 0000000..e0cf006
--- /dev/null
+++ b/contrib/statement_trigger_row/Makefile
@@ -0,0 +1,17 @@
+# contrib/statement_trigger_row/Makefile
+
+MODULES = statement_trigger_row
+
+EXTENSION = statement_trigger_row
+DATA = statement_trigger_row--1.0.sql
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/statement_trigger_row
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/statement_trigger_row/sql/easy_way.sql b/contrib/statement_trigger_row/sql/easy_way.sql
new file mode 100644
index 0000000..019ae7f
--- /dev/null
+++ b/contrib/statement_trigger_row/sql/easy_way.sql
@@ -0,0 +1,85 @@
+/*
+ * If these were surfaced to PL/pgsql, this is what it might look like.
+ */
+
+BEGIN;
+
+CREATE TABLE a(
+ id SERIAL PRIMARY KEY,
+ i INT
+);
+
+CREATE FUNCTION summarize_a_inserts()
+RETURNS TRIGGER LANGUAGE plpgsql
+AS $$
+DECLARE
+ the_sum BIGINT;
+BEGIN
+ SELECT INTO the_sum sum(NEW.i)
+ FROM
+ new_a;
+ RAISE NOTICE 'Total change: %.', the_sum;
+ RETURN NULL;
+END;
+$$;
+
+CREATE FUNCTION summarize_a_updates()
+RETURNS TRIGGER LANGUAGE plpgsql
+AS $$
+DECLARE
+ the_sum BIGINT;
+BEGIN
+ SELECT INTO the_sum sum(COALESCE(NEW.i,0) - COALESCE(OLD.i, 0))
+ FROM
+ old_a
+ JOIN
+ new_a
+ ON(old_a.id = new_a.id);
+ RAISE NOTICE 'Total change: %.', the_sum;
+ RETURN NULL;
+END;
+$$;
+
+CREATE FUNCTION summarize_a_deletes()
+RETURNS TRIGGER LANGUAGE plpgsql
+AS $$
+DECLARE
+ the_sum BIGINT;
+BEGIN
+ SELECT INTO the_sum -1 * sum(OLD.i)
+ FROM
+ old_a;
+ RAISE NOTICE 'Total change: %.', the_sum;
+ RETURN NULL;
+END;
+$$;
+
+CREATE TRIGGER statement_after_insert_a
+ AFTER INSERT ON a
+ REFERENCING
+ NEW TABLE AS new_a
+ FOR EACH STATEMENT
+ EXECUTE PROCEDURE summarize_a_inserts();
+
+CREATE TRIGGER statement_after_update_a
+ AFTER UPDATE ON a
+ REFERENCING
+ OLD TABLE AS old_a
+ NEW TABLE AS new_a
+ FOR EACH STATEMENT
+ EXECUTE PROCEDURE summarize_a_updates();
+
+CREATE TRIGGER statement_after_delete_a
+ AFTER DELETE ON a
+ REFERENCING
+ OLD TABLE AS old_a
+ FOR EACH STATEMENT
+ EXECUTE PROCEDURE summarize_a_deletes();
+
+INSERT INTO a(i)
+SELECT * FROM generate_series(1,10000);
+
+UPDATE a SET i=i+1;
+
+ROLLBACK;
+
diff --git a/contrib/statement_trigger_row/sql/hard_way.sql b/contrib/statement_trigger_row/sql/hard_way.sql
new file mode 100644
index 0000000..c6f7c1d
--- /dev/null
+++ b/contrib/statement_trigger_row/sql/hard_way.sql
@@ -0,0 +1,68 @@
+CREATE TABLE IF NOT EXISTS h(
+ i INTEGER
+);
+
+CREATE FUNCTION set_up_h_rows()
+RETURNS TRIGGER LANGUAGE plpgsql
+AS $$
+BEGIN
+ CREATE TEMPORARY TABLE IF NOT EXISTS h_rows(LIKE a) ON COMMIT DROP;
+ RETURN NULL;
+END;
+$$;
+
+CREATE TRIGGER statement_before_writing_h
+ BEFORE INSERT OR UPDATE OR DELETE ON a
+ FOR EACH STATEMENT
+ EXECUTE PROCEDURE set_up_h_rows();
+
+CREATE OR REPLACE FUNCTION stash_h_row_deltas()
+RETURNS TRIGGER
+LANGUAGE plpgsql
+AS $$
+BEGIN
+ INSERT INTO h_rows(i)
+ VALUES(
+ CASE TG_OP
+ WHEN 'INSERT' THEN COALESCE(NEW.i,0)
+ WHEN 'UPDATE' THEN COALESCE(NEW.i,0) - COALESCE(OLD.i,0)
+ WHEN 'DELETE' THEN -1 * COALESCE(OLD.i,0)
+ END
+ );
+ IF TG_OP IN ('INSERT','UPDATE')
+ THEN
+ RETURN NEW;
+ ELSE
+ RETURN OLD;
+ END IF;
+END;
+$$;
+
+CREATE TRIGGER during_trg
+ BEFORE INSERT OR UPDATE OR DELETE ON a
+ FOR EACH ROW
+ EXECUTE PROCEDURE stash_h_row_deltas();
+
+CREATE FUNCTION summarize_h_rows()
+RETURNS TRIGGER LANGUAGE plpgsql
+AS $$
+DECLARE the_sum BIGINT;
+BEGIN
+ SELECT INTO the_sum sum(i) FROM h_rows;
+ RAISE NOTICE 'Total change: %.', the_sum;
+ TRUNCATE h_rows;
+ RETURN NULL;
+END;
+$$;
+
+CREATE TRIGGER statement_after_writing_h
+ AFTER INSERT OR UPDATE OR DELETE ON a
+ FOR EACH STATEMENT
+ EXECUTE PROCEDURE summarize_h_rows();
+
+INSERT INTO h(i)
+SELECT * FROM generate_series(1,10000);
+
+UPDATE h SET i=i+1;
+
+DELETE FROM h WHERE i < 5000;
diff --git a/contrib/statement_trigger_row/sql/statement_trigger_row.sql b/contrib/statement_trigger_row/sql/statement_trigger_row.sql
new file mode 100644
index 0000000..4d5925a
--- /dev/null
+++ b/contrib/statement_trigger_row/sql/statement_trigger_row.sql
@@ -0,0 +1,18 @@
+CREATE TABLE IF NOT EXISTS e(
+ i INT
+);
+
+CREATE TRIGGER statement_dml_e
+ AFTER INSERT OR UPDATE OR DELETE ON e
+ REFERENCING
+ OLD TABLE AS old_e
+ NEW TABLE AS new_e
+ FOR EACH STATEMENT
+ EXECUTE PROCEDURE statement_trigger_row();
+
+INSERT INTO e(i)
+SELECT * FROM generate_series(1,10000);
+
+UPDATE e SET i=i+1;
+
+DELETE FROM e WHERE i < 5000;
diff --git a/contrib/statement_trigger_row/statement_trigger_row--1.0.sql b/contrib/statement_trigger_row/statement_trigger_row--1.0.sql
new file mode 100644
index 0000000..ffc0e36
--- /dev/null
+++ b/contrib/statement_trigger_row/statement_trigger_row--1.0.sql
@@ -0,0 +1,10 @@
+/* contrib/statement_trigger_row--1.0.sql */
+
+
+-- Complain if script is sourced in psql, rather than via CREATE EXTENSION.
+\echo Use "CREATE EXTENSION statement_trigger_row" to load this file. \quit
+
+CREATE FUNCTION statement_trigger_row()
+RETURNS pg_catalog.trigger
+AS 'MODULE_PATHNAME'
+LANGUAGE C;
diff --git a/contrib/statement_trigger_row/statement_trigger_row.c b/contrib/statement_trigger_row/statement_trigger_row.c
new file mode 100644
index 0000000..048eb3b
--- /dev/null
+++ b/contrib/statement_trigger_row/statement_trigger_row.c
@@ -0,0 +1,140 @@
+/*-------------------------------------------------------------------------
+ *
+ * statement_trigger_row.c
+ * statement_trigger_row support for PostgreSQL
+ *
+ * Portions Copyright (c) 2011-2014, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ * contrib/statement_trigger_row.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "executor/spi.h"
+#include "commands/trigger.h"
+#include "utils/rel.h"
+
+#ifdef PG_MODULE_MAGIC
+PG_MODULE_MAGIC;
+#endif
+
+extern Datum statement_trigger_row(PG_FUNCTION_ARGS);
+
+PG_FUNCTION_INFO_V1(statement_trigger_row);
+
+Datum
+statement_trigger_row(PG_FUNCTION_ARGS)
+{
+ TriggerData *trigdata = (TriggerData *) fcinfo->context;
+ TupleDesc tupdesc;
+ TupleTableSlot *slot;
+ Tuplestorestate *new_tuplestore;
+ Tuplestorestate *old_tuplestore;
+ int64 delta = 0;
+
+ if (!CALLED_AS_TRIGGER(fcinfo))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("statement_trigger_row: not called by trigger manager")));
+ }
+
+ if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("statement_trigger_row: not called by AFTER trigger")));
+ }
+
+ if (TRIGGER_FIRED_BY_TRUNCATE(trigdata->tg_event))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("You may not call this function in a TRUNCATE trigger.")));
+ }
+
+ tupdesc = trigdata->tg_relation->rd_att;
+
+ slot = MakeSingleTupleTableSlot(tupdesc);
+
+ if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
+ {
+ if (trigdata->tg_newdelta == NULL)
+ ereport(ERROR,
+ (errmsg("You must include NEW TABLE AS in your CREATE TRIGGER statement")));
+
+ new_tuplestore = trigdata->tg_newdelta;
+ /*
+ * Ensure that we're at the right place in the tuplestore, as
+ * other triggers may have messed with the state.
+ */
+ tuplestore_rescan(new_tuplestore);
+
+ /* Iterate through the new tuples, adding. */
+ while (tuplestore_gettupleslot(new_tuplestore, true, false, slot)) {
+ bool isnull;
+ Datum val = slot_getattr(slot, 1, &isnull);
+ if (!isnull)
+ delta += DatumGetInt32(val);
+ }
+ }
+ else if (TRIGGER_FIRED_BY_DELETE(trigdata->tg_event))
+ {
+ if (trigdata->tg_olddelta == NULL)
+ ereport(ERROR,
+ (errmsg("You must include OLD TABLE AS in your CREATE TRIGGER statement")));
+
+ old_tuplestore = trigdata->tg_olddelta;
+ tuplestore_rescan(old_tuplestore);
+ /* Iterate through the old tuples, subtracting. */
+ while (tuplestore_gettupleslot(old_tuplestore, true, false, slot)) {
+ bool isnull;
+ Datum val = slot_getattr(slot, 1, &isnull);
+ if (!isnull)
+ delta -= DatumGetInt32(val);
+ }
+ }
+ else /* It's an UPDATE */
+ {
+ if (trigdata->tg_olddelta == NULL)
+ ereport(ERROR,
+ (errmsg("You must include OLD TABLE AS in your CREATE TRIGGER statement")));
+ if (trigdata->tg_newdelta == NULL)
+ ereport(ERROR,
+ (errmsg("You must include NEW TABLE AS in your CREATE TRIGGER statement")));
+
+ old_tuplestore = trigdata->tg_olddelta;
+ new_tuplestore = trigdata->tg_newdelta;
+
+ tuplestore_rescan(old_tuplestore);
+ tuplestore_rescan(new_tuplestore);
+
+ /* Iterate through both the new and old tuples, incrementing
+ * or decrementing as needed. */
+ while (tuplestore_gettupleslot(new_tuplestore, true, false, slot)) {
+ bool isnull;
+ Datum val = slot_getattr(slot, 1, &isnull);
+ if (!isnull)
+ delta += DatumGetInt32(val);
+ }
+
+ while (tuplestore_gettupleslot(old_tuplestore, true, false, slot)) {
+ bool isnull;
+ Datum val = slot_getattr(slot, 1, &isnull);
+ if (!isnull)
+ delta -= DatumGetInt32(val);
+ }
+
+ }
+
+ ExecDropSingleTupleTableSlot(slot);
+
+ ereport(NOTICE, (errmsg("Total change: " INT64_FORMAT, delta)));
+
+ return PointerGetDatum(NULL);
+
+}
diff --git a/contrib/statement_trigger_row/statement_trigger_row.control b/contrib/statement_trigger_row/statement_trigger_row.control
new file mode 100644
index 0000000..846ea2d
--- /dev/null
+++ b/contrib/statement_trigger_row/statement_trigger_row.control
@@ -0,0 +1,5 @@
+# statement_trigger_row extension
+comment = 'Statement trigger row'
+default_version = '1.0'
+module_pathname = '$libdir/statement_trigger_row'
+relocatable = true
David Fetter <david@fetter.org> wrote:
On Sat, Jun 21, 2014 at 11:06:26AM -0700, Kevin Grittner wrote:
Here is v2.
I've taken the liberty of making an extension that uses this.
Preliminary tests indicate a 10x performance improvement over the
user-space hack I did that's similar in functionality.
Wow, this goes well beyond what I expected for a review! Thanks!
As I said in an earlier post, I think that this is best committed
as a series of patches, one for the core portion and one for each
PL which implements the ability to use the transition (delta)
relations in AFTER triggers. Your extension covers the C trigger
angle, and it seems to me to be worth committing to contrib as a
sample of how to use this feature in C.
It is very encouraging that you were able to use this without
touching what I did in core, and that it runs 10x faster than the
alternatives before the patch.
Because this review advances the patch so far, it may be feasible
to get it committed in this CF. I'll see what is needed to get
there and maybe have a patch toward that end in a few days. The
minimum that would require, IMV, is a plpgsql implementation,
moving the new pg_trigger columns to the variable portion of the
record so they can be null capable, more docs, and regression
tests.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jun 28, 2014 at 07:35:10AM -0700, Kevin Grittner wrote:
David Fetter <david@fetter.org> wrote:
On Sat, Jun 21, 2014 at 11:06:26AM -0700, Kevin Grittner wrote:
Here is v2.
I've taken the liberty of making an extension that uses this.
Preliminary tests indicate a 10x performance improvement over the
user-space hack I did that's similar in functionality.Wow, this goes well beyond what I expected for a review!� Thanks!
It was the minimum I could come up with to test whether the patch
worked.
As I said in an earlier post, I think that this is best committed
as a series of patches, one for the core portion and one for each
PL which implements the ability to use the transition (delta)
relations in AFTER triggers.
Right. I'm still holding out hope of having the transition relations
available in some more general way, but that seems more like a
refactoring job than anything fundamental.
Your extension covers the C trigger angle, and it seems to me to be
worth committing to contrib as a sample of how to use this feature
in C.
It's missing a few pieces like surfacing transition table names. I'll
work on those. Also, it's not clear to me how to access the pre- and
post- relations at the same time, this being necessary for many use
cases. I guess I need to think more about how that would be done.
It is very encouraging that you were able to use this without
touching what I did in core, and that it runs 10x faster than the
alternatives before the patch.
The alternative included was pretty inefficient, so there's that.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jun 28, 2014 at 10:35 AM, Kevin Grittner <kgrittn@ymail.com> wrote:
David Fetter <david@fetter.org> wrote:
On Sat, Jun 21, 2014 at 11:06:26AM -0700, Kevin Grittner wrote:
Here is v2.
I've taken the liberty of making an extension that uses this.
Preliminary tests indicate a 10x performance improvement over the
user-space hack I did that's similar in functionality.Wow, this goes well beyond what I expected for a review! Thanks!
As I said in an earlier post, I think that this is best committed
as a series of patches, one for the core portion and one for each
PL which implements the ability to use the transition (delta)
relations in AFTER triggers. Your extension covers the C trigger
angle, and it seems to me to be worth committing to contrib as a
sample of how to use this feature in C.It is very encouraging that you were able to use this without
touching what I did in core, and that it runs 10x faster than the
alternatives before the patch.Because this review advances the patch so far, it may be feasible
to get it committed in this CF. I'll see what is needed to get
there and maybe have a patch toward that end in a few days. The
minimum that would require, IMV, is a plpgsql implementation,
moving the new pg_trigger columns to the variable portion of the
record so they can be null capable, more docs, and regression
tests.
Not to rain on your parade, but this patch hasn't really had a serious
code review yet. Performance testing is good, but it's not the same
thing.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 30, 2014 at 11:03:06AM -0400, Robert Haas wrote:
On Sat, Jun 28, 2014 at 10:35 AM, Kevin Grittner <kgrittn@ymail.com> wrote:
David Fetter <david@fetter.org> wrote:
On Sat, Jun 21, 2014 at 11:06:26AM -0700, Kevin Grittner wrote:
Here is v2.
I've taken the liberty of making an extension that uses this.
Preliminary tests indicate a 10x performance improvement over the
user-space hack I did that's similar in functionality.Wow, this goes well beyond what I expected for a review! Thanks!
As I said in an earlier post, I think that this is best committed
as a series of patches, one for the core portion and one for each
PL which implements the ability to use the transition (delta)
relations in AFTER triggers. Your extension covers the C trigger
angle, and it seems to me to be worth committing to contrib as a
sample of how to use this feature in C.It is very encouraging that you were able to use this without
touching what I did in core, and that it runs 10x faster than the
alternatives before the patch.Because this review advances the patch so far, it may be feasible
to get it committed in this CF. I'll see what is needed to get
there and maybe have a patch toward that end in a few days. The
minimum that would require, IMV, is a plpgsql implementation,
moving the new pg_trigger columns to the variable portion of the
record so they can be null capable, more docs, and regression
tests.Not to rain on your parade, but this patch hasn't really had a serious
code review yet. Performance testing is good, but it's not the same
thing.
Happy to help with that, too.
What I wanted to start with is whether there was even rudimentary
functionality, which I established by writing that extension. I
happened to notice, basically as a sanity check, that doing this via
tuplestores happened, at least in one case, to be quicker than doing
it in user space with temp tables.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Fetter <david@fetter.org> wrote:
It's missing a few pieces like surfacing transition table names.
I'll work on those. Also, it's not clear to me how to access the
pre- and post- relations at the same time, this being necessary
for many use cases. I guess I need to think more about how that
would be done.
If you're going to do any work on the C extension, please start
from the attached. I renamed it to something which seemed more
meaningful (to me, at least), and cleaned up some cosmetic issues.
The substance is the same.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
transition-table-c-v2.difftext/x-diff; name=transition-table-c-v2.diffDownload
diff --git a/contrib/Makefile b/contrib/Makefile
index b37d0dd..bcd7c28 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -48,6 +48,7 @@ SUBDIRS = \
postgres_fdw \
seg \
spi \
+ transition_tables \
tablefunc \
tcn \
test_decoding \
diff --git a/contrib/transition_tables/.gitignore b/contrib/transition_tables/.gitignore
new file mode 100644
index 0000000..5dcb3ff
--- /dev/null
+++ b/contrib/transition_tables/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/contrib/transition_tables/Makefile b/contrib/transition_tables/Makefile
new file mode 100644
index 0000000..8a75350
--- /dev/null
+++ b/contrib/transition_tables/Makefile
@@ -0,0 +1,20 @@
+# contrib/transition_tables/Makefile
+
+MODULE_big = transition_tables
+OBJS = transition_tables.o
+
+EXTENSION = transition_tables
+DATA = transition_tables--1.0.sql
+
+REGRESS = transition_tables
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/transition_tables
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/transition_tables/expected/transition_tables.out b/contrib/transition_tables/expected/transition_tables.out
new file mode 100644
index 0000000..7dfed35
--- /dev/null
+++ b/contrib/transition_tables/expected/transition_tables.out
@@ -0,0 +1,18 @@
+CREATE EXTENSION transition_tables;
+CREATE TABLE IF NOT EXISTS e(
+ i INT
+);
+CREATE TRIGGER statement_dml_e
+ AFTER INSERT OR UPDATE OR DELETE ON e
+ REFERENCING
+ OLD TABLE AS old_e
+ NEW TABLE AS new_e
+ FOR EACH STATEMENT
+ EXECUTE PROCEDURE transition_tables();
+INSERT INTO e(i)
+SELECT * FROM generate_series(1,10000);
+NOTICE: Total change: 50005000
+UPDATE e SET i=i+1;
+NOTICE: Total change: 10000
+DELETE FROM e WHERE i < 5000;
+NOTICE: Total change: -12497499
diff --git a/contrib/transition_tables/sql/transition_tables.sql b/contrib/transition_tables/sql/transition_tables.sql
new file mode 100644
index 0000000..04f8bd7
--- /dev/null
+++ b/contrib/transition_tables/sql/transition_tables.sql
@@ -0,0 +1,20 @@
+CREATE EXTENSION transition_tables;
+
+CREATE TABLE IF NOT EXISTS e(
+ i INT
+);
+
+CREATE TRIGGER statement_dml_e
+ AFTER INSERT OR UPDATE OR DELETE ON e
+ REFERENCING
+ OLD TABLE AS old_e
+ NEW TABLE AS new_e
+ FOR EACH STATEMENT
+ EXECUTE PROCEDURE transition_tables();
+
+INSERT INTO e(i)
+SELECT * FROM generate_series(1,10000);
+
+UPDATE e SET i=i+1;
+
+DELETE FROM e WHERE i < 5000;
diff --git a/contrib/transition_tables/transition_tables--1.0.sql b/contrib/transition_tables/transition_tables--1.0.sql
new file mode 100644
index 0000000..6c1625e
--- /dev/null
+++ b/contrib/transition_tables/transition_tables--1.0.sql
@@ -0,0 +1,10 @@
+/* contrib/transition_tables--1.0.sql */
+
+
+-- Complain if script is sourced in psql, rather than via CREATE EXTENSION.
+\echo Use "CREATE EXTENSION transition_tables" to load this file. \quit
+
+CREATE FUNCTION transition_tables()
+RETURNS pg_catalog.trigger
+AS 'MODULE_PATHNAME'
+LANGUAGE C;
diff --git a/contrib/transition_tables/transition_tables.c b/contrib/transition_tables/transition_tables.c
new file mode 100644
index 0000000..2b9264c
--- /dev/null
+++ b/contrib/transition_tables/transition_tables.c
@@ -0,0 +1,144 @@
+/*-------------------------------------------------------------------------
+ *
+ * transition_tables.c
+ * sample of accessing trigger transition tables from a C trigger
+ *
+ * Portions Copyright (c) 2011-2014, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ * contrib/transition_tables.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "commands/trigger.h"
+#include "utils/rel.h"
+
+PG_MODULE_MAGIC;
+
+PG_FUNCTION_INFO_V1(transition_tables);
+
+Datum
+transition_tables(PG_FUNCTION_ARGS)
+{
+ TriggerData *trigdata = (TriggerData *) fcinfo->context;
+ TupleDesc tupdesc;
+ TupleTableSlot *slot;
+ Tuplestorestate *new_tuplestore;
+ Tuplestorestate *old_tuplestore;
+ int64 delta = 0;
+
+ if (!CALLED_AS_TRIGGER(fcinfo))
+ ereport(ERROR,
+ (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
+ errmsg("transition_tables: must be called as trigger")));
+
+ if (!TRIGGER_FIRED_AFTER(trigdata->tg_event))
+ ereport(ERROR,
+ (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
+ errmsg("transition_tables: must be called after the change")));
+
+ if (!TRIGGER_FIRED_FOR_STATEMENT(trigdata->tg_event))
+ ereport(ERROR,
+ (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
+ errmsg("transition_tables: must be called for each statement")));
+
+ tupdesc = trigdata->tg_relation->rd_att;
+ slot = MakeSingleTupleTableSlot(tupdesc);
+
+ /*
+ * Since other code may have already used the tuplestore(s), reset to the
+ * start before reading.
+ */
+ if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))
+ {
+ if (trigdata->tg_newdelta == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
+ errmsg("NEW TABLE was not specified in the CREATE TRIGGER statement")));
+
+ new_tuplestore = trigdata->tg_newdelta;
+ tuplestore_rescan(new_tuplestore);
+
+ /* Iterate through the new tuples, adding. */
+ while (tuplestore_gettupleslot(new_tuplestore, true, false, slot))
+ {
+ bool isnull;
+ Datum val = slot_getattr(slot, 1, &isnull);
+
+ if (!isnull)
+ delta += DatumGetInt32(val);
+ }
+ }
+ else if (TRIGGER_FIRED_BY_DELETE(trigdata->tg_event))
+ {
+ if (trigdata->tg_olddelta == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
+ errmsg("OLD TABLE was not specified in the CREATE TRIGGER statement")));
+
+ old_tuplestore = trigdata->tg_olddelta;
+ tuplestore_rescan(old_tuplestore);
+
+ /* Iterate through the old tuples, subtracting. */
+ while (tuplestore_gettupleslot(old_tuplestore, true, false, slot))
+ {
+ bool isnull;
+ Datum val = slot_getattr(slot, 1, &isnull);
+
+ if (!isnull)
+ delta -= DatumGetInt32(val);
+ }
+ }
+ else if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
+ {
+ if (trigdata->tg_olddelta == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
+ errmsg("OLD TABLE was not specified in the CREATE TRIGGER statement")));
+ if (trigdata->tg_newdelta == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
+ errmsg("NEW TABLE was not specified in the CREATE TRIGGER statement")));
+
+ old_tuplestore = trigdata->tg_olddelta;
+ new_tuplestore = trigdata->tg_newdelta;
+ tuplestore_rescan(old_tuplestore);
+ tuplestore_rescan(new_tuplestore);
+
+ /*
+ * Iterate through both the new and old tuples, adding or subtracting
+ * as needed.
+ */
+ while (tuplestore_gettupleslot(new_tuplestore, true, false, slot))
+ {
+ bool isnull;
+ Datum val = slot_getattr(slot, 1, &isnull);
+
+ if (!isnull)
+ delta += DatumGetInt32(val);
+ }
+ while (tuplestore_gettupleslot(old_tuplestore, true, false, slot))
+ {
+ bool isnull;
+ Datum val = slot_getattr(slot, 1, &isnull);
+
+ if (!isnull)
+ delta -= DatumGetInt32(val);
+ }
+
+ }
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
+ errmsg("transition_tables: only INSERT, UPDATE, and DELETE triggers may use transition tables")));
+
+ ExecDropSingleTupleTableSlot(slot);
+
+ ereport(NOTICE, (errmsg("Total change: " INT64_FORMAT, delta)));
+
+ return PointerGetDatum(NULL); /* after trigger; value doesn't matter */
+}
diff --git a/contrib/transition_tables/transition_tables.control b/contrib/transition_tables/transition_tables.control
new file mode 100644
index 0000000..486dde7
--- /dev/null
+++ b/contrib/transition_tables/transition_tables.control
@@ -0,0 +1,5 @@
+# transition_tables extension
+comment = 'Sample code for using trigger transition tables'
+default_version = '1.0'
+module_pathname = '$libdir/transition_tables'
+relocatable = true
Kevin Grittner <kgrittn@ymail.com> wrote:
Because this review advances the patch so far, it may be feasible
to get it committed in this CF. I'll see what is needed to get
there and maybe have a patch toward that end in a few days.
It appears that I need to create a new execution node and a way for
SPI calls to use it. That seems beyond the scope of what is fair
to include in this CF, even if I got something put together in the
next couple days.
FWIW, I think that once I have the other pieces, what I initially
posted is committable as the first patch of a series. A second
patch would add the new execution node and code to allow SPI calls
to use it. The patch that David submitted, as modified by myself
and with further refinements that David is working on would be the
third patch. An implementation in plpgsql, would be the fourth.
Other PLs would be left for people more familiar with those
languages to implement.
What I was hoping for in this CF was a review to confirm the
approach before proceeding to build on this foundation. David
found nothing to complain about, and went to the trouble of writing
code to confirm that it was actually generating complete results
which were usable. Robert doesn't feel this constitutes "a serious
code review". I'm not aware of any changes which are needed to the
pending patch once the follow-on patches are complete. I'm moving
this to Needs Review status. People will have another chance to
review this patch when the other code is available, but if we want
incremental maintenance of materialized views in 9.5, delaying
review of what I have submitted in this CF until the next CF will
put that goal in jeopardy.
The one thing I don't feel great about is that it's using
tuplestores of the actual tuples rather than just their TIDs; but
it seems to me that we need the full tuple to support triggers on
FDWs, so the TID approach would be an optimization for a subset of
the cases, and would probably be more appropriate, if we do it at
all, in a follow-on patch after this is working (although I think
it is an optimization we should get into 9.5 if we are going to do
it). If you disagree with that assessment, now would be a good
time to explain your reasoning.
A minor point is psql tab-completion for the REFERENCING clause.
It seems to me that's not critical, but I might slip it in anyway
before commit.
I took a look at whether I could avoid making OLD and NEW
non-reserved keywords, but I didn't see how to do that without
making FOR at least partially reserved. If someone sees a way to
do this without creating three new unreserved keywords
(REFERENCING, OLD, and NEW) I'm all ears.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jul 5, 2014 at 5:38 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
it seems to me that we need the full tuple to support triggers on
FDWs, so the TID approach would be an optimization for a subset of
the cases, and would probably be more appropriate, if we do it at
all, in a follow-on patch
If you disagree with that assessment, now would be a good
time to explain your reasoning.
Maybe I just have a limited imagination because I've never found a use
for FDWs personally. But recording changes from a trigger on a FDW
table doesn't seem that useful, since you can only capture changes
done by the local node. I expect that in many situations there are
multiple writers accessing the same underlying remote table. Certainly
it's can't guarantee the consistency of materialized views.
I took a look at whether I could avoid making OLD and NEW
non-reserved keywords, but I didn't see how to do that without
making FOR at least partially reserved. If someone sees a way to
do this without creating three new unreserved keywords
(REFERENCING, OLD, and NEW) I'm all ears.
Sorry, I know I am very late to make this point, so feel free to ignore this.
I'm not a fan of the SQL standard syntax for this feature. One nice
thing about PostgreSQL's triggers is that you can declare the trigger
function once and re-use it on many tables. It would make more sense
if the same function declaration could say what variable/relation
names it wants to use. They're more like function argument names, not
some metadata about a table-function relationship.
Putting these in the CREATE TRIGGER command means you have to repeat
them for each table you want to apply the trigger to. It introduces
the possibility of making more mistakes without any gain in
flexibility.
But then again, I understand that there's value in supporting standard syntax.
Regards,
Marti
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Marti Raudsepp <marti@juffo.org> wrote:
On Sat, Jul 5, 2014 at 5:38 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
it seems to me that we need the full tuple to support triggers on
FDWs, so the TID approach would be an optimization for a subset of
the cases, and would probably be more appropriate, if we do it at
all, in a follow-on patch
If you disagree with that assessment, now would be a good
time to explain your reasoning.Maybe I just have a limited imagination because I've never found a use
for FDWs personally. But recording changes from a trigger on a FDW
table doesn't seem that useful,
It's a matter of whether AFTER triggers on an FDW can see the
modified data in table form. We just recently added the ability
for FDW triggers to see the data in *row* form; it seemed odd to
immediately follow that with a new way to get at the data and
cripple FDW triggers for it.
since you can only capture changes
done by the local node. I expect that in many situations there are
multiple writers accessing the same underlying remote table. Certainly
it's can't guarantee the consistency of materialized views.
While I expect the techniques used here to help with development of
incremental maintenance of materialized views, this seems like a
useful feature in its own right. I think the question is what the
basis would be for supporting access to the changes in row format
but not table format for FDWs, if we're supporting both formats for
other tables.
I took a look at whether I could avoid making OLD and NEW
non-reserved keywords, but I didn't see how to do that without
making FOR at least partially reserved. If someone sees a way to
do this without creating three new unreserved keywords
(REFERENCING, OLD, and NEW) I'm all ears.Sorry, I know I am very late to make this point, so feel free to ignore this.
I'm not a fan of the SQL standard syntax for this feature. One nice
thing about PostgreSQL's triggers is that you can declare the trigger
function once and re-use it on many tables. It would make more sense
if the same function declaration could say what variable/relation
names it wants to use. They're more like function argument names, not
some metadata about a table-function relationship.Putting these in the CREATE TRIGGER command means you have to repeat
them for each table you want to apply the trigger to. It introduces
the possibility of making more mistakes without any gain in
flexibility.But then again, I understand that there's value in supporting standard
syntax.
Do you have some other suggestion? Keep in mind that it must allow
the code which will *generate* the transition tables to know
whether any of the attached triggers use a given transition table
for the specific operation, regardless of the language of the
trigger function. Using the standard syntax has the advantage of
making it pretty easy to put the information exactly where it is
needed for easy access at run time.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jul 28, 2014 at 6:24 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
Do you have some other suggestion? Keep in mind that it must allow
the code which will *generate* the transition tables to know
whether any of the attached triggers use a given transition table
for the specific operation, regardless of the language of the
trigger function.
You will need to access the pg_proc record of the trigger function
anyway, so it's just a matter of coming up with syntax that makes
sense, right?
What I had in mind was that we could re-use function argument
declaration syntax. For instance, use the "argmode" specifier to
declare OLD and NEW. Shouldn't cause grammar conflicts because the
current OUT and INOUT aren't reserved keywords.
We could also re-use the refcursor type, which already has bindings in
some PLs, if that's not too much overhead. That would make the
behavior straightforward without introducing new constructs, plus you
can pass them around between functions. Though admittedly it's
annoying to integrate cursor results into queries.
Something like:
CREATE FUNCTION trig(OLD old_rows refcursor, NEW new_rows refcursor)
RETURNS trigger LANGUAGE plpgsql AS '...';
Or maybe if the grammar allows, we could spell out "NEW TABLE", "OLD
TABLE", but that's redundant since you can already deduce that from
the refcursor type.
It could also be extended for different types, like tid[], and maybe
"record" for the FOR EACH ROW variant (dunno if that can be made to
work).
Regards,
Marti
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2014-07-28 19:27 GMT+02:00 Marti Raudsepp <marti@juffo.org>:
On Mon, Jul 28, 2014 at 6:24 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
Do you have some other suggestion? Keep in mind that it must allow
the code which will *generate* the transition tables to know
whether any of the attached triggers use a given transition table
for the specific operation, regardless of the language of the
trigger function.You will need to access the pg_proc record of the trigger function
anyway, so it's just a matter of coming up with syntax that makes
sense, right?What I had in mind was that we could re-use function argument
declaration syntax. For instance, use the "argmode" specifier to
declare OLD and NEW. Shouldn't cause grammar conflicts because the
current OUT and INOUT aren't reserved keywords.We could also re-use the refcursor type, which already has bindings in
some PLs, if that's not too much overhead. That would make the
behavior straightforward without introducing new constructs, plus you
can pass them around between functions. Though admittedly it's
annoying to integrate cursor results into queries.Something like:
CREATE FUNCTION trig(OLD old_rows refcursor, NEW new_rows refcursor)
RETURNS trigger LANGUAGE plpgsql AS '...';
I dislike this proposal - it is strongly inconsistent with current trigger
design
Regards
Pavel
Show quoted text
Or maybe if the grammar allows, we could spell out "NEW TABLE", "OLD
TABLE", but that's redundant since you can already deduce that from
the refcursor type.It could also be extended for different types, like tid[], and maybe
"record" for the FOR EACH ROW variant (dunno if that can be made to
work).Regards,
Marti--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jul 29, 2014 at 9:49 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
I dislike this proposal - it is strongly inconsistent with current trigger
design
The real point I was trying to convey (in my previous email) is that
these declarations should be part of the trigger *function* not the
function-to-table relationship. CREATE TRIGGER shouldn't be in the
business of declaring new local variables for the trigger function.
Whether we define new syntax for that or re-use the argument list is
secondary.
But the inconsistency is deliberate, I find the current trigger API
horrible. Magic variables... Text-only TG_ARGV for arguments...
RETURNS trigger... No way to invoke trigger functions directly for
testing.
By not imitating past mistakes, maybe we can eventually arrive at a
language that makes sense.
Regards,
Marti
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2014-07-29 9:41 GMT+02:00 Marti Raudsepp <marti@juffo.org>:
On Tue, Jul 29, 2014 at 9:49 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:I dislike this proposal - it is strongly inconsistent with current
trigger
design
The real point I was trying to convey (in my previous email) is that
these declarations should be part of the trigger *function* not the
function-to-table relationship. CREATE TRIGGER shouldn't be in the
business of declaring new local variables for the trigger function.
Whether we define new syntax for that or re-use the argument list is
secondary.But the inconsistency is deliberate, I find the current trigger API
horrible. Magic variables... Text-only TG_ARGV for arguments...
RETURNS trigger...
A notation RETURNS TRIGGER I don't like too much too - RETURNS void or
RETURNS record are much more natural.
My dream is some like CREATE OR REPLACE TRIGGER FUNCTION trg() RETURNS
RECORD
but it is only syntactic sugar - and I don't see any benefit why we should
to implement it.
No way to invoke trigger functions directly for
testing.
It is horrible idea. I can agree, it is a limit - but not too hard - there
is simple possibility to take code from trigger to auxiliary function. But
current design is simply and robust with few possible user errors.
By not imitating past mistakes, maybe we can eventually arrive at a
language that makes sense.
Sorry I disagree. Can be subjective is this API is too or not too bad for
redesign. More objective arguments - there are no performance issue, no
security issue. I am thinking, so it has sense, so I don't see reason why
to change it and why we should to have two API. Last argument, if we change
something, then we should to use a ANSI SQL syntax everywhere it is
possible (when we don't get any new special functionality).
Regards
Pavel
Show quoted text
Regards,
Marti
Marti Raudsepp <marti@juffo.org> wrote:
The real point I was trying to convey (in my previous email) is
that these declarations should be part of the trigger *function*
not the function-to-table relationship. CREATE TRIGGER shouldn't
be in the business of declaring new local variables for the
trigger function. Whether we define new syntax for that or re-use
the argument list is secondary.
I think the reason the standard includes it in the trigger
definition is that it allows the trigger code to be specified in
the CREATE TRIGGER statement; and in fact, PostgreSQL is the first
database product I have used which requires a trigger function to
be created first.
<triggered action> ::=
[ FOR EACH { ROW | STATEMENT } ]
[ WHEN <left paren> <search condition> <right paren> ]
<triggered SQL statement>
<triggered SQL statement> ::=
<SQL procedure statement>
| BEGIN ATOMIC { <SQL procedure statement> <semicolon> }... END
If we ever support this part of the standard, we need to be able to
specify these in the CREATE TRIGGER statement (or stay with the
concept of reserving "special names" for these relations). So if
we use non-standard syntax now, we are likely to be looking at an
ugly hybrid of techniques in the future.
That said, if we want to have a way to specify this for a function
(for now), we could use the CREATE FUNCTION's WITH clause. I
assume that a mismatch between the name specified there and the
name which is used in the function body would be a run-time error
when the trigger function is fired, not an error at the time that
CREATE FUNCTION is run, since we only ensure valid syntax at CREATE
FUNCTION time; we don't resolve any table names or column names at
that time. For example, on a freshly-created database with no
tables created:
test=# create function xxx() returns trigger language plpgsql
test-# as $$ begin update adsf set qwerty = 'xxx'; end; $$;
CREATE FUNCTION
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 21 June 2014 23:36, Kevin Grittner <kgrittn@ymail.com> wrote:
Kevin Grittner <kgrittn@ymail.com> wrote:
I didn't change the tuplestores to TID because it seemed to me that
it would preclude using transition relations with FDW triggers, and
it seemed bad not to support that. Does anyone see a way around
that, or feel that it's OK to not support FDW triggers in this
regard?
I think it is ok to use tuplestores for now, but as mentioned by you
somewhere else in the thread, later on we should change this to using
tids as an optimization.
Does this look good otherwise, as far as it goes?
I didn't yet extensively go through the patch, but before that, just a
few quick comments:
I see that the tupelstores for transition tables are stored per query
depth. If the
DML involves a table that has multiple child tables, it seems as though
a single tuplestore would be shared among all these tables. That means
if we define
such triggers using transition table clause for all the child tables, then
the trigger function for a child table will see tuples from other child tables
as well. Is that true ? If it is, it does not make sense.
For fdw tuplestore, this issue does not arise because the DML won't involve
multiple target tables I suppose.
-----------
I tried to google some SQLs that use REFERENCING clause with triggers.
It looks like in some database systems, even the WHEN clause of CREATE TRIGGER
can refer to a transition table, just like how it refers to NEW and
OLD row variables.
For e.g. :
CREATE TRIGGER notify_dept
AFTER UPDATE ON weather
REFERENCING NEW_TABLE AS N_TABLE
NEW AS N_ROW
FOR EACH ROW
WHEN ((SELECT AVG (temperature) FROM N_TABLE) > 10)
BEGIN
notify_department(N_ROW.temperature, N_ROW.city);
END
Above, it is used to get an aggregate value of all the changed rows. I think
we do not currently support aggregate expressions in the where clause, but with
transition tables, it makes more sense to support it later if not now.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thanks for your review and comments, Amit!
Amit Khandekar <amit.khandekar@enterprisedb.com> wrote:
On 21 June 2014 23:36, Kevin Grittner <kgrittn@ymail.com> wrote:
Kevin Grittner <kgrittn@ymail.com> wrote:
I didn't change the tuplestores to TID because it seemed to me that
it would preclude using transition relations with FDW triggers, and
it seemed bad not to support that. Does anyone see a way around
that, or feel that it's OK to not support FDW triggers in this
regard?I think it is ok to use tuplestores for now, but as mentioned by you
somewhere else in the thread, later on we should change this to using
tids as an optimization.
Well, the optimization would probably be to use a tuplestore of
tids referencing modified tuples in the base table, rather than a
tuplestore of the data itself. But I think we're in agreement.
Does this look good otherwise, as far as it goes?
I didn't yet extensively go through the patch, but before that, just a
few quick comments:I see that the tupelstores for transition tables are stored per query
depth. If the
DML involves a table that has multiple child tables, it seems as though
a single tuplestore would be shared among all these tables. That means
if we define
such triggers using transition table clause for all the child tables, then
the trigger function for a child table will see tuples from other child tables
as well. Is that true ?
I don't think so. I will make a note of the concern to confirm by testing.
If it is, it does not make sense.
I agree.
I tried to google some SQLs that use REFERENCING clause with triggers.
It looks like in some database systems, even the WHEN clause of CREATE TRIGGER
can refer to a transition table, just like how it refers to NEW and
OLD row variables.For e.g. :
CREATE TRIGGER notify_dept
AFTER UPDATE ON weather
REFERENCING NEW_TABLE AS N_TABLE
NEW AS N_ROW
FOR EACH ROW
WHEN ((SELECT AVG (temperature) FROM N_TABLE) > 10)
BEGIN
notify_department(N_ROW.temperature, N_ROW.city);
ENDAbove, it is used to get an aggregate value of all the changed rows. I think
we do not currently support aggregate expressions in the where clause, but with
transition tables, it makes more sense to support it later if not now.
Interesting point; I had not thought about that. Will see if I can
include support for that in the patch for the next CF; failing
that; I will at least be careful to not paint myself into a corner
where it is unduly hard to do later.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 7 August 2014 19:49, Kevin Grittner <kgrittn@ymail.com> wrote:
Amit Khandekar <amit.khandekar@enterprisedb.com> wrote:
On 21 June 2014 23:36, Kevin Grittner <kgrittn@ymail.com> wrote:
Kevin Grittner <kgrittn@ymail.com> wrote:
I didn't change the tuplestores to TID because it seemed to me that
it would preclude using transition relations with FDW triggers, and
it seemed bad not to support that. Does anyone see a way around
that, or feel that it's OK to not support FDW triggers in this
regard?I think it is ok to use tuplestores for now, but as mentioned by you
somewhere else in the thread, later on we should change this to using
tids as an optimization.Well, the optimization would probably be to use a tuplestore of
tids referencing modified tuples in the base table, rather than a
tuplestore of the data itself. But I think we're in agreement.
Right, that's what I meant.
I see that the tupelstores for transition tables are stored per query
depth. If the
DML involves a table that has multiple child tables, it seems as though
a single tuplestore would be shared among all these tables. That means
if we define
such triggers using transition table clause for all the child tables, then
the trigger function for a child table will see tuples from other child tables
as well. Is that true ?I don't think so. I will make a note of the concern to confirm by testing.
Thanks. I will wait for this.
I tried to google some SQLs that use REFERENCING clause with triggers.
It looks like in some database systems, even the WHEN clause of CREATE TRIGGER
can refer to a transition table, just like how it refers to NEW and
OLD row variables.For e.g. :
CREATE TRIGGER notify_dept
AFTER UPDATE ON weather
REFERENCING NEW_TABLE AS N_TABLE
NEW AS N_ROW
FOR EACH ROW
WHEN ((SELECT AVG (temperature) FROM N_TABLE) > 10)
BEGIN
notify_department(N_ROW.temperature, N_ROW.city);
ENDAbove, it is used to get an aggregate value of all the changed rows. I think
we do not currently support aggregate expressions in the where clause, but with
transition tables, it makes more sense to support it later if not now.Interesting point; I had not thought about that. Will see if I can
include support for that in the patch for the next CF; failing
that; I will at least be careful to not paint myself into a corner
where it is unduly hard to do later.
We currently do the WHEN checks while saving the AFTER trigger events,
and also add the tuples one by one while saving the trigger events. If
and when we support WHEN, we would need to make all of these tuples
saved *before* the first WHEN clause execution, and that seems to
demand more changes in the current trigger code.
More comments below :
---------------
Are we later going to extend this support for constraint triggers as
well ? I think these variables would make sense even for deferred
constraint triggers. I think we would need some more changes if we
want to support this, because there is no query depth while executing
deferred triggers and so the tuplestores might be inaccessible with
the current design.
---------------
The following (and similarly other) statements :
trigdesc->trig_insert_new_table |=
(TRIGGER_FOR_INSERT(tgtype) &&
TRIGGER_USES_TRANSITION_TABLE(trigger->tgnewtable)) ? true : false;
can be simplfied to :
trigdesc->trig_insert_new_table |=
(TRIGGER_FOR_INSERT(tgtype) &&
TRIGGER_USES_TRANSITION_TABLE(trigger->tgnewtable));
-------------
AfterTriggerExecute()
{
.....
/*
* Set up the tuplestore information.
*/
if (trigdesc->trig_delete_old_table || trigdesc->trig_update_old_table)
LocTriggerData.tg_olddelta =
GetCurrentTriggerDeltaTuplestore(afterTriggers->old_tuplestores);
.....
}
Above, trigdesc->trig_update_old_table is true if at least one of the
triggers of the table has transition tables. So this will cause the
delta table to be available on all of the triggers of the table even
if only one out of them uses transition tables. May be this would be
solved by using LocTriggerData.tg_trigger->tg_olddelta rather than
trigdesc->trig_update_old_table to decide whether
LocTriggerData.tg_olddelta should be assigned.
---------------
GetCurrentTriggerDeltaTuplestore() is now used for getting fdw
tuplestore also, so it should have a more general name.
---------------
#define TRIGGER_USES_TRANSITION_TABLE(namepointer) \
((namepointer) != (char *) NULL && (*(namepointer)) != '\0')
Since all other code sections assume trigger->tgoldtable to be
non-NULL, we can skip the NULL check above.
---------------
We should add a check to make sure the user does not supply same name
for OLD TABLE and NEW TABLE.
---------------
The below code comment needs to be changed.
* Only tables are initially supported, and only for AFTER EACH STATEMENT
* triggers, but other permutations are accepted by the parser so we can give
* a meaningful message from C code.
The comment implies that with ROW triggers we do not support TABLE
transition variables. But the patch does make these variables visible
through ROW triggers.
--------------
Other than these, there are no more issues that I could find.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Amit Khandekar <amit.khandekar@enterprisedb.com> wrote:
On 7 August 2014 19:49, Kevin Grittner <kgrittn@ymail.com> wrote:
Amit Khandekar <amit.khandekar@enterprisedb.com> wrote:
I tried to google some SQLs that use REFERENCING clause with triggers.
It looks like in some database systems, even the WHEN clause of CREATE TRIGGER
can refer to a transition table, just like how it refers to NEW and
OLD row variables.For e.g. :
CREATE TRIGGER notify_dept
AFTER UPDATE ON weather
REFERENCING NEW_TABLE AS N_TABLE
NEW AS N_ROW
FOR EACH ROW
WHEN ((SELECT AVG (temperature) FROM N_TABLE) > 10)
BEGIN
notify_department(N_ROW.temperature, N_ROW.city);
ENDAbove, it is used to get an aggregate value of all the changed rows. I think
we do not currently support aggregate expressions in the where clause, but with
transition tables, it makes more sense to support it later if not now.Interesting point; I had not thought about that. Will see if I can
include support for that in the patch for the next CF; failing
that; I will at least be careful to not paint myself into a corner
where it is unduly hard to do later.We currently do the WHEN checks while saving the AFTER trigger events,
and also add the tuples one by one while saving the trigger events. If
and when we support WHEN, we would need to make all of these tuples
saved *before* the first WHEN clause execution, and that seems to
demand more changes in the current trigger code.
In that case my inclination is to get things working with the less
invasive patch that doesn't try to support transition table usage
in WHEN clauses, and make support for that a later patch.
---------------
Are we later going to extend this support for constraint triggers as
well ? I think these variables would make sense even for deferred
constraint triggers. I think we would need some more changes if we
want to support this, because there is no query depth while executing
deferred triggers and so the tuplestores might be inaccessible with
the current design.
Hmm, I would also prefer to exclude that from an initial patch, but
this and the WHEN clause issue may influence a decision I've been
struggling with. This is my first non-trivial foray into the
planner territory, and I've been struggling with how best to bridge
the gap between where the tuplestores are *captured* in the trigger
code and where they are referenced by name in a query and
incorporated into a plan for the executor. (The execution level
itself was almost trivial; it's getting the tuplestore reference
through the parse analysis and planning phases that is painful for
me.) At one point I create a "tuplestore registry" using a
process-local hashmap where each Tuplestorestate and its associated
name, TupleDesc, etc. would be registered, yielding a Tsrid (based
on Oid) to use through the parse analysis and planning steps, but
then I ripped it back out again in favor of just passing the
pointer to the structure which was stored in the registry; because
the registry seemed to me to introduce more risk of memory leaks,
references to freed memory, etc. While it helped a little with
abstraction, it seemed to make things more fragile. But I'm still
torn on this, and unsure whether such a registry is a good idea.
Any thoughts on that?
---------------
The following (and similarly other) statements :
trigdesc->trig_insert_new_table |=
(TRIGGER_FOR_INSERT(tgtype) &&
TRIGGER_USES_TRANSITION_TABLE(trigger->tgnewtable)) ? true : false;can be simplfied to :
trigdesc->trig_insert_new_table |=
(TRIGGER_FOR_INSERT(tgtype) &&
TRIGGER_USES_TRANSITION_TABLE(trigger->tgnewtable));
Yeah, I did recognize that, but I always get squeamish about
logical operations with values which do not equal true or false.
TRIGGER_FOR_INSERT and similar macros don't necessarily return true
for something which is not false. I should just get over that and
trust the compiler a bit more.... :-)
---------------
AfterTriggerExecute()
{
.....
/*
* Set up the tuplestore information.
*/
if (trigdesc->trig_delete_old_table || trigdesc->trig_update_old_table)
LocTriggerData.tg_olddelta =
GetCurrentTriggerDeltaTuplestore(afterTriggers->old_tuplestores);
.....
}
Above, trigdesc->trig_update_old_table is true if at least one of the
triggers of the table has transition tables. So this will cause the
delta table to be available on all of the triggers of the table even
if only one out of them uses transition tables. May be this would be
solved by using LocTriggerData.tg_trigger->tg_olddelta rather than
trigdesc->trig_update_old_table to decide whether
LocTriggerData.tg_olddelta should be assigned.
Good point. Will do.
---------------
GetCurrentTriggerDeltaTuplestore() is now used for getting fdw
tuplestore also, so it should have a more general name.
Well, "delta" *was* my attempt at a more general name. I need to
do another pass over the naming choices to make sure I'm being
consistent, but I attempted to use these distinctions:
transition - OLD or NEW, ROW or TABLE data for a trigger; this is
the terminology used in the SQL standard
oldtable/newtable - transition data for a relation representing
what a statement affected; corresponding to the REFERENCING
[OLD|NEW] TABLE clauses in the SQL standard
tuplestore - for the planner and executor, since we may find
other uses for feeding in tuplestores; I don't want to assume in
the naming there that these are from triggers at all
delta - a tuplestore representing changes to data; perhaps this
is too close in concept to transition in the trigger code since
there is no other source for delta data and the naming should
just use transition
Any specific suggestions? Maybe eliminate use of "delta" and make
that GetCurrentTriggerTransitionTuplestore()?
---------------
#define TRIGGER_USES_TRANSITION_TABLE(namepointer) \
((namepointer) != (char *) NULL && (*(namepointer)) != '\0')
Since all other code sections assume trigger->tgoldtable to be
non-NULL, we can skip the NULL check above.
I intentionally left both tests in initially because I wasn't sure
which representation would be used. Will review whether it is time
to get off the fence on that. ;-)
---------------
We should add a check to make sure the user does not supply same name
for OLD TABLE and NEW TABLE.
I already caught that and implemented a check in my development code.
---------------
The below code comment needs to be changed.
* Only tables are initially supported, and only for AFTER EACH STATEMENT
* triggers, but other permutations are accepted by the parser so we can give
* a meaningful message from C code.
The comment implies that with ROW triggers we do not support TABLE
transition variables. But the patch does make these variables visible
through ROW triggers.
Oops. Will fix.
Thanks for the review!
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12 August 2014 20:09, Kevin Grittner <kgrittn@ymail.com> wrote:
Amit Khandekar <amit.khandekar@enterprisedb.com> wrote:
On 7 August 2014 19:49, Kevin Grittner <kgrittn@ymail.com> wrote:
Amit Khandekar <amit.khandekar@enterprisedb.com> wrote:
I tried to google some SQLs that use REFERENCING clause with triggers.
It looks like in some database systems, even the WHEN clause of CREATE TRIGGER
can refer to a transition table, just like how it refers to NEW and
OLD row variables.For e.g. :
CREATE TRIGGER notify_dept
AFTER UPDATE ON weather
REFERENCING NEW_TABLE AS N_TABLE
NEW AS N_ROW
FOR EACH ROW
WHEN ((SELECT AVG (temperature) FROM N_TABLE) > 10)
BEGIN
notify_department(N_ROW.temperature, N_ROW.city);
ENDAbove, it is used to get an aggregate value of all the changed rows. I think
we do not currently support aggregate expressions in the where clause, but with
transition tables, it makes more sense to support it later if not now.Interesting point; I had not thought about that. Will see if I can
include support for that in the patch for the next CF; failing
that; I will at least be careful to not paint myself into a corner
where it is unduly hard to do later.We currently do the WHEN checks while saving the AFTER trigger events,
and also add the tuples one by one while saving the trigger events. If
and when we support WHEN, we would need to make all of these tuples
saved *before* the first WHEN clause execution, and that seems to
demand more changes in the current trigger code.In that case my inclination is to get things working with the less
invasive patch that doesn't try to support transition table usage
in WHEN clauses, and make support for that a later patch.
Agreed.
---------------
Are we later going to extend this support for constraint triggers as
well ? I think these variables would make sense even for deferred
constraint triggers. I think we would need some more changes if we
want to support this, because there is no query depth while executing
deferred triggers and so the tuplestores might be inaccessible with
the current design.Hmm, I would also prefer to exclude that from an initial patch, but
this and the WHEN clause issue may influence a decision I've been
struggling with. This is my first non-trivial foray into the
planner territory, and I've been struggling with how best to bridge
the gap between where the tuplestores are *captured* in the trigger
code and where they are referenced by name in a query and
incorporated into a plan for the executor. (The execution level
itself was almost trivial; it's getting the tuplestore reference
through the parse analysis and planning phases that is painful for
me.
I am not sure why you think we would need to refer the tuplestore in
the parse analysis and planner phases. It seems that we would need
them only in execution phase. Or may be I didn't understand your
point.
) At one point I create a "tuplestore registry" using a
process-local hashmap where each Tuplestorestate and its associated
name, TupleDesc, etc. would be registered, yielding a Tsrid (based
on Oid) to use through the parse analysis and planning steps, but
then I ripped it back out again in favor of just passing the
pointer to the structure which was stored in the registry; because
the registry seemed to me to introduce more risk of memory leaks,
references to freed memory, etc. While it helped a little with
abstraction, it seemed to make things more fragile. But I'm still
torn on this, and unsure whether such a registry is a good idea.
I feel it is ok to use direct tuplestore pointers as handles like how
you have done in the patch. I may be biased with doing that as against
the above method of accessing tuplestore by its name through hash
lookup; the reason of my bias might be because of one particular way I
see how deferred constraint triggers can be supported. In the after
trigger event structure, we can store these delta tuplestores pointers
as well. This way, we don't need to worry about how to lookup these
tuplestores, and also need not worry about the mechanism that moves
these events from deferred event list to immediate event list in case
of SET CONSTRAINTS. Only thing we would need to make sure is to
cleanup these tuplestores exactly where the event structures get
cleaned up.
This is all my speculations. But what I think is that we don't have to
heavily refactor your patch changes in order to extend support for
deferred constraint triggers. And for WHEN clause, we anyways have to
do some changes in the existing trigger code.
---------------
The following (and similarly other) statements :
trigdesc->trig_insert_new_table |=
(TRIGGER_FOR_INSERT(tgtype) &&
TRIGGER_USES_TRANSITION_TABLE(trigger->tgnewtable)) ? true : false;can be simplfied to :
trigdesc->trig_insert_new_table |=
(TRIGGER_FOR_INSERT(tgtype) &&
TRIGGER_USES_TRANSITION_TABLE(trigger->tgnewtable));Yeah, I did recognize that, but I always get squeamish about
logical operations with values which do not equal true or false.
TRIGGER_FOR_INSERT and similar macros don't necessarily return true
for something which is not false. I should just get over that and
trust the compiler a bit more.... :-)
I am ok with either way :) . May be your change is more readable to
others as well, and it does seem to be readable to me now that I see
again.
Well, "delta" *was* my attempt at a more general name. I need to
do another pass over the naming choices to make sure I'm being
consistent, but I attempted to use these distinctions:transition - OLD or NEW, ROW or TABLE data for a trigger; this is
the terminology used in the SQL standard
Ah ok, so transition applies to both OLD/NEW and ROW/TABLE. Then is
that case, the one you suggested below
(GetCurrentTriggerTransitionTuplestore) sounds very good to me.
oldtable/newtable - transition data for a relation representing
what a statement affected; corresponding to the REFERENCING
[OLD|NEW] TABLE clauses in the SQL standardtuplestore - for the planner and executor, since we may find
other uses for feeding in tuplestores; I don't want to assume in
the naming there that these are from triggers at alldelta - a tuplestore representing changes to data; perhaps this
is too close in concept to transition in the trigger code since
there is no other source for delta data and the naming should
just use transition
All these names sound good to me. Thanks.
Any specific suggestions? Maybe eliminate use of "delta" and make
that GetCurrentTriggerTransitionTuplestore()?
Yes this sounds good.
---------------
#define TRIGGER_USES_TRANSITION_TABLE(namepointer) \
((namepointer) != (char *) NULL && (*(namepointer)) != '\0')
Since all other code sections assume trigger->tgoldtable to be
non-NULL, we can skip the NULL check above.I intentionally left both tests in initially because I wasn't sure
which representation would be used. Will review whether it is time
to get off the fence on that. ;-)
OK.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
The execution level
itself was almost trivial; it's getting the tuplestore reference
through the parse analysis and planning phases that is painful for
me.I am not sure why you think we would need to refer the tuplestore in
the parse analysis and planner phases. It seems that we would need
them only in execution phase. Or may be I didn't understand your
point.
Ah I think I understand now. That might be because you are thinking of
having an infrastructure common to triggers and materialized views,
right ?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Amit Khandekar <amit.khandekar@enterprisedb.com> wrote:
The execution level itself was almost trivial; it's getting the
tuplestore reference through the parse analysis and planning
phases that is painful for me.I am not sure why you think we would need to refer the
tuplestore in the parse analysis and planner phases. It seems
that we would need them only in execution phase. Or may be I
didn't understand your point.Ah I think I understand now. That might be because you are
thinking of having an infrastructure common to triggers and
materialized views, right ?
Well, it's more immediate than that. The identifiers in the
trigger function are not resolved to particular objects until there
is a request to fire the trigger. At that time the parse analysis
needs to find the name defined somewhere. It's not defined in the
catalogs like a table or view, and it's not defined in the query
itself like a CTE or VALUES clause. The names specified in trigger
creation must be recognized as needing to resolve to the new
TuplestoreScan, and it needs to match those to the tuplestores
themselves. Row counts, costing, etc. needs to be provided so the
optimizer can pick a good plan in what might be a complex query
with many options. I'm finding the planner work here to be harder
than everything else put together.
On the bright side, once I'm done, I might know enough about the
planner to do things a lot faster next time. :-)
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 15 August 2014 04:04, Kevin Grittner <kgrittn@ymail.com> wrote:
Amit Khandekar <amit.khandekar@enterprisedb.com> wrote:
The execution level itself was almost trivial; it's getting the
tuplestore reference through the parse analysis and planning
phases that is painful for me.I am not sure why you think we would need to refer the
tuplestore in the parse analysis and planner phases. It seems
that we would need them only in execution phase. Or may be I
didn't understand your point.Ah I think I understand now. That might be because you are
thinking of having an infrastructure common to triggers and
materialized views, right ?Well, it's more immediate than that. The identifiers in the
trigger function are not resolved to particular objects until there
is a request to fire the trigger.
Ah ok, you are talking about changes specific to the PL language
handlers. Yes, I agree that in the plpgsql parser (and in any PL
handler), we need to parse such table references in the SQL construct,
and transform it into something else.
At that time the parse analysis
needs to find the name defined somewhere. It's not defined in the
catalogs like a table or view, and it's not defined in the query
itself like a CTE or VALUES clause. The names specified in trigger
creation must be recognized as needing to resolve to the new
TuplestoreScan, and it needs to match those to the tuplestores
themselves.
One approach that comes to my mind is by transforming such transition
table references into a RangeFunction reference while in plpgsql
parser/lexer. This RangeFunction would point to a set-returning
catalog function that would return rows from the delta tuplestore. So
the tuplestore itself would remain a blackbox. Once we have such a
function, any language handler can re-use the same interface.
Row counts, costing, etc. needs to be provided so the
optimizer can pick a good plan in what might be a complex query
with many options.
I am also not sure about the costing, but I guess it may be possible
to supply some costs to the FunctionScan plan node.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Amit Khandekar <amit.khandekar@enterprisedb.com> wrote:
On 15 August 2014 04:04, Kevin Grittner <kgrittn@ymail.com> wrote:
The identifiers in the trigger function are not resolved to
particular objects until there is a request to fire the trigger.Ah ok, you are talking about changes specific to the PL language
handlers. Yes, I agree that in the plpgsql parser (and in any PL
handler), we need to parse such table references in the SQL construct,
and transform it into something else.At that time the parse analysis
needs to find the name defined somewhere. It's not defined in the
catalogs like a table or view, and it's not defined in the query
itself like a CTE or VALUES clause. The names specified in trigger
creation must be recognized as needing to resolve to the new
TuplestoreScan, and it needs to match those to the tuplestores
themselves.
For now I have added the capability to register named tuplestores
(with the associated TupleDesc) to SPI. This seems like a pretty
useful place to be able to do this, although I tried to arrange for
it to be reasonably easy to add other registration mechanisms
later.
One approach that comes to my mind is by transforming such transition
table references into a RangeFunction reference while in plpgsql
parser/lexer. This RangeFunction would point to a set-returning
catalog function that would return rows from the delta tuplestore. So
the tuplestore itself would remain a blackbox. Once we have such a
function, any language handler can re-use the same interface.
plpgsql seems entirely the wrong place for that. What I have done
is for trigger.c to add the tuplestores to the TriggerData
structure, and not worry about it beyond that. I have provided a
way to register named tuplestores to SPI, and for the subsequent
steps to recognize those names as relation names. The change to
plpgsql are minimal, since they only need to take the tuplestores
from TriggerData and register them with SPI before making the SPI
calls to plan and execute.
Row counts, costing, etc. needs to be provided so the
optimizer can pick a good plan in what might be a complex query
with many options.I am also not sure about the costing, but I guess it may be possible
to supply some costs to the FunctionScan plan node.
I went with a bogus row estimate for now. I think we can arrange
for a tuplestore to keep a count of rows added, and a function to
retrieve that, and thereby get a better estimate, but that is not
yet done.
I think this is approaching a committable state, although I think
it should probably be broken down to four separate patches. Two of
them are very small and easy to generate: the SPI changes and the
plpgsql changes are in files that are distinct from all the other
changes. What remains is to separate the parsing of the CREATE
TRIGGER statement and the trigger code that generates the
tuplestores for the transition tables from the analysis, planning,
and execution phases which deal with a more generic concept of
named tuplestores. Some of the changes for those two things both
touch files related to parsing -- for different things, but in the
same files.
To avoid polluting paring code with SPI and tuplestore includes, I
created a very thin abstraction layer for parse analysis to use. I
didn't do the same for the executor yet, but it may be a good idea
there, too.
It probably still needs more documentation and definitely needs
more regression tests. I have used these transition tables in
plpgsql in simple tests, but there may be bugs still lurking.
New patch attached.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
Kevin Grittner <kgrittn@ymail.com> wrote:
I think this is approaching a committable state, although I think
it should probably be broken down to four separate patches.
And here they are. This should net to the same set of changes as
the prior post, but the changes are logically separated. They are
labeled as v3 to match the last post.
trigger-transition-tables allows transition table names to be
specified in a REFERENCING clause of CREATE TRIGGER, per spec, and
creates tuplestores when needed in the TriggerData structure. It
doesn't worry about who does what with that data. This doesn't
depend on anything else.
15 files changed, 577 insertions(+), 43 deletions(-)
spi-tuplestore-registry allows tuplestores, with associated name
and TupleDesc, to be registered with the current SPI connection.
Queries planned or executed on that connection will recognize the
name as a tuplestore relation. It doesn't care who is registering
the tuplestores or what happens to them. It doesn't depend on
anything else.
5 files changed, 445 insertions(+)
executor-tuplestore-relations covers parse analysis,
planner/optimizer, and executor layers. It pulls information from
the registry in a couple places, but is not very intertwined with
SPI. That is the only registry it knows right now, but it should
be easy to add other registries if needed. It doesn't care where
the tuplestore came from, so we should be able to use this for
other things. I have it in mind to use it for incremental
maintenance of materialized views, but I expect that other uses
will be found. It has a logical dependency on the
spi-tuplestore-registry patch. While it doesn't have a logical
dependency on trigger-transition-tables, they both modified some of
the same files, and this won't apply cleanly unless
trigger-transition-tables is applied first. If you hand-correct
the failed hunks, it compiles and runs fine without
trigger-transition-tables.
30 files changed, 786 insertions(+), 9 deletions(-)
plpgsql-after-trigger-transition-tables takes the tuplestores from
TriggerData and registers them with SPI before trigger planning and
execution. It depends on the trigger-transition-tables and
spi-tuplestore-registry patches to build, and won't do anything
useful at run time without the executor-tuplestore-relations patch.
3 files changed, 37 insertions(+), 11 deletions(-)
Hopefully this will make review easier.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
On 08/27/2014 02:26 AM, Kevin Grittner wrote:
spi-tuplestore-registry allows tuplestores, with associated name
and TupleDesc, to be registered with the current SPI connection.
Queries planned or executed on that connection will recognize the
name as a tuplestore relation. It doesn't care who is registering
the tuplestores or what happens to them. It doesn't depend on
anything else.
5 files changed, 445 insertions(+)...
plpgsql-after-trigger-transition-tables takes the tuplestores from
TriggerData and registers them with SPI before trigger planning and
execution. It depends on the trigger-transition-tables and
spi-tuplestore-registry patches to build, and won't do anything
useful at run time without the executor-tuplestore-relations patch.
3 files changed, 37 insertions(+), 11 deletions(-)
This is a surprising way to expose the NEW/OLD relations to the
planner/executor. The problem is the same as with making PL/pgSQL
variables available to the planner/executor in queries within a PL/pgSQL
function, and the solution we have for that is the "parser hooks" you
pass to SPI_prepare_params. This tuplestore registry is a different
solution to the same problem - we could've implemented parameters with a
registry like this as well. Let's not mix two different designs.
I suggest adding a new hook to the ParseState struct, (p_rangevar_hook
?). The planner calls it whenever it sees a reference to a table, and
the hook function returns back some sort of placeholder reference to the
tuplestore. With variables, the hook returns a Param node, and at
execution time, the executor calls the paramFetch hook to fetch the
value of the param. For relations/tuplestores, I guess we'll need to
invent something like a Param node, but for holding information about
the relation. Like your TsrData struct, but without the pointer to the
tuplestore. At execution time, in the SPI_execute call, you pass the
pointer to the tuplestore in the ParamListInfo struct, like you pass
parameter values.
Does this make sense? In essence, make the relations work like PL/pgSQL
variables do. If you squint a little, the new/old relation is a variable
from the function's point of view, and a parameter from the
planner/executor's point of view. It's just a variable/parameter that
holds a set of tuples, instead of a single Datum.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 8/27/14, 2:23 AM, Heikki Linnakangas wrote:
Does this make sense? In essence, make the relations work like PL/pgSQL variables do. If you squint a little, the new/old relation is a variable from the function's point of view, and a parameter from the planner/executor's point of view. It's just a variable/parameter that holds a set of tuples, instead of a single Datum.
Something to keep in mind is that users will definitely think about NEW/OLD as tables. I suspect that it won't be long after release before someone asks why they can't create an index on it. :)
--
Jim C. Nasby, Data Architect jim@nasby.net
512.569.9461 (cell) http://jim.nasby.net
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 27, 2014 at 11:51:40AM -0500, Jim Nasby wrote:
On 8/27/14, 2:23 AM, Heikki Linnakangas wrote:
Does this make sense? In essence, make the relations work like
PL/pgSQL variables do. If you squint a little, the new/old relation
is a variable from the function's point of view, and a parameter
from the planner/executor's point of view. It's just a
variable/parameter that holds a set of tuples, instead of a single
Datum.Something to keep in mind is that users will definitely think about
NEW/OLD as tables. I suspect that it won't be long after release
before someone asks why they can't create an index on it. :)
Continuing with this digression, that request seems more likely with
views and foreign tables, given that they persist across statements.
I'm given to understand that other systems have at least the former.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
On 08/27/2014 02:26 AM, Kevin Grittner wrote:
spi-tuplestore-registry allows tuplestores, with associated name
and TupleDesc, to be registered with the current SPI connection.
Queries planned or executed on that connection will recognize the
name as a tuplestore relation. It doesn't care who is registering
the tuplestores or what happens to them. It doesn't depend on
anything else.
5 files changed, 445 insertions(+)...
plpgsql-after-trigger-transition-tables takes the tuplestores from
TriggerData and registers them with SPI before trigger planning and
execution. It depends on the trigger-transition-tables and
spi-tuplestore-registry patches to build, and won't do anything
useful at run time without the executor-tuplestore-relations patch.
3 files changed, 37 insertions(+), 11 deletions(-)This is a surprising way to expose the NEW/OLD relations to the
planner/executor. The problem is the same as with making PL/pgSQL
variables available to the planner/executor in queries within a PL/pgSQL
function, and the solution we have for that is the "parser hooks" you
pass to SPI_prepare_params. This tuplestore registry is a different
solution to the same problem - we could've implemented parameters with a
registry like this as well. Let's not mix two different designs.I suggest adding a new hook to the ParseState struct, (p_rangevar_hook
?). The planner calls it whenever it sees a reference to a table, and
the hook function returns back some sort of placeholder reference to the
tuplestore. With variables, the hook returns a Param node, and at
execution time, the executor calls the paramFetch hook to fetch the
value of the param. For relations/tuplestores, I guess we'll need to
invent something like a Param node, but for holding information about
the relation. Like your TsrData struct, but without the pointer to the
tuplestore. At execution time, in the SPI_execute call, you pass the
pointer to the tuplestore in the ParamListInfo struct, like you pass
parameter values.Does this make sense?
I see your point, but SPI first has to be made aware of the
tuplestores and their corresponding names and TupleDesc structures.
Does it make sense to keep the SPI_register_tuplestore() and
SPI_unregister_tuplestore() functions for the client side of the
API, and pass things along to the parse analysis through execution
phases using the techniques you describe? That would eliminate the
need for the SPI_get_caller_tuplestore() function and the
parse_tuplestore.[ch] files, and change how the data is fetched in
parse analysis and execution phases, but that seems fairly minimal
-- there are exactly three places that would need to call the new
hooks where the patch is now getting the information from the
registry.
In essence, make the relations work like PL/pgSQL
variables do. If you squint a little, the new/old relation is a variable
from the function's point of view, and a parameter from the
planner/executor's point of view. It's just a variable/parameter that
holds a set of tuples, instead of a single Datum.
I don't have to squint that hard -- I've always been comfortable
with the definition of a table as a relation variable, and it's not
too big a stretch to expand that to a tuplestore. ;-) In fact, I
will be surprised if someone doesn't latch onto this to create a
new "declared temporary table" that only exists within the scope of
a compound statement (i.e., a BEGIN/END block). You would DECLARE
them just like you would a scalar variable in a PL, and they would
have the same scope.
I'll take a look at doing this in the next couple days, and see
whether doing it that way is as easy as it seems on the face of it.
Thanks!
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jim Nasby <jim@nasby.net> wrote:
Something to keep in mind is that users will definitely think about NEW/OLD as
tables. I suspect that it won't be long after release before someone asks
why they can't create an index on it. :)
I'm comfortable saying "No" to that. But it's a good point -- I'll
review error checking and documentation to make sure that it is
clear.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Kevin Grittner (kgrittn@ymail.com) wrote:
Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
In essence, make the relations work like PL/pgSQL
variables do. If you squint a little, the new/old relation is a variable
from the function's point of view, and a parameter from the
planner/executor's point of view. It's just a variable/parameter that
holds a set of tuples, instead of a single Datum.I don't have to squint that hard -- I've always been comfortable
with the definition of a table as a relation variable, and it's not
too big a stretch to expand that to a tuplestore. ;-) In fact, I
will be surprised if someone doesn't latch onto this to create a
new "declared temporary table" that only exists within the scope of
a compound statement (i.e., a BEGIN/END block). You would DECLARE
them just like you would a scalar variable in a PL, and they would
have the same scope.I'll take a look at doing this in the next couple days, and see
whether doing it that way is as easy as it seems on the face of it.
(not following this very closely, but saw this...)
Yes, please? :)
Thanks!
Stephen
On 08/28/2014 12:03 AM, Kevin Grittner wrote:
Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
I suggest adding a new hook to the ParseState struct, (p_rangevar_hook
?). The planner calls it whenever it sees a reference to a table, and
the hook function returns back some sort of placeholder reference to the
tuplestore. With variables, the hook returns a Param node, and at
execution time, the executor calls the paramFetch hook to fetch the
value of the param. For relations/tuplestores, I guess we'll need to
invent something like a Param node, but for holding information about
the relation. Like your TsrData struct, but without the pointer to the
tuplestore. At execution time, in the SPI_execute call, you pass the
pointer to the tuplestore in the ParamListInfo struct, like you pass
parameter values.Does this make sense?
I see your point, but SPI first has to be made aware of the
tuplestores and their corresponding names and TupleDesc structures.
Does it make sense to keep the SPI_register_tuplestore() and
SPI_unregister_tuplestore() functions for the client side of the
API, and pass things along to the parse analysis through execution
phases using the techniques you describe?
Sorry, I didn't understand that. What do you mean by "first", and the
"client side of the API"? I don't see any need for the
SPI_register_tuplestore() and and SPI_unregister_tuplestore() functions
if you use the hooks.
In essence, make the relations work like PL/pgSQL
variables do. If you squint a little, the new/old relation is a variable
from the function's point of view, and a parameter from the
planner/executor's point of view. It's just a variable/parameter that
holds a set of tuples, instead of a single Datum.I don't have to squint that hard -- I've always been comfortable
with the definition of a table as a relation variable, and it's not
too big a stretch to expand that to a tuplestore. ;-) In fact, I
will be surprised if someone doesn't latch onto this to create a
new "declared temporary table" that only exists within the scope of
a compound statement (i.e., a BEGIN/END block). You would DECLARE
them just like you would a scalar variable in a PL, and they would
have the same scope.
Yeah, that would be cool :-).
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Aug 28, 2014 at 12:03 AM, Kevin Grittner <kgrittn@ymail.com> wrote:
In essence, make the relations work like PL/pgSQL
variables do. If you squint a little, the new/old relation is a variable
from the function's point of view, and a parameter from the
planner/executor's point of view. It's just a variable/parameter that
holds a set of tuples, instead of a single Datum.
will be surprised if someone doesn't latch onto this to create a
new "declared temporary table" that only exists within the scope of
a compound statement (i.e., a BEGIN/END block). You would DECLARE
them just like you would a scalar variable in a PL, and they would
have the same scope.I'll take a look at doing this in the next couple days, and see
whether doing it that way is as easy as it seems on the face of it.
We already have all this with refcursor variables. Admittedly cursors
have some weird semantics (mutable position) and currently they're
cumbersome to combine into queries, but would a new "relation
variable" be sufficiently better to warrant a new type?
Why not extend refcursors and make them easier to use instead?
Regards,
Marti
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
On 08/28/2014 12:03 AM, Kevin Grittner wrote:
Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
I suggest adding a new hook to the ParseState struct, (p_rangevar_hook
?). The planner calls it whenever it sees a reference to a table, and
the hook function returns back some sort of placeholder reference to the
tuplestore. With variables, the hook returns a Param node, and at
execution time, the executor calls the paramFetch hook to fetch the
value of the param. For relations/tuplestores, I guess we'll need to
invent something like a Param node, but for holding information about
the relation. Like your TsrData struct, but without the pointer to the
tuplestore. At execution time, in the SPI_execute call, you pass the
pointer to the tuplestore in the ParamListInfo struct, like you pass
parameter values.Does this make sense?
I see your point, but SPI first has to be made aware of the
tuplestores and their corresponding names and TupleDesc structures.
Does it make sense to keep the SPI_register_tuplestore() and
SPI_unregister_tuplestore() functions for the client side of the
API, and pass things along to the parse analysis through execution
phases using the techniques you describe?Sorry, I didn't understand that. What do you mean by "first", and the
"client side of the API"? I don't see any need for the
SPI_register_tuplestore() and and SPI_unregister_tuplestore() functions
if you use the hooks.
If we were to go with the hooks as you propose, we would still need
to take the information from TriggerData and put it somewhere else
for the hook to reference. The hooks are generalized for plpgsql,
not just for triggers, and it doesn't seem appropriate for them to
be fishing around in the TriggerData structure. And what if we add
other sources for tuplestores? The lookup during parse analysis
each time an apparent relation name is encountered must be simple
and fast.
I want named tuplestores to be easy to use from *all* PLs (for
trigger usage) as well as useful for other purposes people may want
to develop. I had to change the hashkey for plpgsql's plan
caching, but that needs to be done regardless of the API (to
prevent problems in the obscure case that someone attaches the same
trigger function to the same table for the same events more than
once with different trigger names and different transition table
names). If you ignore that, the *entire* change to use this in
plpgsql is to add these lines to plpgsql_exec_trigger():
/*
* Capture the NEW and OLD transition TABLE tuplestores (if specified for
* this trigger).
*/
if (trigdata->tg_newtable)
{
Tsr tsr = palloc(sizeof(TsrData));
tsr->name = trigdata->tg_trigger->tgnewtable;
tsr->tstate = trigdata->tg_newtable;
tsr->tupdesc = trigdata->tg_relation->rd_att;
tsr->relid = trigdata->tg_relation->rd_id;
SPI_register_tuplestore(tsr);
}
if (trigdata->tg_oldtable)
{
Tsr tsr = palloc(sizeof(TsrData));
tsr->name = trigdata->tg_trigger->tgoldtable;
tsr->tstate = trigdata->tg_oldtable;
tsr->tupdesc = trigdata->tg_relation->rd_att;
tsr->relid = trigdata->tg_relation->rd_id;
SPI_register_tuplestore(tsr);
}
With the new SPI functions, the code to implement this in each
other PL should be about the same (possibly identical), and areas
using SPI only need similar code to make tuplestores visible to the
planner and usable in the executor if someone has another use for
this. You just do the above once you have run SPI_connect() and
before preparing or executing any query that references the named
tuplestore. It remains available on that SPI connection until
SPI_finish() is called or you explicitly unregister it (by name).
If we use the hooks, I think it will be several times as much code,
more invasive, and probably more fragile. More importantly, these
hooks are not used by the other PLs included with core, and are not
used in any of the other core code, anywhere. They all use SPI, so
they can do the above very easily, but adding infrastructure for
them to use the hooks would be a lot more work, and I'm not seeing
a corresponding benefit.
I think there is some room to change the API as used by the parser,
planner, and executor so that no changes are needed there if we add
some other registration mechanism besides SPI, but I think having a
registry for tuplestores that sort of takes the place of the
catalogs and related caches (but is far simpler, being process
local) is a better model than what you propose.
In summary, thinking of the definition of a named tuplestore as a
variable is the wrong parallel; it is more like a lightweight
relation, and the comparison should be to that, not to function
parameters or local variables.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@ymail.com> wrote:
Kevin Grittner <kgrittn@ymail.com> wrote:
executor-tuplestore-relations covers parse analysis,
planner/optimizer, and executor layers.
30 files changed, 786 insertions(+), 9 deletions(-)
Testing and further review found a few places that needed to add
lines for the new RTE kind that I had missed. Delta patch
attached.
7 files changed, 58 insertions(+)
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
executor-tuplestore-relations-v3a.difftext/x-diff; name=executor-tuplestore-relations-v3a.diffDownload
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 799242b..2e61edf 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2250,6 +2250,9 @@ JumbleRangeTable(pgssJumbleState *jstate, List *rtable)
APP_JUMB_STRING(rte->ctename);
APP_JUMB(rte->ctelevelsup);
break;
+ case RTE_TUPLESTORE:
+ APP_JUMB_STRING(rte->tsrname);
+ break;
default:
elog(ERROR, "unrecognized RTE kind: %d", (int) rte->rtekind);
break;
@@ -2638,6 +2641,13 @@ JumbleExpr(pgssJumbleState *jstate, Node *node)
JumbleQuery(jstate, (Query *) cte->ctequery);
}
break;
+ case T_TuplestoreRelation:
+ {
+ TuplestoreRelation *tsr = (TuplestoreRelation *) node;
+
+ APP_JUMB_STRING(tsr->name);
+ }
+ break;
case T_SetOperationStmt:
{
SetOperationStmt *setop = (SetOperationStmt *) node;
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 781a736..de31026 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -717,6 +717,7 @@ ExplainPreScanNode(PlanState *planstate, Bitmapset **rels_used)
case T_FunctionScan:
case T_ValuesScan:
case T_CteScan:
+ case T_TuplestoreScan:
case T_WorkTableScan:
case T_ForeignScan:
*rels_used = bms_add_member(*rels_used,
@@ -930,6 +931,9 @@ ExplainNode(PlanState *planstate, List *ancestors,
case T_CteScan:
pname = sname = "CTE Scan";
break;
+ case T_TuplestoreScan:
+ pname = sname = "Tuplestore Scan";
+ break;
case T_WorkTableScan:
pname = sname = "WorkTable Scan";
break;
@@ -1300,6 +1304,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
case T_SeqScan:
case T_ValuesScan:
case T_CteScan:
+ case T_TuplestoreScan:
case T_WorkTableScan:
case T_SubqueryScan:
show_scan_qual(plan->qual, "Filter", planstate, ancestors, es);
@@ -2160,6 +2165,11 @@ ExplainTargetRel(Plan *plan, Index rti, ExplainState *es)
objectname = rte->ctename;
objecttag = "CTE Name";
break;
+ case T_TuplestoreScan:
+ Assert(rte->rtekind == RTE_TUPLESTORE);
+ objectname = rte->tsrname;
+ objecttag = "Tuplestore Name";
+ break;
case T_WorkTableScan:
/* Assert it's on a self-reference CTE */
Assert(rte->rtekind == RTE_CTE);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 93f3905..255348b 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2442,6 +2442,13 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
WRITE_NODE_FIELD(ctecoltypmods);
WRITE_NODE_FIELD(ctecolcollations);
break;
+ case RTE_TUPLESTORE:
+ WRITE_STRING_FIELD(tsrname);
+ WRITE_OID_FIELD(relid);
+ WRITE_NODE_FIELD(ctecoltypes);
+ WRITE_NODE_FIELD(ctecoltypmods);
+ WRITE_NODE_FIELD(ctecolcollations);
+ break;
default:
elog(ERROR, "unrecognized RTE kind: %d", (int) node->rtekind);
break;
diff --git a/src/backend/nodes/print.c b/src/backend/nodes/print.c
index 9f7f322..e11a116 100644
--- a/src/backend/nodes/print.c
+++ b/src/backend/nodes/print.c
@@ -287,6 +287,10 @@ print_rt(const List *rtable)
printf("%d\t%s\t[cte]",
i, rte->eref->aliasname);
break;
+ case RTE_TUPLESTORE:
+ printf("%d\t%s\t[tuplestore]",
+ i, rte->eref->aliasname);
+ break;
default:
printf("%d\t%s\t[unknown rtekind]",
i, rte->eref->aliasname);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 69d9989..bcaeeb0 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1240,6 +1240,13 @@ _readRangeTblEntry(void)
READ_NODE_FIELD(ctecoltypmods);
READ_NODE_FIELD(ctecolcollations);
break;
+ case RTE_TUPLESTORE:
+ READ_STRING_FIELD(tsrname);
+ READ_OID_FIELD(relid);
+ READ_NODE_FIELD(ctecoltypes);
+ READ_NODE_FIELD(ctecoltypmods);
+ READ_NODE_FIELD(ctecolcollations);
+ break;
default:
elog(ERROR, "unrecognized RTE kind: %d",
(int) local_node->rtekind);
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index fb6c44c..518691a 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -2473,6 +2473,15 @@ transformLockingClause(ParseState *pstate, Query *qry, LockingClause *lc,
LCS_asString(lc->strength)),
parser_errposition(pstate, thisrel->location)));
break;
+ case RTE_TUPLESTORE:
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ /*------
+ translator: %s is a SQL row locking clause such as FOR UPDATE */
+ errmsg("%s cannot be applied to a tuplestore",
+ LCS_asString(lc->strength)),
+ parser_errposition(pstate, thisrel->location)));
+ break;
default:
elog(ERROR, "unrecognized RTE type: %d",
(int) rte->rtekind);
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index bd8cbff..a30b43e 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -2180,6 +2180,7 @@ expandRTE(RangeTblEntry *rte, int rtindex, int sublevels_up,
}
break;
case RTE_CTE:
+ case RTE_TUPLESTORE:
{
ListCell *aliasp_item = list_head(rte->eref->colnames);
ListCell *lct;
@@ -2654,6 +2655,16 @@ get_rte_attribute_is_dropped(RangeTblEntry *rte, AttrNumber attnum)
/* Subselect, Values, CTE RTEs never have dropped columns */
result = false;
break;
+ case RTE_TUPLESTORE:
+ {
+ Tsr tsr;
+
+ Assert(rte->tsrname);
+ tsr = get_visible_tuplestore(rte->tsrname);
+ Assert(tsr);
+ result = tsr->tupdesc->attrs[attnum - 1]->attisdropped;
+ }
+ break;
case RTE_JOIN:
{
/*
On 08/30/2014 12:15 AM, Kevin Grittner wrote:
Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
On 08/28/2014 12:03 AM, Kevin Grittner wrote:
Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
I suggest adding a new hook to the ParseState struct, (p_rangevar_hook
?). The planner calls it whenever it sees a reference to a table, and
the hook function returns back some sort of placeholder reference to the
tuplestore. With variables, the hook returns a Param node, and at
execution time, the executor calls the paramFetch hook to fetch the
value of the param. For relations/tuplestores, I guess we'll need to
invent something like a Param node, but for holding information about
the relation. Like your TsrData struct, but without the pointer to the
tuplestore. At execution time, in the SPI_execute call, you pass the
pointer to the tuplestore in the ParamListInfo struct, like you pass
parameter values.Does this make sense?
I see your point, but SPI first has to be made aware of the
tuplestores and their corresponding names and TupleDesc structures.
Does it make sense to keep the SPI_register_tuplestore() and
SPI_unregister_tuplestore() functions for the client side of the
API, and pass things along to the parse analysis through execution
phases using the techniques you describe?Sorry, I didn't understand that. What do you mean by "first", and the
"client side of the API"? I don't see any need for the
SPI_register_tuplestore() and and SPI_unregister_tuplestore() functions
if you use the hooks.If we were to go with the hooks as you propose, we would still need
to take the information from TriggerData and put it somewhere else
for the hook to reference.
Sure.
The hooks are generalized for plpgsql,
not just for triggers, and it doesn't seem appropriate for them to
be fishing around in the TriggerData structure.
PLpgSQL_execstate seems like the appropriate place.
And what if we add other sources for tuplestores?
What about it?
The lookup during parse analysis
each time an apparent relation name is encountered must be simple
and fast.
We already use hooks for ColumnRefs, which are called even more often,
and we haven't had a problem making that fast enough.
I want named tuplestores to be easy to use from *all* PLs (for
trigger usage) as well as useful for other purposes people may want
to develop.
I'm not sure other PLs would even want to resolve the old/new relations
like PL/pgSQL does. It might be more natural to access the new/old
tuplestores as perl or python hashes or arrays, for example. But if they
do, it's not that difficult to write the hooks.
I had to change the hashkey for plpgsql's plan
caching, but that needs to be done regardless of the API (to
prevent problems in the obscure case that someone attaches the same
trigger function to the same table for the same events more than
once with different trigger names and different transition table
names). If you ignore that, the *entire* change to use this in
plpgsql is to add these lines to plpgsql_exec_trigger():/*
* Capture the NEW and OLD transition TABLE tuplestores (if specified for
* this trigger).
*/
if (trigdata->tg_newtable)
{
Tsr tsr = palloc(sizeof(TsrData));tsr->name = trigdata->tg_trigger->tgnewtable;
tsr->tstate = trigdata->tg_newtable;
tsr->tupdesc = trigdata->tg_relation->rd_att;
tsr->relid = trigdata->tg_relation->rd_id;
SPI_register_tuplestore(tsr);
}
if (trigdata->tg_oldtable)
{
Tsr tsr = palloc(sizeof(TsrData));tsr->name = trigdata->tg_trigger->tgoldtable;
tsr->tstate = trigdata->tg_oldtable;
tsr->tupdesc = trigdata->tg_relation->rd_att;
tsr->relid = trigdata->tg_relation->rd_id;
SPI_register_tuplestore(tsr);
}With the new SPI functions, the code to implement this in each
other PL should be about the same (possibly identical), and areas
using SPI only need similar code to make tuplestores visible to the
planner and usable in the executor if someone has another use for
this. You just do the above once you have run SPI_connect() and
before preparing or executing any query that references the named
tuplestore.
With hooks, the code to implement them in other PLs would be about the
same too, if they want the same behavior.
It remains available on that SPI connection until
SPI_finish() is called or you explicitly unregister it (by name).
Yeah, I don't like that. The SPI interface is currently stateless. Well,
except for cursors and plans explicitly saved with SPI_keepplan. But the
way queries are parsed is stateless - you pass all the necessary
information as part of the SPI_execute call (or similar), using direct
arguments and the ParamListInfo struct.
If you don't want to use hooks, I nevertheless feel that the old/new
relations should be passed as part of the ParamListInfo struct, one way
or another. With hooks, you would set the parserSetup hook, which in
turn would set up the table-ref hook similar to the column-ref hooks,
but if you don't want to do that, you could also add new fields directly
to ParamListInfo for the relations, like the "ParamExternData
params[1]" array that's there currently.
If we use the hooks, I think it will be several times as much code,
more invasive, and probably more fragile. More importantly, these
hooks are not used by the other PLs included with core, and are not
used in any of the other core code, anywhere. They all use SPI, so
they can do the above very easily, but adding infrastructure for
them to use the hooks would be a lot more work, and I'm not seeing
a corresponding benefit.
If they use SPI, they can use the hooks just as well.
I think there is some room to change the API as used by the parser,
planner, and executor so that no changes are needed there if we add
some other registration mechanism besides SPI, but I think having a
registry for tuplestores that sort of takes the place of the
catalogs and related caches (but is far simpler, being process
local) is a better model than what you propose.In summary, thinking of the definition of a named tuplestore as a
variable is the wrong parallel; it is more like a lightweight
relation, and the comparison should be to that, not to function
parameters or local variables.
I'm not too convinced. Marti Raudsepp mentioned refcursor variables, and
I think he's on to something. Refcursor are a bit difficult to
understand, so I wouldn't use those directly, but it would make a lot of
sense if you could e.g. loop over the rows directly with a FOR loop,
e.g. "FOR recordvar IN new LOOP ... END LOOP" without having to do
"SELECT * FROM new". Introducing a new "relation variable" datatype for
this would be nice. I won't insist that you have to implement that right
now, but let's not foreclose the option to add it later.
BTW, do we expect the old/new relations to be visible to dynamic SQL,
ie. EXECUTE? I think most users would say yes, even though the old/new
variables are not - you have to pass them with EXECUTE USING. Admittedly
that supports thinking of them more as lightweight relations rather than
variables.
Another question is whether you would expect the NEW/OLD table to be
visible to functions that you call from the trigger? For example, if the
trigger does:
...
PERFORM myfunction()
...
Can myfunction do "SELECT * FROM new" ? If not, is there a work-around?
I guess that's not this patch's problem, because we don't have a way to
pass sets as arguments in general. Refcursor is the closest thing, but
it's cumbersome.
In any case, I think having the parser call back into SPI, in
parse_tuplestore.c, is a modularity violation. The tuplestores need to
somehow find their way into ParseState. If you go with the
SPI_register_tuplestore API, then you should still have the tuplestores
in the ParseState struct, and have SPI fill in that information. Now
that I look back, I think you also referred to that earlier in the
paragraph that I didn't understand. Let me try again:
I see your point, but SPI first has to be made aware of the
tuplestores and their corresponding names and TupleDesc structures.
Does it make sense to keep the SPI_register_tuplestore() and
SPI_unregister_tuplestore() functions for the client side of the
API, and pass things along to the parse analysis through execution
phases using the techniques you describe? That would eliminate the
need for the SPI_get_caller_tuplestore() function and the
parse_tuplestore.[ch] files, and change how the data is fetched in
parse analysis and execution phases, but that seems fairly minimal
-- there are exactly three places that would need to call the new
hooks where the patch is now getting the information from the
registry.
Yes, if you decide to keep the SPI_register_tuplestore() function, then
IMHO you should still use hooks to pass that information to the parser,
planner and executor.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
On 08/30/2014 12:15 AM, Kevin Grittner wrote:
If we were to go with the hooks as you propose, we would still need
to take the information from TriggerData and put it somewhere else
for the hook to reference.
Sure.
FWIW, I agree with Heikki on this point. It makes a lot more sense for
the parser to provide hooks comparable to the existing hooks for resolving
column refs, and it's not apparent that loading such functionality into
SPI is sane at all.
OTOH, I agree with Kevin that the things we're talking about are
lightweight relations not variables.
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
On Mon, Sep 1, 2014 at 9:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
OTOH, I agree with Kevin that the things we're talking about are
lightweight relations not variables.
My worry is that PL/pgSQL and Postgres's SQL dialect is turning into a
Frankenstein monster with many ways to do the same thing, each having
different semantics that require effort to reason about.
Variables and function arguments are non-contriversial, every
experienced coder understands their semantics without thinking twice
-- even if they're not familiar with Postgres.
The concept of "lightweight relations" that pop into existence when a
certain kind of trigger definition is used somewhere in the function
stack, without a CREATE TABLE, without being discoverable in
information_schema etc., I find needs some more justification than
I've seen in this thread. So far I've only heard that it's more
convenient to implement in the current PostgreSQL code base.
I'm sure more questions would pop up in practice, but as Heikki
mentioned: Are such relations also visible to other functions called
by the trigger function?
* If yes, this introduces non-obvious dependencies between functions.
What happens when one trigger with delta relations invokes another
trigger, does the previous one get shadowed or overwritten? What are
the interactions with search_path? Can an unprivileged function
override relation names when calling a SECURITY DEFINER function?
* If not, this further inhibits developers from properly modularizing
their trigger code (this is already a problem due to the current magic
trigger variables).
Even if these questions have reasonable answers, it takes mental
effort to remember the details. Procedure code debugging, especially
triggers, is hard enough due to poor tooling; increasing the cognitive
load should not be done lightly.
You could argue that CREATE TEMP TABLE already has some of these
problems, but it's very rare that people actually need to use that. If
delta relations get built on this new mechanism, avoiding won't be an
option any more.
Regards,
Marti
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Marti Raudsepp <marti@juffo.org> wrote:
On Mon, Sep 1, 2014 at 9:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The concept of "lightweight relations" that pop into existence when a
certain kind of trigger definition is used somewhere in the function
stack, without a CREATE TABLE, without being discoverable in
information_schema etc., I find needs some more justification than
I've seen in this thread. So far I've only heard that it's more
convenient to implement in the current PostgreSQL code base.
It is required by the SQL standard.
I'm sure more questions would pop up in practice, but as Heikki
mentioned: Are such relations also visible to other functions called
by the trigger function?
* If yes, this introduces non-obvious dependencies between functions.
What happens when one trigger with delta relations invokes another
trigger, does the previous one get shadowed or overwritten?
This is indeed a killer objection. As things stand in the patch, a
function called from a trigger function might have the table of the
same name (if it's not a not schema-qualified reference) shadowed,
or it might not -- depending on whether it was already planned.
That's obviously not acceptable. Passing the metadata from the
TriggerData structure to the PLpgSQL_execstate structure to the
PLpgSQL_expr structure and on to the ParseState structure, and
passing it down to child ParseState structures as needed, along
with similar passing of the Tuplestorestate pointer (and associated
name) to the execution state structures should fix that.
What are the interactions with search_path?
Pretty much the same as the interactions of RTEs with search_path.
If the apparent relation name is not schema-qualified, parse
analysis first tries to resolve the name as an RTE, and if that
fails it tries to resolve it as a named tuplestore, and if that
fails it goes to the catalogs using search_path.
Can an unprivileged function override relation names when calling
a SECURITY DEFINER function?
By changing things to the way Heikki and Tom suggest, any called
functions are not aware of or affected by a named tuplestore in the
caller's context. (Changing *back*, actually -- I had this largely
done that way before; but it seemed like a rather fragile relay
race, passing the baton from one structure to another at odd
places. I guess there's no helping that. Or maybe once I post a
version changed back to that someone can show me something I missed
that makes it better.)
You could argue that CREATE TEMP TABLE already has some of these
problems, but it's very rare that people actually need to use that. If
delta relations get built on this new mechanism, avoiding won't be an
option any more.
Not true -- you don't have them unless you request them in CREATE
TRIGGER. Nobody can be using this now, so a table owner must
*choose* to add the REFERENCING clause to the CREATE TRIGGER
statement for it to matter in the trigger function that is then
referenced. Perhaps if we implement the ability to specify the
trigger code in the CREATE TRIGGER statement itself (rather than
requiring that a trigger function be created first) it will be
easier to look at and cope with.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@ymail.com> wrote:
Marti Raudsepp <marti@juffo.org> wrote:
What are the interactions with search_path?
Pretty much the same as the interactions of RTEs with search_path.
If the apparent relation name is not schema-qualified, parse
analysis first tries to resolve the name as an RTE, and if that
fails it tries to resolve it as a named tuplestore, and if that
fails it goes to the catalogs using search_path.
Argh. s/RTE/CTE/
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Sep 3, 2014 at 10:49 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
Marti Raudsepp <marti@juffo.org> wrote:
The concept of "lightweight relations" that pop into existence when a
certain kind of trigger definition is used somewhere in the function
stack, without a CREATE TABLE, without being discoverable in
information_schema etc., I find needs some more justification than
I've seen in this thread. So far I've only heard that it's more
convenient to implement in the current PostgreSQL code base.It is required by the SQL standard.
I had a cursory read of the SQL 20nn draft and I don't get this
impression. The only place I could find discussing the behavior of
"transition tables" is in Foundation "4.39.1 General description of
triggers", which says:
"Special variables make the data in the transition table(s) available
to the triggered action. For a statement-level
trigger the variable is one whose value is a transition table."
There is no information about the scoping of such variables, so I
assume it refers to a regular locally scoped variable.
Did I miss something? Are you reading a different version of the spec?
Regards,
Marti
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Marti Raudsepp <marti@juffo.org> wrote:
On Wed, Sep 3, 2014 at 10:49 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
Marti Raudsepp <marti@juffo.org> wrote:
The concept of "lightweight relations" that pop into existence when a
certain kind of trigger definition is used somewhere in the function
stack, without a CREATE TABLE, without being discoverable in
information_schema etc., I find needs some more justification than
I've seen in this thread. So far I've only heard that it's more
convenient to implement in the current PostgreSQL code base.It is required by the SQL standard.
I had a cursory read of the SQL 20nn draft and I don't get this
impression. The only place I could find discussing the behavior of
"transition tables" is in Foundation "4.39.1 General description of
triggers", which says:"Special variables make the data in the transition table(s) available
to the triggered action. For a statement-level
trigger the variable is one whose value is a transition table."There is no information about the scoping of such variables, so I
assume it refers to a regular locally scoped variable.Did I miss something?
Apparently. I did a search on the document and counted and got 101
occurrences of "transition table". I might be off by a few, but
that should be pretty close. Perhaps this, from 4.14 most directly
answers your point:
| A transient table is a named table that may come into existence
| implicitly during the evaluation of a <query expression> or the
| execution of a trigger. A transient table is identified by a
| <query name> if it arises during the evaluation of a <query
| expression>, or by a <transition table name> if it arises during
| the execution of a trigger. Such tables exist only for the
| duration of the executing SQL-statement containing the <query
| expression> or for the duration of the executing trigger.
Are you reading a different version of the spec?
I'm looking at a draft of 200x from 2006-02-01.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
On 08/30/2014 12:15 AM, Kevin Grittner wrote:
If we were to go with the hooks as you propose, we would still need
to take the information from TriggerData and put it somewhere else
for the hook to reference.Sure.
FWIW, I agree with Heikki on this point. It makes a lot more sense for
the parser to provide hooks comparable to the existing hooks for resolving
column refs, and it's not apparent that loading such functionality into
SPI is sane at all.OTOH, I agree with Kevin that the things we're talking about are
lightweight relations not variables.
Try as I might, I was unable to find any sensible way to use hooks.
If the issue was only the parsing, the route was fairly obvious,
but the execution side needs to access the correct tuplestore(s)
for each run, too -- so the sort of information provided by
relcache needed to be passed in to based on the context within the
process (i.e., if you have nested triggers firing, each needs to
use a different tuplestore with the same name in the same
function, even though it's using the same plan). On both sides it
seemed easier to pass things through the same sort of techniques as
"normal" parameters; I couldn't find a way to use hooks that didn't
just make things uglier.
I see the down side of making this a feature which can only be used
from SPI, so I've updated the patch to allow it from other
contexts. On the other hand, I see many uses for it where SPI *is*
used, and the SPI interface needs to change to support that. The
functions I had added are one way to do that on the
parsing/planning side without breaking any existing code. The same
information (i.e., the metadata) needs to be passed to the executor
along with references to the actual tuplestores, and that needs to
go through a different path -- at least for the plpgsql usage.
I broke out the changes from the previous patch in multiple commits
in my repository on github:
commit 2520db8fbb41c68a82c2c750c8543154c6d85eb9
Author: Kevin Grittner <kgrittn@postgresql.org>
Date: Mon Sep 15 01:17:14 2014 -0500
Use executor state rather than SPI to get named tuplestores.
spi.c and trigger.c will need a little more clean-up to match this,
and it's likely that not all places that need to pass the baton are
doing so, but at least this passes regression tests.
commit de9067258125226d1625f160c3eee9aff90ca598
Author: Kevin Grittner <kgrittn@postgresql.org>
Date: Sun Sep 14 22:01:04 2014 -0500
Pass the Tsrcache through ParserState to parse analysis.
commit e94779c1e22ec587446a7aa2593ba5f102b6a28b
Author: Kevin Grittner <kgrittn@postgresql.org>
Date: Sun Sep 14 19:23:45 2014 -0500
Modify SPI to use Tsrcache instead of List.
commit 7af841881d9113eb4c8dca8e82dc1867883bf75d
Author: Kevin Grittner <kgrittn@postgresql.org>
Date: Sun Sep 14 18:45:54 2014 -0500
Create context-specific Tsrcache.
Not used yet.
commit 35790a4b6c236d09e0be261be9b0017d34eaf9c9
Author: Kevin Grittner <kgrittn@postgresql.org>
Date: Sun Sep 14 16:49:37 2014 -0500
Create Tsrmd structure so tuplestore.h isn't needed in parser.
commit 93d57c580da095b71d9214f69fede71d2f8ed840
Author: Kevin Grittner <kgrittn@postgresql.org>
Date: Sun Sep 14 15:59:45 2014 -0500
Improve some confusing naming for TuplestoreRelation node.
Anyone who has already reviewed the earlier patch may want to look
at these individually:
https://github.com/kgrittn/postgres/compare/transition
Attached is a new patch including all of that. Hopefully I haven't
misunderstood what Heikki and Tom wanted; if I have, just let me
know where I went wrong.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
trigger-transition-tables-v4.patch.gzapplication/x-tgz; name=trigger-transition-tables-v4.patch.gzDownload
�6�T trigger-transition-tables-v4.patch �]{s�F��;�c_��T �)QJdU)�pO&�$u�V*���!�5�x�V���y`@�����\Te� ==�~Nc0���`N���4�/j����NJ|��4)9��>���d_|�F��Ys�c5��O0l���*�=6�&�����,��?�����2_��e����O����b�{�[M�z����uG.O��������<�6�N��4��b�@�2���%� N-����?�����o��9�3;���N"6���z���w����^B��������������G�d��f���~���9���VS���I.�IX��h7��k����9�_ |�7�����3'u�nm((����2�����^{GH�vr��F�(,8�����C\_(h� ��m����������44��l��4�u�t:��n���{Y�'��?a�y���U%�%�*Fe(��V��? ���%$\6���{w�|m�������_�`sS��,4����)
|b���J}���rD���Qo�Z�������z��M<k�k5�D�!�;Y�g�J�����V���Tu��/4#\i>� �c[��?��E��N8�c~UPy���vl�?��Y���SlU)Ys� �����������?�����U�S�y��<tf\Y�n����%�\������'�*�N��p����g���x0��(���� *��&-"�������D�Jw8-�"c�F���c���������bH�����^Xd�������i�L����j����S�w ���������QdSl��\ *��b���2p%X��8�I #�C8�E�O���'���Rv���y�b���^ �OL}_��$w#��<&��t�'C�w���V4G��/)�6hJ�En
l��LgA�uRT!��#9�Y��L�;�z��i4�T�h�S�<�1��������I��/p�)��O�>� �=8x�K��_;�p����b�8�� '>0�B�_������_��B��<�G��~����N|x�4
<����y^�^�9����~�~�l�����{�v�G� 4���@�H������l|��iZY���d
5��@��4����C����S*G�8����p�e��[s�VSnK�]i8�����m�U��h�=�+;��o��c���Uw����q92� %��g�����qw�BJ�hs \)�o��8��D� �K� �����u)��$�}�y�B2���MS���U`��e�)��7�����q��������u���@�a�����3��A�5�
H|bhH0���B�������H�� y";�>�l<�������Gv2�g����p�8����]�g�)�=V� X�:�jJ����s��l88���7
o����1��c�����ja����:�Vc������{�\��k*�8G�!c?�<P4���',
�k�A�2$tFN��ux�@{AP;�p���xo������lJ��b�����k�P���,�s�/��i2H�G������M���5q�������q���{���&;��}Zvz�Qw8Fv���`�J����t}0��-��y��"�����k�?aI���y������k���$[l�A�G���9Ps`T3� !g>��y�#5?dIF�Ox�U��;{Vs�-l@@�'}+��0���a�j�l��������*d`��+'`6��%y�xZ8�}��lElV'!Rb#u��+�<�vF����f�C�@_ t9��=�-�VM@_8*F�!b��&B�4=�-PGH<�b3�#J,���jh�`�u� = V,���tS��(��N ��k3+�*��~���E�������!+�<bBs��H?�TL���|G�����cy�b:
4ljZ.��J���q� &q���e!A�L�7FMV�l���)KT���UK%��Q-���P�b{�9�G�C��0p1A��\X� ���z�t*�S��H���GZy$���gb�����!��/��J�
�@QY<���~z n��p4�(>V�L������O�o�\���^��iX��N!�Q�T��)�������n�i����<��c�:V����G�.�� ��R�$�r@��~���}g���+�/��w��id�����-w/�����|@�g@�I�
����h��$�t��B�M=��G����0�|J��'-2���V���<�s����u��v�2��
U�)zb ��s��S���H�*����z�L]�,R�aD�IA%d�>|T�����Z��[i�,�6���
�QP��rQ@��7�F�.��)xN�vnV�U2�Qq2Qj�{{�v�y��/�C�
,v�]O��X��>������"�������x^�>u�m�}�� ������%���.K���1B��H�M�P��t �*5����������8[�5����)d(��A�I4�Q��\�Oj2@5w�UH�F>�A��^�����,J�i�C��>�##�Uj�O3��"�>�~
�1�IF��.��.E �7�D<N`�a� S&� ��IL�:� "9MB��)�}p���e����B�s�N���t�V���*��e"��)�O���hN���&=��S+��[���n����Ie�F���C����L���Q�������#]�s.��K
DA�0������Qx=; ��z������ikAF�B(��t����9U��3xs��I�a�K�
?������bPH��Gd�V:5lTO!K����/����7�P����p� ���I���-JC�it���� Lf��Y������.���=��d�$��$�G@n*7y�iR�_����_:W�N+�lJT�K�*.�j9��&PD����
kj����N1K����_Y�������2���� ��4��v`�T#���u�� / N��|<�$[��~<�~9��6�������9���e@[fe�����/`���/�S�_���~��&%�0����a�������t���~-�K���-�Z�}4 %���$Xl&n]���P��Z~@���^T�4����)
��R�N�|���(�3����nk#xg
�`]@���|����5?���:��������������8)�W��E�����%5���fB(
[����b.��Zf�R�Nu>�T��!�IF������#kj������4GB�D��9a6�����#btD��
>�� �
�����g��t���u��Q�A�[������!r���
���+w.G�s�q��!O�8d��_�O=�}��"*IRT�!,'X���s�Q"%b�$�|� S Q�SCU�6�
���s�
�~,�X�W.?%��zv��������=����('���2+��������.a�����!�*+�9�/h��.K�T�er]����B�i[��=)
�)�� �Z��BN+~����sQ�����W���OOu?8���$'�|S�%Gu�h��J2��� ;�
�����{|?�\����8��#D�����9=�|�����������}��Bz!Q�I�ZI�j�&�R�� ��LY��@��f����L����4q�����-�m� �����N�3�����������2�?h�Y�a�\�l*�0���p�@5O9��2�y���#UdTY���1�V��;����M ��O�B�Yx���L�!2�Ek�;|��?�'���=�w�4��;��Pg��Q$'�N�ry���������[��`y�
������tU^Gq� ��R���n�f^�0��������"�g\���O���l���y����������e'Y�s|\m�k{.�����/�}|�(���@<�p�&��7�l>����z�c^8�;z��7������o����k4���|9z2��v�>�!%�=�r��>��{�>��H��N�/�k�v�] ��%���� ��������fn��d�k/y�?����@&�3�+���E�����6Qf�B��rM�3�As~%��e�bL������P��6�~Bp�|�ON�%t"�[�B� |?�������L�����h5�;?C���y�"�p�6��'��������������6�
�gpj�����o�I���.-��p=���F�B~��j�h+r_�*C��;���[��[�i�mNB�X� ��j�
g^~�2�q�����-��O��l�5�,��O�x8M/+��*{��5�j�<����\e� �r�k�����z��
����@sD/��
w*��{a���i���!����<���Ju�E/$��/����D����p��h�:1-z��|�+�0CC�b^U���J���J����������8�����*���=����Gd
����c���Jq7���P�mf�C��~�I�p$_����4b��1��~���F,�>