On 2016/03/16 16:25, Etsuro Fujita wrote:
> PG9.5 allows us to add an oid system column to foreign tables, using
> ALTER FOREIGN TABLE SET WITH OIDS, but currently, that column reads as
> zeroes in postgres_fdw. That seems to me like a bug. So, I'd like to
> propose to fix that, by retrieving that column from the remote server
> when requested. I'm attaching a proposed patch for that.
I rebased the patch against HEAD. Updated patch attached. I don't
think this is a 9.6 issue, but I think this is somewhat related to the
9.6 issue [1] that postgres_fdw join pushdown incorrectly retrieves
system columns other than ctid and oid from the remote server, so ISTM
that it's better to address this in conjunction with [1].
Best regards,
Etsuro Fujita
[1] http://www.postgresql.org/message-id/[email protected]
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***************
*** 287,299 **** foreign_expr_walker(Node *node,
/* Var belongs to foreign table */
/*
! * System columns other than ctid should not be sent to
! * the remote, since we don't make any effort to ensure
! * that local and remote values match (tableoid, in
* particular, almost certainly doesn't match).
*/
if (var->varattno < 0 &&
! var->varattno != SelfItemPointerAttributeNumber)
return false;
/* Else check the collation */
--- 287,300 ----
/* Var belongs to foreign table */
/*
! * System columns other than ctid and oid should not be
! * sent to the remote, since we don't make any effort to
! * ensure that local and remote values match (tableoid, in
* particular, almost certainly doesn't match).
*/
if (var->varattno < 0 &&
! var->varattno != SelfItemPointerAttributeNumber &&
! var->varattno != ObjectIdAttributeNumber)
return false;
/* Else check the collation */
***************
*** 911,918 **** deparseTargetList(StringInfo buf,
}
/*
! * Add ctid if needed. We currently don't support retrieving any other
! * system columns.
*/
if (bms_is_member(SelfItemPointerAttributeNumber - FirstLowInvalidHeapAttributeNumber,
attrs_used))
--- 912,919 ----
}
/*
! * Add ctid and oid if needed. We currently don't support retrieving any
! * other system columns.
*/
if (bms_is_member(SelfItemPointerAttributeNumber - FirstLowInvalidHeapAttributeNumber,
attrs_used))
***************
*** 930,935 **** deparseTargetList(StringInfo buf,
--- 931,952 ----
*retrieved_attrs = lappend_int(*retrieved_attrs,
SelfItemPointerAttributeNumber);
}
+ if (bms_is_member(ObjectIdAttributeNumber - FirstLowInvalidHeapAttributeNumber,
+ attrs_used))
+ {
+ if (!first)
+ appendStringInfoString(buf, ", ");
+ else if (is_returning)
+ appendStringInfoString(buf, " RETURNING ");
+ first = false;
+
+ if (qualify_col)
+ ADD_REL_QUALIFIER(buf, rtindex);
+ appendStringInfoString(buf, "oid");
+
+ *retrieved_attrs = lappend_int(*retrieved_attrs,
+ ObjectIdAttributeNumber);
+ }
/* Don't generate bad syntax if no undropped columns */
if (first && !is_returning)
***************
*** 1571,1583 **** deparseColumnRef(StringInfo buf, int varno, int varattno, PlannerInfo *root,
{
RangeTblEntry *rte;
! /* varattno can be a whole-row reference, ctid or a regular table column */
if (varattno == SelfItemPointerAttributeNumber)
{
if (qualify_col)
ADD_REL_QUALIFIER(buf, varno);
appendStringInfoString(buf, "ctid");
}
else if (varattno == 0)
{
/* Whole row reference */
--- 1588,1609 ----
{
RangeTblEntry *rte;
! /*
! * varattno can be a ctid, an oid, a whole-row reference, or a regular
! * table column
! */
if (varattno == SelfItemPointerAttributeNumber)
{
if (qualify_col)
ADD_REL_QUALIFIER(buf, varno);
appendStringInfoString(buf, "ctid");
}
+ else if (varattno == ObjectIdAttributeNumber)
+ {
+ if (qualify_col)
+ ADD_REL_QUALIFIER(buf, varno);
+ appendStringInfoString(buf, "oid");
+ }
else if (varattno == 0)
{
/* Whole row reference */
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 2286,2292 **** DEALLOCATE st2;
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
! -- System columns, except ctid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS false)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
QUERY PLAN
--- 2286,2292 ----
DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
! -- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS false)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
QUERY PLAN
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 4298,4303 **** make_tuple_from_result_row(PGresult *res,
--- 4298,4304 ----
Datum *values;
bool *nulls;
ItemPointer ctid = NULL;
+ Oid oid = InvalidOid;
ConversionLocation errpos;
ErrorContextCallback errcallback;
MemoryContext oldcontext;
***************
*** 4355,4361 **** make_tuple_from_result_row(PGresult *res,
else
valstr = PQgetvalue(res, row, j);
! /* convert value to internal representation */
errpos.cur_attno = i;
if (i > 0)
{
--- 4356,4366 ----
else
valstr = PQgetvalue(res, row, j);
! /*
! * convert value to internal representation
! *
! * Note: we ignore system columns other than ctid and oid in result
! */
errpos.cur_attno = i;
if (i > 0)
{
***************
*** 4370,4376 **** make_tuple_from_result_row(PGresult *res,
}
else if (i == SelfItemPointerAttributeNumber)
{
! /* ctid --- note we ignore any other system column in result */
if (valstr != NULL)
{
Datum datum;
--- 4375,4381 ----
}
else if (i == SelfItemPointerAttributeNumber)
{
! /* ctid */
if (valstr != NULL)
{
Datum datum;
***************
*** 4379,4384 **** make_tuple_from_result_row(PGresult *res,
--- 4384,4400 ----
ctid = (ItemPointer) DatumGetPointer(datum);
}
}
+ else if (i == ObjectIdAttributeNumber)
+ {
+ /* oid */
+ if (valstr != NULL)
+ {
+ Datum datum;
+
+ datum = DirectFunctionCall1(oidin, CStringGetDatum(valstr));
+ oid = DatumGetObjectId(datum);
+ }
+ }
errpos.cur_attno = 0;
j++;
***************
*** 4410,4415 **** make_tuple_from_result_row(PGresult *res,
--- 4426,4437 ----
if (ctid)
tuple->t_self = tuple->t_data->t_ctid = *ctid;
+ /*
+ * If we have an OID to return, install it.
+ */
+ if (OidIsValid(oid))
+ HeapTupleSetOid(tuple, oid);
+
/* Clean up */
MemoryContextReset(temp_context);
***************
*** 4436,4441 **** conversion_error_callback(void *arg)
--- 4458,4465 ----
attname = NameStr(tupdesc->attrs[errpos->cur_attno - 1]->attname);
else if (errpos->cur_attno == SelfItemPointerAttributeNumber)
attname = "ctid";
+ else if (errpos->cur_attno == ObjectIdAttributeNumber)
+ attname = "oid";
relname = RelationGetRelationName(errpos->rel);
}
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 557,563 **** DEALLOCATE st3;
DEALLOCATE st4;
DEALLOCATE st5;
! -- System columns, except ctid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS false)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'ft1'::regclass LIMIT 1;
--- 557,563 ----
DEALLOCATE st4;
DEALLOCATE st5;
! -- System columns, except ctid and oid, should not be sent to remote
EXPLAIN (VERBOSE, COSTS false)
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'pg_class'::regclass LIMIT 1;
SELECT * FROM ft1 t1 WHERE t1.tableoid = 'ft1'::regclass LIMIT 1;
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
