On Thu, Apr 8, 2021 at 10:09 AM Amul Sul <sula...@gmail.com> wrote:
>
> On Wed, Apr 7, 2021 at 6:52 PM Bharath Rupireddy
> <bharath.rupireddyforpostg...@gmail.com> wrote:
> >
> > On Wed, Apr 7, 2021 at 6:04 PM Ashutosh Bapat
> > <ashutosh.bapat....@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
From 819d653cc2be54e822826f08905f6ce8ec2f8418 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Thu, 8 Apr 2021 13:35:38 +0530
Subject: [PATCH v1] Disallow RESTART option for CREATE SEQUENCE
Restarting a sequence while defining it doesn't make any sense
and it may override the START value specified. Hence, we throw
error for CREATE SEQUENCE if RESTART option is specified.
However, it can be used with ALTER SEQUENCE.
If users want their sequences to be starting from a value different
than START value, then they can specify RESTART value with ALTER
SEQUENCE.
---
src/backend/commands/sequence.c | 13 +++++++++++++
src/test/regress/expected/sequence.out | 4 ++++
src/test/regress/sql/sequence.sql | 1 +
3 files changed, 18 insertions(+)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 0415df9ccb..5b75229360 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1290,6 +1290,19 @@ init_params(ParseState *pstate, List *options, bool for_identity,
}
else if (strcmp(defel->defname, "restart") == 0)
{
+ /*
+ * 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.
+ */
+ if (isInit)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("RESTART option is not allowed while creating sequence"),
+ parser_errposition(pstate, defel->location)));
+
if (restart_value)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index 8b928b2888..b9593bc8e4 100644
--- a/src/test/regress/expected/sequence.out
+++ b/src/test/regress/expected/sequence.out
@@ -16,6 +16,10 @@ CREATE SEQUENCE sequence_testx INCREMENT BY 1 START -10;
ERROR: START value (-10) cannot be less than MINVALUE (1)
CREATE SEQUENCE sequence_testx CACHE 0;
ERROR: CACHE (0) must be greater than zero
+CREATE SEQUENCE sequence_testx RESTART 5;
+ERROR: RESTART option is not allowed while creating sequence
+LINE 1: CREATE SEQUENCE sequence_testx RESTART 5;
+ ^
-- OWNED BY errors
CREATE SEQUENCE sequence_testx OWNED BY nobody; -- nonsense word
ERROR: invalid OWNED BY option
diff --git a/src/test/regress/sql/sequence.sql b/src/test/regress/sql/sequence.sql
index 7928ee23ee..367157fe2b 100644
--- a/src/test/regress/sql/sequence.sql
+++ b/src/test/regress/sql/sequence.sql
@@ -10,6 +10,7 @@ CREATE SEQUENCE sequence_testx INCREMENT BY 1 MAXVALUE -20;
CREATE SEQUENCE sequence_testx INCREMENT BY -1 START 10;
CREATE SEQUENCE sequence_testx INCREMENT BY 1 START -10;
CREATE SEQUENCE sequence_testx CACHE 0;
+CREATE SEQUENCE sequence_testx RESTART 5;
-- OWNED BY errors
CREATE SEQUENCE sequence_testx OWNED BY nobody; -- nonsense word
--
2.25.1
