Thread: WIP -- renaming implicit sequences
Hi, Here is an unfinished patch to implement something which appears on the TODO list under ALTER: automatic renaming of sequences created with serial when the table and column names change. I've often wanted this feature and it seemed like a good starter project. I'd be grateful for any feedback and advice on how I could get it into acceptable shape. Example: hack=# create table foo (id serial primary key); NOTICE: CREATE TABLE will create implicit sequence "foo_id_seq" for serial column "foo.id" NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "foo_pkey" for table "foo" CREATE TABLE hack=# alter table foo rename to bar; NOTICE: ALTER TABLE will rename implicit sequence "foo_id_seq" to "bar_id_seq" ALTER TABLE hack=# alter table bar rename id to snacks; NOTICE: ALTER TABLE will rename implicit sequence "bar_id_seq" to "bar_snacks_seq" ALTER TABLE Sequences are considered to be renameable if they are owned by the table, and have a name conforming to the name pattern used by CREATE TABLE (table_column_seq with optional trailing numbers). If you've manually renamed a SEQUENCE so that it doesn't conform, it won't touch it. If you've created a SEQUENCE and declared it to be OWNED BY the table, then it will be renamed only if it happens to conform. I'm not sure what to do about permissions. I guess it should silently skip renaming sequences if the user doesn't have appropriate privileges. Useful? Why would anyone not want this behaviour? Have I used inappropriate locking levels? What should I read to understand the rules of locking? Have I failed to handle errors? Have I made memory ownership mistakes? Thanks! Thomas Munro
Attachment
Thomas Munro <munro@ip9.org> writes:
> Here is an unfinished patch to implement something which appears on
> the TODO list under ALTER: automatic renaming of sequences created
> with serial when the table and column names change. I've often wanted
> this feature and it seemed like a good starter project.
Hmm ... this seems a bit inconsistent with the fact that we got rid of
automatic renaming of indexes a year or three back. Won't renaming of
serials have all the same problems that caused us to give up on renaming
indexes?
regards, tom lane
On 12 January 2012 00:58, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <munro@ip9.org> writes: >> Here is an unfinished patch to implement something which appears on >> the TODO list under ALTER: automatic renaming of sequences created >> with serial when the table and column names change. I've often wanted >> this feature and it seemed like a good starter project. > > Hmm ... this seems a bit inconsistent with the fact that we got rid of > automatic renaming of indexes a year or three back. Won't renaming of > serials have all the same problems that caused us to give up on renaming > indexes? Ah. I assume this is it: http://archives.postgresql.org/pgsql-committers/2009-12/msg00209.php I used ChooseRelationName to generate a new unique name during transformation. Presumably the implicit ALTER SEQUENCE will fail at execution if someone manages to nab the name after ChooseRelationName's get_relname_relid check. So with the patch, ALTER TABLE RENAME with serial columns has the same concurrency caveat as CREATE TABLE with serial columns, which I hoped was reasonable or at least symmetrical.
On 12 January 2012 00:58, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Hmm ... this seems a bit inconsistent with the fact that we got rid of > automatic renaming of indexes a year or three back. Won't renaming of > serials have all the same problems that caused us to give up on renaming > indexes? I was sort of planning to do something similar for constraints (once the patch to support renaming constraints lands) and indexes (I didn't know they'd previously been automatically renamed and that had been dropped). Would you say that I should abandon this, no chance of being accepted?Is there a technical problem I'm missing, other thanthe gap between unique name generation and execution of the implicit ALTERs? Maybe I should look into writing a 'tidy rename' procedure for tables and columns instead, rather than modifying the behaviour of core ALTER TABLE.
On Sat, Jan 14, 2012 at 5:51 PM, Thomas Munro <munro@ip9.org> wrote: > On 12 January 2012 00:58, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Hmm ... this seems a bit inconsistent with the fact that we got rid of >> automatic renaming of indexes a year or three back. Won't renaming of >> serials have all the same problems that caused us to give up on renaming >> indexes? > > I was sort of planning to do something similar for constraints (once > the patch to support renaming constraints lands) and indexes (I didn't > know they'd previously been automatically renamed and that had been > dropped). > > Would you say that I should abandon this, no chance of being accepted? > Is there a technical problem I'm missing, other than the gap between > unique name generation and execution of the implicit ALTERs? > > Maybe I should look into writing a 'tidy rename' procedure for tables > and columns instead, rather than modifying the behaviour of core ALTER > TABLE. +1 for that approach. I think this is the kind of thing that seems cooler on paper than it is in real life. For example, consider this: rhaas=# create table foo (a serial); NOTICE: CREATE TABLE will create implicit sequence "foo_a_seq" for serial column "foo.a" CREATE TABLE rhaas=# alter sequence foo_a_seq rename to bob; ALTER SEQUENCE If somebody renames the table or the column at this point, it's a good bet that they *don't* want bob renamed. Also, some application code I've had in the past had sequence names hardcoded into it - it did things like SELECT nextval(...) followed by INSERT ..., for lack of INSERT .. RETURNING, which didn't exist at the time. So it's not implausible that renaming a sequence could be unwanted, though in practice the likelihood is fairly low: had I renamed a table, I probably would have renamed the sequence as well. Another, admittedly minor consideration is that this can introduce some subtle concurrency bugs that we don't have today. For example, suppose we choose a new name for the sequence which isn't in use, but then between the time when we pick the name and the time we do the insert the name becomes used, due to some concurrent transaction. Now we'll fail with a rather baffling error message. That isn't necessarily a huge problem - we have lots of code that is prone to such race conditions - but it's not wonderful either. Someday it would be nice to figure out a cleaner solution to these kinds of issues... maybe allowing the btree AM to return normally with an indication that there's a unique constraint violation, rather than throwing an ERROR. I think we should remove this from the TODO list, or at least document that there are a number of reasons why it might be a deeper hole than it appears to be at first glance. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of jue ene 19 18:07:33 -0300 2012: > I think we should remove this from the TODO list, or at least document > that there are a number of reasons why it might be a deeper hole than > it appears to be at first glance. Maybe not remove it, but instead add a link to this discussion. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Jan 19, 2012 at 6:30 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > > Excerpts from Robert Haas's message of jue ene 19 18:07:33 -0300 2012: > >> I think we should remove this from the TODO list, or at least document >> that there are a number of reasons why it might be a deeper hole than >> it appears to be at first glance. > > Maybe not remove it, but instead add a link to this discussion. I don't see what that accomplishes, unless someone's arguing that we really do want this behavior...? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of jue ene 19 20:53:42 -0300 2012: > > On Thu, Jan 19, 2012 at 6:30 PM, Alvaro Herrera > <alvherre@commandprompt.com> wrote: > > > > Excerpts from Robert Haas's message of jue ene 19 18:07:33 -0300 2012: > > > >> I think we should remove this from the TODO list, or at least document > >> that there are a number of reasons why it might be a deeper hole than > >> it appears to be at first glance. > > > > Maybe not remove it, but instead add a link to this discussion. > > I don't see what that accomplishes, unless someone's arguing that we > really do want this behavior...? Well, it documents what happened when we tried to do it. Sort of like "features we do not want". But then, maybe it's just TODO bloat. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Jan 19, 2012 at 7:15 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: >> >> I think we should remove this from the TODO list, or at least document >> >> that there are a number of reasons why it might be a deeper hole than >> >> it appears to be at first glance. >> > >> > Maybe not remove it, but instead add a link to this discussion. >> >> I don't see what that accomplishes, unless someone's arguing that we >> really do want this behavior...? > > Well, it documents what happened when we tried to do it. Sort of like > "features we do not want". But then, maybe it's just TODO bloat. I share your urge to memorialize the conversation somewhere, but I fear it will just cause future people to spend time thinking about it that isn't really warranted. I guess we could move it to the features we do not want section, but that's mostly bigger picture stuff. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 19/01/2012, Robert Haas <robertmhaas@gmail.com> wrote: > rhaas=# alter sequence foo_a_seq rename to bob; > ALTER SEQUENCE > > If somebody renames the table or the column at this point, it's a good > bet that they *don't* want bob renamed. FWIW the patch addresses this case and wouldn't try to rename 'bob'. > Another, admittedly minor consideration is that this can introduce > some subtle concurrency bugs that we don't have today. For example, > suppose we choose a new name for the sequence which isn't in use, but > then between the time when we pick the name and the time we do the > insert the name becomes used, due to some concurrent transaction. Now > we'll fail with a rather baffling error message. That isn't > necessarily a huge problem - we have lots of code that is prone to > such race conditions - but it's not wonderful either. ... I thought about this, and it seemed to me that (1) the same race already applies when you CREATE a table with a serial column and (2) anyone running a bunch of DDL concurrently with other DDL operations already needs to coordinate their action or deal with occasional name collisions in general. But yeah I see that it's not ideal. > I think we should remove this from the TODO list, or at least document > that there are a number of reasons why it might be a deeper hole than > it appears to be at first glance. Fair enough, I'll leave it there. Thanks for the feedback!