From 0bb8528b5c738b45d0b65844750588c16bf75c52 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 13 May 2015 14:05:17 -0400 Subject: [PATCH] Fix postgres_fdw to return the right ctid value in EvalPlanQual cases. If a postgres_fdw foreign table is a non-locked source relation in an UPDATE, DELETE, or SELECT FOR UPDATE/SHARE, and the query selects its ctid column, the wrong value would be returned if an EvalPlanQual recheck occurred. This happened because the foreign table's result row was copied via the ROW_MARK_COPY code path, and EvalPlanQualFetchRowMarks just unconditionally set the reconstructed tuple's t_self to "invalid". To fix that, we can have EvalPlanQualFetchRowMarks copy the composite datum's t_ctid field, and be sure to initialize that along with t_self when postgres_fdw constructs a tuple to return. If we just did that much then EvalPlanQualFetchRowMarks would start returning "(0,0)" as ctid for all other ROW_MARK_COPY cases, which perhaps does not matter much, but then again maybe it might. The cause of that is that heap_form_tuple, which is the ultimate source of all composite datums, simply leaves t_ctid as zeroes in newly constructed tuples. That seems like a bad idea on general principles: a field that's really not been initialized shouldn't appear to have a valid value. So let's eat the trivial additional overhead of doing "ItemPointerSetInvalid(&(td->t_ctid))" in heap_form_tuple. This closes out our handling of Etsuro Fujita's report that tableoid and ctid weren't correctly set in postgres_fdw EvalPlanQual cases. Along the way we did a great deal of work to improve FDWs' ability to control row locking behavior; which was not wasted effort by any means, but it didn't end up being a fix for this problem because that feature would be too expensive for postgres_fdw to use all the time. Although the fix for the tableoid misbehavior was back-patched, I'm hesitant to do so here; it seems far less likely that people would care about remote ctid than tableoid, and even such a minor behavioral change as this in heap_form_tuple is perhaps best not back-patched. So commit to HEAD only, at least for the moment. Etsuro Fujita, with some adjustments by me --- contrib/postgres_fdw/postgres_fdw.c | 8 +++++++- src/backend/access/common/heaptuple.c | 2 ++ src/backend/executor/execMain.c | 5 +++-- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 0c442608426..4f7123b84f4 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -2964,8 +2964,14 @@ make_tuple_from_result_row(PGresult *res, tuple = heap_form_tuple(tupdesc, values, nulls); + /* + * If we have a CTID to return, install it in both t_self and t_ctid. + * t_self is the normal place, but if the tuple is converted to a + * composite Datum, t_self will be lost; setting t_ctid allows CTID to be + * preserved during EvalPlanQual re-evaluations (see ROW_MARK_COPY code). + */ if (ctid) - tuple->t_self = *ctid; + tuple->t_self = tuple->t_data->t_ctid = *ctid; /* Clean up */ MemoryContextReset(temp_context); diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c index 6cd4e8e11ae..f58f81e1ed7 100644 --- a/src/backend/access/common/heaptuple.c +++ b/src/backend/access/common/heaptuple.c @@ -727,6 +727,8 @@ heap_form_tuple(TupleDesc tupleDescriptor, HeapTupleHeaderSetDatumLength(td, len); HeapTupleHeaderSetTypeId(td, tupleDescriptor->tdtypeid); HeapTupleHeaderSetTypMod(td, tupleDescriptor->tdtypmod); + /* We also make sure that t_ctid is invalid unless explicitly set */ + ItemPointerSetInvalid(&(td->t_ctid)); HeapTupleHeaderSetNatts(td, numberOfAttributes); td->t_hoff = hoff; diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 43d3c44c827..7c29b4b42ae 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -2613,10 +2613,11 @@ EvalPlanQualFetchRowMarks(EPQState *epqstate) /* build a temporary HeapTuple control structure */ tuple.t_len = HeapTupleHeaderGetDatumLength(td); - ItemPointerSetInvalid(&(tuple.t_self)); + tuple.t_data = td; /* relation might be a foreign table, if so provide tableoid */ tuple.t_tableOid = erm->relid; - tuple.t_data = td; + /* also copy t_ctid in case there's valid data there */ + tuple.t_self = td->t_ctid; /* copy and store tuple */ EvalPlanQualSetTuple(epqstate, erm->rti,