(2011/11/14 11:25), Robert Haas wrote:
> My vote is to nuke 'em all. :-)
+1.
IIRC, main purpose of supporting tableoid for foreign tables was to be
basis of foreign table inheritance, which was not included in 9.1, and
we have not supported it yet. Other system columns are essentially
garbage, but they survived at 9.1 development because (maybe) it seemed
little odd to have system columns partially at that time.
So, IMHO removing all system columns from foreign tables seems
reasonable, unless it doesn't break any external tool seriously (Perhaps
there would be few tools which assume that foreign tables have system
columns).
If there seems to be a consensus on removing system column from foreign
tables, I'd like to work on this issue. Attached is a halfway patch,
and ISTM there is no problem so far.
Regards,
--
Shigeru Hanada
diff --git a/contrib/file_fdw/input/file_fdw.source
b/contrib/file_fdw/input/file_fdw.source
index 8e3d553..8ddeb17 100644
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
*************** EXECUTE st(100);
*** 111,119 ****
EXECUTE st(100);
DEALLOCATE st;
- -- tableoid
- SELECT tableoid::regclass, b FROM agg_csv;
-
-- updates aren't supported
INSERT INTO agg_csv VALUES(1,2.0);
UPDATE agg_csv SET a = 1;
--- 111,116 ----
diff --git a/contrib/file_fdw/output/file_fdw.source
b/contrib/file_fdw/output/file_fdw.source
index 84f0750..adf03c5 100644
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
*************** EXECUTE st(100);
*** 174,188 ****
(1 row)
DEALLOCATE st;
- -- tableoid
- SELECT tableoid::regclass, b FROM agg_csv;
- tableoid | b
- ----------+---------
- agg_csv | 99.097
- agg_csv | 0.09561
- agg_csv | 324.78
- (3 rows)
-
-- updates aren't supported
INSERT INTO agg_csv VALUES(1,2.0);
ERROR: cannot change foreign table "agg_csv"
--- 174,179 ----
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index e11d896..33f91d8 100644
*** a/src/backend/catalog/heap.c
--- b/src/backend/catalog/heap.c
*************** CheckAttributeNamesTypes(TupleDesc tupde
*** 399,408 ****
/*
* first check for collision with system attribute names
*
! * Skip this for a view or type relation, since those don't have system
! * attributes.
*/
! if (relkind != RELKIND_VIEW && relkind != RELKIND_COMPOSITE_TYPE)
{
for (i = 0; i < natts; i++)
{
--- 399,409 ----
/*
* first check for collision with system attribute names
*
! * Skip this for a view or type relation or foreign table, since those
! * don't have system attributes.
*/
! if (relkind != RELKIND_VIEW && relkind != RELKIND_COMPOSITE_TYPE &&
! relkind != RELKIND_FOREIGN_TABLE)
{
for (i = 0; i < natts; i++)
{
*************** AddNewAttributeTuples(Oid new_rel_oid,
*** 695,704 ****
/*
* Next we add the system attributes. Skip OID if rel has no OIDs. Skip
! * all for a view or type relation. We don't bother with making
datatype
! * dependencies here, since presumably all these types are pinned.
*/
! if (relkind != RELKIND_VIEW && relkind != RELKIND_COMPOSITE_TYPE)
{
for (i = 0; i < (int) lengthof(SysAtt); i++)
{
--- 696,707 ----
/*
* Next we add the system attributes. Skip OID if rel has no OIDs. Skip
! * all for a view or type relation or foreign table. We don't bother
with
! * making datatype dependencies here, since presumably all these types
are
! * pinned.
*/
! if (relkind != RELKIND_VIEW && relkind != RELKIND_COMPOSITE_TYPE &&
! relkind != RELKIND_FOREIGN_TABLE)
{
for (i = 0; i < (int) lengthof(SysAtt); i++)
{
diff --git a/src/backend/executor/nodeForeignscan.c
b/src/backend/executor/nodeForeignscan.c
index 841ae69..f7b8393 100644
*** a/src/backend/executor/nodeForeignscan.c
--- b/src/backend/executor/nodeForeignscan.c
*************** ForeignNext(ForeignScanState *node)
*** 51,67 ****
MemoryContextSwitchTo(oldcontext);
/*
! * 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);
- }
return slot;
}
--- 51,62 ----
MemoryContextSwitchTo(oldcontext);
/*
! * XXX If we support system columns and any of them are requested, we
have
! * to force the tuple into physical-tuple form to avoid "cannot extract
! * system attribute from virtual tuple" errors later.
! *
! * After materializing, we can also set tableoid of the result tuple.
*/
return slot;
}
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 63958c3..542584b 100644
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
*************** _copyForeignScan(ForeignScan *from)
*** 590,596 ****
/*
* copy remainder of node
*/
- COPY_SCALAR_FIELD(fsSystemCol);
COPY_NODE_FIELD(fdwplan);
return newnode;
--- 590,595 ----
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index f7d39ed..b416bb2 100644
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
*************** _outForeignScan(StringInfo str, ForeignS
*** 557,563 ****
_outScanInfo(str, (Scan *) node);
- WRITE_BOOL_FIELD(fsSystemCol);
WRITE_NODE_FIELD(fdwplan);
}
--- 557,562 ----
diff --git a/src/backend/optimizer/plan/createplan.c
b/src/backend/optimizer/plan/createplan.c
index 8138b01..c848ee2 100644
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
*************** static CteScan *make_ctescan(List *qptli
*** 123,129 ****
static WorkTableScan *make_worktablescan(List *qptlist, List *qpqual,
Index scanrelid, int wtParam);
static ForeignScan *make_foreignscan(List *qptlist, List *qpqual,
! Index scanrelid, bool fsSystemCol, FdwPlan
*fdwplan);
static BitmapAnd *make_bitmap_and(List *bitmapplans);
static BitmapOr *make_bitmap_or(List *bitmapplans);
static NestLoop *make_nestloop(List *tlist,
--- 123,129 ----
static WorkTableScan *make_worktablescan(List *qptlist, List *qpqual,
Index scanrelid, int wtParam);
static ForeignScan *make_foreignscan(List *qptlist, List *qpqual,
! Index scanrelid, FdwPlan *fdwplan);
static BitmapAnd *make_bitmap_and(List *bitmapplans);
static BitmapOr *make_bitmap_or(List *bitmapplans);
static NestLoop *make_nestloop(List *tlist,
*************** create_foreignscan_plan(PlannerInfo *roo
*** 1827,1833 ****
RelOptInfo *rel = best_path->path.parent;
Index scan_relid = rel->relid;
RangeTblEntry *rte;
- bool fsSystemCol;
int i;
/* it should be a base rel... */
--- 1827,1832 ----
*************** create_foreignscan_plan(PlannerInfo *roo
*** 1842,1862 ****
/* Reduce RestrictInfo list to bare expressions; ignore pseudoconstants
*/
scan_clauses = extract_actual_clauses(scan_clauses, false);
- /* Detect whether any system columns are requested from rel */
- fsSystemCol = false;
- for (i = rel->min_attr; i < 0; i++)
- {
- if (!bms_is_empty(rel->attr_needed[i - rel->min_attr]))
- {
- fsSystemCol = true;
- break;
- }
- }
-
scan_plan = make_foreignscan(tlist,
scan_clauses,
scan_relid,
- fsSystemCol,
best_path->fdwplan);
copy_path_costsize(&scan_plan->scan.plan, &best_path->path);
--- 1841,1849 ----
*************** static ForeignScan *
*** 3170,3176 ****
make_foreignscan(List *qptlist,
List *qpqual,
Index scanrelid,
- bool fsSystemCol,
FdwPlan *fdwplan)
{
ForeignScan *node = makeNode(ForeignScan);
--- 3157,3162 ----
*************** make_foreignscan(List *qptlist,
*** 3182,3188 ****
plan->lefttree = NULL;
plan->righttree = NULL;
node->scan.scanrelid = scanrelid;
- node->fsSystemCol = fsSystemCol;
node->fdwplan = fdwplan;
return node;
--- 3168,3173 ----
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 6685864..8f1dfa3 100644
*** a/src/include/nodes/plannodes.h
--- b/src/include/nodes/plannodes.h
*************** typedef struct WorkTableScan
*** 467,473 ****
typedef struct ForeignScan
{
Scan scan;
- bool fsSystemCol; /* true if any "system column" is
needed */
/* use struct pointer to avoid including fdwapi.h here */
struct FdwPlan *fdwplan;
} ForeignScan;
--- 467,472 ----
diff --git a/src/test/regress/expected/foreign_data.out
b/src/test/regress/expected/foreign_data.out
index 2687d51..c11574b 100644
*** a/src/test/regress/expected/foreign_data.out
--- b/src/test/regress/expected/foreign_data.out
*************** ALTER FOREIGN TABLE ft1 ALTER COLUMN c8
*** 699,706 ****
ERROR: "ft1" is not a table
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE char(10);
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 SET DATA TYPE text;
- ALTER FOREIGN TABLE ft1 ALTER COLUMN xmin OPTIONS (ADD p1 'v1'); -- ERROR
- ERROR: cannot alter system column "xmin"
ALTER FOREIGN TABLE ft1 ALTER COLUMN c7 OPTIONS (ADD p1 'v1', ADD p2 'v2'),
ALTER COLUMN c8 OPTIONS (ADD p1 'v1', ADD p2 'v2');
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 OPTIONS (SET p2 'V2', DROP p1);
--- 699,704 ----
diff --git a/src/test/regress/sql/foreign_data.sql
b/src/test/regress/sql/foreign_data.sql
index 051dfa3..fe3750e 100644
*** a/src/test/regress/sql/foreign_data.sql
--- b/src/test/regress/sql/foreign_data.sql
*************** ALTER FOREIGN TABLE ft1 ALTER COLUMN c7
*** 297,303 ****
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE char(10) USING '0'; -- ERROR
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE char(10);
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 SET DATA TYPE text;
- ALTER FOREIGN TABLE ft1 ALTER COLUMN xmin OPTIONS (ADD p1 'v1'); -- ERROR
ALTER FOREIGN TABLE ft1 ALTER COLUMN c7 OPTIONS (ADD p1 'v1', ADD p2 'v2'),
ALTER COLUMN c8 OPTIONS (ADD p1 'v1', ADD p2 'v2');
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 OPTIONS (SET p2 'V2', DROP p1);
--- 297,302 ----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
