CREATE SEQUENCE with RESTART option

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

CREATE SEQUENCE with RESTART option

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


Reply | Threaded
Open this post in threaded view
|

Re: CREATE SEQUENCE with RESTART option

Ashutosh Bapat-2
On Wed, Apr 7, 2021 at 3:56 PM Bharath Rupireddy
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: CREATE SEQUENCE with RESTART option

Bharath Rupireddy
On Wed, Apr 7, 2021 at 6:04 PM Ashutosh Bapat
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: CREATE SEQUENCE with RESTART option

amul sul
On Wed, Apr 7, 2021 at 6:52 PM Bharath Rupireddy
<[hidden email]> wrote:

>
> On Wed, Apr 7, 2021 at 6:04 PM Ashutosh Bapat
> <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: CREATE SEQUENCE with RESTART option

Bharath Rupireddy
On Thu, Apr 8, 2021 at 10:09 AM Amul Sul <[hidden email]> wrote:

>
> On Wed, Apr 7, 2021 at 6:52 PM Bharath Rupireddy
> <[hidden email]> wrote:
> >
> > On Wed, Apr 7, 2021 at 6:04 PM Ashutosh Bapat
> > <[hidden email]> 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

v1-0001-Disallow-RESTART-option-for-CREATE-SEQUENCE.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: CREATE SEQUENCE with RESTART option

Suraj Kharage-2


On Thu, Apr 8, 2021 at 2:03 PM Bharath Rupireddy <[hidden email]> 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 & Regards, 
Suraj kharage, 

Reply | Threaded
Open this post in threaded view
|

Re: CREATE SEQUENCE with RESTART option

Bharath Rupireddy
On Thu, Apr 8, 2021 at 3:16 PM Suraj Kharage
<[hidden email]> 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