Re: [HACKERS] A question about code in DefineRelation()
On 04/25/2014 04:39 PM, Hadi Moshayedi wrote: On second thought I noticed that that makes CREATE FOREIGN TABLE include an OID column in newly-created foreign tables wrongly, when the default_with_oids parameter is set to on. Please find attached a patch. Yeah, that's a bug. The interactions with pg_dump are interesting. If you have any tables with OIDs in your database, pg_dump will output "set default_with_oids=true" before creating those tables. And if you have any foreign tables that end up being dumped after the table with OIDs, it will also be created with "default_with_oids=true", and will end up with OIDs. Fortunately, nothing very bad happens if a foreign table has oids. It will just be all-zeros if you select it. Committed, and backpatched. 9.2 and 9.1 needed a slightly different patch because the code has changed, but it was still straightforward. The fix makes sense to me, since in ALTER TABLE SET WITH OIDS we check that the relation is a table and not a foreign table: 3160 case AT_AddOids: /* SET WITH OIDS */ 3161 ATSimplePermissions(rel, ATT_TABLE); So, I think we should be consistent between DefineRelation() and alter table. Yeah, default_with_oids is definitely not supposed to affect foreign tables. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A question about code in DefineRelation()
Hi Hadi, Sorry for the delay. (2014/04/25 22:39), Hadi Moshayedi wrote: On second thought I noticed that that makes CREATE FOREIGN TABLE include an OID column in newly-created foreign tables wrongly, when the default_with_oids parameter is set to on. Please find attached a patch. The fix makes sense to me, since in ALTER TABLE SET WITH OIDS we check that the relation is a table and not a foreign table: 3160 case AT_AddOids:/* SET WITH OIDS */ 3161 ATSimplePermissions(rel, ATT_TABLE); So, I think we should be consistent between DefineRelation() and alter table. Thank you for the review. I added this to 2014-06 and marked it as "Ready for Committer". 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
Re: [HACKERS] A question about code in DefineRelation()
> > On second thought I noticed that that makes CREATE FOREIGN TABLE include > an OID column in newly-created foreign tables wrongly, when the > default_with_oids parameter is set to on. Please find attached a patch. > > The fix makes sense to me, since in ALTER TABLE SET WITH OIDS we check that the relation is a table and not a foreign table: 3160 case AT_AddOids: /* SET WITH OIDS */ 3161 ATSimplePermissions(rel, ATT_TABLE); So, I think we should be consistent between DefineRelation() and alter table. -- Hadi.
Re: [HACKERS] A question about code in DefineRelation()
(2014/04/04 13:35), Etsuro Fujita wrote: > If I understand correctly, foreign tables cannot have an OID column, but > the following code in DefineRelation() assumes that foreign tables *can* > have that coulum: On second thought I noticed that that makes CREATE FOREIGN TABLE include an OID column in newly-created foreign tables wrongly, when the default_with_oids parameter is set to on. Please find attached a patch. Thanks, Best regards, Etsuro Fujita *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** *** 565,572 DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId) descriptor = BuildDescForRelation(schema); localHasOids = interpretOidsOption(stmt->options, ! (relkind == RELKIND_RELATION || ! relkind == RELKIND_FOREIGN_TABLE)); descriptor->tdhasoid = (localHasOids || parentOidCount > 0); /* --- 565,571 descriptor = BuildDescForRelation(schema); localHasOids = interpretOidsOption(stmt->options, ! relkind == RELKIND_RELATION); descriptor->tdhasoid = (localHasOids || parentOidCount > 0); /* -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] A question about code in DefineRelation()
If I understand correctly, foreign tables cannot have an OID column, but the following code in DefineRelation() assumes that foreign tables *can* have that coulum: 560 /* 561 * Create a tuple descriptor from the relation schema. Note that this 562 * deals with column names, types, and NOT NULL constraints, but not 563 * default values or CHECK constraints; we handle those below. 564 */ 565 descriptor = BuildDescForRelation(schema); 566 567 localHasOids = interpretOidsOption(stmt->options, 568(relkind == RELKIND_RELATION || 569 relkind == RELKIND_FOREIGN_TABLE)); Is this intended to support an OID column on foreign tables in future? Thanks, 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
