Re: 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

>
> 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.

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