Thread: Re: [PATCHES] Proposed patch for sequence-renaming problems
Tom Lane wrote:
> > It is
> > only the last one where recommending regclass helps, but is it worth
> > improving sequence/schema renaming by exposing and recommending a
> > ::regclass syntax that will go away as soon as we fix this properly?
>
> Please explain what you think a "proper" fix is. I think this patch is
> a proper fix. I see no better alternative that we might implement
> later.
>
> The only other thing that's been discussed is the SQL2003 syntax
> NEXT VALUE FOR sequencename
> but this is in fact just syntactic sugar for something functionally
> equivalent to nextval('sequencename'::regclass). It cannot completely
> replace all uses of the nextval function, because only a constant table
> name can appear.
>
> Hmm ... given the proposed patch, it would indeed take only a few more
> lines in gram.y to support the NEXT VALUE FOR syntax ...
Just to follow up, I agree we can't totally replace all instances of
nextval() with regclass because regclass requires a constant string, but
I would like to have the regclass behavior with simple syntax and
require people who want "late binding" of the sequence name to use some
extra syntax, like ::text or something. This seems like the only way
sequence naming will be sustainable from release to release. Saying
"use ::regclass" over and over again, when 99% of users should be using
it for nextval in default clauses, is going to get very tiring.
The other question is whether we should be playing with this at all
during beta. Should we just disable ALTER SCHEMA RENAME and return to
this during 8.2? I am worried these side missions will delay our final
release of 8.1.
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square,
Pennsylvania19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Just to follow up, I agree we can't totally replace all instances of
> nextval() with regclass because regclass requires a constant string, but
> I would like to have the regclass behavior with simple syntax and
> require people who want "late binding" of the sequence name to use some
> extra syntax, like ::text or something.
That would require a considerably more invasive change, AFAICS: remove
the text-input version of nextval() and introduce an implicit coercion
from text to regclass to avoid breaking existing dumps. I'd prefer not
to mess with that during beta, because there'd be nontrivial risk of
breaking existing behaviors. Because the proposed patch just adds on
new functions and doesn't change the behavior of existing DEFAULT
clauses, I don't think it can break anything.
However, we could certainly add the NEXT VALUE FOR syntax if that will
satisfy your concern about syntax.
> The other question is whether we should be playing with this at all
> during beta. Should we just disable ALTER SCHEMA RENAME and return to
> this during 8.2? I am worried these side missions will delay our final
> release of 8.1.
I'm prepared to argue that this is a bug fix, not only for ALTER SCHEMA
RENAME but for some very long-standing problems with renaming of
sequences. As I said before, you are seriously mistaken to consider
that disabling ALTER SCHEMA RENAME would eliminate the problem.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Just to follow up, I agree we can't totally replace all instances of > > nextval() with regclass because regclass requires a constant string, but > > I would like to have the regclass behavior with simple syntax and > > require people who want "late binding" of the sequence name to use some > > extra syntax, like ::text or something. > > That would require a considerably more invasive change, AFAICS: remove > the text-input version of nextval() and introduce an implicit coercion > from text to regclass to avoid breaking existing dumps. I'd prefer not > to mess with that during beta, because there'd be nontrivial risk of > breaking existing behaviors. Because the proposed patch just adds on > new functions and doesn't change the behavior of existing DEFAULT > clauses, I don't think it can break anything. > > However, we could certainly add the NEXT VALUE FOR syntax if that will > satisfy your concern about syntax. I am personally fine with use ::regclass internally, especially for SERIAL. It is documenting its use (and recommending it) that has me concerned. We are placing additional burdens on users --- burdens that will not exist in 8.2 when we have more time to fix it right. Is it worth telling users to use ::regclass in their code for 8.1 just to fix this, and then telling them in 8.2 it is not necessary to use this? > > The other question is whether we should be playing with this at all > > during beta. Should we just disable ALTER SCHEMA RENAME and return to > > this during 8.2? I am worried these side missions will delay our final > > release of 8.1. > > I'm prepared to argue that this is a bug fix, not only for ALTER SCHEMA > RENAME but for some very long-standing problems with renaming of > sequences. As I said before, you are seriously mistaken to consider > that disabling ALTER SCHEMA RENAME would eliminate the problem. If it was that bad, we should have fixed it during development, not during beta. The only reason it is getting attention now is because it is triggered more by a new feature we are adding, a feature we can easily remove. I know we both don't want to open up the entire TODO list for fixing during beta, especially fixing that isn't 100% complete and who's user-visible behavior will change in the next major release. Now, if we use ::regclass internally for just SERIAL, and don't document its use for sequences (or at last minimize its visibility), or we add NEXT VALUE FOR support and tell everyone to use that, that is fine with me because it is probably the best way for users to use this in defaults for all future releases. Am I correct that NEXT VALUE FOR is behavior which will be feature-complete and will be the recommended way to use sequences in defaults in all future releases? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Tom Lane wrote: > However, we could certainly add the NEXT VALUE FOR syntax if that will > satisfy your concern about syntax. Since the NEXT VALUE FOR syntax has a special meaning, would it be better to support the oracle-style syntax sequence.nextval for now (and use the ::regclass for this)? I am not sure how easy that is considering schema.sequence.nextval. Just a thought. Best Regards, Michael Paesold
Michael Paesold wrote: > Tom Lane wrote: > > However, we could certainly add the NEXT VALUE FOR syntax if that will > > satisfy your concern about syntax. > > Since the NEXT VALUE FOR syntax has a special meaning, would it be better to > support the oracle-style syntax sequence.nextval for now (and use the > ::regclass for this)? I am not sure how easy that is considering > schema.sequence.nextval. Yes, that is the direction I thought we were going. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Michael Paesold wrote:
>> Since the NEXT VALUE FOR syntax has a special meaning, would it be better to
>> support the oracle-style syntax sequence.nextval for now (and use the
>> ::regclass for this)? I am not sure how easy that is considering
>> schema.sequence.nextval.
> Yes, that is the direction I thought we were going.
We are further away than ever from being able to support that:
regression=# select seq.nextval;
ERROR: missing FROM-clause entry for table "seq"
Given that that proposal has been on the TODO list for years, and no one
has ever offered any workable way to implement it, I think waiting until
a way appears is equivalent to saying none of this will ever get fixed.
I'm not prepared to accept "fix it in 8.2" unless you can present an
implementation plan that can fix it in 8.2, and "use the Oracle syntax"
isn't a plan.
Moreover, providing a regclass-based nextval function doesn't foreclose
us from supporting the Oracle syntax if someone does have a bright idea
about it.
regards, tom lane
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/
Peter Eisentraut <peter_e@gmx.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
Tom Lane wrote: > Peter Eisentraut <peter_e@gmx.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@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:
>> 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@candle.pha.pa.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 dependencyo it allows old dumps to load with no change in behavioro it allows new nextval() calls
tohave 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@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:
>> 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@candle.pha.pa.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.adsrcFROM pg_namespace n, pg_class c, pg_attribute a, pg_attrdef dWHERE
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@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 wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman@candle.pha.pa.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.adsrcFROM pg_namespace n, pg_class c, pg_attribute a, pg_attrdef dWHERE
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@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:
> 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 likenextval(('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