Re: Triggers on foreign tables

This version looks basically good.  I have some cosmetic things to sweep up
before commit.  One point is a bit more substantial:

On Tue, Feb 04, 2014 at 01:16:22PM +0100, Ronan Dunklau wrote:
> Le lundi 3 février 2014 23:28:45 Noah Misch a écrit :
> > On Sun, Feb 02, 2014 at 11:53:51AM +0100, Ronan Dunklau wrote:
> > > We can remove ate_ptr2 and rely on the AFTER_TRIGGER_2CTIDS flag, but the
> > > rescanning and ate_ptr1 (renamed ate_tupleindex in the attached patch)
> > > can't go away.
> > > 
> > > Consider for example the case of a foreign table with more than one AFTER
> > > UPDATE triggers. Unless we store the tuples once for each trigger, we will
> > > have to rescan the tuplestore.
> > 
> > Will we?  Within a given query level, when do (non-deferred) triggers
> > execute in an order other than the enqueue order?
> 
> Let me explain what I had in mind.
> 
> Looking at the code in AfterTriggerSaveEvent:
> 
> - we build a "template" AfterTriggerEvent, and store the tuple(s) 
> - for each suitable after trigger that matches the trigger type, as well as 
> the WHEN condition if any, a copy of the previously built AfterTriggerEvent is 
> queued
> 
> Later, those events are fired in order.
> 
> This means that more than one event can be fired for one tuple.
> 
> Take this example:
[snip]

Thanks; that illuminated the facts I was missing.

> On a side note, this made me realize that it is better to avoid storing a 
> tuple entirely if there is no enabled trigger (the f1 = 4 case above). The 
> attached patch does that, so the previous sequence becomes:
> 
> 0-0-2-2-4-6-6
> 
> It also prevents from initalizing a tuplestore at all if its not needed.

That's a sensible improvement.

> > > To mitigate the effects of this behaviour, I added the option to perform a
> > > reverse_seek when the looked-up tuple is nearer from the current index
> > > than
> > > from the start.
> > 
> > If there's still a need to seek within the tuplestore, that should get rid
> > of the O(n^2) effect.  I'm hoping that per-query-level tuplestores will
> > eliminate the need to seek entirely.
> 
> I think the only case when seeking is still needed is when there are more than 
> one after trigger that need to be fired, since the abovementioned change 
> prevents from seeking to skip tuples.

Agreed.  More specifically, I see only two scenarios for retrieving tuples
from the tuplestore.  Scenario one is that we need the next tuple (or pair of
tuples, depending on the TriggerEvent).  Scenario two is that we need the
tuple(s) most recently retrieved.  If that's correct, I'm inclined to
rearrange afterTriggerInvokeEvents() and AfterTriggerExecute() to remember the
tuple or pair of tuples most recently retrieved.  They'll then never call
tuplestore_advance() just to reposition.  Do you see a problem with that?


I was again somewhat tempted to remove ate_tupleindex, perhaps by defining the
four flag bits this way:

#define AFTER_TRIGGER_DONE                0x10000000
#define AFTER_TRIGGER_IN_PROGRESS        0x20000000
/* two bits describing the size of and tuple sources for this event */
#define AFTER_TRIGGER_TUP_BITS            0xC0000000
#define AFTER_TRIGGER_FDW_REUSE            0x00000000
#define AFTER_TRIGGER_FDW_FETCH            0x40000000
#define AFTER_TRIGGER_1CTID                0x80000000
#define AFTER_TRIGGER_2CTID                0xC0000000

AFTER_TRIGGER_FDW_FETCH and AFTER_TRIGGER_FDW_REUSE correspond to the
aforementioned scenarios one and two, respectively.  I think, though, I'll
rate this as needless micro-optimization and not bother; opinions welcome.
(The savings is four bytes per foreign table trigger event.)

Thanks,
nm

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com