Обсуждение: Re: [PATCHES] Proposed patch for sequence-renaming problems

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

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

Tom Lane wrote:
> 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.

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

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

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@candle.pha.pa.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@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
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend
> 

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

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

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

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

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.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@candle.pha.pa.us               |  (610)
359-1001+  If your life is a hard drive,     |  13 Roberts Road +  Christ can be your backup.        |  Newtown Square,
Pennsylvania19073
 

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

Bruce Momjian <pgman@candle.pha.pa.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:
> Bruce Momjian <pgman@candle.pha.pa.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@candle.pha.pa.us               |  (610)
359-1001+  If your life is a hard drive,     |  13 Roberts Road +  Christ can be your backup.        |  Newtown Square,
Pennsylvania19073