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

Thomas Munro <munro(at)ip9(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thomas Munro <munro(at)ip9(dot)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(at)sss(dot)pgh(dot)pa(dot)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.

On Sat, Jan 14, 2012 at 5:51 PM, Thomas Munro <munro(at)ip9(dot)org> wrote:
> On 12 January 2012 00:58, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)gmail(dot)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!