Thread: Proposed patch for sequence-renaming problems

Attached is a fully-worked-out patch to make SERIAL column default
expressions refer to the target sequence with a "regclass" literal
instead of a "text" literal.  Since the regclass literal is actually
just an OID, it is impervious to renamings and schema changes of
the target sequence.  This fixes the long-standing hazard of renaming
a serial column's sequence, as well as the recently added hazard of
renaming the schema the sequence is in; and it lets us get rid of a
very klugy solution in ALTER TABLE SET SCHEMA.

I've arranged for stored regclass literals to create dependencies on
the referenced relation, which provides useful improvements even for
handwritten defaults: given

    create sequence myseq;
    create table foo (f1 int default nextval('myseq'::regclass));

the system will not allow myseq to be dropped while the default
expression remains.  (This also ensures that pg_dump will emit the
sequence before the table.)

The patch also fixes a couple of places where code was still looking
at the deprecated pg_attrdef.adsrc column, instead of reverse-compiling
pg_attrdef.adbin.  This ensures that psql's \d command shows the
up-to-date form of a column default.  (That should have happened quite
some time ago; not sure why it was overlooked.)

I propose applying this to fix the open issue that ALTER SCHEMA RENAME
breaks serial columns.  Comments, objections?

            regards, tom lane

I looked over the patch, and while it does fix the problem for SERIAL, I
am concerned about expecting users to user ::regclass in normal usage,
and I am concerned about adding something we will have to support in the
future when we come up with a better solution.  Why is regclass not
being used automatically?

---------------------------------------------------------------------------

Tom Lane wrote:
> Attached is a fully-worked-out patch to make SERIAL column default
> expressions refer to the target sequence with a "regclass" literal
> instead of a "text" literal.  Since the regclass literal is actually
> just an OID, it is impervious to renamings and schema changes of
> the target sequence.  This fixes the long-standing hazard of renaming
> a serial column's sequence, as well as the recently added hazard of
> renaming the schema the sequence is in; and it lets us get rid of a
> very klugy solution in ALTER TABLE SET SCHEMA.
>
> I've arranged for stored regclass literals to create dependencies on
> the referenced relation, which provides useful improvements even for
> handwritten defaults: given
>
>     create sequence myseq;
>     create table foo (f1 int default nextval('myseq'::regclass));
>
> the system will not allow myseq to be dropped while the default
> expression remains.  (This also ensures that pg_dump will emit the
> sequence before the table.)
>
> The patch also fixes a couple of places where code was still looking
> at the deprecated pg_attrdef.adsrc column, instead of reverse-compiling
> pg_attrdef.adbin.  This ensures that psql's \d command shows the
> up-to-date form of a column default.  (That should have happened quite
> some time ago; not sure why it was overlooked.)
>
> I propose applying this to fix the open issue that ALTER SCHEMA RENAME
> breaks serial columns.  Comments, objections?
>
>             regards, tom lane
>

Content-Description: seq-regclass.patch.gz

[ Type application/octet-stream treated as attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I looked over the patch, and while it does fix the problem for SERIAL, I
> am concerned about expecting users to user ::regclass in normal usage,
> and I am concerned about adding something we will have to support in the
> future when we come up with a better solution.  Why is regclass not
> being used automatically?

If we provide both nextval(text) and nextval(regclass), then the parser
will interpret "nextval('something')" as nextval(text) because that's
the more preferred resolution of an unknown-type literal.  The only way
to make regclass be used "automatically" would be to remove the
text-input variant.  That is where I want to go eventually, but it seems
pretty risky to jump there in one step.  The proposed patch adds
regclass-based functions alongside the existing functionality, so that
people can migrate as they choose; it does not open any risks of
breaking cases that work now.

            regards, tom lane

aTom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I looked over the patch, and while it does fix the problem for SERIAL, I
> > am concerned about expecting users to user ::regclass in normal usage,
> > and I am concerned about adding something we will have to support in the
> > future when we come up with a better solution.  Why is regclass not
> > being used automatically?
>
> If we provide both nextval(text) and nextval(regclass), then the parser
> will interpret "nextval('something')" as nextval(text) because that's
> the more preferred resolution of an unknown-type literal.  The only way
> to make regclass be used "automatically" would be to remove the
> text-input variant.  That is where I want to go eventually, but it seems
> pretty risky to jump there in one step.  The proposed patch adds
> regclass-based functions alongside the existing functionality, so that
> people can migrate as they choose; it does not open any risks of
> breaking cases that work now.

What I am primarily worried about in your patch is the exposure of
::regclass as a recommended way of doing things.  I know we can
discourage its us later, but once people start using something, it is
hard to change.

Right now, we have three cases, SERIAL, DEFAULT with no-schema seqname,
and DEFAULT with schema-specified seqname.  If we just do regclass
internally for SERIAL, we don't have any user-visible change, except for
the psql \d display of the default.

The second case already works because there is no class name.  It is
only the last one where recommending regclass helps, but is it worth
improving sequence/schema renaming by exposing and recommending a
::regclass syntax that will go away as soon as we fix this properly?

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

[Sorry for the subject mangling -- my outgoing MTA is too stupid]

On Tue, Sep 27, 2005 at 06:46:55PM -0400, Tom Lane wrote:
> Attached is a fully-worked-out patch to make SERIAL column default
> expressions refer to the target sequence with a "regclass" literal
> instead of a "text" literal.  Since the regclass literal is actually
> just an OID, it is impervious to renamings and schema changes of
> the target sequence.  This fixes the long-standing hazard of renaming
> a serial column's sequence, as well as the recently added hazard of
> renaming the schema the sequence is in; and it lets us get rid of a
> very klugy solution in ALTER TABLE SET SCHEMA.

Why did you rename the C function nextval() to nextval_text()?  Doing
this causes a compatibility problem for code using that function.  I
understand that it would be better for that code to be "upgraded" to
call nextval_oid(), but if we can allow the code to continue to compile
untouched, why not let it?  (For example the change in the contrib area
would be unnecessary AFAICS.)

I vaguely remember there being a dependency we weren't able to track
because of the sequence being a literal instead of an expression; are we
able to do better now?  (Of course it would be better to state what the
specific problem was but I can't remember right now.)

As an unrelated note, since we are going to force an initdb for the next
beta, it would be nice to include the 64 bit parameter to pg_control ...

--
Alvaro Herrera                                http://www.PlanetPostgreSQL.org
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Why did you rename the C function nextval() to nextval_text()?

I did that deliberately to make sure I'd catch all the dependencies.
If you like we can argue about whether to undo that aspect of the
patch --- it's surely not very critical --- but my vision of the future
path of development is that the text variant will go away entirely.
So I didn't like the idea of having "nextval" and "nextval_oid";
seems like that gives pride of place to the wrong thing.

> As an unrelated note, since we are going to force an initdb for the next
> beta, it would be nice to include the 64 bit parameter to pg_control ...

See other thread.

            regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> What I am primarily worried about in your patch is the exposure of
> ::regclass as a recommended way of doing things.  I know we can
> discourage its us later, but once people start using something, it is
> hard to change.

Why shouldn't it be a recommended way of doing things?  It is certainly
far better than the existing text-argument way.

> Right now, we have three cases, SERIAL, DEFAULT with no-schema seqname,
> and DEFAULT with schema-specified seqname.  If we just do regclass
> internally for SERIAL, we don't have any user-visible change, except for
> the psql \d display of the default.

> The second case already works because there is no class name.

No, it really wouldn't "work" at all.  It's unsafe if the user changes
the search path for example, and it certainly doesn't handle any of the
renaming or change-of-schema cases.

> It is
> only the last one where recommending regclass helps, but is it worth
> improving sequence/schema renaming by exposing and recommending a
> ::regclass syntax that will go away as soon as we fix this properly?

Please explain what you think a "proper" fix is.  I think this patch is
a proper fix.  I see no better alternative that we might implement
later.

The only other thing that's been discussed is the SQL2003 syntax
    NEXT VALUE FOR sequencename
but this is in fact just syntactic sugar for something functionally
equivalent to nextval('sequencename'::regclass).  It cannot completely
replace all uses of the nextval function, because only a constant table
name can appear.

Hmm ... given the proposed patch, it would indeed take only a few more
lines in gram.y to support the NEXT VALUE FOR syntax ...

            regards, tom lane

I wrote:
> The only other thing that's been discussed is the SQL2003 syntax
>     NEXT VALUE FOR sequencename
> but this is in fact just syntactic sugar for something functionally
> equivalent to nextval('sequencename'::regclass).

I have to take that back.  It's not just syntactic sugar for nextval(),
because the SQL2003 spec says

: If there are multiple instances of <next value expression>s specifying
: the same sequence generator within a single SQL-statement, all those
: instances return the same value for a given row processed by that
: SQL-statement.

So it's really sort of a magic combination of nextval() and currval().
To meet the spec semantics, we'd need some sort of layer over nextval()
that would keep track of whether a new value should be obtained or not.

I don't think we should use the spec syntax until we're prepared to
meet the spec semantics, so NEXT VALUE FOR as part of the current patch
seems "out".

A relatively simple Plan B would be to use different SQL names for the
variant functions, ie, keep nextval() as is and instead invent, say,
next_value(regclass).  Then we tell people to use next_value('foo')
and they don't need to write the cast explicitly.  This seems
notationally nicer but a major pain in the neck from the point of view
of documentation and explanation --- for instance, instead of saying
"nextval does this" we'd have to say "next_value and nextval do this".
Not at all sure that I like it better.

            regards, tom lane

Tom Lane wrote:
> I wrote:
> > The only other thing that's been discussed is the SQL2003 syntax
> >     NEXT VALUE FOR sequencename
> > but this is in fact just syntactic sugar for something functionally
> > equivalent to nextval('sequencename'::regclass).
>
> I have to take that back.  It's not just syntactic sugar for nextval(),
> because the SQL2003 spec says
>
> : If there are multiple instances of <next value expression>s specifying
> : the same sequence generator within a single SQL-statement, all those
> : instances return the same value for a given row processed by that
> : SQL-statement.
>
> So it's really sort of a magic combination of nextval() and currval().
> To meet the spec semantics, we'd need some sort of layer over nextval()
> that would keep track of whether a new value should be obtained or not.
>
> I don't think we should use the spec syntax until we're prepared to
> meet the spec semantics, so NEXT VALUE FOR as part of the current patch
> seems "out".

OK.

> A relatively simple Plan B would be to use different SQL names for the
> variant functions, ie, keep nextval() as is and instead invent, say,
> next_value(regclass).  Then we tell people to use next_value('foo')
> and they don't need to write the cast explicitly.  This seems
> notationally nicer but a major pain in the neck from the point of view
> of documentation and explanation --- for instance, instead of saying
> "nextval does this" we'd have to say "next_value and nextval do this".
> Not at all sure that I like it better.

Agreed, two names is a mess.

I still think we shouldn't be hashing this out during beta, but ...

What would the final nextval() behavior be?  ::regclass binding?  How
would late binding be done?  What syntax?

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I still think we shouldn't be hashing this out during beta, but ...

We're looking at ways to fix some bugs.  It's never been the case that
our first-resort response to a bug is "pull out features".

> What would the final nextval() behavior be?  ::regclass binding?  How
> would late binding be done?  What syntax?

If I were prepared to say all that today, I would have just done it ;-)

The more I think about it, the more I think that two sets of function
names might not be such an awful idea.  next_value(), curr_value(), and
set_value() seem like they'd work well enough.  Then we'd just say that
nextval and friends are deprecated except when you need late binding,
and we'd be done.

            regards, tom lane

Tom Lane wrote:

>The more I think about it, the more I think that two sets of function
>names might not be such an awful idea.  next_value(), curr_value(), and
>set_value() seem like they'd work well enough.  Then we'd just say that
>nextval and friends are deprecated except when you need late binding,
>and we'd be done.
>
>
>
>

Personally, I like this more than the overloading idea.

cheers

andrew

On Wed, 28 Sep 2005, Tom Lane wrote:

> I wrote:
> > The only other thing that's been discussed is the SQL2003 syntax
> >     NEXT VALUE FOR sequencename
> > but this is in fact just syntactic sugar for something functionally
> > equivalent to nextval('sequencename'::regclass).
>
> I have to take that back.  It's not just syntactic sugar for nextval(),
> because the SQL2003 spec says
>
> : If there are multiple instances of <next value expression>s specifying
> : the same sequence generator within a single SQL-statement, all those
> : instances return the same value for a given row processed by that
> : SQL-statement.
>
> So it's really sort of a magic combination of nextval() and currval().
> To meet the spec semantics, we'd need some sort of layer over nextval()
> that would keep track of whether a new value should be obtained or not.
>
> I don't think we should use the spec syntax until we're prepared to
> meet the spec semantics, so NEXT VALUE FOR as part of the current patch
> seems "out".

Well, AFAICT, the only part of the spec we cannot implement is what you
quote above. Therefore, why can't we support NEXT VALUE FOR seqname and
reject table creation/alteration which would add more than one reference
to the same sequence. That will allow us to avoid an intermediate step
in getting to the SQL2003 syntax. Having to support three different
sequence incrementation mechanisms for three flavours of PostgreSQL is
going to be a real PITA.

Thanks,

Gavin

Gavin Sherry wrote:
> > So it's really sort of a magic combination of nextval() and currval().
> > To meet the spec semantics, we'd need some sort of layer over nextval()
> > that would keep track of whether a new value should be obtained or not.
> >
> > I don't think we should use the spec syntax until we're prepared to
> > meet the spec semantics, so NEXT VALUE FOR as part of the current patch
> > seems "out".
>
> Well, AFAICT, the only part of the spec we cannot implement is what you
> quote above. Therefore, why can't we support NEXT VALUE FOR seqname and
> reject table creation/alteration which would add more than one reference
> to the same sequence. That will allow us to avoid an intermediate step
> in getting to the SQL2003 syntax. Having to support three different
> sequence incrementation mechanisms for three flavours of PostgreSQL is
> going to be a real PITA.

Well, that is an _excellent_ point.  We would have three mechanisms,
which is confusing.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Gavin Sherry wrote:
> > I don't think we should use the spec syntax until we're prepared to
> > meet the spec semantics, so NEXT VALUE FOR as part of the current patch
> > seems "out".
>
> Well, AFAICT, the only part of the spec we cannot implement is what you
> quote above. Therefore, why can't we support NEXT VALUE FOR seqname and
> reject table creation/alteration which would add more than one reference
> to the same sequence. That will allow us to avoid an intermediate step
> in getting to the SQL2003 syntax. Having to support three different
> sequence incrementation mechanisms for three flavours of PostgreSQL is
> going to be a real PITA.

Oh, if we went in that direction, how would we fix loading previous
dumps?  Isn't NEXT VALUE for going to map internally to the
early-binding version of nextval().  I am thinking mucking with
nextval() and its casts is going to be required for ALTER SCHEMA RENAME
to work with any kind of reliability.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Gavin Sherry <swm@linuxworld.com.au> writes:
> Well, AFAICT, the only part of the spec we cannot implement is what you
> quote above. Therefore, why can't we support NEXT VALUE FOR seqname and
> reject table creation/alteration which would add more than one reference
> to the same sequence.

And how are you going to determine whether a query (not a table
definition) contains more than one NEXT VALUE FOR the same sequence?
Bear in mind some of them could be hidden down inside views or
functions.

            regards, tom lane

Here's an updated version of the patch.  There's now just one nextval()
function, taking regclass, and backwards compatibility is handled
through an implicit text-to-regclass cast.  Existing dumps will not see
any behavioral changes because nextval('foo') will be dumped as
nextval('foo'::text), but new entries of "nextval('foo')" will be
captured as regclass constants instead.

I noted that this version caused a couple more regression tests to fail;
for instance, the constraints test was expecting that it could drop and
recreate a sequence that was referenced by a default expression spelled
as "nextval('foo')".  So we are paying for improved ease of use by
taking a larger backwards-compatibility risk than the original patch
did.

Last call for objections ...

            regards, tom lane

Tom Lane wrote:
> Here's an updated version of the patch.  There's now just one nextval()
> function, taking regclass, and backwards compatibility is handled
> through an implicit text-to-regclass cast.  Existing dumps will not see
> any behavioral changes because nextval('foo') will be dumped as
> nextval('foo'::text), but new entries of "nextval('foo')" will be
> captured as regclass constants instead.
>
> I noted that this version caused a couple more regression tests to fail;
> for instance, the constraints test was expecting that it could drop and
> recreate a sequence that was referenced by a default expression spelled
> as "nextval('foo')".  So we are paying for improved ease of use by
> taking a larger backwards-compatibility risk than the original patch
> did.
>
> Last call for objections ...

No objection, but +1 from me. If this is the best solution people can
agree on, better now than later. The missing dependencies for sequences
were a bug in the first place, IMHO.

Best Regards,
Michael Paesold