Horton

Thread

Commits

  1. Handle OID column inheritance correctly in ALTER TABLE ... INHERIT.

  1. ALTER TABLE parent SET WITHOUT OIDS and the oid column

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2016-12-26T10:06:59Z

    I suspect the following is a bug:

create table foo (a int) with oids;
CREATE TABLE
create table bar (a int);
CREATE TABLE
alter table bar inherit foo;
ERROR:  table "bar" without OIDs cannot inherit from table "foo" with OIDs

alter table bar set with oids;
ALTER TABLE
alter table bar inherit foo;
ALTER TABLE

alter table foo set without oids;
ERROR:  relation 16551 has non-inherited attribute "oid"

Because:

select attinhcount from pg_attribute where attrelid = 'bar'::regclass and
attname = 'oid';
 attinhcount
-------------
           0
(1 row)

Which also means "oid" can be safely dropped from bar breaking the
invariant that if the parent table has oid column, its child tables must too:

alter table bar drop oid;  -- or, alter table bar set without oids;
ALTER TABLE

Attached patches modifies MergeAttributesIntoExisting() such that we
increment attinhcount not only for user attributes, but also for the oid
system column if one exists.

Thoughts?

Thanks,
Amit

  2. Re: ALTER TABLE parent SET WITHOUT OIDS and the oid column

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2016-12-26T10:11:42Z

    On 2016/12/26 19:06, Amit Langote wrote:
> I suspect the following is a bug:

A better subject line could be: "ALTER TABLE INHERIT and the oid column"

Thanks,
Amit

  3. Re: ALTER TABLE parent SET WITHOUT OIDS and the oid column

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2016-12-27T13:24:56Z

    On Mon, Dec 26, 2016 at 3:36 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> I suspect the following is a bug:
>
> create table foo (a int) with oids;
> CREATE TABLE
> create table bar (a int);
> CREATE TABLE
> alter table bar inherit foo;
> ERROR:  table "bar" without OIDs cannot inherit from table "foo" with OIDs
>
> alter table bar set with oids;
> ALTER TABLE
> alter table bar inherit foo;
> ALTER TABLE
>
> alter table foo set without oids;
> ERROR:  relation 16551 has non-inherited attribute "oid"
>
> Because:
>
> select attinhcount from pg_attribute where attrelid = 'bar'::regclass and
> attname = 'oid';
>  attinhcount
> -------------
>            0
> (1 row)
>
> Which also means "oid" can be safely dropped from bar breaking the
> invariant that if the parent table has oid column, its child tables must too:
>
> alter table bar drop oid;  -- or, alter table bar set without oids;
> ALTER TABLE
>
> Attached patches modifies MergeAttributesIntoExisting() such that we
> increment attinhcount not only for user attributes, but also for the oid
> system column if one exists.
>
> Thoughts?

Yes, if a child inherits from a parent with OID, child gets OID column
and its inhcount is set to 1.
postgres=# create table foo (a int) with oids;
CREATE TABLE
postgres=# create table bar() inherits (foo);
CREATE TABLE
postgres=# \d+ foo
                                    Table "public.foo"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats
target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
 a      | integer |           |          |         | plain   |              |
Child tables: bar
Has OIDs: yes

postgres=# \d+ bar
                                    Table "public.bar"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats
target | Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
 a      | integer |           |          |         | plain   |              |
Inherits: foo
Has OIDs: yes

postgres=# select attname, attinhcount from pg_attribute where
attrelid = 'bar'::regclass;
 attname  | attinhcount
----------+-------------
 tableoid |           0
 cmax     |           0
 xmax     |           0
 cmin     |           0
 xmin     |           0
 oid      |           1
 ctid     |           0
 a        |           1
(8 rows)

So, I think your patch is on the right track.

We allow creating user attribute with name "oid" so you do not want to
search system attribute oid by name. Instead search by attribute id
ObjectIdAttributeNumber.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  4. Re: ALTER TABLE parent SET WITHOUT OIDS and the oid column

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2016-12-28T00:54:20Z

    On 2016/12/27 22:24, Ashutosh Bapat wrote:
> On Mon, Dec 26, 2016 at 3:36 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Attached patches modifies MergeAttributesIntoExisting() such that we
>> increment attinhcount not only for user attributes, but also for the oid
>> system column if one exists.
>>
>> Thoughts?
> 
> Yes, if a child inherits from a parent with OID, child gets OID column
> and its inhcount is set to 1.

[ ... ]

> So, I think your patch is on the right track.
> 
> We allow creating user attribute with name "oid" so you do not want to
> search system attribute oid by name. Instead search by attribute id
> ObjectIdAttributeNumber.

Good point.  Although, there can only be one of the two in a table at any
given time - either the "oid" system column or user-defined column named
"oid".  I was afraid that with the patch, the attinhcount of a
user-defined oid column may get incremented twice, but before that could
happen, one would get the error: "child table without OIDs cannot
inherited from table with OIDs"

create table foo (a int) with oids;
create table bar (a int, oid int);
alter table bar inherit foo;
ERROR:  table "bar" without OIDs cannot inherit from table "foo" with OIDs

And then:

alter table bar set with oids;
ERROR:  column "oid" of relation "bar" already exists

Thanks,
Amit

  5. Re: ALTER TABLE parent SET WITHOUT OIDS and the oid column

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2016-12-28T06:38:31Z

    >>
>> We allow creating user attribute with name "oid" so you do not want to
>> search system attribute oid by name. Instead search by attribute id
>> ObjectIdAttributeNumber.
>
> Good point.  Although, there can only be one of the two in a table at any
> given time - either the "oid" system column or user-defined column named
> "oid".  I was afraid that with the patch, the attinhcount of a
> user-defined oid column may get incremented twice, but before that could
> happen, one would get the error: "child table without OIDs cannot
> inherited from table with OIDs"
>
> create table foo (a int) with oids;
> create table bar (a int, oid int);
> alter table bar inherit foo;
> ERROR:  table "bar" without OIDs cannot inherit from table "foo" with OIDs
>
> And then:
>
> alter table bar set with oids;
> ERROR:  column "oid" of relation "bar" already exists
>

Right. But I think it's better to use attribute id, in case the code
raising this error changes for any reason in future. Or at least we
should add an assert or an elog to make sure that we are updating the
system attribute OID and not user defined one.

The code updating attinhcount and then updating the catalogs is same
for user defined attributes and OID. Should we separate it out into a
function and use that function instead of duplicating the code? The
problem with duplicating the code is one has to remember to update
both copies while changing it.

Your test uses tablenames starting with "_". I have not seen that
style in the testcases. Is it intentional?

Rest of the patch looks good to me.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  6. Re: ALTER TABLE parent SET WITHOUT OIDS and the oid column

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-04T23:05:46Z

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> Right. But I think it's better to use attribute id, in case the code
> raising this error changes for any reason in future.

I agree.  The parent's "tdhasoid" flag is definitely based on the
existence of an ObjectIdAttributeNumber system column, not on whether the
column's name is "oid".  So doing a lookup by name to find the matching
child column is just weird, and cannot possibly lead to anything good.

> The code updating attinhcount and then updating the catalogs is same
> for user defined attributes and OID. Should we separate it out into a
> function and use that function instead of duplicating the code?

Didn't really seem worth the trouble ... maybe if it gets any longer
it'd be appropriate to do that.

> Your test uses tablenames starting with "_". I have not seen that
> style in the testcases. Is it intentional?

Yeah, I did not like that either.

Pushed with those corrections and some further fooling with the test case.

			regards, tom lane

  7. Re: ALTER TABLE parent SET WITHOUT OIDS and the oid column

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-01-05T03:56:11Z

    >
>> The code updating attinhcount and then updating the catalogs is same
>> for user defined attributes and OID. Should we separate it out into a
>> function and use that function instead of duplicating the code?
>
> Didn't really seem worth the trouble ... maybe if it gets any longer
> it'd be appropriate to do that.

Ok.

>
> Pushed with those corrections and some further fooling with the test case.
>

Thanks.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  8. Re: ALTER TABLE parent SET WITHOUT OIDS and the oid column

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-01-05T04:33:00Z

    On 2017/01/05 8:05, Tom Lane wrote:
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>> Right. But I think it's better to use attribute id, in case the code
>> raising this error changes for any reason in future.
> 
> I agree.  The parent's "tdhasoid" flag is definitely based on the
> existence of an ObjectIdAttributeNumber system column, not on whether the
> column's name is "oid".  So doing a lookup by name to find the matching
> child column is just weird, and cannot possibly lead to anything good.

You beat me to revising the patch along the lines suggested by Ashutosh.

>> The code updating attinhcount and then updating the catalogs is same
>> for user defined attributes and OID. Should we separate it out into a
>> function and use that function instead of duplicating the code?
> 
> Didn't really seem worth the trouble ... maybe if it gets any longer
> it'd be appropriate to do that.
> 
>> Your test uses tablenames starting with "_". I have not seen that
>> style in the testcases. Is it intentional?
> 
> Yeah, I did not like that either.
> 
> Pushed with those corrections and some further fooling with the test case.

Thanks for reviewing and committing the patch!

Regards,
Amit