Horton

Thread

  1. CREATE SEQUENCE with RESTART option

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-07T10:25:50Z

    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

  2. Re: CREATE SEQUENCE with RESTART option

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2021-04-07T12:34:28Z

    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

  3. Re: CREATE SEQUENCE with RESTART option

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-07T13:21:49Z

    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

  4. Re: CREATE SEQUENCE with RESTART option

    Amul Sul <sulamul@gmail.com> — 2021-04-08T04:38:35Z

    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

  5. Re: CREATE SEQUENCE with RESTART option

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-08T08:32:57Z

    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

  6. Re: CREATE SEQUENCE with RESTART option

    Suraj Kharage <suraj.kharage@enterprisedb.com> — 2021-04-08T09:46:26Z

    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?

-- 
--

Thanks & Regards,
Suraj kharage,



edbpostgres.com

  7. Re: CREATE SEQUENCE with RESTART option

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-08T10:34:09Z

    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

  8. Re: CREATE SEQUENCE with RESTART option

    Cary Huang <cary.huang@highgo.ca> — 2021-07-23T21:49:24Z

    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 think it 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

  9. Re: CREATE SEQUENCE with RESTART option

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-07-24T16:26:40Z

    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 think it 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.

  10. Re: CREATE SEQUENCE with RESTART option

    Michael Paquier <michael@paquier.xyz> — 2021-07-26T07:57:53Z

    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

  11. Re: CREATE SEQUENCE with RESTART option

    Michael Paquier <michael@paquier.xyz> — 2021-07-28T06:20:02Z

    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

  12. Re: CREATE SEQUENCE with RESTART option

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-07-28T14:53:25Z

    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.

  13. Re: CREATE SEQUENCE with RESTART option

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-07-28T16:58:41Z

    
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

  14. Re: CREATE SEQUENCE with RESTART option

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-28T17:16:19Z

    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

  15. Re: CREATE SEQUENCE with RESTART option

    Michael Paquier <michael@paquier.xyz> — 2021-07-28T23:49:17Z

    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