On 2015/01/16 1:24, Alvaro Herrera wrote:
Etsuro Fujita wrote:
*** 817,826 **** InitPlan(QueryDesc *queryDesc, int eflags)
--- 818,833 ----
break;
case ROW_MARK_COPY:
/* there's no real table here ... */
+ relkind = rt_fetch(rc->rti,
rangeTable)->relkind;
+ if (relkind == RELKIND_FOREIGN_TABLE)
+ relid = getrelid(rc->rti, rangeTable);
+ else
+ relid = InvalidOid;
relation = NULL;
break;
--- 2326,2342 ----
/* build a temporary HeapTuple control structure */
tuple.t_len = HeapTupleHeaderGetDatumLength(td);
! /* if relid is valid, rel is a foreign table; set
system columns */
! if (OidIsValid(erm->relid))
! {
! tuple.t_self = td->t_ctid;
! tuple.t_tableOid = erm->relid;
! }
! else
! {
! ItemPointerSetInvalid(&(tuple.t_self));
! tuple.t_tableOid = InvalidOid;
! }
tuple.t_data = td;
I find this arrangement confusing and unnecessary -- surely if you have
access to the ExecRowMark here, you could just obtain the relid with
RelationGetRelid instead of saving the OID beforehand?
I don't think so because we don't have the Relation (ie, erm->relation =
NULL) here since InitPlan don't open/lock relations having markType =
ROW_MARK_COPY as shown above, which include foreign tables selected for
update/share. But I noticed we should open/lock such foreign tables as
well in InitPlan because I think ExecOpenScanRelation is assuming that,
IIUC.
> And if you have
the Relation, you could just consult the relkind at that point instead
of relying on the relid being set or not as a flag to indicate whether
the rel is foreign.
Followed your revision. Attached is an updated version of the patch.
Thanks for the comment!
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 2947,2953 **** make_tuple_from_result_row(PGresult *res,
tuple = heap_form_tuple(tupdesc, values, nulls);
if (ctid)
! tuple->t_self = *ctid;
/* Clean up */
MemoryContextReset(temp_context);
--- 2947,2953 ----
tuple = heap_form_tuple(tupdesc, values, nulls);
if (ctid)
! tuple->t_self = tuple->t_data->t_ctid = *ctid;
/* Clean up */
MemoryContextReset(temp_context);
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***************
*** 795,800 **** InitPlan(QueryDesc *queryDesc, int eflags)
--- 795,801 ----
{
PlanRowMark *rc = (PlanRowMark *) lfirst(l);
Oid relid;
+ char relkind;
Relation relation;
ExecRowMark *erm;
***************
*** 817,823 **** InitPlan(QueryDesc *queryDesc, int eflags)
break;
case ROW_MARK_COPY:
/* there's no real table here ... */
! relation = NULL;
break;
default:
elog(ERROR, "unrecognized markType: %d", rc->markType);
--- 818,831 ----
break;
case ROW_MARK_COPY:
/* there's no real table here ... */
! relkind = rt_fetch(rc->rti, rangeTable)->relkind;
! if (relkind == RELKIND_FOREIGN_TABLE)
! {
! relid = getrelid(rc->rti, rangeTable);
! relation = heap_open(relid, AccessShareLock);
! }
! else
! relation = NULL;
break;
default:
elog(ERROR, "unrecognized markType: %d", rc->markType);
***************
*** 1115,1125 **** CheckValidRowMarkRel(Relation rel, RowMarkType markType)
RelationGetRelationName(rel))));
break;
case RELKIND_FOREIGN_TABLE:
! /* Should not get here; planner should have used ROW_MARK_COPY */
! ereport(ERROR,
! (errcode(ERRCODE_WRONG_OBJECT_TYPE),
! errmsg("cannot lock rows in foreign table \"%s\"",
! RelationGetRelationName(rel))));
break;
default:
ereport(ERROR,
--- 1123,1134 ----
RelationGetRelationName(rel))));
break;
case RELKIND_FOREIGN_TABLE:
! /* Allow opening/locking a foreign table only if ROW_MARK_COPY */
! if (markType != ROW_MARK_COPY)
! ereport(ERROR,
! (errcode(ERRCODE_WRONG_OBJECT_TYPE),
! errmsg("cannot lock rows in foreign table \"%s\"",
! RelationGetRelationName(rel))));
break;
default:
ereport(ERROR,
***************
*** 1792,1798 **** ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist)
aerm->rowmark = erm;
/* Look up the resjunk columns associated with this rowmark */
! if (erm->relation)
{
Assert(erm->markType != ROW_MARK_COPY);
--- 1801,1808 ----
aerm->rowmark = erm;
/* Look up the resjunk columns associated with this rowmark */
! if (erm->relation &&
! erm->relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE)
{
Assert(erm->markType != ROW_MARK_COPY);
***************
*** 2256,2262 **** EvalPlanQualFetchRowMarks(EPQState *epqstate)
/* clear any leftover test tuple for this rel */
EvalPlanQualSetTuple(epqstate, erm->rti, NULL);
! if (erm->relation)
{
Buffer buffer;
--- 2266,2273 ----
/* clear any leftover test tuple for this rel */
EvalPlanQualSetTuple(epqstate, erm->rti, NULL);
! if (erm->relation &&
! erm->relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE)
{
Buffer buffer;
***************
*** 2318,2325 **** EvalPlanQualFetchRowMarks(EPQState *epqstate)
/* build a temporary HeapTuple control structure */
tuple.t_len = HeapTupleHeaderGetDatumLength(td);
! ItemPointerSetInvalid(&(tuple.t_self));
! tuple.t_tableOid = InvalidOid;
tuple.t_data = td;
/* copy and store tuple */
--- 2329,2346 ----
/* build a temporary HeapTuple control structure */
tuple.t_len = HeapTupleHeaderGetDatumLength(td);
! /* if relation is opened, the rel is foreign; set system columns */
! if (erm->relation)
! {
! Assert(erm->relation->rd_rel->relkind == RELKIND_FOREIGN_TABLE);
! tuple.t_self = td->t_ctid;
! tuple.t_tableOid = RelationGetRelid(erm->relation);
! }
! else
! {
! ItemPointerSetInvalid(&(tuple.t_self));
! tuple.t_tableOid = InvalidOid;
! }
tuple.t_data = td;
/* copy and store tuple */
*** a/src/backend/executor/nodeForeignscan.c
--- b/src/backend/executor/nodeForeignscan.c
***************
*** 22,27 ****
--- 22,28 ----
*/
#include "postgres.h"
+ #include "access/htup_details.h"
#include "executor/executor.h"
#include "executor/nodeForeignscan.h"
#include "foreign/fdwapi.h"
***************
*** 53,65 **** ForeignNext(ForeignScanState *node)
/*
* If any system columns are requested, we have to force the tuple into
* physical-tuple form to avoid "cannot extract system attribute from
! * virtual tuple" errors later. We also insert a valid value for
! * tableoid, which is the only actually-useful system column.
*/
if (plan->fsSystemCol && !TupIsNull(slot))
{
HeapTuple tup = ExecMaterializeSlot(slot);
tup->t_tableOid = RelationGetRelid(node->ss.ss_currentRelation);
}
--- 54,69 ----
/*
* If any system columns are requested, we have to force the tuple into
* physical-tuple form to avoid "cannot extract system attribute from
! * virtual tuple" errors later. We also insert a valid value for TID
! * and tableoid, which are the only actually-useful system columns.
*/
if (plan->fsSystemCol && !TupIsNull(slot))
{
HeapTuple tup = ExecMaterializeSlot(slot);
+ /* We assume that t_ctid is initialized with its own TID */
+ tup->t_self = tup->t_data->t_ctid;
+
tup->t_tableOid = RelationGetRelid(node->ss.ss_currentRelation);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
