Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Fix pg_dump to handle collations applied to columns of composite types.
- acfa1f45ed3e 9.1.0 cited

pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Noah Misch <noah@leadboat.com> — 2011-03-29T21:50:43Z

I took a look at the open item concerning typed tables and pg_upgrade:
http://archives.postgresql.org/pgsql-hackers/2011-03/msg00767.php

"pg_dump --binary-upgrade" emits commands to recreate the dropped-column
situation of the database, and it does not currently understand that it must
alter the parent type when the subject is a typed table.  Actually, pg_dump
doesn't handle dropped columns in composite types at all.  pg_upgrade runs fine
on a database that received these commands, but the outcome is wrong:

create type t as (x int, y int);
create table has_a (tcol t);
insert into has_a values ('(1,2)');
table has_a; -- (1,2)
alter type t drop attribute y cascade, add attribute z int cascade;
table has_a; -- (1,)
table has_a; -- after pg_upgrade: (1,2)

Fixing that looks clear enough, but the right fix for the typed table issue is
less clear to me.  The pg_attribute tuples for a typed table will include any
attributes dropped from the parent type after the table's creation, but not
those attributes dropped before the table's creation.  Example:

create type t as (x int, y int);
create table is_a of t;
alter type t drop attribute y cascade;
create table is_a2 of t;
select * from pg_attribute where attrelid = 'is_a'::regclass;
select * from pg_attribute where attrelid = 'is_a2'::regclass;

To reproduce that catalog state, the dump would need to create the type, create
all typed tables predating the DROP ATTRIBUTE, and finally create typed tables
postdating the DROP ATTRIBUTE.  That implies an extra dump entry for the DROP
ATTRIBUTE with the appropriate dependencies to compel that order of events.  Is
there a better way?

nm

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Robert Haas <robertmhaas@gmail.com> — 2011-03-30T14:14:03Z

On Tue, Mar 29, 2011 at 5:50 PM, Noah Misch <noah@leadboat.com> wrote:
> I took a look at the open item concerning typed tables and pg_upgrade:
> http://archives.postgresql.org/pgsql-hackers/2011-03/msg00767.php

Thanks!

> [ helpful summary of problem clipped ]

> To reproduce that catalog state, the dump would need to create the type, create
> all typed tables predating the DROP ATTRIBUTE, and finally create typed tables
> postdating the DROP ATTRIBUTE.  That implies an extra dump entry for the DROP
> ATTRIBUTE with the appropriate dependencies to compel that order of events.  Is
> there a better way?

I think so.  We have this same problem with regular table inheritance,
and the way we fix it is to jigger the tuple descriptor for the child
table so that it matches what we need, and THEN attach it to the
parent:

CREATE TABLE child (
    a integer,
    "........pg.dropped.2........" INTEGER /* dummy */
);

-- For binary upgrade, recreate inherited column.
UPDATE pg_catalog.pg_attribute
SET attislocal = false
WHERE attname = 'a'
  AND attrelid = 'child'::pg_catalog.regclass;

-- For binary upgrade, recreate dropped column.
UPDATE pg_catalog.pg_attribute
SET attlen = 4, attalign = 'i', attbyval = false
WHERE attname = '........pg.dropped.2........'
  AND attrelid = 'child'::pg_catalog.regclass;
ALTER TABLE ONLY child DROP COLUMN "........pg.dropped.2........";

-- For binary upgrade, set up inheritance this way.
ALTER TABLE ONLY child INHERIT parent;

I think we need to do something similar here -- use the same hack
shown above to get the dropped column into the right state, and then
jigger things so that the child is a typed table associated with the
parent.  Perhaps it would be reasonable to extend ALTER TABLE .. [NO]
INHERIT to accept a type name as the final argument.  If used in this
way, it converts a typed table into a regular table or visca versa.
We could also do it with a direct catalog change, but there are some
dependencies that would need to be frobbed, which makes me a bit
reluctant to go that way.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Peter Eisentraut <peter_e@gmx.net> — 2011-03-30T16:55:43Z

On tis, 2011-03-29 at 17:50 -0400, Noah Misch wrote:
> Fixing that looks clear enough, but the right fix for the typed table
> issue is less clear to me.  The pg_attribute tuples for a typed table
> will include any attributes dropped from the parent type after the
> table's creation, but not those attributes dropped before the table's
> creation.  Example:
> 
> create type t as (x int, y int);
> create table is_a of t;
> alter type t drop attribute y cascade;
> create table is_a2 of t;
> select * from pg_attribute where attrelid = 'is_a'::regclass;
> select * from pg_attribute where attrelid = 'is_a2'::regclass;
> 
> To reproduce that catalog state, the dump would need to create the
> type, create all typed tables predating the DROP ATTRIBUTE, and
> finally create typed tables postdating the DROP ATTRIBUTE.  That
> implies an extra dump entry for the DROP ATTRIBUTE with the
> appropriate dependencies to compel that order of events.  Is
> there a better way? 

Maybe we could just copy the dropped attributes from the type when the
table is created.  That might be as simple as removing the

        if (attr->attisdropped)
            continue;

in transformOfType().

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Robert Haas <robertmhaas@gmail.com> — 2011-03-30T19:32:26Z

On Wed, Mar 30, 2011 at 12:55 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> Maybe we could just copy the dropped attributes from the type when the
> table is created.  That might be as simple as removing the
>
>        if (attr->attisdropped)
>            continue;
>
> in transformOfType().

Seems a bit fragile...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Noah Misch <noah@leadboat.com> — 2011-03-31T01:30:52Z

On Wed, Mar 30, 2011 at 10:14:03AM -0400, Robert Haas wrote:
> On Tue, Mar 29, 2011 at 5:50 PM, Noah Misch <noah@leadboat.com> wrote:
> > To reproduce that catalog state, the dump would need to create the type, create
> > all typed tables predating the DROP ATTRIBUTE, and finally create typed tables
> > postdating the DROP ATTRIBUTE. ?That implies an extra dump entry for the DROP
> > ATTRIBUTE with the appropriate dependencies to compel that order of events. ?Is
> > there a better way?
> 
> I think so.  We have this same problem with regular table inheritance,
> and the way we fix it is to jigger the tuple descriptor for the child
> table so that it matches what we need, and THEN attach it to the
> parent:
<snipped example>
> I think we need to do something similar here -- use the same hack
> shown above to get the dropped column into the right state, and then
> jigger things so that the child is a typed table associated with the
> parent.

Good idea; I agree.

> Perhaps it would be reasonable to extend ALTER TABLE .. [NO]
> INHERIT to accept a type name as the final argument.  If used in this
> way, it converts a typed table into a regular table or visca versa.

Why extend ALTER TABLE ... INHERIT?  I would have guessed independent syntax.

> We could also do it with a direct catalog change, but there are some
> dependencies that would need to be frobbed, which makes me a bit
> reluctant to go that way.

Agreed; it's also an independently-useful capability to have.

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Robert Haas <robertmhaas@gmail.com> — 2011-03-31T01:37:56Z

On Wed, Mar 30, 2011 at 9:30 PM, Noah Misch <noah@leadboat.com> wrote:
>> Perhaps it would be reasonable to extend ALTER TABLE .. [NO]
>> INHERIT to accept a type name as the final argument.  If used in this
>> way, it converts a typed table into a regular table or visca versa.
>
> Why extend ALTER TABLE ... INHERIT?  I would have guessed independent syntax.

I just didn't feel the need to invent something new, but we could if
someone would rather.

>> We could also do it with a direct catalog change, but there are some
>> dependencies that would need to be frobbed, which makes me a bit
>> reluctant to go that way.
>
> Agreed; it's also an independently-useful capability to have.

Yep.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Noah Misch <noah@leadboat.com> — 2011-04-09T00:12:19Z

On Wed, Mar 30, 2011 at 09:37:56PM -0400, Robert Haas wrote:
> On Wed, Mar 30, 2011 at 9:30 PM, Noah Misch <noah@leadboat.com> wrote:
> >> Perhaps it would be reasonable to extend ALTER TABLE .. [NO]
> >> INHERIT to accept a type name as the final argument. ?If used in this
> >> way, it converts a typed table into a regular table or visca versa.
> >
> > Why extend ALTER TABLE ... INHERIT? ?I would have guessed independent syntax.
> 
> I just didn't feel the need to invent something new, but we could if
> someone would rather.
> 
> >> We could also do it with a direct catalog change, but there are some
> >> dependencies that would need to be frobbed, which makes me a bit
> >> reluctant to go that way.
> >
> > Agreed; it's also an independently-useful capability to have.
> 
> Yep.

Implemented as attached.  The first patch just adds the ALTER TABLE subcommands
to attach and detach a table from a composite type.  A few open questions
concerning typed tables will probably yield minor changes to these subcommands.
I implemented them to be agnostic toward the outcome of those decisions.

The second patch updates pg_dump to use those new subcommands.  It's based
significantly on Peter's recent patch.  The new bits follow pg_dump's design for
table inheritance.

I tested pg_upgrade of these previously-mentioned test cases:

  create type t as (x int, y int);
  create table has_a (tcol t);
  insert into has_a values ('(1,2)');
  table has_a; -- (1,2)
  alter type t drop attribute y cascade, add attribute z int cascade;
  table has_a; -- (1,)
  table has_a; -- after pg_upgrade: (1,2)

  create type t as (x int, y int);
  create table is_a of t;
  alter type t drop attribute y cascade;
  create table is_a2 of t;
  select * from pg_attribute where attrelid = 'is_a'::regclass;
  select * from pg_attribute where attrelid = 'is_a2'::regclass;

  create type unused as (x int);
  alter type unused drop attribute x;

I also tested a regular dump+reload of the regression database, and a pg_upgrade
of the same.  The latter failed further along, due (indirectly) to this failure
to create a TOAST table:

  create table p ();
  create table ch () inherits (p);
  alter table p add column a text;
  select oid::regclass,reltoastrelid from pg_class where oid::regclass IN ('p','ch');
  insert into ch values (repeat('x', 1000000));

If I "drop table a_star cascade" in the regression database before attempting
pg_upgrade, it completes cleanly.

nm

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Robert Haas <robertmhaas@gmail.com> — 2011-04-14T03:01:17Z

On Fri, Apr 8, 2011 at 5:12 PM, Noah Misch <noah@leadboat.com> wrote:
> Implemented as attached.  The first patch just adds the ALTER TABLE subcommands
> to attach and detach a table from a composite type.  A few open questions
> concerning typed tables will probably yield minor changes to these subcommands.
> I implemented them to be agnostic toward the outcome of those decisions.

I suppose one issue is whether anyone would care to bikeshed on the
proposed syntax.  Any takers?

I think you only need an AccessShareLock on InheritsRelationId, since
you are only selecting from it.

If we adopt the elsewhere-proposed approach of forbidding the use of
rowtypes to create typed tables, the circularity-checking logic here
can become simpler.  I think it's not actually water-tight right now:

rhaas=# create table a (x int);
CREATE TABLE
rhaas=# create table b of a;
CREATE TABLE
rhaas=# create table c () inherits (b);
CREATE TABLE
rhaas=# create table d of c;
CREATE TABLE
rhaas=# alter table a of d;
ALTER TABLE

pg_dump is not happy with this situation.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Tom Lane <tgl@sss.pgh.pa.us> — 2011-04-14T03:46:45Z

Robert Haas <robertmhaas@gmail.com> writes:
> If we adopt the elsewhere-proposed approach of forbidding the use of
> rowtypes to create typed tables, the circularity-checking logic here
> can become simpler.  I think it's not actually water-tight right now:

> rhaas=# create table a (x int);
> CREATE TABLE
> rhaas=# create table b of a;
> CREATE TABLE
> rhaas=# create table c () inherits (b);
> CREATE TABLE
> rhaas=# create table d of c;
> CREATE TABLE
> rhaas=# alter table a of d;
> ALTER TABLE

"alter table a of d"?  What the heck does that mean, and why would it be
a good idea?

			regards, tom lane

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Noah Misch <noah@leadboat.com> — 2011-04-14T12:36:19Z

On Wed, Apr 13, 2011 at 11:46:45PM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > If we adopt the elsewhere-proposed approach of forbidding the use of
> > rowtypes to create typed tables, the circularity-checking logic here
> > can become simpler.  I think it's not actually water-tight right now:
> 
> > rhaas=# create table a (x int);
> > CREATE TABLE
> > rhaas=# create table b of a;
> > CREATE TABLE
> > rhaas=# create table c () inherits (b);
> > CREATE TABLE
> > rhaas=# create table d of c;
> > CREATE TABLE
> > rhaas=# alter table a of d;
> > ALTER TABLE
> 
> "alter table a of d"?  What the heck does that mean, and why would it be
> a good idea?

CREATE TABLE a ...; ...; ALTER TABLE a OF d;  =  CREATE TABLE a OF d;

It's a good idea as a heavy lifter for `pg_dump --binary-upgrade'.  See the rest
of this thread for the full background.

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Noah Misch <noah@leadboat.com> — 2011-04-15T15:58:30Z

Robert,

Thanks for the review.

On Wed, Apr 13, 2011 at 08:01:17PM -0700, Robert Haas wrote:
> I think you only need an AccessShareLock on InheritsRelationId, since
> you are only selecting from it.

True; fixed.

> If we adopt the elsewhere-proposed approach of forbidding the use of
> rowtypes to create typed tables, the circularity-checking logic here
> can become simpler.  I think it's not actually water-tight right now:
> 
> rhaas=# create table a (x int);
> CREATE TABLE
> rhaas=# create table b of a;
> CREATE TABLE
> rhaas=# create table c () inherits (b);
> CREATE TABLE
> rhaas=# create table d of c;
> CREATE TABLE
> rhaas=# alter table a of d;
> ALTER TABLE
> 
> pg_dump is not happy with this situation.

Good test case.

Since we're going to forbid hanging a typed table off a table rowtype, I believe
the circularity check becomes entirely superfluous.  I'm suspicious that I'm
missing some way to introduce problematic circularity using composite-typed
columns, but I couldn't come up with a problematic example.  The current check
would not detect such a problem, if one does exist, anyway.

When we're done with the relkind-restriction patch, I'll post a new version of
this one.  It will remove the circularity check and add a relkind check.

nm

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Noah Misch <noah@leadboat.com> — 2011-04-18T23:50:41Z

On Fri, Apr 15, 2011 at 11:58:30AM -0400, Noah Misch wrote:
> When we're done with the relkind-restriction patch, I'll post a new version of
> this one.  It will remove the circularity check and add a relkind check.

Here it is.  Changes from tt1v1-alter-of.patch to tt1v2-alter-of.patch:
* Use transformOfType()'s relkind check in ATExecAddOf()
* Remove circularity check
* Open pg_inherits with AccessShareLock
* Fix terminology in ATExecDropOf() comment
* Rebase over pgindent changes

Changes from tt2v1-binary-upgrade.patch to tt2v2-binary-upgrade.patch:
* Rebase over dumpCompositeType() changes from commit acfa1f45

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Robert Haas <robertmhaas@gmail.com> — 2011-04-20T02:36:14Z

On Mon, Apr 18, 2011 at 7:50 PM, Noah Misch <noah@leadboat.com> wrote:
> On Fri, Apr 15, 2011 at 11:58:30AM -0400, Noah Misch wrote:
>> When we're done with the relkind-restriction patch, I'll post a new version of
>> this one.  It will remove the circularity check and add a relkind check.
>
> Here it is.  Changes from tt1v1-alter-of.patch to tt1v2-alter-of.patch:
> * Use transformOfType()'s relkind check in ATExecAddOf()
> * Remove circularity check
> * Open pg_inherits with AccessShareLock
> * Fix terminology in ATExecDropOf() comment
> * Rebase over pgindent changes
>
> Changes from tt2v1-binary-upgrade.patch to tt2v2-binary-upgrade.patch:
> * Rebase over dumpCompositeType() changes from commit acfa1f45

I think there's a bug in the tt1v1 patch.  I'm getting intermittent
regression test failures at this point:

ALTER TABLE tt7 OF tt_t1;                       -- reassign an
already-typed table
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
connection to server was lost

In src/test/regress/log/postmaster.log:

TRAP: FailedAssertion("!(((bool)((relation)->rd_refcnt == 0)))", File:
"relcache.c", Line: 1756)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Noah Misch <noah@leadboat.com> — 2011-04-20T13:57:21Z

On Tue, Apr 19, 2011 at 10:36:14PM -0400, Robert Haas wrote:
> On Mon, Apr 18, 2011 at 7:50 PM, Noah Misch <noah@leadboat.com> wrote:
> > On Fri, Apr 15, 2011 at 11:58:30AM -0400, Noah Misch wrote:
> >> When we're done with the relkind-restriction patch, I'll post a new version of
> >> this one. ?It will remove the circularity check and add a relkind check.
> >
> > Here it is. ?Changes from tt1v1-alter-of.patch to tt1v2-alter-of.patch:
> > * Use transformOfType()'s relkind check in ATExecAddOf()
> > * Remove circularity check
> > * Open pg_inherits with AccessShareLock
> > * Fix terminology in ATExecDropOf() comment
> > * Rebase over pgindent changes
> >
> > Changes from tt2v1-binary-upgrade.patch to tt2v2-binary-upgrade.patch:
> > * Rebase over dumpCompositeType() changes from commit acfa1f45
> 
> I think there's a bug in the tt1v1 patch.  I'm getting intermittent
> regression test failures at this point:
> 
> ALTER TABLE tt7 OF tt_t1;                       -- reassign an
> already-typed table
> server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
> connection to server was lost
> 
> In src/test/regress/log/postmaster.log:
> 
> TRAP: FailedAssertion("!(((bool)((relation)->rd_refcnt == 0)))", File:
> "relcache.c", Line: 1756)

How frequently does it happen for you?  I ran `make check' about fifteen times
without hitting an error.  I ran the new test cases under valgrind, and that
also came out clean.

Could you try a fresh build and see if it still happens?  If it does, could you
grab a "bt full" and "p *relation->rd_rel" in gdb?

Thanks,
nm

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Robert Haas <robertmhaas@gmail.com> — 2011-04-20T17:10:19Z

On Wed, Apr 20, 2011 at 9:57 AM, Noah Misch <noah@leadboat.com> wrote:
> On Tue, Apr 19, 2011 at 10:36:14PM -0400, Robert Haas wrote:
>> On Mon, Apr 18, 2011 at 7:50 PM, Noah Misch <noah@leadboat.com> wrote:
>> > On Fri, Apr 15, 2011 at 11:58:30AM -0400, Noah Misch wrote:
>> >> When we're done with the relkind-restriction patch, I'll post a new version of
>> >> this one. ?It will remove the circularity check and add a relkind check.
>> >
>> > Here it is. ?Changes from tt1v1-alter-of.patch to tt1v2-alter-of.patch:
>> > * Use transformOfType()'s relkind check in ATExecAddOf()
>> > * Remove circularity check
>> > * Open pg_inherits with AccessShareLock
>> > * Fix terminology in ATExecDropOf() comment
>> > * Rebase over pgindent changes
>> >
>> > Changes from tt2v1-binary-upgrade.patch to tt2v2-binary-upgrade.patch:
>> > * Rebase over dumpCompositeType() changes from commit acfa1f45
>>
>> I think there's a bug in the tt1v1 patch.  I'm getting intermittent
>> regression test failures at this point:
>>
>> ALTER TABLE tt7 OF tt_t1;                       -- reassign an
>> already-typed table
>> server closed the connection unexpectedly
>>         This probably means the server terminated abnormally
>>         before or while processing the request.
>> connection to server was lost
>>
>> In src/test/regress/log/postmaster.log:
>>
>> TRAP: FailedAssertion("!(((bool)((relation)->rd_refcnt == 0)))", File:
>> "relcache.c", Line: 1756)
>
> How frequently does it happen for you?  I ran `make check' about fifteen times
> without hitting an error.  I ran the new test cases under valgrind, and that
> also came out clean.
>
> Could you try a fresh build and see if it still happens?  If it does, could you
> grab a "bt full" and "p *relation->rd_rel" in gdb?

I reproduced it this morning after git clean -dfx, updating to the
latest master branch, and re-applying your patch.  The most recent
time I reproduced it, it took 7 tries, but I think the average
frequency of failure is around 25%.

gdb info attached, courtesy of defining SLEEP_ON_ASSERT to 1 in
pg_config_manual.h

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Noah Misch <noah@leadboat.com> — 2011-04-20T22:36:37Z

On Wed, Apr 20, 2011 at 01:10:19PM -0400, Robert Haas wrote:
> On Wed, Apr 20, 2011 at 9:57 AM, Noah Misch <noah@leadboat.com> wrote:
> > On Tue, Apr 19, 2011 at 10:36:14PM -0400, Robert Haas wrote:
> >> On Mon, Apr 18, 2011 at 7:50 PM, Noah Misch <noah@leadboat.com> wrote:
> >> > On Fri, Apr 15, 2011 at 11:58:30AM -0400, Noah Misch wrote:
> >> >> When we're done with the relkind-restriction patch, I'll post a new version of
> >> >> this one. ?It will remove the circularity check and add a relkind check.
> >> >
> >> > Here it is. ?Changes from tt1v1-alter-of.patch to tt1v2-alter-of.patch:
> >> > * Use transformOfType()'s relkind check in ATExecAddOf()
> >> > * Remove circularity check
> >> > * Open pg_inherits with AccessShareLock
> >> > * Fix terminology in ATExecDropOf() comment
> >> > * Rebase over pgindent changes
> >> >
> >> > Changes from tt2v1-binary-upgrade.patch to tt2v2-binary-upgrade.patch:
> >> > * Rebase over dumpCompositeType() changes from commit acfa1f45
> >>
> >> I think there's a bug in the tt1v1 patch. ?I'm getting intermittent
> >> regression test failures at this point:

It neglected to call CatalogUpdateIndexes(); attached new version fixes that.
The failure was intermittent due to HOT; stubbing out HeapSatisfiesHOTUpdate()
with "return false" made the failure appear every time.

Thanks for the failure data.

nm

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Robert Haas <robertmhaas@gmail.com> — 2011-04-21T02:02:50Z

On Wed, Apr 20, 2011 at 6:36 PM, Noah Misch <noah@leadboat.com> wrote:
> On Wed, Apr 20, 2011 at 01:10:19PM -0400, Robert Haas wrote:
>> On Wed, Apr 20, 2011 at 9:57 AM, Noah Misch <noah@leadboat.com> wrote:
>> > On Tue, Apr 19, 2011 at 10:36:14PM -0400, Robert Haas wrote:
>> >> On Mon, Apr 18, 2011 at 7:50 PM, Noah Misch <noah@leadboat.com> wrote:
>> >> > On Fri, Apr 15, 2011 at 11:58:30AM -0400, Noah Misch wrote:
>> >> >> When we're done with the relkind-restriction patch, I'll post a new version of
>> >> >> this one. ?It will remove the circularity check and add a relkind check.
>> >> >
>> >> > Here it is. ?Changes from tt1v1-alter-of.patch to tt1v2-alter-of.patch:
>> >> > * Use transformOfType()'s relkind check in ATExecAddOf()
>> >> > * Remove circularity check
>> >> > * Open pg_inherits with AccessShareLock
>> >> > * Fix terminology in ATExecDropOf() comment
>> >> > * Rebase over pgindent changes
>> >> >
>> >> > Changes from tt2v1-binary-upgrade.patch to tt2v2-binary-upgrade.patch:
>> >> > * Rebase over dumpCompositeType() changes from commit acfa1f45
>> >>
>> >> I think there's a bug in the tt1v1 patch. ?I'm getting intermittent
>> >> regression test failures at this point:
>
> It neglected to call CatalogUpdateIndexes(); attached new version fixes that.
> The failure was intermittent due to HOT; stubbing out HeapSatisfiesHOTUpdate()
> with "return false" made the failure appear every time.
>
> Thanks for the failure data.

Thanks for the patch!

I've now committed this part; the actual fix for pg_dump is still
outstanding.  I am not too in love with the syntax you've chosen here,
but since I don't have a better idea I'll wait and see if anyone else
wants to bikeshed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Tom Lane <tgl@sss.pgh.pa.us> — 2011-04-21T02:51:01Z

Robert Haas <robertmhaas@gmail.com> writes:
> I've now committed this part; the actual fix for pg_dump is still
> outstanding.  I am not too in love with the syntax you've chosen here,
> but since I don't have a better idea I'll wait and see if anyone else
> wants to bikeshed.

How about "ALTER TABLE tabname [NOT] OF TYPE typename"?  It's at least a
smidgeon less ambiguous.

			regards, tom lane

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Robert Haas <robertmhaas@gmail.com> — 2011-04-21T03:05:25Z

On Wed, Apr 20, 2011 at 10:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I've now committed this part; the actual fix for pg_dump is still
>> outstanding.  I am not too in love with the syntax you've chosen here,
>> but since I don't have a better idea I'll wait and see if anyone else
>> wants to bikeshed.
>
> How about "ALTER TABLE tabname [NOT] OF TYPE typename"?  It's at least a
> smidgeon less ambiguous.

I thought of that, but I hate to make CREATE TABLE and ALTER TABLE
almost-but-not-quite symmetrical.  But one might well wonder why we
didn't decide on:

CREATE TABLE n OF TYPE t;

...rather than the actual syntax:

CREATE TABLE n OF t;

...which has brevity to recommend it, but likewise isn't terribly clear.

I presume someone will now refer to a standard of some kind....

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Tom Lane <tgl@sss.pgh.pa.us> — 2011-04-21T03:37:04Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 20, 2011 at 10:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> How about "ALTER TABLE tabname [NOT] OF TYPE typename"? It's at least a
>> smidgeon less ambiguous.

> I thought of that, but I hate to make CREATE TABLE and ALTER TABLE
> almost-but-not-quite symmetrical.

Oh, good point.

> But one might well wonder why we didn't decide on:
> CREATE TABLE n OF TYPE t;
> ...rather than the actual syntax:
> CREATE TABLE n OF t;
> ...which has brevity to recommend it, but likewise isn't terribly clear.

> I presume someone will now refer to a standard of some kind....

SQL:2008 11.3 <table definition>, the bits around <typed table clause>
to be specific.

The SQL committee's taste in syntax is, uh, not mine.  They are
amazingly long-winded in places and then they go and do something
like this ...

			regards, tom lane

Re: pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE

Robert Haas <robertmhaas@gmail.com> — 2011-04-21T05:31:12Z

On Apr 20, 2011, at 11:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
>> But one might well wonder why we didn't decide on:
>> CREATE TABLE n OF TYPE t;
>> ...rather than the actual syntax:
>> CREATE TABLE n OF t;
>> ...which has brevity to recommend it, but likewise isn't terribly clear.
> 
>> I presume someone will now refer to a standard of some kind....
> 
> SQL:2008 11.3 <table definition>, the bits around <typed table clause>
> to be specific.

Right on schedule...

> The SQL committee's taste in syntax is, uh, not mine.  They are
> amazingly long-winded in places and then they go and do something
> like this ...

Not to mention that it won't do to use existing syntax (like function call notation) when you could invent bespoke syntax, ideally involving new keywords.

...Robert