Обсуждение: 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

(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

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

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