Re: Proposed p.tch 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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)alvh(dot)no-ip(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)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.

I am thinking we need to have nextval('') do early binding and have
nextval(''::text) (or some other name) do late binding. The fact is
that 99% of users would prefer early binding, is my guess.

Also, when there is no standard, Oracle is the standard, and for Oracle,
nextval is early binding.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us> writes:
> I am thinking we need to have nextval('') do early binding and have
> nextval(''::text) (or some other name) do late binding.

You can't have that in exactly that form, because text is invariably
the preferred resolution of unknown-type literals, and we certainly
dare not tinker with that rule. There is therefore no way that the
above two syntaxes are going to act differently. If we were willing to
change the name of the existing nextval functionality, we could have,
say,

nextval(regclass)
nextval_late(text)

where the latter is the new spelling of the existing function.
To make this work without breaking existing dumps (which will all say
"nextval('foo'::text)" it'd be necessary to introduce an implicit cast
from text to regclass. That scares me a fair amount --- cross category
implicit casts are generally evil. However, it might be OK given that
there are so few functions that take regclass arguments.

This still wouldn't put us in a place where existing dumps are
automatically fixed up during import. We'd parse the expressions as
nextval('foo'::text::regclass), which will work but it's effectively
still late-binding --- the actual constant is just text not regclass.
There's no way to fold it down to 'foo'::regclass automatically because
(1) we don't do constant-folding before storing expressions, and (2)
even if we did, the text to regclass cast cannot be marked immutable
(it's only stable). So people would still have to go through and change
their schemas by hand to get to the early-binding behavior.

Given all that, it seems the better part of valor to leave nextval()
as-is: there's too much risk and too little reward in that path. The
next best alternative is to use some other name than nextval for the
early-binding form, and to encourage people to move to the new name.
Same amount of manual schema-updating, much less risk of breaking existing
code due to unforeseen side-effects, much less confusion about what does
which.

BTW, I've gone back to thinking that next_value is the best alternative
spelling, because it calls to mind the SQL standard syntax NEXT VALUE
FOR (which I assume we'll want to support eventually).

> Also, when there is no standard, Oracle is the standard, and for Oracle,
> nextval is early binding.

Oracle does not spell nextval in any of these ways, so that argument
carries little weight. If we were using exactly the Oracle syntax, then
sure, but we're not. Also, we have to put some weight on backward
compatibility with our own past practice.

So on the whole I like leaving nextval() as-is and introducing a
separate function next_value(regclass).

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I am thinking we need to have nextval('') do early binding and have
> > nextval(''::text) (or some other name) do late binding.
>
> You can't have that in exactly that form, because text is invariably
> the preferred resolution of unknown-type literals, and we certainly
> dare not tinker with that rule. There is therefore no way that the
> above two syntaxes are going to act differently. If we were willing to
> change the name of the existing nextval functionality, we could have,
> say,
>
> nextval(regclass)
> nextval_late(text)

This is the first proposal I like. 99% of users think that nextval() is
doing early binding (or never thought of it), so I think moving to that
syntax is a win. Is late/dynamic/string/virtual the right suffix?

> where the latter is the new spelling of the existing function.
> To make this work without breaking existing dumps (which will all say
> "nextval('foo'::text)" it'd be necessary to introduce an implicit cast
> from text to regclass. That scares me a fair amount --- cross category
> implicit casts are generally evil. However, it might be OK given that
> there are so few functions that take regclass arguments.
>
> This still wouldn't put us in a place where existing dumps are
> automatically fixed up during import. We'd parse the expressions as
> nextval('foo'::text::regclass), which will work but it's effectively
> still late-binding --- the actual constant is just text not regclass.
> There's no way to fold it down to 'foo'::regclass automatically because
> (1) we don't do constant-folding before storing expressions, and (2)
> even if we did, the text to regclass cast cannot be marked immutable
> (it's only stable). So people would still have to go through and change
> their schemas by hand to get to the early-binding behavior.

I am thinking we should hard-code something in the backend so if the
function oid is nextval/currval/setval, we strip off any text casting
internally. These functions are already pretty special in their usage
so I don't see a problem in fixing it this way.

Ideally we could do a test in the grammar and strip off the ::text
there.

> Given all that, it seems the better part of valor to leave nextval()
> as-is: there's too much risk and too little reward in that path. The
> next best alternative is to use some other name than nextval for the
> early-binding form, and to encourage people to move to the new name.
> Same amount of manual schema-updating, much less risk of breaking existing
> code due to unforeseen side-effects, much less confusion about what does
> which.
>
> BTW, I've gone back to thinking that next_value is the best alternative
> spelling, because it calls to mind the SQL standard syntax NEXT VALUE
> FOR (which I assume we'll want to support eventually).

True, but it doesn't have the standard behavior. Would we change that
when we add NEXT VALUE?

> > Also, when there is no standard, Oracle is the standard, and for Oracle,
> > nextval is early binding.
>
> Oracle does not spell nextval in any of these ways, so that argument
> carries little weight. If we were using exactly the Oracle syntax, then
> sure, but we're not. Also, we have to put some weight on backward
> compatibility with our own past practice.
>
> So on the whole I like leaving nextval() as-is and introducing a
> separate function next_value(regclass).

I disagree. nextval() is too embedded in people's thinking to make them
change when we have the ability to have it do the _right_ _thing_, and
give a "dynamic" alternative for those you need it.

Also, Oracle does use nextval as my_docs_seq.nextval so the use of that
keyword is a good policy to continue.

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

Just to summarize, I am arguing from a usability perspective, because I
believe the simplest API is one that will last for many releases and not
have to be redesigned, nor require too much adjustment from our users.

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

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > I am thinking we need to have nextval('') do early binding and have
> > > nextval(''::text) (or some other name) do late binding.
> >
> > You can't have that in exactly that form, because text is invariably
> > the preferred resolution of unknown-type literals, and we certainly
> > dare not tinker with that rule. There is therefore no way that the
> > above two syntaxes are going to act differently. If we were willing to
> > change the name of the existing nextval functionality, we could have,
> > say,
> >
> > nextval(regclass)
> > nextval_late(text)
>
> This is the first proposal I like. 99% of users think that nextval() is
> doing early binding (or never thought of it), so I think moving to that
> syntax is a win. Is late/dynamic/string/virtual the right suffix?
>
> > where the latter is the new spelling of the existing function.
> > To make this work without breaking existing dumps (which will all say
> > "nextval('foo'::text)" it'd be necessary to introduce an implicit cast
> > from text to regclass. That scares me a fair amount --- cross category
> > implicit casts are generally evil. However, it might be OK given that
> > there are so few functions that take regclass arguments.
> >
> > This still wouldn't put us in a place where existing dumps are
> > automatically fixed up during import. We'd parse the expressions as
> > nextval('foo'::text::regclass), which will work but it's effectively
> > still late-binding --- the actual constant is just text not regclass.
> > There's no way to fold it down to 'foo'::regclass automatically because
> > (1) we don't do constant-folding before storing expressions, and (2)
> > even if we did, the text to regclass cast cannot be marked immutable
> > (it's only stable). So people would still have to go through and change
> > their schemas by hand to get to the early-binding behavior.
>
> I am thinking we should hard-code something in the backend so if the
> function oid is nextval/currval/setval, we strip off any text casting
> internally. These functions are already pretty special in their usage
> so I don't see a problem in fixing it this way.
>
> Ideally we could do a test in the grammar and strip off the ::text
> there.
>
> > Given all that, it seems the better part of valor to leave nextval()
> > as-is: there's too much risk and too little reward in that path. The
> > next best alternative is to use some other name than nextval for the
> > early-binding form, and to encourage people to move to the new name.
> > Same amount of manual schema-updating, much less risk of breaking existing
> > code due to unforeseen side-effects, much less confusion about what does
> > which.
> >
> > BTW, I've gone back to thinking that next_value is the best alternative
> > spelling, because it calls to mind the SQL standard syntax NEXT VALUE
> > FOR (which I assume we'll want to support eventually).
>
> True, but it doesn't have the standard behavior. Would we change that
> when we add NEXT VALUE?
>
> > > Also, when there is no standard, Oracle is the standard, and for Oracle,
> > > nextval is early binding.
> >
> > Oracle does not spell nextval in any of these ways, so that argument
> > carries little weight. If we were using exactly the Oracle syntax, then
> > sure, but we're not. Also, we have to put some weight on backward
> > compatibility with our own past practice.
> >
> > So on the whole I like leaving nextval() as-is and introducing a
> > separate function next_value(regclass).
>
> I disagree. nextval() is too embedded in people's thinking to make them
> change when we have the ability to have it do the _right_ _thing_, and
> give a "dynamic" alternative for those you need it.
>
> Also, Oracle does use nextval as my_docs_seq.nextval so the use of that
> keyword is a good policy to continue.
>
> --
> Bruce Momjian | http://candle.pha.pa.us
> pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
> + If your life is a hard drive, | 13 Roberts Road
> + Christ can be your backup. | Newtown Square, Pennsylvania 19073
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us> writes:
> I am thinking we should hard-code something in the backend so if the
> function oid is nextval/currval/setval, we strip off any text casting
> internally.

NO. No bloody way ... that is far dirtier than any other proposal
that's been made in this thread. I don't even want to think about
what strange corner-case semantics that might create.

>> So on the whole I like leaving nextval() as-is and introducing a
>> separate function next_value(regclass).

> I disagree. nextval() is too embedded in people's thinking to make them
> change

Why? And what's your evidence for this? You could equally well argue
that the fact that nextval takes a text argument is too embedded to
change.

> when we have the ability to have it do the _right_ _thing_,

We have no ability to make it do what you think is the right thing,
at least not without introducing kluges that are certain to come back
to haunt us.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I am thinking we should hard-code something in the backend so if the
> > function oid is nextval/currval/setval, we strip off any text casting
> > internally.
>
> NO. No bloody way ... that is far dirtier than any other proposal
> that's been made in this thread. I don't even want to think about
> what strange corner-case semantics that might create.

Well, it would be

if ((oid == xxx || oid == yyy) && cast_exists)
remove cast;

Seems safe to me.

> >> So on the whole I like leaving nextval() as-is and introducing a
> >> separate function next_value(regclass).
>
> > I disagree. nextval() is too embedded in people's thinking to make them
> > change
>
> Why? And what's your evidence for this? You could equally well argue
> that the fact that nextval takes a text argument is too embedded to
> change.

99% of people using nextval think (or don't care) that it is early
binding. I see no reason to re-educate people just to keep nextval() as
late binding.

> > when we have the ability to have it do the _right_ _thing_,
>
> We have no ability to make it do what you think is the right thing,
> at least not without introducing kluges that are certain to come back
> to haunt us.

Well, then, let's leave it all for 8.2 where we can discuss/test and
come up with a plan.

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

Also, why is the nextval ::text casting output by pg_dump anyway?

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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I am thinking we should hard-code something in the backend so if the
> > function oid is nextval/currval/setval, we strip off any text casting
> > internally.
>
> NO. No bloody way ... that is far dirtier than any other proposal
> that's been made in this thread. I don't even want to think about
> what strange corner-case semantics that might create.
>
> >> So on the whole I like leaving nextval() as-is and introducing a
> >> separate function next_value(regclass).
>
> > I disagree. nextval() is too embedded in people's thinking to make them
> > change
>
> Why? And what's your evidence for this? You could equally well argue
> that the fact that nextval takes a text argument is too embedded to
> change.
>
> > when we have the ability to have it do the _right_ _thing_,
>
> We have no ability to make it do what you think is the right thing,
> at least not without introducing kluges that are certain to come back
> to haunt us.
>
> regards, tom lane
>

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

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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Am Mittwoch, 28. September 2005 18:10 schrieb Bruce Momjian:
> 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.

That was my thinking. The issue has probably been there since 7.3. I don't
think we need to shove in a solution now, especially when there is so much
disagreement about the behavior.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On Wed, Sep 28, 2005 at 10:41:12PM -0400, Bruce Momjian wrote:
>
> Also, why is the nextval ::text casting output by pg_dump anyway?

AFAICS, pg_dump outputs "serial" (at least in 7.4.7 which is what I
have to hand) when it should meaning that dumps restored will get the
new syntax anyway. Or am I missing something?

Isn't this what adddepend was for? I can beleive there are version
dependancies here...
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

Martijn van Oosterhout wrote:
-- Start of PGP signed section.
> On Wed, Sep 28, 2005 at 10:41:12PM -0400, Bruce Momjian wrote:
> >
> > Also, why is the nextval ::text casting output by pg_dump anyway?
>
> AFAICS, pg_dump outputs "serial" (at least in 7.4.7 which is what I
> have to hand) when it should meaning that dumps restored will get the
> new syntax anyway. Or am I missing something?
>
> Isn't this what adddepend was for? I can beleive there are version
> dependancies here...

Right, we think we can get SERIAL to work fine. It is manual use of
nextval() in DEFAULT that we are worried about, and there is probably
not too much use of that. There is no dependency info about that.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)linuxworld(dot)com(dot)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

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> That was my thinking. The issue has probably been there since 7.3. I don't
> think we need to shove in a solution now, especially when there is so much
> disagreement about the behavior.

Well, we have a new issue that has made the problem much worse (ie ALTER
SCHEMA RENAME), and these problems are not going to get any easier to
solve later. I think we should agree on something and do it.

Ripping out ALTER SCHEMA RENAME is not a solution unless you have a path
to a solution later with more work. So far there has been nothing in
the way of "here is a proposal that will work but it'll take too much
time to implement for 8.1". Eventually we are going to have to settle
on one of the lesser evils, so why not now?

regards, tom lane

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> This still wouldn't put us in a place where existing dumps are
>> automatically fixed up during import. We'd parse the expressions as
>> nextval('foo'::text::regclass), which will work but it's effectively
>> still late-binding --- the actual constant is just text not regclass.
>> There's no way to fold it down to 'foo'::regclass automatically

> I am thinking we should hard-code something in the backend so if the
> function oid is nextval/currval/setval, we strip off any text casting
> internally. These functions are already pretty special in their usage
> so I don't see a problem in fixing it this way.

You have not thought about it hard. We cannot do that without breaking
existing dumps. Consider

create sequence seq;

create table foo (f1 int default nextval('seq'::text));

In current releases, there is no dependency from foo to seq and
therefore pg_dump could dump the above two objects in either order.
(With the names I used in the example, recent pg_dumps would in
fact choose to dump the table first.) If we try to force the 'seq'
literal into regclass form then the script will fail, because the
seq relation does not exist yet.

I think people missed the significance of the part of my patch that
created dependencies for regclass literals. Without that, it really
just doesn't work.

I see no prospect that we can have a transparent migration to
nextval(regclass) without breaking things left and right. Therefore
I still feel that the best solution is to introduce new functions
with different names. That has zero chance of breaking anything
that works now. If people don't want to migrate, well, they don't
have to.

regards, tom lane

Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > That was my thinking. The issue has probably been there since 7.3. I don't
> > think we need to shove in a solution now, especially when there is so much
> > disagreement about the behavior.
>
> Well, we have a new issue that has made the problem much worse (ie ALTER
> SCHEMA RENAME), and these problems are not going to get any easier to
> solve later. I think we should agree on something and do it.
>
> Ripping out ALTER SCHEMA RENAME is not a solution unless you have a path
> to a solution later with more work. So far there has been nothing in
> the way of "here is a proposal that will work but it'll take too much
> time to implement for 8.1". Eventually we are going to have to settle
> on one of the lesser evils, so why not now?

Well, we are only giving ourselves a few weeks to solve this, and I
think a hack to make it work cleanly for users is better than supporting
two function names perpetually.

Remember the now, now(), CURRENT_TIMESTAMP issue of early binding. It
is still confusing to remember which is which, and doing it for
sequences new function names is confusing too.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> This still wouldn't put us in a place where existing dumps are
> >> automatically fixed up during import. We'd parse the expressions as
> >> nextval('foo'::text::regclass), which will work but it's effectively
> >> still late-binding --- the actual constant is just text not regclass.
> >> There's no way to fold it down to 'foo'::regclass automatically
>
> > I am thinking we should hard-code something in the backend so if the
> > function oid is nextval/currval/setval, we strip off any text casting
> > internally. These functions are already pretty special in their usage
> > so I don't see a problem in fixing it this way.
>
> You have not thought about it hard. We cannot do that without breaking
> existing dumps. Consider
>
> create sequence seq;
>
> create table foo (f1 int default nextval('seq'::text));
>
> In current releases, there is no dependency from foo to seq and
> therefore pg_dump could dump the above two objects in either order.
> (With the names I used in the example, recent pg_dumps would in
> fact choose to dump the table first.) If we try to force the 'seq'
> literal into regclass form then the script will fail, because the
> seq relation does not exist yet.

Good point. That is why I liked having ::regclass and ::text versions
of nextval(), but prefer ::regclass _unless_ there is a ::text cast on
the function call. So, instead of removing the ::text cast (as you
said, a bad idea), let's hack up the precidence to prefer ::regclass
when there is no cast. In fact, since few functions take ::regclass,
could we make regclass prefered to text for all cases? That might be
cleaner.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> Ripping out ALTER SCHEMA RENAME is not a solution unless you have a path
>> to a solution later with more work.

> Well, we are only giving ourselves a few weeks to solve this, and I
> think a hack to make it work cleanly for users is better than supporting
> two function names perpetually.

Well, if you are dead set on having only one function name, then I think
the best solution is this:

* only one function, taking regclass

* add an implicit text-to-regclass coercion

With this, nextval('foo') is early binding and nextval('foo'::text) is
late binding, and existing dumps are going to continue to behave as late
binding unless changed manually.

The implicit coercion is a bit risky, but in practice these are likely
to be the only functions in the system that are declared to take
regclass, so the odds of the implicit coercion firing unexpectedly seem
low.

Does that sound like a workable compromise?

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> Ripping out ALTER SCHEMA RENAME is not a solution unless you have a path
> >> to a solution later with more work.
>
> > Well, we are only giving ourselves a few weeks to solve this, and I
> > think a hack to make it work cleanly for users is better than supporting
> > two function names perpetually.
>
> Well, if you are dead set on having only one function name, then I think
> the best solution is this:
>
> * only one function, taking regclass
>
> * add an implicit text-to-regclass coercion
>
> With this, nextval('foo') is early binding and nextval('foo'::text) is
> late binding, and existing dumps are going to continue to behave as late
> binding unless changed manually.
>
> The implicit coercion is a bit risky, but in practice these are likely
> to be the only functions in the system that are declared to take
> regclass, so the odds of the implicit coercion firing unexpectedly seem
> low.
>
> Does that sound like a workable compromise?

Personally, I _love_ it. I hope others do as well. :-)

Let me explain why I thought two function names would be confusing. We
have been telling people to use nextval() since we added sequences in
6.4, and since 99% of people would want early binding (with
dependencies), I think making them all move to a new function name would
be a long-running education effort. If it can be avoided, that is
better.

I think the solution you propose is great because:

o it fixes SERIAL dependency
o it allows old dumps to load with no change in behavior
o it allows new nextval() calls to have early binding, unless
::text is used

In fact, the use of ::text is dump is the only thing that is _allowing_
this migration idea to work.

I think I can easily explain this issue in the release notes, and show
how users can update their schemas to the new behavior.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> Does that sound like a workable compromise?

> Personally, I _love_ it. I hope others do as well. :-)

OK, I'll work up a patch.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> Does that sound like a workable compromise?
>
> > Personally, I _love_ it. I hope others do as well. :-)
>
> OK, I'll work up a patch.

Here is a query that shows nextval(::text) usage as defaults:

SELECT n.nspname, c.relname, a.attname, d.adsrc
FROM pg_namespace n, pg_class c, pg_attribute a, pg_attrdef d
WHERE n.oid = c.relnamespace AND
c.oid = a.attrelid AND
a.attrelid = d.adrelid AND
a.attnum = d.adnum AND
d.adsrc ~ '.*nextval\\(''[^'']*''::TEXT\\)';

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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 wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > Tom Lane wrote:
> > >> Does that sound like a workable compromise?
> >
> > > Personally, I _love_ it. I hope others do as well. :-)
> >
> > OK, I'll work up a patch.
>
> Here is a query that shows nextval(::text) usage as defaults:
>
> SELECT n.nspname, c.relname, a.attname, d.adsrc
> FROM pg_namespace n, pg_class c, pg_attribute a, pg_attrdef d
> WHERE n.oid = c.relnamespace AND
> c.oid = a.attrelid AND
> a.attrelid = d.adrelid AND
> a.attnum = d.adnum AND
> d.adsrc ~ '.*nextval\\(''[^'']*''::TEXT\\)';

Sorry, this is the right one:

SELECT n.nspname, c.relname, a.attname, d.adsrc
FROM pg_namespace n, pg_class c, pg_attribute a, pg_attrdef d
WHERE n.oid = c.relnamespace AND
c.oid = a.attrelid AND
a.attrelid = d.adrelid AND
a.attnum = d.adnum AND
d.adsrc ~ '.*nextval\\(''[^'']*''::text\\)';

Followed by the appropriate:

ALTER TABLE sp.test2 ALTER COLUMN x DROP DEFAULT;

ALTER TABLE sp.test2 ALTER COLUMN x SET DEFAULT nextval('sp.aa');

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

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

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Sorry, this is the right one:

> SELECT n.nspname, c.relname, a.attname, d.adsrc
> FROM pg_namespace n, pg_class c, pg_attribute a, pg_attrdef d
> WHERE n.oid = c.relnamespace AND
> c.oid = a.attrelid AND
> a.attrelid = d.adrelid AND
> a.attnum = d.adnum AND
> d.adsrc ~ '.*nextval\\(''[^'']*''::text\\)';

Doesn't actually work with the finished patch; the adsrc values look
more like
nextval(('seq'::text)::regclass)

> Followed by the appropriate:
> ALTER TABLE sp.test2 ALTER COLUMN x DROP DEFAULT;
> ALTER TABLE sp.test2 ALTER COLUMN x SET DEFAULT nextval('sp.aa');

AFAIK you don't need to bother with the DROP step.

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