Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> On 2015/05/12 7:42, Tom Lane wrote:
>> I don't much like the division of labor between LockForeignRow and
>> FetchForeignRow. In principle that would lead to not one but two
>> extra remote accesses per locked row in SELECT FOR UPDATE, at least
>> in the case that an EvalPlanQual recheck is required. (I see that
>> in your prototype patch for postgres_fdw you attempt to avoid that
>> by saving a retrieved tuple in LockForeignRow and then returning it
>> in FetchForeignRow, but that seems extremely ugly and bug-prone,
>> since there is nothing in the API spec insisting that those calls match
>> up one-to-one.) The fact that locking and fetching a tuple are separate
>> operations in the heapam API is a historical artifact that probably
>> doesn't make sense to duplicate in the FDW API.

> I understand your concern about the postgres_fdw patch. However, I
> think we should divide this into the two routines as the core patch
> does, because I think that would allow an FDW author to implement these
> routines so as to improve the efficiency for scenarios that seldom need
> fetching, by not retrieving and saving locked tuples in LockForeignRow.

I find it hard to envision a situation where an FDW could lock a row
without being able to fetch its contents more or less for free. We have
IIRC discussed changing the way that works even for heapam, since the
current design requires multiple buffer lock/unlock cycles which aren't
free either. In any case, I think that the temptation to do probably-
buggy stuff like what you did in your prototype would be too strong for
most people, and that we're much better off with a simpler one-step API.

An additional advantage of the way I changed this is that it makes the
logic in nodeLockRows simpler too; we no longer need the very messy
hack added by commit 2db576ba8c449fca.

>> Another problem is that we fail to detect whether an EvalPlanQual recheck
>> is required during a SELECT FOR UPDATE on a remote table, which we
>> certainly ought to do if the objective is to make postgres_fdw semantics
>> match the local ones.

> I think that is interesting in theory, but I'm not sure that is worth
> much in practice.

Hm, well, AFAICS the entire point of this patch is to make it possible for
FDWs to duplicate the semantics used for local tables, so I'm not sure
why you'd want to ignore that aspect of it.

Anyway, I redid the patch along those lines, improved the documentation,
and have committed it.

I did a very basic update of your postgres_fdw patch to test this with,
and attach that so that you don't have to repeat the effort. I'm not sure
whether we want to try to convert that into something committable. I'm
afraid that the extra round trips involved in doing row locking this way
will be so expensive that no one really wants it for postgres_fdw. It's
more credible that FDWs operating against local storage would have use
for it.

regards, tom lane