On 2015/05/13 3:24, Tom Lane wrote:
Etsuro Fujita <fujita.ets...@lab.ntt.co.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
