Re: Triggers on foreign tables

On Tue, Jan 07, 2014 at 12:11:28PM +0100, Ronan Dunklau wrote:
> Since the last discussion about it
(http://www.postgresql.org/message-id/CADyhKSUGP6oJb1pybTiMOP3s5fg_yOkgUTo-7RBcLTNVaJ57Pw@mail.gmail.com),I
 
> finally managed to implement row triggers for foreign tables.

> For statement-level triggers, nothing has changed since the last patch.
> Statement-level triggers are just allowed in the command checking.
> 
> For row-level triggers, it works as follows:
> 
> - during rewrite phase, every attribute of the foreign table is duplicated as
> a resjunk entry if a trigger is defined on the relation. These entries are
> then used to store the old values for a tuple. There is room for improvement
> here: we could check if any trigger is in fact a row-level trigger

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?

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

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

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

On Tue, Jan 07, 2014 at 05:31:10PM +0100, Ronan Dunklau wrote:
> --- a/contrib/postgres_fdw/expected/postgres_fdw.out
> +++ b/contrib/postgres_fdw/expected/postgres_fdw.out

> +NOTICE:  TG_NAME: trig_row_after
> +NOTICE:  TG_WHEN: AFTER
> +NOTICE:  TG_LEVEL: ROW
> +NOTICE:  TG_OP: DELETE
> +NOTICE:  TG_RELID::regclass: rem1
> +NOTICE:  TG_RELNAME: rem1
> +NOTICE:  TG_TABLE_NAME: rem1
> +NOTICE:  TG_TABLE_SCHEMA: public
> +NOTICE:  TG_NARGS: 2
> +NOTICE:  TG_ARGV: [23, skidoo]
> +NOTICE:  OLD: (2,bye)
> +NOTICE:  TG_NAME: trig_row_before
> +NOTICE:  TG_WHEN: BEFORE
> +NOTICE:  TG_LEVEL: ROW
> +NOTICE:  TG_OP: DELETE
> +NOTICE:  TG_RELID::regclass: rem1
> +NOTICE:  TG_RELNAME: rem1
> +NOTICE:  TG_TABLE_NAME: rem1
> +NOTICE:  TG_TABLE_SCHEMA: public
> +NOTICE:  TG_NARGS: 2
> +NOTICE:  TG_ARGV: [23, skidoo]
> +NOTICE:  OLD: (11,"bye remote")
> +NOTICE:  TG_NAME: trig_row_after
> +NOTICE:  TG_WHEN: AFTER
> +NOTICE:  TG_LEVEL: ROW
> +NOTICE:  TG_OP: DELETE
> +NOTICE:  TG_RELID::regclass: rem1
> +NOTICE:  TG_RELNAME: rem1
> +NOTICE:  TG_TABLE_NAME: rem1
> +NOTICE:  TG_TABLE_SCHEMA: public
> +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

> --- a/contrib/postgres_fdw/sql/postgres_fdw.sql
> +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
> @@ -384,3 +384,188 @@ insert into loc1(f2) values('bye');
>  insert into rem1(f2) values('bye remote');
>  select * from loc1;
>  select * from rem1;
> +
> +-- ==================================================================+-- test local triggers
> +-- ==================================================================+
> +-- Trigger functions "borrowed" from triggers regress test.
> +CREATE FUNCTION trigger_func() RETURNS trigger LANGUAGE plpgsql AS $$
> +BEGIN
> +    RAISE NOTICE 'trigger_func(%) called: action = %, when = %, level = %', TG_ARGV[0], TG_OP, TG_WHEN, TG_LEVEL;
> +    RETURN NULL;
> +END;$$;
> +
> +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.

> --- a/doc/src/sgml/trigger.sgml
> +++ b/doc/src/sgml/trigger.sgml

> @@ -96,6 +96,7 @@
>      after may only be defined at statement level, while triggers that fire
>      instead of an <command>INSERT</command>, <command>UPDATE</command>,
>      or <command>DELETE</command> may only be defined at row level.
> +    On foreign tables, triggers can not be defined at row level.

This is obsolete.

> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -3150,13 +3150,16 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
>              /* No command-specific prep needed */
>              break;
>          case AT_EnableTrig:        /* ENABLE TRIGGER variants */
> -        case AT_EnableAlwaysTrig:
> -        case AT_EnableReplicaTrig:
>          case AT_EnableTrigAll:
>          case AT_EnableTrigUser:
>          case AT_DisableTrig:    /* DISABLE TRIGGER variants */
>          case AT_DisableTrigAll:
>          case AT_DisableTrigUser:
> +            ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
> +            pass = AT_PASS_MISC;
> +            break;
> +        case AT_EnableReplicaTrig:
> +        case AT_EnableAlwaysTrig:

Why permit some variants, but not every variant, of ALTER TABLE t ENABLE
TRIGGER <type> on foreign tables?

>          case AT_EnableRule:        /* ENABLE/DISABLE RULE variants */
>          case AT_EnableAlwaysRule:
>          case AT_EnableReplicaRule:
> diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
> index 4eff184..da3c568 100644
> --- a/src/backend/commands/trigger.c
> +++ b/src/backend/commands/trigger.c

> @@ -1844,9 +1864,7 @@ ExecCallTriggerFunc(TriggerData *trigdata,
>       */
>      InitFunctionCallInfoData(fcinfo, finfo, 0,
>                               InvalidOid, (Node *) trigdata, NULL);
> -
>      pgstat_init_function_usage(&fcinfo, &fcusage);
> -

Please don't change unrelated whitespace.

>      MyTriggerDepth++;
>      PG_TRY();
>      {
> @@ -1946,10 +1964,10 @@ ExecASInsertTriggers(EState *estate, ResultRelInfo *relinfo)
> 
>  TupleTableSlot *
>  ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo,
> -                     TupleTableSlot *slot)
> +                     TupleTableSlot *tupleslot)
>  {
>      TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
> -    HeapTuple    slottuple = ExecMaterializeSlot(slot);
> +    HeapTuple    slottuple = ExecMaterializeSlot(tupleslot);
>      HeapTuple    newtuple = slottuple;
>      HeapTuple    oldtuple;
>      TriggerData LocTriggerData;
> @@ -2003,9 +2021,9 @@ ExecBRInsertTriggers(EState *estate, ResultRelInfo *relinfo,
>          if (newslot->tts_tupleDescriptor != tupdesc)
>              ExecSetSlotDescriptor(newslot, tupdesc);
>          ExecStoreTuple(newtuple, newslot, InvalidBuffer, false);
> -        slot = newslot;
> +        tupleslot = newslot;
>      }
> -    return slot;
> +    return tupleslot;

Keep the old variable name.  Exceptions can be made if the name was deceptive,
but "tupleslot" communicates the same thing as "slot".

> @@ -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.

> +        if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE){
> +            TriggerData LocTriggerData;
> +            trigtuple = ExtractOldTuple(tupleslot, relinfo);
> +            LocTriggerData.tg_trigtuple = trigtuple;
> +            LocTriggerData.tg_newtuple = NULL;
> +            LocTriggerData.tg_trigtuplebuf = InvalidBuffer;
> +            LocTriggerData.tg_newtuplebuf = InvalidBuffer;
> +            ExecARForeignTrigger(estate,
> +                    LocTriggerData,
> +                    relinfo,
> +                    TRIGGER_EVENT_DELETE,
> +                    TRIGGER_TYPE_DELETE);
> +        }
> +        else
> +        {
> +          trigtuple = GetTupleForTrigger(estate,

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.

> @@ -2604,7 +2704,11 @@ GetTupleForTrigger(EState *estate,
>      HeapTupleData tuple;
>      HeapTuple    result;
>      Buffer        buffer;
> -
> +    /*
> +     *    Foreign tables tuples are NOT stored on the heap.
> +     *    Instead, old attributes values are stored as resjunk attributes in the
> +     *    original tuple. So, we build a new tuple from those attributes here.
> +     */

Obsolete comment.  That's done elsewhere, not here.

> @@ -2731,6 +2835,90 @@ ltrmark:;
>  }
> 
>  /*
> + * Get an old tuple from a "mixed tuple", containing both the new values as well
> + * as well as the old ones as resjunk columns.
> + */
> +static HeapTuple
> +ExtractOldTuple(TupleTableSlot *planSlot,
> +    ResultRelInfo *relinfo)
> +{
> +    Relation       relation = relinfo->ri_RelationDesc;
> +    TupleDesc base_tuple_desc = relation->rd_att;
> +    Datum * datum = palloc0(sizeof(Datum) * base_tuple_desc->natts);
> +    bool * isnull = palloc0(sizeof(bool) * base_tuple_desc->natts);
> +    HeapTuple tuple;
> +    /*
> +     * NewSlot is not null: this indicates a BEFORE trigger.
> +     */
> +    JunkFilter * junk_filter = relinfo->ri_junkFilter;
> +    List * target_list = junk_filter->jf_targetList;
> +    ListCell * lc;
> +    foreach(lc, target_list)
> +    {
> +      TargetEntry *tle = (TargetEntry *) lfirst(lc);
> +      /*
> +      * Only consider previously added resjunk entries
> +      */
> +      if(tle->resjunk)
> +      {
> +        if(IsA(tle->expr, Var))
> +        {
> +            Var * junkvar = (Var * )tle->expr;
> +            /*
> +             * Do not copy system identifiers.
> +             */
> +            if(junkvar->varoattno > 0)
> +            {
> +            datum[junkvar->varoattno - 1] = slot_getattr(planSlot,
> +                tle->resno,
> +                &(isnull[junkvar->varoattno - 1]));
> +            }

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.

Thanks,
nm

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