Proposed patch for sequence-renaming problems

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

Tom Lane wrote:

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.

::regclass just seems too low-level to be something we should recommend.
We have tried to move away from :: casts in the default clauses. What
really concerns me is that for the most common case, where you want oid
binding at object creation time, you need to use ::regclass, while
ideally we would have the ::regclass behavior be the default. (I don't
have problems with people seeing ::regclass after the object is created,
and I think it does help clarify that is it not a string that is stored.

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.

True, but the rename of the schema case would work.

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 ...

Yes, I was thinking of something cleaner-looking. I have no trouble
fixing ALTER SCHEMA RENAME, but I would like it to be something that is
well thought out that will last unchanged from release to release,
rather than something hastily done. Just the fact you are considering
making ::regclass the default for nextval() in a later release means it
isn't a long-term solution, in terms of syntax that the user has to use
_now_, but not later.

-- 
  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

Tom Lane wrote:

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 ...

Just to follow up, I agree we can't totally replace all instances of
nextval() with regclass because regclass requires a constant string, but
I would like to have the regclass behavior with simple syntax and
require people who want "late binding" of the sequence name to use some
extra syntax, like ::text or something. This seems like the only way
sequence naming will be sustainable from release to release. Saying
"use ::regclass" over and over again, when 99% of users should be using
it for nextval in default clauses, is going to get very tiring.

The other question is whether we should be playing with this at all
during beta. Should we just disable ALTER SCHEMA RENAME and return to
this during 8.2? I am worried these side missions will delay our final
release of 8.1.

-- 
  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:

Just to follow up, I agree we can't totally replace all instances of
nextval() with regclass because regclass requires a constant string, but
I would like to have the regclass behavior with simple syntax and
require people who want "late binding" of the sequence name to use some
extra syntax, like ::text or something.

That would require a considerably more invasive change, AFAICS: remove
the text-input version of nextval() and introduce an implicit coercion
from text to regclass to avoid breaking existing dumps. I'd prefer not
to mess with that during beta, because there'd be nontrivial risk of
breaking existing behaviors. Because the proposed patch just adds on
new functions and doesn't change the behavior of existing DEFAULT
clauses, I don't think it can break anything.

However, we could certainly add the NEXT VALUE FOR syntax if that will
satisfy your concern about syntax.

The other question is whether we should be playing with this at all
during beta. Should we just disable ALTER SCHEMA RENAME and return to
this during 8.2? I am worried these side missions will delay our final
release of 8.1.

I'm prepared to argue that this is a bug fix, not only for ALTER SCHEMA
RENAME but for some very long-standing problems with renaming of
sequences. As I said before, you are seriously mistaken to consider
that disabling ALTER SCHEMA RENAME would eliminate the problem.

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:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Just to follow up, I agree we can't totally replace all instances of
nextval() with regclass because regclass requires a constant string, but
I would like to have the regclass behavior with simple syntax and
require people who want "late binding" of the sequence name to use some
extra syntax, like ::text or something.

That would require a considerably more invasive change, AFAICS: remove
the text-input version of nextval() and introduce an implicit coercion
from text to regclass to avoid breaking existing dumps. I'd prefer not
to mess with that during beta, because there'd be nontrivial risk of
breaking existing behaviors. Because the proposed patch just adds on
new functions and doesn't change the behavior of existing DEFAULT
clauses, I don't think it can break anything.

However, we could certainly add the NEXT VALUE FOR syntax if that will
satisfy your concern about syntax.

I am personally fine with use ::regclass internally, especially for
SERIAL. It is documenting its use (and recommending it) that has me
concerned. We are placing additional burdens on users --- burdens that
will not exist in 8.2 when we have more time to fix it right.

Is it worth telling users to use ::regclass in their code for 8.1 just
to fix this, and then telling them in 8.2 it is not necessary to use
this?

The other question is whether we should be playing with this at all
during beta. Should we just disable ALTER SCHEMA RENAME and return to
this during 8.2? I am worried these side missions will delay our final
release of 8.1.

I'm prepared to argue that this is a bug fix, not only for ALTER SCHEMA
RENAME but for some very long-standing problems with renaming of
sequences. As I said before, you are seriously mistaken to consider
that disabling ALTER SCHEMA RENAME would eliminate the problem.

If it was that bad, we should have fixed it during development, not
during beta. The only reason it is getting attention now is because it
is triggered more by a new feature we are adding, a feature we can
easily remove.

I know we both don't want to open up the entire TODO list for fixing
during beta, especially fixing that isn't 100% complete and who's
user-visible behavior will change in the next major release.

Now, if we use ::regclass internally for just SERIAL, and don't document
its use for sequences (or at last minimize its visibility), or we add
NEXT VALUE FOR support and tell everyone to use that, that is fine with
me because it is probably the best way for users to use this in
defaults for all future releases.

Am I correct that NEXT VALUE FOR is behavior which will be
feature-complete and will be the recommended way to use sequences in
defaults in all future releases?

-- 
  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

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:

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".

True, but your first guess was that none of this could be fixed in 8.2,
then you proposed a 50% fix that was user-visible. Given those options,
I do prefer removal of a minor feature.

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.

I don't like the val/value distinction (the added "ue" means what?).
Perhaps next_seq/curr_seq/set_seq would work more cleanly. I never
liked that the function names had no reference to "seq"uence in them.

Didn't next_val() come from Oracle? Does it make sense to make new
non-Oracle compatible commands for this, especially since Oracle
probably does early binding? What would make more sense perhaps would
be for next_val to do early binding, and a new function do late binding,
perhaps next_val_str().

-- 
  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

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

Tom Lane wrote:

However, we could certainly add the NEXT VALUE FOR syntax if that will
satisfy your concern about syntax.

Since the NEXT VALUE FOR syntax has a special meaning, would it be better to
support the oracle-style syntax sequence.nextval for now (and use the
::regclass for this)? I am not sure how easy that is considering
schema.sequence.nextval.

Just a thought.

Best Regards,
Michael Paesold

Michael Paesold wrote:

Tom Lane wrote:

However, we could certainly add the NEXT VALUE FOR syntax if that will
satisfy your concern about syntax.

Since the NEXT VALUE FOR syntax has a special meaning, would it be better to
support the oracle-style syntax sequence.nextval for now (and use the
::regclass for this)? I am not sure how easy that is considering
schema.sequence.nextval.

Yes, that is the direction I thought we were going.

-- 
  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:

Michael Paesold wrote:

Since the NEXT VALUE FOR syntax has a special meaning, would it be better to
support the oracle-style syntax sequence.nextval for now (and use the
::regclass for this)? I am not sure how easy that is considering
schema.sequence.nextval.

Yes, that is the direction I thought we were going.

We are further away than ever from being able to support that:

regression=# select seq.nextval;
ERROR: missing FROM-clause entry for table "seq"

Given that that proposal has been on the TODO list for years, and no one
has ever offered any workable way to implement it, I think waiting until
a way appears is equivalent to saying none of this will ever get fixed.
I'm not prepared to accept "fix it in 8.2" unless you can present an
implementation plan that can fix it in 8.2, and "use the Oracle syntax"
isn't a plan.

Moreover, providing a regclass-based nextval function doesn't foreclose
us from supporting the Oracle syntax if someone does have a bright idea
about it.

regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:

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.

I don't like the val/value distinction (the added "ue" means what?).
Perhaps next_seq/curr_seq/set_seq would work more cleanly. I never
liked that the function names had no reference to "seq"uence in them.

That doesn't really respond to the "means what?" question --- which of
"nextval" and "next_seq" is the early binding form, and how do you
remember? For that matter, how do you even remember that they're
related? Still, I have no strong objection to those names, and am happy
to go with them if that will resolve the discussion.

Didn't next_val() come from Oracle? Does it make sense to make new
non-Oracle compatible commands for this, especially since Oracle
probably does early binding? What would make more sense perhaps would
be for next_val to do early binding, and a new function do late binding,
perhaps next_val_str().

We already have the function to do late binding, namely nextval(text).
I see no percentage in inventing some random new name for a function
that's been there forever --- unless the new name adheres to some
standard, which these don't.

regards, tom lane