Re: [HACKERS] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Apr 20, 2011, at 11:37 PM, Tom Lane 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 , the bits around > 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 -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
Robert Haas writes: > On Wed, Apr 20, 2011 at 10:51 PM, Tom Lane 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 , the bits around 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 -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Wed, Apr 20, 2011 at 10:51 PM, Tom Lane wrote: > Robert Haas 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 -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
Robert Haas 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 -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Wed, Apr 20, 2011 at 6:36 PM, Noah Misch 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 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 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 -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Wed, Apr 20, 2011 at 01:10:19PM -0400, Robert Haas wrote: > On Wed, Apr 20, 2011 at 9:57 AM, Noah Misch 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 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 diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index c194862..4e02438 100644 *** a/doc/src/sgml/ref/alter_table.sgml --- b/doc/src/sgml/ref/alter_table.sgml *** *** 63,68 ALTER TABLE name --- 63,70 RESET ( storage_parameter [, ... ] ) INHERIT parent_table NO INHERIT parent_table + OF type_name + NOT OF OWNER TO new_owner SET TABLESPACE new_tablespace *** *** 491,496 ALTER TABLE name --- 493,522 + OF type_name + + + This form links the table to a composite type as though CREATE + TABLE OF had formed it. The table's list of column names and types + must precisely match that of the composite type; the presence of + an oid system column is permitted to differ. The table must + not inherit from any other table. These restrictions ensure + that CREATE TABLE OF would permit an equivalent table + definition. + + + + + + NOT OF + + + This form dissociates a typed table from its type. + + + + + OWNER diff --git a/src/backend/commands/tablecindex 1f709a4..bb77c53 100644 *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** *** 81,86 --- 81,87 #include "utils/snapmgr.h" #include "utils/syscache.h" #include "utils/tqual.h" + #include "utils/typcache.h" /* *** *** 357,362 static void ATExecEnableDisableRule(Relation rel, char *rulename, --- 358,366 static void ATPrepAddInherit(Relation child_rel); static void ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode); static void ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode); + static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid); + static void ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode); + static void ATExecDropOf(Relation rel, LOCKMODE lockmode); static void ATExecGenericOptions(Relation rel, List *options); static void copy_relation_data(SMgrRelation rel, SMgrRelation dst, *** *** 2684,2689 AlterTableGetLockLevel(List *cmds) --- 2688,2703 break; /* +* These subcommands affect implicit row type conversion. They +* have affects similar to CREATE/DROP CAST on queries. We +* don't provide for invalidating parse trees as a result of +* such changes. Do avoid concurrent pg_class updates, though. +*/ + case AT_AddOf: + case AT_DropOf: + cmd_lockmode = ShareUpdateExclusiveLock; + + /* * These subcommands affect general strategies for performance * and maintenance, though don't change the semantic results * from normal data reads and writes. Delaying an ALTER TABLE *** *** 2942,2954 ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, case AT_EnableAlwaysRule: case AT_EnableReplicaRule: case AT_DisableRule: - ATSimplePermissions(rel, ATT_TABLE); - /* These commands never recurse */ - /* No command-specific prep needed */ -
Re: [HACKERS] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Wed, Apr 20, 2011 at 9:57 AM, Noah Misch 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 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
(gdb) bt full
#0 0x7fff884caf8a in __semwait_signal ()
No symbol table info available.
#1 0x7fff884cae19 in nanosleep ()
No symbol table info available.
#2 0x7fff88517df0 in sleep ()
No symbol table info available.
#3 0x0001002f9d77 in ExceptionalCondition (conditionName=, errorType=, fileName=, lineNumber=) at assert.c:54
No locals.
#4 0x0001002ed3bf in RelationDestroyRelation (relation=0x100faae20) at
relcache.c:1756
No locals.
#5 0x0001002f1d58 in RelationClearRelation (relation=0x100faae20,
rebuild=) at
relcache.c:1930
newrel = (Relation) 0x0
save_relid = 41137
keep_tupdesc =
#6 0x0001002f22a2 in RelationCacheInvalidateEntry (relationId=41137) at
relcache.c:2027
relation = (Relation) #7 0x0001002e9bf6 in
LocalExecuteInvalidationMessage (msg=0x10114b660) at inval.c:501
i =
#8 0x0001002e93df in ProcessInvalidationMessages (hdr=0x10114ae48,
func=0x1002e9ab0 ) at inval.c:391
_cindex =
_chunk = (InvalidationChunk *) 0x10114b650
#9 0x0001002e9480 in CommandEndInvalidationMessages () at inval.c:1047
No locals.
#10 0x000100124294 in ATRewriteCatalogs [inlined] () at
/Users/rhaas/pgsql/src/backend/commands/tablecmds.c:3022
rel = (Relation) 0x100faae20
lcmd = (ListCell *) 0x101175858
pass = 8
ltab = (ListCell *) 0x10118ea90
lockmode = #11 0x000100124294 in ATController (rel=, cmds=, recurse=, lockmode=4) at tablecmds.c:2756
rel = (Relation) 0x100faae20
lcmd = (ListCell *) 0x101175858
pass = 8
ltab = (ListCell *) 0x10118ea90
lockmode = #12 0x00010023b185 in standard_ProcessUtility
(parsetree=0x101041a40, queryString=0x101040e38 "ALTER TABLE tt0 OF tt_t0;",
params=0x0, isTopLevel=1 '\001', dest=0x101041dc8, completionTag=) at utility.c:781
stmt =
stmts =
l = (ListCell *) 0x10118e850
validnsps = {0x100389abe "toast", 0x0}
#13 0x00010023628b in MemoryContextSwitchTo [inlined] () at
/Users/rhaas/pgsql/src/include/utils/palloc.h:1184
No locals.
#14 0x00010023628b in PortalRunUtility (portal=0x1010f5a38,
utilityStmt=0x101041a40, isTopLevel=, dest=0x101041dc8, completionTag=0x7fff5fbfdd30 "") at
pquery.c:1192
context = (MemoryContext) #15 0x000100237a45 in PortalRunMulti
(portal=0x1010f5a38, isTopLevel=, dest=0x101041dc8, altdest=0x101041dc8,
completionTag=0x7fff5fbfdd30 "") at pquery.c:1315
stmt = (Node *) 0x101041a40
active_snapshot_set = 0 '\0'
stmtlist_item = (ListCell *) 0x101041d68
#16 0x0001002383e8 in PortalRun (portal=0x1010f5a38,
count=9223372036854775807, isTopLevel=, dest=0x101041dc8, altdest=0x101041dc8,
completionTag=0x7fff5fbfdd30 "") at pquery.c:813
save_exception_stack = (sigjmp_buf *) 0x7fff5
Re: [HACKERS] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Tue, Apr 19, 2011 at 10:36:14PM -0400, Robert Haas wrote:
> On Mon, Apr 18, 2011 at 7:50 PM, Noah Misch 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
--
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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Mon, Apr 18, 2011 at 7:50 PM, Noah Misch 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
--
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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
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 diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index c194862..4e02438 100644 *** a/doc/src/sgml/ref/alter_table.sgml --- b/doc/src/sgml/ref/alter_table.sgml *** *** 63,68 ALTER TABLE name --- 63,70 RESET ( storage_parameter [, ... ] ) INHERIT parent_table NO INHERIT parent_table + OF type_name + NOT OF OWNER TO new_owner SET TABLESPACE new_tablespace *** *** 491,496 ALTER TABLE name --- 493,522 + OF type_name + + + This form links the table to a composite type as though CREATE + TABLE OF had formed it. The table's list of column names and types + must precisely match that of the composite type; the presence of + an oid system column is permitted to differ. The table must + not inherit from any other table. These restrictions ensure + that CREATE TABLE OF would permit an equivalent table + definition. + + + + + + NOT OF + + + This form dissociates a typed table from its type. + + + + + OWNER diff --git a/src/backend/commands/tablecindex 1f709a4..f0e6f24 100644 *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** *** 81,86 --- 81,87 #include "utils/snapmgr.h" #include "utils/syscache.h" #include "utils/tqual.h" + #include "utils/typcache.h" /* *** *** 357,362 static void ATExecEnableDisableRule(Relation rel, char *rulename, --- 358,366 static void ATPrepAddInherit(Relation child_rel); static void ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE lockmode); static void ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode); + static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid); + static void ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode); + static void ATExecDropOf(Relation rel, LOCKMODE lockmode); static void ATExecGenericOptions(Relation rel, List *options); static void copy_relation_data(SMgrRelation rel, SMgrRelation dst, *** *** 2684,2689 AlterTableGetLockLevel(List *cmds) --- 2688,2703 break; /* +* These subcommands affect implicit row type conversion. They +* have affects similar to CREATE/DROP CAST on queries. We +* don't provide for invalidating parse trees as a result of +* such changes. Do avoid concurrent pg_class updates, though. +*/ + case AT_AddOf: + case AT_DropOf: + cmd_lockmode = ShareUpdateExclusiveLock; + + /* * These subcommands affect general strategies for performance * and maintenance, though don't change the semantic results * from normal data reads and writes. Delaying an ALTER TABLE *** *** 2942,2954 ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, case AT_EnableAlwaysRule: case AT_EnableReplicaRule: case AT_DisableRule: - ATSimplePermissions(rel, ATT_TABLE); - /* These commands never recurse */ - /* No command-specific prep needed */ - pass = AT_PASS_MISC; - break; case AT_DropInherit:/* NO INHERIT */ ATSimplePermissions(rel, ATT_TABLE); /* No command-specific prep needed */ pass = AT_PASS_MISC; break; --- 2956,2966 case AT_EnableAlwaysRule: case AT_EnableReplicaRule: case AT_DisableRule: case AT_DropInherit:/* NO INHERIT */ + case AT_AddOf: /* OF */ + case AT_DropOf: /* NOT OF */ ATSimplePermissions(rel, ATT_TABLE
Re: [HACKERS] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
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 -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Wed, Apr 13, 2011 at 11:46:45PM -0400, Tom Lane wrote: > Robert Haas 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. -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
Robert Haas 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 -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Fri, Apr 8, 2011 at 5:12 PM, Noah Misch 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 -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Wed, Mar 30, 2011 at 09:37:56PM -0400, Robert Haas wrote:
> On Wed, Mar 30, 2011 at 9:30 PM, Noah Misch 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', 100));
If I "drop table a_star cascade" in the regression database before attempting
pg_upgrade, it completes cleanly.
nm
diff --git a/doc/src/sgml/ref/alter_table.sgml
b/doc/src/sgml/ref/alter_table.sgml
index c194862..4e02438 100644
*** a/doc/src/sgml/ref/alter_table.sgml
--- b/doc/src/sgml/ref/alter_table.sgml
***
*** 63,68 ALTER TABLE name
--- 63,70
RESET ( storage_parameter [,
... ] )
INHERIT parent_table
NO INHERIT parent_table
+ OF type_name
+ NOT OF
OWNER TO new_owner
SET TABLESPACE new_tablespace
***
*** 491,496 ALTER TABLE name
--- 493,522
+ OF type_name
+
+
+ This form links the table to a composite type as though CREATE
+ TABLE OF had formed it. The table's list of column names and types
+ must precisely match that of the composite type; the presence of
+ an oid system column is permitted to differ. The table must
+ not inherit from any other table. These restrictions ensure
+ that CREATE TABLE OF would permit an equivalent table
+ definition.
+
+
+
+
+
+ NOT OF
+
+
+ This form dissociates a typed table from its type.
+
+
+
+
+
OWNER
diff --git a/src/backend/commands/tablecindex bd18db3..0d657a3 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 81,86
--- 81,87
#include "utils/snapmgr.h"
#include "utils/syscache.h"
#include "utils/tqual.h"
+ #include "utils/typcache.h"
/*
***
*** 357,362 static void ATExecEnableDisableRule(Relation rel, char
*rulename,
--- 358,366
static void ATPrepAddInherit(Relation child_rel);
static void ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCKMODE
lockmode);
static void ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE
lockmode);
+ static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid);
+ static void ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE
lockmode);
+ static void ATExecDropOf(Relation rel, LOCKMODE lockmode);
static void ATExecGenericOptions(Relation rel, List *options);
static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
***
*** 2679,2684 AlterTableGetLockLevel(List *cmds)
--- 2683,2698
break;
/*
+* These subcommands affect implicit row type
conversion. They have
+* affects similar to CREATE/DROP CAST on queries. We
Re: [HACKERS] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Wed, Mar 30, 2011 at 9:30 PM, Noah Misch 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 -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Wed, Mar 30, 2011 at 10:14:03AM -0400, Robert Haas wrote: > On Tue, Mar 29, 2011 at 5:50 PM, Noah Misch 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: > 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. -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Wed, Mar 30, 2011 at 12:55 PM, Peter Eisentraut 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 -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
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(). -- 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] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
On Tue, Mar 29, 2011 at 5:50 PM, Noah Misch 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_dump --binary-upgrade vs. ALTER TYPE ... DROP ATTRIBUTE
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
