Thread: CREATE SEQUENCE with RESTART option
Hi, It looks like we do allow $subject which has following behaviour: create sequence myseq restart 200; --> sequence is starting from restart value overriding start value create sequence myseq start 100 restart 200; --> sequence is starting from restart value overriding start value create sequence myseq start 100 restart; --> sequence is starting from start value no overriding of start value occurs create sequence myseq restart; --> sequence is starting from default start value no overriding of start value occurs While we have documented the "restart" option behaviour for ALTER SEQUENCE, we have no mention of it in the CREATE SEQUENCE docs page. Do we need to document the above behaviour for CREATE SEQUENCE? Alternatively, do we need to throw an error if the user is not supposed to use the "restart" option with CREATE SEQUENCE? IMO, allowing the "restart" option for CREATE SEQUENCE doesn't make sense when we have the "start" option, so it's better to throw an error. Thoughts? With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Wed, Apr 7, 2021 at 3:56 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> It looks like we do allow $subject which has following behaviour:
> create sequence myseq restart 200; --> sequence is starting from
> restart value overriding start value
> create sequence myseq start 100 restart 200; --> sequence is starting
> from restart value overriding start value
> create sequence myseq start 100 restart; --> sequence is starting from
> start value no overriding of start value occurs
> create sequence myseq restart; --> sequence is starting from default
> start value no overriding of start value occurs
>
> While we have documented the "restart" option behaviour for ALTER
> SEQUENCE, we have no mention of it in the CREATE SEQUENCE docs page.
> Do we need to document the above behaviour for CREATE SEQUENCE?
> Alternatively, do we need to throw an error if the user is not
> supposed to use the "restart" option with CREATE SEQUENCE?
>
> IMO, allowing the "restart" option for CREATE SEQUENCE doesn't make
> sense when we have the "start" option, so it's better to throw an
> error.
Using restart in CREATE SEQUENCE command looks, umm, funny. But
looking at the code it makes me wonder whether it's intentional.
1567 /* RESTART [WITH] */
1568 if (restart_value != NULL)
1569 {
1570 if (restart_value->arg != NULL)
1571 seqdataform->last_value = defGetInt64(restart_value);
1572 else
1573 seqdataform->last_value = seqform->seqstart;
1574 seqdataform->is_called = false;
1575 seqdataform->log_cnt = 0;
1576 }
1577 else if (isInit)
1578 {
1579 seqdataform->last_value = seqform->seqstart;
1580 seqdataform->is_called = false;
1581 }
"restart" as the name suggests "restarts" a sequence from a given
value or start of sequence. "start" on the other hand specifies the
"start" value of sequence and is also the value used to "restart" by
default from.
So here's what will happen in each of the cases you mentioned
> create sequence myseq restart 200; --> sequence is starting from
> restart value overriding start value
the first time sequence will be used it will use value 200, but if
someone does a "restart" it will start from default start of that
sequence.
> create sequence myseq start 100 restart 200; --> sequence is starting
> from restart value overriding start value
the first time sequence will be used it will use value 200, but if
someone does a "restart" it will start from 100
> create sequence myseq start 100 restart; --> sequence is starting from
> start value no overriding of start value occurs
the first time sequence will be used it will use value 100, and if
someone does a "restart" it will start from 100
> create sequence myseq restart; --> sequence is starting from default
> start value no overriding of start value occurs
this is equivalent to "create sequence myseq"
This is the behaviour implied when we read
https://www.postgresql.org/docs/13/sql-createsequence.html and
https://www.postgresql.org/docs/13/sql-altersequence.html together.
At best CREATE SEQUENCE .... START ... RESTART ... can be a shorthand
for CREATE SEQUENCE ... START; ALTER SEQUENCE ... RESTART run back to
back. So it looks useful but in rare cases.
Said all that I agree that if we are supporting CREATE SEQUENCE ...
RESTART then we should document it, correctly. If that's not the
intention, we should disallow RESTART with CREATE SEQUENCE.
--
Best Wishes,
Ashutosh Bapat
On Wed, Apr 7, 2021 at 6:04 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote: > At best CREATE SEQUENCE .... START ... RESTART ... can be a shorthand > for CREATE SEQUENCE ... START; ALTER SEQUENCE ... RESTART run back to > back. So it looks useful but in rare cases. I personally feel that let's not mix up START and RESTART in CREATE SEQUENCE. If required, users will run ALTER SEQUENCE RESTART separately, that will be a clean way. > Said all that I agree that if we are supporting CREATE SEQUENCE ... > RESTART then we should document it, correctly. If that's not the > intention, we should disallow RESTART with CREATE SEQUENCE. As I mentioned upthread, it's better to disallow (throw error) if RESTART is specified for CREATE SEQUENCE. Having said that, I would like to hear from others. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Wed, Apr 7, 2021 at 6:52 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Wed, Apr 7, 2021 at 6:04 PM Ashutosh Bapat > <ashutosh.bapat.oss@gmail.com> wrote: > > At best CREATE SEQUENCE .... START ... RESTART ... can be a shorthand > > for CREATE SEQUENCE ... START; ALTER SEQUENCE ... RESTART run back to > > back. So it looks useful but in rare cases. > > I personally feel that let's not mix up START and RESTART in CREATE > SEQUENCE. If required, users will run ALTER SEQUENCE RESTART > separately, that will be a clean way. > > > Said all that I agree that if we are supporting CREATE SEQUENCE ... > > RESTART then we should document it, correctly. If that's not the > > intention, we should disallow RESTART with CREATE SEQUENCE. > > As I mentioned upthread, it's better to disallow (throw error) if > RESTART is specified for CREATE SEQUENCE. Having said that, I would > like to hear from others. > FWIW, +1. The RESTART clause in the CREATE SEQUENCE doesn't make sense to me, it should be restricted, IMO. Regards, Amul
On Thu, Apr 8, 2021 at 10:09 AM Amul Sul <sulamul@gmail.com> wrote: > > On Wed, Apr 7, 2021 at 6:52 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > On Wed, Apr 7, 2021 at 6:04 PM Ashutosh Bapat > > <ashutosh.bapat.oss@gmail.com> wrote: > > > At best CREATE SEQUENCE .... START ... RESTART ... can be a shorthand > > > for CREATE SEQUENCE ... START; ALTER SEQUENCE ... RESTART run back to > > > back. So it looks useful but in rare cases. > > > > I personally feel that let's not mix up START and RESTART in CREATE > > SEQUENCE. If required, users will run ALTER SEQUENCE RESTART > > separately, that will be a clean way. > > > > > Said all that I agree that if we are supporting CREATE SEQUENCE ... > > > RESTART then we should document it, correctly. If that's not the > > > intention, we should disallow RESTART with CREATE SEQUENCE. > > > > As I mentioned upthread, it's better to disallow (throw error) if > > RESTART is specified for CREATE SEQUENCE. Having said that, I would > > like to hear from others. > > > > FWIW, +1. > > The RESTART clause in the CREATE SEQUENCE doesn't make sense > to me, it should be restricted, IMO. Thanks! Attaching a patch that throws an error if the RESTART option is specified with CREATE SEQUENCE. Please have a look and let me know if the error message wording is fine or not. Is it better to include the reason as to why we disallow something like "Because it may override the START option." in err_detail along with the error message? With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Attachment
On Thu, Apr 8, 2021 at 2:03 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>
> The RESTART clause in the CREATE SEQUENCE doesn't make sense
> to me, it should be restricted, IMO.
+1
Thanks! Attaching a patch that throws an error if the RESTART option
is specified with CREATE SEQUENCE. Please have a look and let me know
if the error message wording is fine or not. Is it better to include
the reason as to why we disallow something like "Because it may
override the START option." in err_detail along with the error
message?
Patch looks good to me. Current error message looks ok to me.
Do we need to add double quotes for RESTART word in the error message since it is an option?
--
On Thu, Apr 8, 2021 at 3:16 PM Suraj Kharage <suraj.kharage@enterprisedb.com> wrote: >> > The RESTART clause in the CREATE SEQUENCE doesn't make sense >> > to me, it should be restricted, IMO. > > +1 > >> >> Thanks! Attaching a patch that throws an error if the RESTART option >> is specified with CREATE SEQUENCE. Please have a look and let me know >> if the error message wording is fine or not. Is it better to include >> the reason as to why we disallow something like "Because it may >> override the START option." in err_detail along with the error >> message? > > > Patch looks good to me. Current error message looks ok to me. > Do we need to add double quotes for RESTART word in the error message since it is an option? Thanks for taking a look at the patch. Looks like the other options are used in the error message without quotes, see "MINVALUE (%s) is out of range for sequence data type "START value (%s) cannot be less than MINVALUE "RESTART value (%s) cannot be less than MINVALUE "CACHE (%s) must be greater than zero With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed Hi I have applied and run your patch, which works fine in my environment. Regarding your comments in the patch: /* * Restarting a sequence while defining it doesn't make any sense * and it may override the START value. Allowing both START and * RESTART option for CREATE SEQUENCE may cause confusion to user. * Hence, we throw error for CREATE SEQUENCE if RESTART option is * specified. However, it can be used with ALTER SEQUENCE. */ I would remove the first sentence, because it seems like a personal opinion to me. I am sure someone, somewhere may thinkit makes total sense :). I would rephrase like this: /* * Allowing both START and RESTART option for CREATE SEQUENCE * could override the START value and cause confusion to user. Hence, * we throw an error for CREATE SEQUENCE if RESTART option is * specified; it can only be used with ALTER SEQUENCE. */ just a thought. thanks! ------------------------------------- Cary Huang HighGo Software Canada www.highgo.ca
On Sat, Jul 24, 2021 at 3:20 AM Cary Huang <cary.huang@highgo.ca> wrote: > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: tested, passed > Documentation: tested, passed > > Hi > > I have applied and run your patch, which works fine in my environment. Regarding your comments in the patch: Thanks for the review. > /* > * Restarting a sequence while defining it doesn't make any sense > * and it may override the START value. Allowing both START and > * RESTART option for CREATE SEQUENCE may cause confusion to user. > * Hence, we throw error for CREATE SEQUENCE if RESTART option is > * specified. However, it can be used with ALTER SEQUENCE. > */ > > I would remove the first sentence, because it seems like a personal opinion to me. I am sure someone, somewhere may thinkit makes total sense :). Agree. > I would rephrase like this: > > /* > * Allowing both START and RESTART option for CREATE SEQUENCE > * could override the START value and cause confusion to user. Hence, > * we throw an error for CREATE SEQUENCE if RESTART option is > * specified; it can only be used with ALTER SEQUENCE. > */ > > just a thought. LGTM. PSA v2 patch. Regards, Bharath Rupireddy.
Attachment
On Sat, Jul 24, 2021 at 09:56:40PM +0530, Bharath Rupireddy wrote: > LGTM. PSA v2 patch. FWIW, like Ashutosh upthread, my vote would be to do nothing here in terms of behavior changes as this is just breaking a behavior for the sake of breaking it, so there are chances that this is going to piss some users that relied accidentally on the existing behavior. I think that we should document that RESTART is accepted within the set of options as a consequence of the set of options supported by gram.y, though. -- Michael
Attachment
On Mon, Jul 26, 2021 at 04:57:53PM +0900, Michael Paquier wrote: > FWIW, like Ashutosh upthread, my vote would be to do nothing here in > terms of behavior changes as this is just breaking a behavior for the > sake of breaking it, so there are chances that this is going to piss > some users that relied accidentally on the existing behavior. In short, I would be tempted with something like the attached, that documents RESTART in CREATE SEQUENCE, while describing its behavior according to START. In terms of regression tests, there is already a lot in this area with ALTER SEQUENCE, but I think that having two tests makes sense for CREATE SEQUENCE: one for RESTART without a value and one with, where both explicitely set START. Thoughts? -- Michael
Attachment
On Wed, Jul 28, 2021 at 11:50 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Jul 26, 2021 at 04:57:53PM +0900, Michael Paquier wrote:
> > FWIW, like Ashutosh upthread, my vote would be to do nothing here in
> > terms of behavior changes as this is just breaking a behavior for the
> > sake of breaking it, so there are chances that this is going to piss
> > some users that relied accidentally on the existing behavior.
>
> In short, I would be tempted with something like the attached, that
> documents RESTART in CREATE SEQUENCE, while describing its behavior
> according to START. In terms of regression tests, there is already a
> lot in this area with ALTER SEQUENCE, but I think that having two
> tests makes sense for CREATE SEQUENCE: one for RESTART without a
> value and one with, where both explicitely set START.
>
> Thoughts?
-1. IMHO, this is something creating more confusion to the user. We
say that we allow both START and RESTART that RESTART is accepted as a
consequence of our internal option handling in gram.y. Instead, I
recommend throwing errorConflictingDefElem or errmsg("START and
RESTART are mutually exclusive options"). We do throw these errors in
a lot of other places for various options. Others may have better
thoughts though.
Regards,
Bharath Rupireddy.
On 2021/07/28 23:53, Bharath Rupireddy wrote:
> On Wed, Jul 28, 2021 at 11:50 AM Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Mon, Jul 26, 2021 at 04:57:53PM +0900, Michael Paquier wrote:
>>> FWIW, like Ashutosh upthread, my vote would be to do nothing here in
>>> terms of behavior changes as this is just breaking a behavior for the
>>> sake of breaking it, so there are chances that this is going to piss
>>> some users that relied accidentally on the existing behavior.
>>
>> In short, I would be tempted with something like the attached, that
>> documents RESTART in CREATE SEQUENCE, while describing its behavior
>> according to START. In terms of regression tests, there is already a
>> lot in this area with ALTER SEQUENCE, but I think that having two
>> tests makes sense for CREATE SEQUENCE: one for RESTART without a
>> value and one with, where both explicitely set START.
>>
>> Thoughts?
>
> -1. IMHO, this is something creating more confusion to the user. We
> say that we allow both START and RESTART that RESTART is accepted as a
> consequence of our internal option handling in gram.y. Instead, I
> recommend throwing errorConflictingDefElem or errmsg("START and
> RESTART are mutually exclusive options"). We do throw these errors in
> a lot of other places for various options. Others may have better
> thoughts though.
Per docs, CREATE SEQUENCE conforms to the SQL standard, with some exceptions.
So I'd agree with Michael if CREATE SEQUENCE with RESTART also conforms to
the SQL standard, but I'd agree with Bharath otherwise.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> On 2021/07/28 23:53, Bharath Rupireddy wrote:
>> -1. IMHO, this is something creating more confusion to the user. We
>> say that we allow both START and RESTART that RESTART is accepted as a
>> consequence of our internal option handling in gram.y. Instead, I
>> recommend throwing errorConflictingDefElem or errmsg("START and
>> RESTART are mutually exclusive options"). We do throw these errors in
>> a lot of other places for various options. Others may have better
>> thoughts though.
> Per docs, CREATE SEQUENCE conforms to the SQL standard, with some exceptions.
> So I'd agree with Michael if CREATE SEQUENCE with RESTART also conforms to
> the SQL standard, but I'd agree with Bharath otherwise.
I do not see any RESTART option in SQL:2021 11.72 <sequence generator
definition>. Since we don't document it either, there's really no
expectation that anyone would use it.
I don't particularly think that we should document it, so I'm with Michael
that we don't need to do anything. This is hardly the only undocumented
corner case in PG.
regards, tom lane
On Wed, Jul 28, 2021 at 01:16:19PM -0400, Tom Lane wrote: > I do not see any RESTART option in SQL:2021 11.72 <sequence generator > definition>. Since we don't document it either, there's really no > expectation that anyone would use it. Okay, good point. I was not aware of that. > I don't particularly think that we should document it, so I'm with Michael > that we don't need to do anything. This is hardly the only undocumented > corner case in PG. More regression tests for CREATE SEQUENCE may be interesting, but that's not an issue either with the ones for ALTER SEQUENCE. Let's drop the patch and move on. -- Michael