On Sat, Jul 24, 2021 at 3:20 AM Cary Huang <cary.hu...@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.
From e726e40d4969f56eb4246a7016e3e2780aa2fbf1 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 24 Jul 2021 16:23:42 +0000
Subject: [PATCH v2] Disallow RESTART option for CREATE SEQUENCE
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.
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 | 12 ++++++++++++
src/test/regress/expected/sequence.out | 4 ++++
src/test/regress/sql/sequence.sql | 1 +
3 files changed, 17 insertions(+)
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 72bfdc07a4..8c50f396e6 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1281,6 +1281,18 @@ init_params(ParseState *pstate, List *options, bool for_identity,
}
else if (strcmp(defel->defname, "restart") == 0)
{
+ /*
+ * 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.
+ */
+ 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)
errorConflictingDefElem(defel, pstate);
restart_value = defel;
diff --git a/src/test/regress/expected/sequence.out b/src/test/regress/expected/sequence.out
index 71c2b0f1df..f60725a9e5 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
