Re: Triggers on foreign tables

On Mon, Mar 17, 2014 at 11:54 PM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>> I hacked on this for awhile, but there remain two matters on which I'm
>> uncertain about the right way forward.
>>
>> (1) To acquire the old tuple for UPDATE/DELETE operations, the patch closely
>> parallels our handling for INSTEAD OF triggers on views. It adds a wholerow
>> resjunk attribute, from which it constructs a HeapTuple before calling a
>> trigger function. This loses the system columns, an irrelevant
>> consideration for views but meaningful for foreign tables. postgres_fdw
>> maintains the "ctid" system column (t_self), but triggers will always see
>> an invalid t_self for the old tuple. One way to fix this is to pass around
>> the old tuple data as ROW(ctid, oid, xmin, cmin, xmax, cmax, tableoid,
>> wholerow). That's fairly close to sufficient, but it doesn't cover t_ctid.
>> Frankly, I would like to find a principled excuse to not worry about making
>> foreign table system columns accessible from triggers. Supporting system
>> columns dramatically affects the mechanism, and what trigger is likely to
>> care? Unfortunately, that argument seems too weak. Does anyone have a
>> cleaner idea for keeping track of the system column values or a stronger
>> argument for not bothering?
>>
> Regarding to the first suggestion,
> I think, it is better not to care about system columns on foreign tables,
> because it fully depends on driver's implementation whether FDW fetches
> "ctid" from its data source, or not.
> Usually, system columns of foreign table, except for tableoid, are nonsense.
> Because of implementation reason, postgres_fdw fetches "ctid" of remote
> tables on UPDATE / DELETE, it is not a common nature for all FDW drivers.
> For example, we can assume an implementation that uses primary key of remote
> table to identify the record to be updated or deleted. In this case, local
> "ctid" does not have meaningful value.
> So, fundamentally, we cannot guarantee FDW driver returns meaningful "ctid"
> or other system columns.

I'm not sure I particularly agree with this reasoning - after all,
just because some people might not find a feature useful isn't a
reason not to have it. On the other hand, I don't think it's a very
useful feature, and I don't feel like we have to have it. Most system
columns can't be updated or indexed, and none of them can be dropped
or renamed, so it's not like they aren't second-class citizens to some
degree already.

By way of comparison, the first version of the index-only scan patch
gave up when it saw an expression index, instead of making an effort
to figure out whether a matching expression was present in the query.
Somebody could have looked at that patch and said, oh, well, that's an
ugly and unacceptable limitation, and we ought to reject the patch
until it's fixed. Well, instead, Tom committed the patch, and we
still have that limitation today, and it's still something we really
ought to fix some day, but in the meantime we have the feature.

Obviously, the line between "your patch is only part of a feature,
please finish it and try again" and "your patch implements a nice
self-contained feature and there are some more things that we could
build on top of it later" is to some extent a matter of judgement; but
for what it's worth, I can't get too excited about this particular
limitation of this particular patch. I just don't think that very
many people are going to miss the functionality in question.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company