delta relations in AFTER triggers

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 triggers

The 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:

http://people.planetpostgresql.org/dfetter/index.php?/archives/71-Querying-Rows-in-Statement-Triggers.html

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

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

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

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