Re: delta relations in AFTER triggers

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);
>>>>   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.
>>>
>>> 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 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
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.