Re: EvalPlanQual behaves oddly for FDW queries involving system columns

On 2015/05/13 3:24, Tom Lane wrote:
> 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.

OK

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

Sorry, my explanation was not correct. For me, the objective was not
necessarily to make it possible for FDWs to duplicate the semantics, but
to make it possible for FDWs to improve the efficiency of SELECT FOR
UPDATE on foreign tables (and UPDATE/DELETE involving foreign tables as
source relations) by making it possible for FDWs to duplicate the
semantics. And I didn't think that the idea of detecting such a thing
would be in itself directly useful for the improved efficiency. Maybe
I'm missing something, though.

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

Thanks for improving and committing the patch. I'm so happy with the
committed version.

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

I think so too. And thanks for updating the patch.

Best regards,
Etsuro Fujita