Thread: WIP: fix SET WITHOUT OIDS, add SET WITH OIDS
We have an open problem with CVS HEAD that ALTER TABLE SET WITHOUT OIDS causes problems: http://archives.postgresql.org/pgsql-hackers/2008-11/msg00332.php I opined at the time that what we really have here is a table whose tuples do not match its declared rowtype, and that the proper fix is to make SET WITHOUT OIDS rewrite the table to physically get rid of the OIDs. The attached patch (which lacks doc changes or regression tests as yet) does that, and also adds the inverse SET WITH OIDS operation to do what you'd expect, ie, add an OID column if it isn't there already. The major objection to this would probably be that SET WITHOUT OIDS has historically been a "free" catalog-change operation, and now it will be expensive on large tables. But given that we've deprecated OIDs in user tables since 8.0, I think most people have been through that conversion already, or have decided to keep their OIDs anyway. I don't think it's worth taking a continuing risk of backend bugs in order to make life a bit easier for any remaining laggards. A different discussion is whether it's appropriate to put in SET WITH OIDS now, when we're well past feature freeze. If we stripped that out of this patch it'd save a few dozen lines of code, but I'm not really seeing the point. The asymmetry of having SET WITHOUT and not SET WITH has always been an implementation artifact anyway. Comments? regards, tom lane Index: src/backend/commands/tablecmds.c =================================================================== RCS file: /cvsroot/pgsql/src/backend/commands/tablecmds.c,v retrieving revision 1.279 diff -c -r1.279 tablecmds.c *** src/backend/commands/tablecmds.c 2 Feb 2009 19:31:38 -0000 1.279 --- src/backend/commands/tablecmds.c 8 Feb 2009 15:52:00 -0000 *************** *** 138,143 **** --- 138,144 ---- List *constraints; /* List of NewConstraint */ List *newvals; /* List of NewColumnValue */ bool new_notnull; /* T if we added new NOT NULL constraints */ + bool new_changeoids; /* T if we added/dropped the OID column */ Oid newTableSpace; /* new tablespace; 0 means no change */ /* Objects to rebuild after completing ALTER TYPE operations */ List *changedConstraintOids; /* OIDs of constraints to rebuild */ *************** *** 269,276 **** static void ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, AlterTableCmd *cmd); static void ATExecAddColumn(AlteredTableInfo *tab, Relation rel, ! ColumnDef *colDef); static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid); static void ATExecDropNotNull(Relation rel, const char *colName); static void ATExecSetNotNull(AlteredTableInfo *tab, Relation rel, const char *colName); --- 270,279 ---- static void ATPrepAddColumn(List **wqueue, Relation rel, bool recurse, AlterTableCmd *cmd); static void ATExecAddColumn(AlteredTableInfo *tab, Relation rel, ! ColumnDef *colDef, bool isOid); static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid); + static void ATPrepAddOids(List **wqueue, Relation rel, bool recurse, + AlterTableCmd *cmd); static void ATExecDropNotNull(Relation rel, const char *colName); static void ATExecSetNotNull(AlteredTableInfo *tab, Relation rel, const char *colName); *************** *** 282,288 **** Node *newValue); static void ATExecSetStorage(Relation rel, const char *colName, Node *newValue); ! static void ATExecDropColumn(Relation rel, const char *colName, DropBehavior behavior, bool recurse, bool recursing); static void ATExecAddIndex(AlteredTableInfo *tab, Relation rel, --- 285,291 ---- Node *newValue); static void ATExecSetStorage(Relation rel, const char *colName, Node *newValue); ! static void ATExecDropColumn(List **wqueue, Relation rel, const char *colName, DropBehavior behavior, bool recurse, bool recursing); static void ATExecAddIndex(AlteredTableInfo *tab, Relation rel, *************** *** 2452,2457 **** --- 2455,2467 ---- /* No command-specific prep needed */ pass = AT_PASS_MISC; break; + case AT_AddOids: /* SET WITH OIDS */ + ATSimplePermissions(rel, false); + /* Performs own recursion */ + if (!rel->rd_rel->relhasoids || recursing) + ATPrepAddOids(wqueue, rel, recurse, cmd); + pass = AT_PASS_ADD_COL; + break; case AT_DropOids: /* SET WITHOUT OIDS */ ATSimplePermissions(rel, false); /* Performs own recursion */ *************** *** 2589,2595 **** { case AT_AddColumn: /* ADD COLUMN */ case AT_AddColumnToView: /* add column via CREATE OR REPLACE VIEW */ ! ATExecAddColumn(tab, rel, (ColumnDef *) cmd->def); break; case AT_ColumnDefault: /* ALTER COLUMN DEFAULT */ ATExecColumnDefault(rel, cmd->name, cmd->def); --- 2599,2605 ---- { case AT_AddColumn: /* ADD COLUMN */ case AT_AddColumnToView: /* add column via CREATE OR REPLACE VIEW */ ! ATExecAddColumn(tab, rel, (ColumnDef *) cmd->def, false); break; case AT_ColumnDefault: /* ALTER COLUMN DEFAULT */ ATExecColumnDefault(rel, cmd->name, cmd->def); *************** *** 2607,2616 **** ATExecSetStorage(rel, cmd->name, cmd->def); break; case AT_DropColumn: /* DROP COLUMN */ ! ATExecDropColumn(rel, cmd->name, cmd->behavior, false, false); break; case AT_DropColumnRecurse: /* DROP COLUMN with recursion */ ! ATExecDropColumn(rel, cmd->name, cmd->behavior, true, false); break; case AT_AddIndex: /* ADD INDEX */ ATExecAddIndex(tab, rel, (IndexStmt *) cmd->def, false); --- 2617,2628 ---- ATExecSetStorage(rel, cmd->name, cmd->def); break; case AT_DropColumn: /* DROP COLUMN */ ! ATExecDropColumn(wqueue, rel, cmd->name, ! cmd->behavior, false, false); break; case AT_DropColumnRecurse: /* DROP COLUMN with recursion */ ! ATExecDropColumn(wqueue, rel, cmd->name, ! cmd->behavior, true, false); break; case AT_AddIndex: /* ADD INDEX */ ATExecAddIndex(tab, rel, (IndexStmt *) cmd->def, false); *************** *** 2644,2649 **** --- 2656,2666 ---- case AT_DropCluster: /* SET WITHOUT CLUSTER */ ATExecDropCluster(rel); break; + case AT_AddOids: /* SET WITH OIDS */ + /* Use the ADD COLUMN code, unless prep decided to do nothing */ + if (cmd->def != NULL) + ATExecAddColumn(tab, rel, (ColumnDef *) cmd->def, true); + break; case AT_DropOids: /* SET WITHOUT OIDS */ /* *************** *** 2748,2756 **** /* * We only need to rewrite the table if at least one column needs to ! * be recomputed. */ ! if (tab->newvals != NIL) { /* Build a temporary relation and copy data */ Oid OIDNewHeap; --- 2765,2773 ---- /* * We only need to rewrite the table if at least one column needs to ! * be recomputed, or we are adding/removing the OID column. */ ! if (tab->newvals != NIL || tab->new_changeoids) { /* Build a temporary relation and copy data */ Oid OIDNewHeap; *************** *** 2976,2983 **** { NewColumnValue *ex = lfirst(l); - needscan = true; - ex->exprstate = ExecPrepareExpr((Expr *) ex->expr, estate); } --- 2993,2998 ---- *************** *** 3000,3006 **** needscan = true; } ! if (needscan) { ExprContext *econtext; Datum *values; --- 3015,3021 ---- needscan = true; } ! if (newrel || needscan) { ExprContext *econtext; Datum *values; *************** *** 3479,3485 **** static void ATExecAddColumn(AlteredTableInfo *tab, Relation rel, ! ColumnDef *colDef) { Oid myrelid = RelationGetRelid(rel); Relation pgclass, --- 3494,3500 ---- static void ATExecAddColumn(AlteredTableInfo *tab, Relation rel, ! ColumnDef *colDef, bool isOid) { Oid myrelid = RelationGetRelid(rel); Relation pgclass, *************** *** 3512,3518 **** Oid ctypeId; int32 ctypmod; ! /* Okay if child matches by type */ ctypeId = typenameTypeId(NULL, colDef->typename, &ctypmod); if (ctypeId != childatt->atttypid || ctypmod != childatt->atttypmod) --- 3527,3533 ---- Oid ctypeId; int32 ctypmod; ! /* Child column must match by type */ ctypeId = typenameTypeId(NULL, colDef->typename, &ctypmod); if (ctypeId != childatt->atttypid || ctypmod != childatt->atttypmod) *************** *** 3521,3526 **** --- 3536,3548 ---- errmsg("child table \"%s\" has different type for column \"%s\"", RelationGetRelationName(rel), colDef->colname))); + /* If it's OID, child column must actually be OID */ + if (isOid && childatt->attnum != ObjectIdAttributeNumber) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("child table \"%s\" has a conflicting \"%s\" column", + RelationGetRelationName(rel), colDef->colname))); + /* Bump the existing child att's inhcount */ childatt->attinhcount++; simple_heap_update(attrdesc, &tuple->t_self, tuple); *************** *** 3560,3571 **** errmsg("column \"%s\" of relation \"%s\" already exists", colDef->colname, RelationGetRelationName(rel)))); ! newattnum = ((Form_pg_class) GETSTRUCT(reltup))->relnatts + 1; ! if (newattnum > MaxHeapAttributeNumber) ! ereport(ERROR, ! (errcode(ERRCODE_TOO_MANY_COLUMNS), ! errmsg("tables can have at most %d columns", ! MaxHeapAttributeNumber))); typeTuple = typenameType(NULL, colDef->typename, &typmod); tform = (Form_pg_type) GETSTRUCT(typeTuple); --- 3582,3599 ---- errmsg("column \"%s\" of relation \"%s\" already exists", colDef->colname, RelationGetRelationName(rel)))); ! /* Determine the new attribute's number */ ! if (isOid) ! newattnum = ObjectIdAttributeNumber; ! else ! { ! newattnum = ((Form_pg_class) GETSTRUCT(reltup))->relnatts + 1; ! if (newattnum > MaxHeapAttributeNumber) ! ereport(ERROR, ! (errcode(ERRCODE_TOO_MANY_COLUMNS), ! errmsg("tables can have at most %d columns", ! MaxHeapAttributeNumber))); ! } typeTuple = typenameType(NULL, colDef->typename, &typmod); tform = (Form_pg_type) GETSTRUCT(typeTuple); *************** *** 3578,3584 **** attribute.attrelid = myrelid; namestrcpy(&(attribute.attname), colDef->colname); attribute.atttypid = typeOid; ! attribute.attstattarget = -1; attribute.attlen = tform->typlen; attribute.attcacheoff = -1; attribute.atttypmod = typmod; --- 3606,3612 ---- attribute.attrelid = myrelid; namestrcpy(&(attribute.attname), colDef->colname); attribute.atttypid = typeOid; ! attribute.attstattarget = (newattnum > 0) ? -1 : 0; attribute.attlen = tform->typlen; attribute.attcacheoff = -1; attribute.atttypmod = typmod; *************** *** 3601,3609 **** heap_close(attrdesc, RowExclusiveLock); /* ! * Update number of attributes in pg_class tuple */ ! ((Form_pg_class) GETSTRUCT(reltup))->relnatts = newattnum; simple_heap_update(pgclass, &reltup->t_self, reltup); --- 3629,3640 ---- heap_close(attrdesc, RowExclusiveLock); /* ! * Update pg_class tuple as appropriate */ ! if (isOid) ! ((Form_pg_class) GETSTRUCT(reltup))->relhasoids = true; ! else ! ((Form_pg_class) GETSTRUCT(reltup))->relnatts = newattnum; simple_heap_update(pgclass, &reltup->t_self, reltup); *************** *** 3665,3671 **** * defaults, not even for domain-typed columns. And in any case we mustn't * invoke Phase 3 on a view, since it has no storage. */ ! if (relkind != RELKIND_VIEW) { defval = (Expr *) build_column_default(rel, attribute.attnum); --- 3696,3702 ---- * defaults, not even for domain-typed columns. And in any case we mustn't * invoke Phase 3 on a view, since it has no storage. */ ! if (relkind != RELKIND_VIEW && attribute.attnum > 0) { defval = (Expr *) build_column_default(rel, attribute.attnum); *************** *** 3702,3712 **** --- 3733,3753 ---- /* * If the new column is NOT NULL, tell Phase 3 it needs to test that. + * (Note we don't do this for an OID column. OID will be marked not + * null, but since it's filled specially, there's no need to test + * anything.) */ tab->new_notnull |= colDef->is_not_null; } /* + * If we are adding an OID column, we have to tell Phase 3 to rewrite + * the table to fix that. + */ + if (isOid) + tab->new_changeoids = true; + + /* * Add needed dependency entries for the new column. */ add_column_datatype_dependency(myrelid, newattnum, attribute.atttypid); *************** *** 3731,3736 **** --- 3772,3801 ---- } /* + * ALTER TABLE SET WITH OIDS + * + * Basically this is an ADD COLUMN for the special OID column. We have + * to cons up a ColumnDef node because the ADD COLUMN code needs one. + */ + static void + ATPrepAddOids(List **wqueue, Relation rel, bool recurse, AlterTableCmd *cmd) + { + /* If we're recursing to a child table, the ColumnDef is already set up */ + if (cmd->def == NULL) + { + ColumnDef *cdef = makeNode(ColumnDef); + + cdef->colname = pstrdup("oid"); + cdef->typename = makeTypeNameFromOid(OIDOID, -1); + cdef->inhcount = 0; + cdef->is_local = true; + cdef->is_not_null = true; + cmd->def = (Node *) cdef; + } + ATPrepAddColumn(wqueue, rel, recurse, cmd); + } + + /* * ALTER TABLE ALTER COLUMN DROP NOT NULL */ static void *************** *** 4088,4099 **** * because we have to decide at runtime whether to recurse or not depending * on whether attinhcount goes to zero or not. (We can't check this in a * static pre-pass because it won't handle multiple inheritance situations ! * correctly.) Since DROP COLUMN doesn't need to create any work queue ! * entries for Phase 3, it's okay to recurse internally in this routine ! * without considering the work queue. */ static void ! ATExecDropColumn(Relation rel, const char *colName, DropBehavior behavior, bool recurse, bool recursing) { --- 4153,4162 ---- * because we have to decide at runtime whether to recurse or not depending * on whether attinhcount goes to zero or not. (We can't check this in a * static pre-pass because it won't handle multiple inheritance situations ! * correctly.) */ static void ! ATExecDropColumn(List **wqueue, Relation rel, const char *colName, DropBehavior behavior, bool recurse, bool recursing) { *************** *** 4178,4184 **** if (childatt->attinhcount == 1 && !childatt->attislocal) { /* Time to delete this child column, too */ ! ATExecDropColumn(childrel, colName, behavior, true, true); } else { --- 4241,4248 ---- if (childatt->attinhcount == 1 && !childatt->attislocal) { /* Time to delete this child column, too */ ! ATExecDropColumn(wqueue, childrel, colName, ! behavior, true, true); } else { *************** *** 4230,4241 **** performDeletion(&object, behavior); /* ! * If we dropped the OID column, must adjust pg_class.relhasoids */ if (attnum == ObjectIdAttributeNumber) { Relation class_rel; Form_pg_class tuple_class; class_rel = heap_open(RelationRelationId, RowExclusiveLock); --- 4294,4307 ---- performDeletion(&object, behavior); /* ! * If we dropped the OID column, must adjust pg_class.relhasoids and ! * tell Phase 3 to physically get rid of the column. */ if (attnum == ObjectIdAttributeNumber) { Relation class_rel; Form_pg_class tuple_class; + AlteredTableInfo *tab; class_rel = heap_open(RelationRelationId, RowExclusiveLock); *************** *** 4254,4259 **** --- 4320,4331 ---- CatalogUpdateIndexes(class_rel, tuple); heap_close(class_rel, RowExclusiveLock); + + /* Find or create work queue entry for this table */ + tab = ATGetQueueEntry(wqueue, rel); + + /* Tell Phase 3 to physically remove the OID column */ + tab->new_changeoids = true; } } Index: src/backend/parser/gram.y =================================================================== RCS file: /cvsroot/pgsql/src/backend/parser/gram.y,v retrieving revision 2.657 diff -c -r2.657 gram.y *** src/backend/parser/gram.y 2 Feb 2009 19:31:39 -0000 2.657 --- src/backend/parser/gram.y 8 Feb 2009 15:52:01 -0000 *************** *** 1636,1641 **** --- 1636,1648 ---- n->behavior = $4; $$ = (Node *)n; } + /* ALTER TABLE <name> SET WITH OIDS */ + | SET WITH OIDS + { + AlterTableCmd *n = makeNode(AlterTableCmd); + n->subtype = AT_AddOids; + $$ = (Node *)n; + } /* ALTER TABLE <name> SET WITHOUT OIDS */ | SET WITHOUT OIDS { Index: src/include/nodes/parsenodes.h =================================================================== RCS file: /cvsroot/pgsql/src/include/nodes/parsenodes.h,v retrieving revision 1.390 diff -c -r1.390 parsenodes.h *** src/include/nodes/parsenodes.h 2 Feb 2009 19:31:40 -0000 1.390 --- src/include/nodes/parsenodes.h 8 Feb 2009 15:52:01 -0000 *************** *** 1127,1132 **** --- 1127,1133 ---- AT_ChangeOwner, /* change owner */ AT_ClusterOn, /* CLUSTER ON */ AT_DropCluster, /* SET WITHOUT CLUSTER */ + AT_AddOids, /* SET WITH OIDS */ AT_DropOids, /* SET WITHOUT OIDS */ AT_SetTableSpace, /* SET TABLESPACE */ AT_SetRelOptions, /* SET (...) -- AM specific parameters */
Tom Lane wrote: > The attached patch (which lacks doc changes or regression > tests as yet) does that, and also adds the inverse SET WITH OIDS > operation to do what you'd expect, ie, add an OID column if it isn't > there already. > > Why would we add an operation to implement a deprecated feature? That seems very strange. (I have no problem with making SET WITHOUT OIDS rewrite the table.) cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes: > Tom Lane wrote: >> The attached patch (which lacks doc changes or regression >> tests as yet) does that, and also adds the inverse SET WITH OIDS >> operation to do what you'd expect, ie, add an OID column if it isn't >> there already. > Why would we add an operation to implement a deprecated feature? Well, it was always intended to be that way: http://archives.postgresql.org/pgsql-patches/2002-12/msg00071.php The originally submitted patch didn't work very well http://archives.postgresql.org/pgsql-patches/2003-01/msg00011.php and what we ended up doing was applying just the SET WITHOUT OIDS half of it, but my feeling always was that that was for lack of round tuits rather than that it was a good place to be. Given the implementation at the time it would've taken a lot of extra code to do SET WITH OIDS, so nobody did get around to it. But subsequent changes in the ALTER code have made it possible to piggyback on ALTER ADD COLUMN easily --- which is what this patch is trying to demonstrate. Now, if you want to argue that we should get rid of SET WITHOUT OIDS altogether, I'm not sure I could dispute it. But if we have the ability to do that ISTM we should offer the reverse too. regards, tom lane
Tom Lane píše v ne 08. 02. 2009 v 11:51 -0500:
> Now, if you want to argue that we should get rid of SET WITHOUT OIDS
> altogether, I'm not sure I could dispute it. But if we have the
> ability to do that ISTM we should offer the reverse too.
By my opinion TABLES with OIDs is obsolete feature. It make sense to
have SET WITHOUT OIDS, because it is useful when people will migrate
form 7.4 to 8.4. But opposite way does not make me sense, because I
think we want to remove OID TABLES in the future. I personally prefer to
say that 8.4 is last version which supports CREATE TABLE ... WITH OIDS.
Zdenek
Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes:
> Tom Lane píše v ne 08. 02. 2009 v 11:51 -0500:
>> Now, if you want to argue that we should get rid of SET WITHOUT OIDS
>> altogether, I'm not sure I could dispute it. But if we have the
>> ability to do that ISTM we should offer the reverse too.
> By my opinion TABLES with OIDs is obsolete feature. It make sense to
> have SET WITHOUT OIDS, because it is useful when people will migrate
> form 7.4 to 8.4. But opposite way does not make me sense, because I
> think we want to remove OID TABLES in the future. I personally prefer to
> say that 8.4 is last version which supports CREATE TABLE ... WITH OIDS.
If we're going to do that we should do it *now*, not later, because
right now is when we have a bug that we could actually save some effort
on. In practice, since we have not ever suggested that we were actually
going to remove the feature, I don't believe that we can do that. Not
in 8.4, and not in 8.5 or any other near-future release either.
The larger point though is that unless we restructure the system to the
point of not using OIDs in system catalogs ... which ain't happening
... the amount of code we could save by removing OIDs for users is
vanishingly small. Probably on the rough order of 100 lines, and about
the same in documentation. (We couldn't, for instances, stop
documenting that OIDs exist.) Doesn't really seem worth breaking
applications for, even deprecated ones.
regards, tom lane
On Sun, Feb 8, 2009 at 11:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> Tom Lane wrote: >>> The attached patch (which lacks doc changes or regression >>> tests as yet) does that, and also adds the inverse SET WITH OIDS >>> operation to do what you'd expect, ie, add an OID column if it isn't >>> there already. > >> Why would we add an operation to implement a deprecated feature? > > Well, it was always intended to be that way: > http://archives.postgresql.org/pgsql-patches/2002-12/msg00071.php > > The originally submitted patch didn't work very well > http://archives.postgresql.org/pgsql-patches/2003-01/msg00011.php > and what we ended up doing was applying just the SET WITHOUT OIDS half > of it, but my feeling always was that that was for lack of round tuits > rather than that it was a good place to be. Given the implementation > at the time it would've taken a lot of extra code to do SET WITH OIDS, > so nobody did get around to it. But subsequent changes in the ALTER > code have made it possible to piggyback on ALTER ADD COLUMN easily --- > which is what this patch is trying to demonstrate. > > Now, if you want to argue that we should get rid of SET WITHOUT OIDS > altogether, I'm not sure I could dispute it. But if we have the ability > to do that ISTM we should offer the reverse too. +1. I really hate it (in any application, not just PostgreSQL) when there's an option to add something but not delete it, delete it but not put it back, etc. Personally, *I* would not have spent the time to implement SET WITH OIDS, but since we now have the patch, I'm 100% in favor of applying it. Most likely, very few people will use it, but it doesn't cost us anything either, so I'm unclear why we would tell Tom to go back and rip that functionality back out of his patch. Sounds like bikeshedding to me. ...Robert
On Sun, Feb 08, 2009 at 11:51:22AM -0500, Tom Lane wrote: > > Now, if you want to argue that we should get rid of SET WITHOUT OIDS > altogether, +1 for removing it altogether. Row OIDs are and ugly wart :P Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
I don't understand what's wrong with the existing setup where DROP OIDS is a free operation. And the space is cleaned up later when the tuple is next written. It seems exactly equivalent to how we handle DROP COLUMN where the natt field of the tuple disagrees with the tuple descriptor and any additional columns are implicitly null. -- Greg On 8 Feb 2009, at 23:12, David Fetter <david@fetter.org> wrote: > On Sun, Feb 08, 2009 at 11:51:22AM -0500, Tom Lane wrote: >> >> Now, if you want to argue that we should get rid of SET WITHOUT OIDS >> altogether, > > +1 for removing it altogether. Row OIDs are and ugly wart :P > > Cheers, > David. > -- > David Fetter <david@fetter.org> http://fetter.org/ > Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter > Skype: davidfetter XMPP: david.fetter@gmail.com > > Remember to vote! > Consider donating to Postgres: http://www.postgresql.org/about/donate > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers
David Fetter wrote: > On Sun, Feb 08, 2009 at 11:51:22AM -0500, Tom Lane wrote: > >> Now, if you want to argue that we should get rid of SET WITHOUT OIDS >> altogether, >> > > +1 for removing it altogether. Row OIDs are and ugly wart :P > > > That might be true but I know of apps that use them. Having the ability to migrate those slowly by using SET WITHOUT OIDS is a Good Thing (tm). cheers andrew
Greg Stark <greg.stark@enterprisedb.com> writes: > I don't understand what's wrong with the existing setup where DROP > OIDS is a free operation. It breaks things, in particular http://archives.postgresql.org/pgsql-hackers/2008-11/msg00332.php We could kluge around that particular problem, but the objection I have to doing so is I'm 100% certain it won't be the last such bug. > It seems exactly equivalent to how we handle DROP COLUMN It is just about exactly *unlike* DROP COLUMN, because in DROP COLUMN we retain a memory that there used to be a column there. A close emulation of DROP COLUMN would involve inventing some representation of "oidisdropped", and going through every one of the multitudinous places that special-case dropped columns in order to see if each one needs a similar special case for dropped OIDs. The bug mentioned above stems directly from not expecting a table to still contain OIDs after SET WITHOUT OIDS, so I don't think this parallel is mistaken. Note that I'm willing to lay a significant side bet that we still have bugs of omission with dropped columns, too. But we'll fix those as we come to them. I don't think it is worth making a similar open-ended commitment of resources just to keep SET WITHOUT OIDS fast. > ... where the > natt field of the tuple disagrees with the tuple descriptor and any > additional columns are implicitly null. No, that's the mechanism that makes ADD COLUMN feasible (and indeed pretty easy). DROP COLUMN is the far newer and uglier mess around attisdropped. regards, tom lane
On 2/9/09, Andrew Dunstan <andrew@dunslane.net> wrote: > David Fetter wrote: > > On Sun, Feb 08, 2009 at 11:51:22AM -0500, Tom Lane wrote: > > > Now, if you want to argue that we should get rid of SET WITHOUT OIDS > > > altogether, > > > > +1 for removing it altogether. Row OIDs are and ugly wart :P > > That might be true but I know of apps that use them. Having the ability to > migrate those slowly by using SET WITHOUT OIDS is a Good Thing (tm). +1 for removal. Also, whether the removal happens or not, I would suggest a setting that makes Postgres accept, but ignore default_with_oids / WITH OIDS settings. The problem is how to migrate apps that definitely do not use oids, in a situation where you have hundred of databases. Scanning all dbs and doing ALTER table would be option, if it would work 100% and would not touch data. Otherwise is cannot be used. Trying to manually manipulate dump files which are filled with "SET default_with_oids" each time database is dumped/reloaded is also not an option. Currently the only sane path seems to patch Postgres to ignore the settings, but that does not seem very user-friendly approach... -- marko
On Mon, Feb 09, 2009 at 02:47:21PM +0200, Marko Kreen wrote: > > That might be true but I know of apps that use them. Having the ability to > > migrate those slowly by using SET WITHOUT OIDS is a Good Thing (tm). > > +1 for removal. > > Also, whether the removal happens or not, I would suggest a setting that > makes Postgres accept, but ignore default_with_oids / WITH OIDS settings. Err, you mean a setting that makes Postgres throw an error on the use of WITH OIDS. Just silently ignoring the option is a fantastic way to break applications silently. Making pg_dump not output the WITH OIDS option on tables may be an easier option. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Please line up in a tree and maintain the heap invariant while > boarding. Thank you for flying nlogn airlines.
Martijn van Oosterhout wrote: > Making pg_dump not output the WITH OIDS option on tables may be an > easier option. Or just run ALTER TABLE WITHOUT OIDS for all the tables before dumping. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On 2/9/09, Martijn van Oosterhout <kleptog@svana.org> wrote: > On Mon, Feb 09, 2009 at 02:47:21PM +0200, Marko Kreen wrote: > > > That might be true but I know of apps that use them. Having the ability to > > > migrate those slowly by using SET WITHOUT OIDS is a Good Thing (tm). > > > > +1 for removal. > > > > Also, whether the removal happens or not, I would suggest a setting that > > makes Postgres accept, but ignore default_with_oids / WITH OIDS settings. > > Err, you mean a setting that makes Postgres throw an error on the use > of WITH OIDS. Just silently ignoring the option is a fantastic way to > break applications silently. For me, ignoring is easier... But yeah, error would be better, if it does not affect reloading the dump. > Making pg_dump not output the WITH OIDS option on tables may be an > easier option. I don't like it - it would require more work from users, but does not seem to be any way safer. You usually do the check if db works on restore time, not dump time... From clarity standpoint, options that turns both default_with_oids and WITH OIDS to errors seems the best. -- marko
On 2/9/09, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > Martijn van Oosterhout wrote: > > Making pg_dump not output the WITH OIDS option on tables may be an > > easier option. > > Or just run ALTER TABLE WITHOUT OIDS for all the tables before dumping. This does not work on dbs that are actually in use... -- marko
On 2/9/09, Martijn van Oosterhout <kleptog@svana.org> wrote: > Making pg_dump not output the WITH OIDS option on tables may be an > easier option. OTOH, the pg_dump already has option --oids. If the option is not given, is there any point putting WITH OIDS / default_with_oids into dump? -- marko
On Mon, Feb 09, 2009 at 03:19:55PM +0200, Marko Kreen wrote: > > Making pg_dump not output the WITH OIDS option on tables may be an > > easier option. > > I don't like it - it would require more work from users, but does > not seem to be any way safer. You usually do the check if db works > on restore time, not dump time... Another idea, have WITH OIDS just append a column to the table called OID with SERIAL type. People see them, go "whoops" and drop them. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Please line up in a tree and maintain the heap invariant while > boarding. Thank you for flying nlogn airlines.
On Feb 9, 2009, at 7:47 AM, Marko Kreen <markokr@gmail.com> wrote: > On 2/9/09, Andrew Dunstan <andrew@dunslane.net> wrote: >> David Fetter wrote: >>> On Sun, Feb 08, 2009 at 11:51:22AM -0500, Tom Lane wrote: >>>> Now, if you want to argue that we should get rid of SET WITHOUT >>>> OIDS >>>> altogether, >>> >>> +1 for removing it altogether. Row OIDs are and ugly wart :P >> >> That might be true but I know of apps that use them. Having the >> ability to >> migrate those slowly by using SET WITHOUT OIDS is a Good Thing (tm). > > +1 for removal. Why? What benefit do we get out of denying users this option? > > Also, whether the removal happens or not, I would suggest a setting > that > makes Postgres accept, but ignore default_with_oids / WITH OIDS > settings. > > The problem is how to migrate apps that definitely do not use oids, > in a situation where you have hundred of databases. > > Scanning all dbs and doing ALTER table would be option, if it would > work 100% and would not touch data. Otherwise is cannot be used. That might be true in your environment, but is certainly not true in general. We have many DDL commands that require full-table rewrites, and they are FAR from useless. > Trying to manually manipulate dump files which are filled with > "SET default_with_oids" each time database is dumped/reloaded is also > not an option. > > Currently the only sane path seems to patch Postgres to ignore the > settings, but that does not seem very ...Robert
On 2/9/09, Robert Haas <robertmhaas@gmail.com> wrote: > On Feb 9, 2009, at 7:47 AM, Marko Kreen <markokr@gmail.com> wrote: > > On 2/9/09, Andrew Dunstan <andrew@dunslane.net> wrote: > > > David Fetter wrote: > > > > On Sun, Feb 08, 2009 at 11:51:22AM -0500, Tom Lane wrote: > > > > > > > > > Now, if you want to argue that we should get rid of SET WITHOUT OIDS > > > > > altogether, > > > > > > > > > > > > > +1 for removing it altogether. Row OIDs are and ugly wart :P > > > > > > > > > > That might be true but I know of apps that use them. Having the ability > to > > > migrate those slowly by using SET WITHOUT OIDS is a Good Thing (tm). > > > > > > > +1 for removal. > > > > Why? What benefit do we get out of denying users this option? Why should we continue to support historical special case? It is not a feature that adds anything to user experience with Postgres. Anyway, that was my vote only. If there are developers interested in supporting oids feel free to do so. But now that I learned that ALTER TABLE WITHOUT OIDS either causes bugs or requires table rewrite, it turned from minor annoyance to big annoyance. So I'd like have a reasonable path for getting rid of them, which we don't have currently. Removing them completely is simplest path, but adding extra features to support it is another. If we are talking about adding a feature, then I like retargeting pg_dump --oids from data-only flag to apply to both data and schema. Yes, this is incompatible change, but the change affects feature we are discouraging anyway. If this does not work, then we need another postgresql.conf option. > > Also, whether the removal happens or not, I would suggest a setting that > > makes Postgres accept, but ignore default_with_oids / WITH OIDS settings. > > > > The problem is how to migrate apps that definitely do not use oids, > > in a situation where you have hundred of databases. > > > > Scanning all dbs and doing ALTER table would be option, if it would > > work 100% and would not touch data. Otherwise is cannot be used. > > > > That might be true in your environment, but is certainly not true in > general. We have many DDL commands that require full-table rewrites, and > they are FAR from useless. Compared to not having the DDL commands or having DDL commands that do not rewrite the tables? ;) -- marko
Marko Kreen wrote: > But now that I learned that ALTER TABLE WITHOUT OIDS either causes bugs > or requires table rewrite, it turned from minor annoyance to big annoyance. > So I'd like have a reasonable path for getting rid of them, which we don't > have currently. Removing them completely is simplest path, but adding > extra features to support it is another. > > If we are talking about adding a feature, then I like retargeting > pg_dump --oids from data-only flag to apply to both data and schema. > Yes, this is incompatible change, but the change affects feature we > are discouraging anyway. > > How about a pg_dump flag that simply suppresses OIDs from the data and schema? cheers andrew
On 2/9/09, Andrew Dunstan <andrew@dunslane.net> wrote: > Marko Kreen wrote: > > But now that I learned that ALTER TABLE WITHOUT OIDS either causes bugs > > or requires table rewrite, it turned from minor annoyance to big > annoyance. > > So I'd like have a reasonable path for getting rid of them, which we don't > > have currently. Removing them completely is simplest path, but adding > > extra features to support it is another. > > > > If we are talking about adding a feature, then I like retargeting > > pg_dump --oids from data-only flag to apply to both data and schema. > > Yes, this is incompatible change, but the change affects feature we > > are discouraging anyway. > > How about a pg_dump flag that simply suppresses OIDs from the data and > schema? But we already have flag that is correlated to use of oids? I don't see why we should bother users who are not using oids with it. -- marko
On Mon, Feb 09, 2009 at 10:44:17AM -0500, Andrew Dunstan wrote: > Marko Kreen wrote: >> But now that I learned that ALTER TABLE WITHOUT OIDS either causes >> bugs or requires table rewrite, it turned from minor annoyance to >> big annoyance. So I'd like have a reasonable path for getting rid >> of them, which we don't have currently. Removing them completely >> is simplest path, but adding extra features to support it is >> another. >> >> If we are talking about adding a feature, then I like retargeting >> pg_dump --oids from data-only flag to apply to both data and >> schema. Yes, this is incompatible change, but the change affects >> feature we are discouraging anyway. >> > > How about a pg_dump flag that simply suppresses OIDs from the data > and schema? Defaults matter. How about one that *preserves* the aforementioned OIDs and have the default, if it finds OIDs, error out with a message like this: You have explicit OIDs in this database, which have been deprecated since 8.1. If despite this, you would like to preserve them, use the --oids option for pg_dump. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Andrew Dunstan wrote: > How about a pg_dump flag that simply suppresses OIDs from the data and > schema? pg_dump -s postgres | sed -e 's/SET default_with_oids = true;/-- &/' -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On 2/9/09, David Fetter <david@fetter.org> wrote: > On Mon, Feb 09, 2009 at 10:44:17AM -0500, Andrew Dunstan wrote: > > Marko Kreen wrote: > >> But now that I learned that ALTER TABLE WITHOUT OIDS either causes > >> bugs or requires table rewrite, it turned from minor annoyance to > >> big annoyance. So I'd like have a reasonable path for getting rid > >> of them, which we don't have currently. Removing them completely > >> is simplest path, but adding extra features to support it is > >> another. > >> > >> If we are talking about adding a feature, then I like retargeting > >> pg_dump --oids from data-only flag to apply to both data and > >> schema. Yes, this is incompatible change, but the change affects > >> feature we are discouraging anyway. > >> > > > > How about a pg_dump flag that simply suppresses OIDs from the data > > and schema? > > > Defaults matter. How about one that *preserves* the aforementioned > OIDs and have the default, if it finds OIDs, error out with a message > like this: > > You have explicit OIDs in this database, which have been > deprecated since 8.1. If despite this, you would like to preserve > them, use the --oids option for pg_dump. +1 for the warning. If --oids is not given, do the check. I would argue that the check should also see if there is index on the oid field, if not it's unusable anyway. So mosts users who have oid columns because of migration from older version, won't be bothered. Or can the oid column be usable without index? -- marko
Heikki Linnakangas wrote: > Andrew Dunstan wrote: >> How about a pg_dump flag that simply suppresses OIDs from the data >> and schema? > > pg_dump -s postgres | sed -e 's/SET default_with_oids = true;/-- &/' > No good for non-text dumps. cheers andrew
Marko Kreen <markokr@gmail.com> writes:
> But now that I learned that ALTER TABLE WITHOUT OIDS either causes bugs
> or requires table rewrite, it turned from minor annoyance to big annoyance.
> So I'd like have a reasonable path for getting rid of them, which we don't
> have currently.
We've had SET WITHOUT OIDS since 7.3 or thereabouts. Anybody who hasn't
applied it in all that time either does not care, or actually needs the
OIDs and will be unhappy if we arbitrarily remove the feature.
regards, tom lane
Andrew Dunstan wrote: > Heikki Linnakangas wrote: >> Andrew Dunstan wrote: >>> How about a pg_dump flag that simply suppresses OIDs from the data >>> and schema? >> >> pg_dump -s postgres | sed -e 's/SET default_with_oids = true;/-- &/' > > No good for non-text dumps. *shrug*, create a text dump then. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
> Heikki Linnakangas wrote:
>> Andrew Dunstan wrote:
>>> How about a pg_dump flag that simply suppresses OIDs from the data
>>> and schema?
>>
>> pg_dump -s postgres | sed -e 's/SET default_with_oids = true;/-- &/'
> No good for non-text dumps.
Also it would fail badly if the dump had in fact been made with -o.
Currently there are two behaviors in pg_dump:
1. With -o: preserve both the existence of oid columns and their
exact contents
2. Without -o: preserve the existence of oid columns, but don't
worry about duplicating their contents (default).
It might be worth extending the switch to provide a third option
to get rid of oid columns altogether, but I'm really not convinced
that this is better than suggesting that people run ALTER SET WITHOUT
OIDS on all their tables.
regards, tom lane
>> Why? What benefit do we get out of denying users this option? > > Why should we continue to support historical special case? It is not > a feature that adds anything to user experience with Postgres. > > Anyway, that was my vote only. If there are developers interested > in supporting oids feel free to do so. > > But now that I learned that ALTER TABLE WITHOUT OIDS either causes bugs > or requires table rewrite, it turned from minor annoyance to big annoyance. > So I'd like have a reasonable path for getting rid of them, which we don't > have currently. Removing them completely is simplest path, but adding > extra features to support it is another. Well, see Tom's point upthread: OIDs are extensively used for system tables, and are not going away. So this is a pipe dream. In the meantime, ALTER TABLE WITHOUT OIDS is (at least for some people) an easier migration path than dump+reload. >> That might be true in your environment, but is certainly not true in >> general. We have many DDL commands that require full-table rewrites, and >> they are FAR from useless. > > Compared to not having the DDL commands or having DDL commands that > do not rewrite the tables? ;) Not having them, of course. If we remove ALTER TABLE WITHOUT OIDS, it's going to encourage people to do stuff like this: CREATE TABLE blah_without_oids AS SELECT * FROM blah; ALTER TABLE blah ... ALTER TABLE blah ... -- move foreign keys, constraints, etc. DROP TABLE blah; ALTER TABLE blah_without_oids RENAME TO blah; ...or else dump+reload. ISTM that if anything that's going to encourage people to keep the OIDs in there because it's too much work to get rid of them. ...Robert
On 2/9/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Marko Kreen <markokr@gmail.com> writes: > > But now that I learned that ALTER TABLE WITHOUT OIDS either causes bugs > > or requires table rewrite, it turned from minor annoyance to big annoyance. > > So I'd like have a reasonable path for getting rid of them, which we don't > > have currently. > > We've had SET WITHOUT OIDS since 7.3 or thereabouts. Anybody who hasn't > applied it in all that time either does not care, or actually needs the > OIDs and will be unhappy if we arbitrarily remove the feature. Sure I did not care. Because I thought I can get rid of them anytime I wanted. But it seems it's not the case... We've set default_with_oids = false, for quite a long time. But there are still tables remaining with oids. And this discussion showed it now easy to get rid of them. I can patch Postgres myself, but I was thinking maybe others want also some solution. -- marko
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> Heikki Linnakangas wrote: >>> Andrew Dunstan wrote: >>>> How about a pg_dump flag that simply suppresses OIDs from the data >>>> and schema? >>> pg_dump -s postgres | sed -e 's/SET default_with_oids = true;/-- &/' > >> No good for non-text dumps. > > Also it would fail badly if the dump had in fact been made with -o. Don't do that then. We're not talking about filtering any old dump you have lying around. We're talking about adding a new flag to pg_dump. If you can run pg_dump with a new flag, surely you can run it without -o in text mode and use sed just as well. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On 2/9/09, Robert Haas <robertmhaas@gmail.com> wrote: > >> Why? What benefit do we get out of denying users this option? > > > > Why should we continue to support historical special case? It is not > > a feature that adds anything to user experience with Postgres. > > > > Anyway, that was my vote only. If there are developers interested > > in supporting oids feel free to do so. > > > > But now that I learned that ALTER TABLE WITHOUT OIDS either causes bugs > > or requires table rewrite, it turned from minor annoyance to big annoyance. > > So I'd like have a reasonable path for getting rid of them, which we don't > > have currently. Removing them completely is simplest path, but adding > > extra features to support it is another. > > > Well, see Tom's point upthread: OIDs are extensively used for system > tables, and are not going away. So this is a pipe dream. In the > meantime, ALTER TABLE WITHOUT OIDS is (at least for some people) an > easier migration path than dump+reload. Sorry, I was talking only about oids in user tables. > >> That might be true in your environment, but is certainly not true in > >> general. We have many DDL commands that require full-table rewrites, and > >> they are FAR from useless. > > > > Compared to not having the DDL commands or having DDL commands that > > do not rewrite the tables? ;) > > > Not having them, of course. > > If we remove ALTER TABLE WITHOUT OIDS, it's going to encourage people > to do stuff like this: > > CREATE TABLE blah_without_oids AS SELECT * FROM blah; > ALTER TABLE blah ... > ALTER TABLE blah ... > -- move foreign keys, constraints, etc. > DROP TABLE blah; > ALTER TABLE blah_without_oids RENAME TO blah; > > ...or else dump+reload. ISTM that if anything that's going to > encourage people to keep the OIDs in there because it's too much work > to get rid of them. By "removing" I mean that in version 8.6 you simply cannot create user table with oids. Thus no need to get rid of them. If we keep the possibility to create tables with oids, obviously the ALTER, etc command must also be kept. -- marko
Marko Kreen wrote: > We've set default_with_oids = false, for quite a long time. But there > are still tables remaining with oids. And this discussion showed it > now easy to get rid of them. Do you still need the oids? If not, run ALTER TABLE WITHOUT OIDS before upgrading to 8.4, while it's still fast. If yes, you couldn't use the option to remove them at pg_dump anyway because you still need them after the upgrade. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Marko Kreen <markokr@gmail.com> writes:
> On 2/9/09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> We've had SET WITHOUT OIDS since 7.3 or thereabouts. Anybody who hasn't
>> applied it in all that time either does not care, or actually needs the
>> OIDs and will be unhappy if we arbitrarily remove the feature.
> Sure I did not care. Because I thought I can get rid of them
> anytime I wanted. But it seems it's not the case...
Sure, you can still get rid of them, because SET WITHOUT OIDS isn't
going away. It will be a bit more expensive than it used to be, but
if you've not applied it before migrating to 8.4, that very strongly
suggests that you don't care about getting rid of oids anyhow.
The other half of this thread seems to be pointed in the direction
of *forcing* users to get rid of oids, which is not happening as far
as I'm concerned. It'd be breaking stuff to no purpose. I've been
known to vote for breaking apps when there was a purpose to it
(eg tightening implicit coercions) but removing the ability to have
oids in user tables wouldn't buy us anything meaningful.
regards, tom lane
On Mon, Feb 9, 2009 at 11:36 AM, Marko Kreen <markokr@gmail.com> wrote: > On 2/9/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Marko Kreen <markokr@gmail.com> writes: >> > But now that I learned that ALTER TABLE WITHOUT OIDS either causes bugs >> > or requires table rewrite, it turned from minor annoyance to big annoyance. >> > So I'd like have a reasonable path for getting rid of them, which we don't >> > have currently. >> >> We've had SET WITHOUT OIDS since 7.3 or thereabouts. Anybody who hasn't >> applied it in all that time either does not care, or actually needs the >> OIDs and will be unhappy if we arbitrarily remove the feature. > > Sure I did not care. Because I thought I can get rid of them > anytime I wanted. But it seems it's not the case... > > We've set default_with_oids = false, for quite a long time. But there > are still tables remaining with oids. And this discussion showed it > now easy to get rid of them. > > I can patch Postgres myself, but I was thinking maybe others want also > some solution. I must be missing something. Why would you need to patch PostgreSQL and how would it help you if you did? ...Robert
On 2/9/09, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > Marko Kreen wrote: > > We've set default_with_oids = false, for quite a long time. But there > > are still tables remaining with oids. And this discussion showed it > > now easy to get rid of them. > > Do you still need the oids? If not, run ALTER TABLE WITHOUT OIDS before > upgrading to 8.4, while it's still fast. If yes, you couldn't use the option > to remove them at pg_dump anyway because you still need them after the > upgrade. Indeed. I must apologize. I seems I read too fast and got the impression the bug applies also to older versions of Postgres. If this is not the case and ALTER still works fine on older versions, most of my comments do not apply, because indeed, we can clean it up on 8.3. There is still minor problem that it will be made expensive on 8.4, but as it is not released yet, it can be solved by advising users to clean up their tables on 8.3. -- marko
On 2/9/09, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Feb 9, 2009 at 11:36 AM, Marko Kreen <markokr@gmail.com> wrote: > > On 2/9/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Marko Kreen <markokr@gmail.com> writes: > >> > But now that I learned that ALTER TABLE WITHOUT OIDS either causes bugs > >> > or requires table rewrite, it turned from minor annoyance to big annoyance. > >> > So I'd like have a reasonable path for getting rid of them, which we don't > >> > have currently. > >> > >> We've had SET WITHOUT OIDS since 7.3 or thereabouts. Anybody who hasn't > >> applied it in all that time either does not care, or actually needs the > >> OIDs and will be unhappy if we arbitrarily remove the feature. > > > > Sure I did not care. Because I thought I can get rid of them > > anytime I wanted. But it seems it's not the case... > > > > We've set default_with_oids = false, for quite a long time. But there > > are still tables remaining with oids. And this discussion showed it > > now easy to get rid of them. > > > > I can patch Postgres myself, but I was thinking maybe others want also > > some solution. > > > I must be missing something. Why would you need to patch PostgreSQL > and how would it help you if you did? We use dumps to move db's around and they contain lot of SET default_with_oids that the pg_dump happily puts there. Remembering to filter them out each time a database is created does not work. So it would be good if we can use such dump, but receiving Postgres would ignore any requests to create tables with oids. -- marko
Marko Kreen <markokr@gmail.com> writes:
> On 2/9/09, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote:
>> Do you still need the oids? If not, run ALTER TABLE WITHOUT OIDS before
>> upgrading to 8.4, while it's still fast. If yes, you couldn't use the option
>> to remove them at pg_dump anyway because you still need them after the
>> upgrade.
> Indeed. I must apologize. I seems I read too fast and got the impression
> the bug applies also to older versions of Postgres. If this is not
> the case and ALTER still works fine on older versions, most of my comments
> do not apply, because indeed, we can clean it up on 8.3.
I think actually we are in violent agreement ;-). The argument for
getting rid of userland OIDs, as far as I can see, is to eliminate
future development effort and risk of bugs associated with them.
Now if OIDs are staying in system tables ... which they are, for the
foreseeable future ... then the only real cost or risk associated with
userland OIDs is driven precisely by ALTER SET WITHOUT OIDS. Because
that creates a situation with a table that used to have OIDs and no
longer does, except there are still vestiges of its having OIDs, ie rows
in the table that contain an OID. So the patch I'm proposing attacks
that problem directly by making sure there is no intermediate status.
Either a table has OIDS (and so do all its rows) or not (and none of
its rows do either). I think this pretty much eliminates the risk of
induced bugs, and it does it without taking away functionality that
applications might depend on.
Unless you want to argue that "SET WITHOUT OIDS is fast" is a property
that apps are depending on, but that seems like a bit of a stretch.
regards, tom lane
On 2/9/09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Marko Kreen <markokr@gmail.com> writes: > > On 2/9/09, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > >> Do you still need the oids? If not, run ALTER TABLE WITHOUT OIDS before > >> upgrading to 8.4, while it's still fast. If yes, you couldn't use the option > >> to remove them at pg_dump anyway because you still need them after the > >> upgrade. > > > Indeed. I must apologize. I seems I read too fast and got the impression > > the bug applies also to older versions of Postgres. If this is not > > the case and ALTER still works fine on older versions, most of my comments > > do not apply, because indeed, we can clean it up on 8.3. > > > I think actually we are in violent agreement ;-). The argument for > getting rid of userland OIDs, as far as I can see, is to eliminate > future development effort and risk of bugs associated with them. > Now if OIDs are staying in system tables ... which they are, for the > foreseeable future ... then the only real cost or risk associated with > userland OIDs is driven precisely by ALTER SET WITHOUT OIDS. Because > that creates a situation with a table that used to have OIDs and no > longer does, except there are still vestiges of its having OIDs, ie rows > in the table that contain an OID. So the patch I'm proposing attacks > that problem directly by making sure there is no intermediate status. > Either a table has OIDS (and so do all its rows) or not (and none of > its rows do either). I think this pretty much eliminates the risk of > induced bugs, and it does it without taking away functionality that > applications might depend on. Yes. I agree with the patch. And I'm all for robustness. > Unless you want to argue that "SET WITHOUT OIDS is fast" is a property > that apps are depending on, but that seems like a bit of a stretch. No. I'm not concerned with ALTER command, I'm concerned about reloading dumps from older versions. So my, uh, new argument is - starting with 8.4, it is very hard to get rid of oids on user tables because all the tools work against user. So either: the 8.4 will be a "flag day" and all users need to clean up their database on 8.3, or we give some option for them to lessen the pain. Considering that default_with_oids went false in 8.1 (?), affected are users who are reusing their dumps or postgresql.conf from 8.0 and below. Maybe there are not many of such users (?) so flag day approach it ok. -- marko
Marko Kreen <markokr@gmail.com> writes:
> No. I'm not concerned with ALTER command, I'm concerned about reloading
> dumps from older versions. So my, uh, new argument is - starting with 8.4,
> it is very hard to get rid of oids on user tables because all the tools
> work against user.
That's a pretty overstated claim. It's exactly the same tool as before,
it's just slower.
> So either: the 8.4 will be a "flag day" and all users need to clean up
> their database on 8.3, or we give some option for them to lessen the pain.
> Considering that default_with_oids went false in 8.1 (?), affected are
> users who are reusing their dumps or postgresql.conf from 8.0 and below.
Indeed. If they have not bothered to remove oids from their tables up
to now, what are the odds that they're going to bother in the future?
IMHO, the only way they'd care is if we try to force them to care
(ie by removing oids as a user option), which I'm against. So I see
no flag day here. They'll still have oids and they still won't care.
regards, tom lane
Tom Lane wrote: > >> Considering that default_with_oids went false in 8.1 (?), affected are >> users who are reusing their dumps or postgresql.conf from 8.0 and below. >> No, they have upgraded along the way. pg_dump carefully preserves the with/without oids property of the tables it is dumping. And rightly so. This has nothing to do with default_without_oids. > > Indeed. If they have not bothered to remove oids from their tables up > to now, what are the odds that they're going to bother in the future? > > IMHO, the only way they'd care is if we try to force them to care > (ie by removing oids as a user option), which I'm against. So I see > no flag day here. They'll still have oids and they still won't care. > > I have clients I have not yet managed to ween off oids, because they have legacy apps, sometimes third party apps, that rely on them. I don't want to make it any harder to get them over the hurdle. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> I have clients I have not yet managed to ween off oids, because they
> have legacy apps, sometimes third party apps, that rely on them. I don't
> want to make it any harder to get them over the hurdle.
Surely the major cost there is going to be fixing those apps; I think
focusing on whether SET WITHOUT OIDS is zero-cost is worrying about
entirely the wrong thing.
Also, if they are using the oids (and presumably relying on them to be
unique), the tables can't be as huge as all that --- they'd have to be
under a billion or so rows, else the 32-bit width of oids would have
forced a change a long time ago. So even a rewriting form of SET WITHOUT
OIDS doesn't seem all that painful. Compared to an app migration that's
still not happened after N years, I can't believe it's a problem.
regards, tom lane
Sorry, I was indeed thinking of newly added columns rather than dropped columns. We define the row representation such that one may have fewer rows than the tupledesc and how to interpret that in such a way as to make adding nullable columns a convenient operation. How is doing the same here and fixing a case where we weren't following the definition any more of a kludge than how we handle newly added columns? Which incidentally I don't think is at all kludgy. I think what you propose would be a mistake. We want to encourage people to move *away* from OIDS. - making drop kids prohibitively expensive and adding an operation to add kids which we hope nobody needs seems like heading in the wrong direction. Sorry for top posting - in this case i'm using google mail's mobile interface but it's no better about this and makes properly threading responses nigh impossible. On 2009-02-09, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Greg Stark <greg.stark@enterprisedb.com> writes: >> I don't understand what's wrong with the existing setup where DROP >> OIDS is a free operation. > > It breaks things, in particular > http://archives.postgresql.org/pgsql-hackers/2008-11/msg00332.php > We could kluge around that particular problem, but the objection > I have to doing so is I'm 100% certain it won't be the last such bug. > >> It seems exactly equivalent to how we handle DROP COLUMN > > It is just about exactly *unlike* DROP COLUMN, because in DROP COLUMN > we retain a memory that there used to be a column there. A close > emulation of DROP COLUMN would involve inventing some representation of > "oidisdropped", and going through every one of the multitudinous places > that special-case dropped columns in order to see if each one needs a > similar special case for dropped OIDs. The bug mentioned above stems > directly from not expecting a table to still contain OIDs after SET > WITHOUT OIDS, so I don't think this parallel is mistaken. > > Note that I'm willing to lay a significant side bet that we still have > bugs of omission with dropped columns, too. But we'll fix those as we > come to them. I don't think it is worth making a similar open-ended > commitment of resources just to keep SET WITHOUT OIDS fast. > >> ... where the >> natt field of the tuple disagrees with the tuple descriptor and any >> additional columns are implicitly null. > > No, that's the mechanism that makes ADD COLUMN feasible (and indeed > pretty easy). DROP COLUMN is the far newer and uglier mess around > attisdropped. > > regards, tom lane > -- greg
Greg Stark <stark@enterprisedb.com> writes:
> I think what you propose would be a mistake. We want to encourage
> people to move *away* from OIDS.
Why? I don't agree with that premise, and therefore not with any
of the rest of your argument.
regards, tom lane
On Sun, 2009-02-08 at 11:51 -0500, Tom Lane wrote: > Now, if you want to argue that we should get rid of SET WITHOUT OIDS > altogether, I'm not sure I could dispute it. But if we have the > ability > to do that ISTM we should offer the reverse too. We should keep the ability to have OIDs. Some people use it, though not many. But the ability to turn this on/off is not an important one, since even the people who use OIDs seldom use this. They have CTAS; let them use it. So I say let's drop support now for ALTER TABLE SET WITHOUT OIDS and don't bother to implement SET WITH OIDS. Less weird corners in the software means fewer bugs. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
> But the ability to turn this on/off is not an important one, since even
> the people who use OIDs seldom use this. They have CTAS; let them use
> it.
Well, CTAS is a vastly inferior solution because you'd have to manually
move indexes, constraints, FKs, etc to the new table. Plus it's just as
slow if not slower than the proposed rewriting code. I think that
Andrew's complaint about not putting barriers in the way of removing
OIDs would apply pretty strongly to that approach.
regards, tom lane
Well for one thing because they don't scale well to billions of records. For another they're even less like the standard or anything any other database has. I agree with you that there's no reason to actively deprecate OIDs or hurt users who use them. But we should make it as easy as possible for users who want to move to a normal primary key, not put obstacles in their way like large full table rewrites. -- Greg On 10 Feb 2009, at 01:49, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Greg Stark <stark@enterprisedb.com> writes: >> I think what you propose would be a mistake. We want to encourage >> people to move *away* from OIDS. > > Why? I don't agree with that premise, and therefore not with any > of the rest of your argument. > > regards, tom lane