Re: [HACKERS] ALTER TABLE parent SET WITHOUT OIDS and the oid column

2017-01-04 Thread Amit Langote 
On 2017/01/05 8:05, Tom Lane wrote:
> Ashutosh Bapat  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




-- 
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] ALTER TABLE parent SET WITHOUT OIDS and the oid column

2017-01-04 Thread Ashutosh Bapat 
>
>> 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


-- 
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] ALTER TABLE parent SET WITHOUT OIDS and the oid column

2017-01-04 Thread Tom Lane 
Ashutosh Bapat  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


-- 
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] ALTER TABLE parent SET WITHOUT OIDS and the oid column

2016-12-27 Thread Ashutosh Bapat 
>>
>> 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


-- 
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] ALTER TABLE parent SET WITHOUT OIDS and the oid column

2016-12-27 Thread Amit Langote 
On 2016/12/27 22:24, Ashutosh Bapat wrote:
> On Mon, Dec 26, 2016 at 3:36 PM, Amit Langote
>  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




-- 
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] ALTER TABLE parent SET WITHOUT OIDS and the oid column

2016-12-27 Thread Ashutosh Bapat 
On Mon, Dec 26, 2016 at 3:36 PM, Amit Langote
 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


-- 
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] ALTER TABLE parent SET WITHOUT OIDS and the oid column

2016-12-26 Thread Amit Langote 
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




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers