Re: Triggers on foreign tables

Hello,

> I initially tried to keep track of them by allocating read pointers on the
> tuple store, but it turned out to be so expensive that I had to find another
> way (24bytes per stored tuple, which are not reclaimable until the end of
> the transaction).
>
> What do you think about this approach ? Is there something I missed which
> would make it not sustainable ?
>
It seems to me reasonable approach to track them.
Just a corner case, it may cause an unexpected problem if someone tried to
update a foreign table with 2^31 of tuples because of int index.
It may make sense to put a check fdw_nextwrite is less than INT_MAX. :-)

I have a few other minor comments:

+static HeapTuple
+ExtractOldTuple(TupleTableSlot *planSlot,
+ ResultRelInfo *relinfo)
+{
+ bool isNull;
+ JunkFilter *junkfilter = relinfo->ri_junkFilter;
+ HeapTuple oldtuple = palloc0(sizeof(HeapTupleData));
+ HeapTupleHeader td;
+ Datum datum = ExecGetJunkAttribute(planSlot,
+ junkfilter->jf_junkAttNo,
+ &isNull);
+
+ /* shouldn't ever get a null result... */
+ if (isNull)
+ elog(ERROR, "wholerow is NULL");
+ td = DatumGetHeapTupleHeader(datum);
+ (*oldtuple).t_len = HeapTupleHeaderGetDatumLength(td);
+ (*oldtuple).t_data = td;
+ return oldtuple;
+}
+

Why not usual coding manner as:
oldtuple->t_len = HeapTupleHeaderGetDatumLength(td);
oldtuple->t_data = td;

Also, it don't put tableOid on the tuple.
oldtuple->t_tableOid = RelationGetRelid(relinfo->ri_RelationDesc);

@@ -730,6 +738,45 @@ rewriteTargetListIU(Query *parsetree, Relation target_relation,
+ /*
+ * For foreign tables, build a similar array for returning tlist.
+ */
+ if (need_full_returning_tlist)
+ {
+ returning_tles = (TargetEntry **) palloc0(numattrs * sizeof(TargetEntry *));
+ foreach(temp, parsetree->returningList)
+ {
+ TargetEntry *old_rtle = (TargetEntry *) lfirst(temp);
+
+ if (IsA(old_rtle->expr, Var))
+ {
+ Var *var = (Var *) old_rtle->expr;
+
+ if (var->varno == parsetree->resultRelation)
+ {
+ attrno = var->varattno;
+ if (attrno < 1 || attrno > numattrs)
+ elog(ERROR, "bogus resno %d in targetlist", attrno);

This checks caused an error when returning list contains a reference to
system column; that has negative attribute number.
Probably, it should be "continue;", instead of elog().

BTW, isn't it sufficient to inhibit optimization by putting whole-row-reference
here, rather than whole-row-reference. Postgres_fdw extracts whole-row-reference
into individual columns reference.

+ att_tup = target_relation->rd_att->attrs[attrno - 1];
+ /* We can (and must) ignore deleted attributes */
+ if (att_tup->attisdropped)
+ continue;
+ returning_tles[attrno - 1] = old_rtle;
+ }
+ }
+ }
+ }
+

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Ronan Dunklau
> Sent: Thursday, January 23, 2014 11:18 PM
> To: Noah Misch
> Cc: pgsql-hackers(at)postgresql(dot)org
> Subject: Re: [HACKERS] Triggers on foreign tables
>
> Thank you very much for this review.
>
> > The need here is awfully similar to a need of INSTEAD OF triggers on views.
> > For them, we add a single "wholerow" resjunk TLE. Is there a good
> > reason to do it differently for triggers on foreign tables?
>
> I wasn't aware of that, it makes for some much cleaner code IMO. Thanks.
>
> > > - for after triggers, the whole queuing mechanism is bypassed for
> > > foreign tables. This is IMO acceptable, since foreign tables cannot
> > > have constraints or constraints triggers, and thus have not need for
> > > deferrable execution. This design avoids the need for storing and
> > > retrieving/identifying remote tuples until the query or transaction
> end.
> >
> > Whether an AFTER ROW trigger is deferred determines whether it runs at
> > the end of the firing query or at the end of the firing query's transaction.
> > In all cases, every BEFORE ROW trigger of a given query fires before
> > any AFTER ROW trigger of the same query. SQL requires that. This
> > proposal would give foreign table AFTER ROW triggers a novel firing
> > time; let's not do that.
> >
> > I think the options going forward are either (a) design a way to queue
> > foreign table AFTER ROW triggers such that we can get the old and/or
> > new rows at the end of the query or (b) not support AFTER ROW triggers
> > on foreign tables for the time being.
> >
>
> I did not know this was mandated by the standard.
>
> The attached patch tries to solve this problem by allocating a tuplestore
> in the global afterTriggers structure. This tuplestore is used for the whole
> transaction, and uses only work_mem per transaction.
>
> Both old and new tuples are stored in this tuplestore. Some additional
> bookkeeping is done on the afterTriggers global structure, to keep track
> of the number of inserted tuples, and the current read pointer position.
> The tuples are identified by their order of insertion during the
> transaction.
> I think this could benefit from some support in the tuplestore API, by
> allowing arbitrary seek without the need to store more ReadPointers.
>
> I initially tried to keep track of them by allocating read pointers on the
> tuple store, but it turned out to be so expensive that I had to find another
> way (24bytes per stored tuple, which are not reclaimable until the end of
> the transaction).
>
> What do you think about this approach ? Is there something I missed which
> would make it not sustainable ?
>
> If you prefer, I also have a patch implementing the rest of the changes
> and keeping the previous behaviour for after triggers.
>
> > It's not clear to me whether SQL/MED contemplates triggers on foreign
> > tables. Its <drop basic column definition> General Rules do mention
> > the possibility of a reference from a trigger column list. On the
> > other hand, I see nothing overriding the fact that CREATE TRIGGER only
> > targets base tables. Is this clearer to anyone else? (This is a
> > minor point, mainly bearing on the Compatibility section of the CREATE
> > TRIGGER documentation.)
>
> I do not have access to the standard specification, any advice regarding
> specs compliance would be welcomed.
>
> >
> > > - the duplicated resjunk attributes are identified by being:
> > > - marked as resjunk in the targetlist
> > > - not being system or whole-row attributes (varno > 0)
> > >
> > > There is still one small issue with the attached patch:
> > > modifications to the tuple performed by the foreign data wrapper
> > > (via the returned TupleTableSlot in ExecForeignUpdate and
> > > ExecForeignInsert hooks) are not visible to the AFTER trigger. This
> > > could be fixed by merging the planslot containing the resjunk
> > > columns with the returned slot before calling the trigger, but I'm not
> really sure how to safely perform that. Any advice ?
> >
> > Currently, FDWs are permitted to skip returning columns not actually
> > referenced by any RETURNING clause. I would change that part of the
> > API contract to require returning all columns when an AFTER ROW
> > trigger is involved. You can't get around doing that by merging old
> > column values, because, among other reasons, an INSERT does not have those
> values at all.
>
> I'm not sure this should be part of the API contract: it would make
> implementing a FDW more complicated than it is now. The attached patch hooks
> on rewriteTargetListIU to add the missing targets to the returning clause,
> when needed.
>
> This also changes the way the query's hasReturning flag is set to exclude
> the case when only resjunk entries are present in the returning list.
>
> > > +NOTICE: TG_NARGS: 2
> > > +NOTICE: TG_ARGV: [23, skidoo]
> > > +NOTICE: OLD: (11,"bye remote")
> > > +insert into rem1 values(1,'insert');
> >
> > Would you trim the verbosity a bit? Maybe merge several of the TG_
> > fields onto one line, and remove the low-importance ones. Perhaps
> > issue one line like this in place of all the current TG_ lines:
> >
> > NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON rem1
> >
>
> Fixed in the attached patch.
>
>
> > > +CREATE TRIGGER trig_stmt_before BEFORE DELETE OR INSERT OR UPDATE
> > > +ON ft1
> > > FOR EACH STATEMENT EXECUTE PROCEDURE trigger_func(); +CREATE TRIGGER
> > > trig_stmt_after AFTER DELETE OR INSERT OR UPDATE ON ft1 FOR EACH
> > > STATEMENT EXECUTE PROCEDURE trigger_func();
> > No test actually fires these triggers.
>
> These should have been placed on the rem1 foreign table, not ft1. Fixed.
>
> > > + On foreign tables, triggers can not be defined at row level.
> >
> > This is obsolete.
>
> I missed that from the earlier version of the patch, thank you.
>
>
> > Why permit some variants, but not every variant, of ALTER TABLE t
> > ENABLE TRIGGER <type> on foreign tables?
>
> I overlooked that. Fixed.
>
>
> > Keep the old variable name. Exceptions can be made if the name was
> > deceptive, but "tupleslot" communicates the same thing as "slot".
>
> Fixed.
>
> >
> > > @@ -2146,7 +2183,8 @@ ExecASDeleteTriggers(EState *estate,
> > > ResultRelInfo *relinfo)> bool ExecBRDeleteTriggers(EState *estate,
> > > EPQState *epqstate,
> > >
> > > ResultRelInfo *relinfo,
> > >
> > > - ItemPointer tupleid)
> > > + ItemPointer tupleid,
> > > + TupleTableSlot *tupleslot)
> >
> > The new argument is unused.
>
> I added it here for coherence. Now removed in the attached patch.
>
> >
> > Please don't change unrelated whitespace.
>
> > Please use pgindent to fix the formatting of your new code. It's fine
> > to introduce occasional whitespace errors, but they're
> > unusually-plentiful here.
>
> I think its done now. One problem I have with running pgindent is that I
> accidentally add chunks that were modified only by pgindent.
>
> > Obsolete comment. That's done elsewhere, not here.
>
> Ok
>
> >
> > For future reference, you mustn't just assume that a resjunk Var is
> > the same resjunk Var you added for this purpose. The target list has
> > many consumers, present and future, so you need to find your resjunk
> > entries more-reliably than this. See other resjunk-adding code for
> > examples. This concern goes away if you borrow the "wholerow"
> > approach from INSTEAD OF triggers.
>
> Using the wholerow approach, the entry is identified by the junkfilter
> jf_junkAttNo attribute. So this concern indeed goes away.
>
>
> Again, thank you for this review.