Re: Triggers on foreign tables

Thank you for taking the time to review this. Please find attached a new
version of the patch.

Le mercredi 29 janvier 2014 09:13:36 Kouhei Kaigai a écrit :

> It may make sense to put a check fdw_nextwrite is less than INT_MAX. :-)

The attached patch checks this, and add documentation for this limitation.
I'm not really sure about how to phrase that correctly in the error message
and the documentation. One can store at most INT_MAX foreign tuples, which
means that at most INT_MAX insert or delete or "half-updates" can occur. By
half-updates, I mean that for update two tuples are stored whereas in contrast
to only one for insert and delete trigger.

Besides, I don't know where this disclaimer should be in the documentation.
Any advice here ?


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


Fixed, thank you.

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

Are system attributes supposed to be accessible for foreign tables? I think
they only make sense for postgres_fdw.
Anyway, I fixed this and added a test case returning ctid, xmin and xmax.

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

The code is more straightforward with a whole-row reference. Done in the
attached patch.


--
Ronan Dunklau
http://dalibo.com - http://dalibo.org