On Mon, 31 May 2021 at 15:10, vignesh C <vignes...@gmail.com> wrote:
>
> On Sat, May 29, 2021 at 9:20 PM Bharath Rupireddy
> <bharath.rupireddyforpostg...@gmail.com> wrote:
> >
> > Thanks. PSA v3 patch.
>
> Thanks for the updated patch, the changes look good to me.
>
Hi,
Having pushed [1], I started looking at this, and I think it's mostly
in good shape.
Since we're improving the CREATE COLLATION errors, I think it's also
worth splitting out the error for LOCALE + LC_COLLATE/LC_CTYPE from
the error for FROM + any other option.
In the case of LOCALE + LC_COLLATE/LC_CTYPE, there is an identical
error in CREATE DATABASE, so we should use the same error message and
detail text here.
Then logically, FROM + any other option should have an error of the
same general form.
And finally, it then makes sense to make the other errors follow the
same pattern (with the "specified more than once" text in the detail),
which is also where we ended up in the discussion over in [1].
So, attached is what I propose.
Regards,
Dean
[1]
https://www.postgresql.org/message-id/CAEZATCXHWa9OoSAetiZiGQy1eM2raa9q-b3K4ZYDwtcARypCcA%40mail.gmail.com
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
new file mode 100644
index ebb0994..40e2626
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -86,19 +86,47 @@ DefineCollation(ParseState *pstate, List
DefElem **defelp;
if (strcmp(defel->defname, "from") == 0)
+ {
+ if (fromEl)
+ errorDuplicateDefElem(defel, pstate);
defelp = &fromEl;
+ }
else if (strcmp(defel->defname, "locale") == 0)
+ {
+ if (localeEl)
+ errorDuplicateDefElem(defel, pstate);
defelp = &localeEl;
+ }
else if (strcmp(defel->defname, "lc_collate") == 0)
+ {
+ if (lccollateEl)
+ errorDuplicateDefElem(defel, pstate);
defelp = &lccollateEl;
+ }
else if (strcmp(defel->defname, "lc_ctype") == 0)
+ {
+ if (lcctypeEl)
+ errorDuplicateDefElem(defel, pstate);
defelp = &lcctypeEl;
+ }
else if (strcmp(defel->defname, "provider") == 0)
+ {
+ if (providerEl)
+ errorDuplicateDefElem(defel, pstate);
defelp = &providerEl;
+ }
else if (strcmp(defel->defname, "deterministic") == 0)
+ {
+ if (deterministicEl)
+ errorDuplicateDefElem(defel, pstate);
defelp = &deterministicEl;
+ }
else if (strcmp(defel->defname, "version") == 0)
+ {
+ if (versionEl)
+ errorDuplicateDefElem(defel, pstate);
defelp = &versionEl;
+ }
else
{
ereport(ERROR,
@@ -112,11 +140,17 @@ DefineCollation(ParseState *pstate, List
*defelp = defel;
}
- if ((localeEl && (lccollateEl || lcctypeEl))
- || (fromEl && list_length(parameters) != 1))
+ if (localeEl && (lccollateEl || lcctypeEl))
ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting or redundant options")));
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options"),
+ errdetail("LOCALE cannot be specified together with LC_COLLATE or LC_CTYPE."));
+
+ if (fromEl && list_length(parameters) != 1)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options"),
+ errdetail("FROM cannot be specified together with any other options."));
if (fromEl)
{
diff --git a/src/backend/commands/define.c b/src/backend/commands/define.c
new file mode 100644
index aafd755..8dd260d
--- a/src/backend/commands/define.c
+++ b/src/backend/commands/define.c
@@ -359,3 +359,21 @@ errorConflictingDefElem(DefElem *defel,
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location));
}
+
+/*
+ * Raise an error about a duplicate DefElem.
+ *
+ * This is similar to errorConflictingDefElem(), except that it is intended for
+ * an option that the user explicitly specified more than once. This should
+ * only be used if defel->defname is guaranteed to match the user-entered
+ * option name, otherwise the detail text might be confusing.
+ */
+void
+errorDuplicateDefElem(DefElem *defel, ParseState *pstate)
+{
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options"),
+ errdetail("Option \"%s\" specified more than once.", defel->defname),
+ parser_errposition(pstate, defel->location));
+}
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
new file mode 100644
index f84d099..91743ca
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -154,5 +154,6 @@ extern TypeName *defGetTypeName(DefElem
extern int defGetTypeLength(DefElem *def);
extern List *defGetStringList(DefElem *def);
extern void errorConflictingDefElem(DefElem *defel, ParseState *pstate) pg_attribute_noreturn();
+extern void errorDuplicateDefElem(DefElem *defel, ParseState *pstate) pg_attribute_noreturn();
#endif /* DEFREM_H */
diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
new file mode 100644
index 0b60b55..c08d8f2
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -701,6 +701,65 @@ View definition:
SELECT ss.c1 + 1 AS c1p
FROM ( SELECT 4 AS c1) ss;
+-- Check conflicting or duplicate options in CREATE COLLATION
+-- FROM
+CREATE COLLATION coll_dup_chk (FROM = "C", FROM = "X");
+ERROR: conflicting or redundant options
+LINE 1: CREATE COLLATION coll_dup_chk (FROM = "C", FROM = "X");
+ ^
+DETAIL: Option "from" specified more than once.
+-- FROM conflicts with any other option
+CREATE COLLATION coll_dup_chk (FROM = "C", VERSION = "1");
+ERROR: conflicting or redundant options
+DETAIL: FROM cannot be specified together with any other options.
+-- LC_COLLATE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_COLLATE = "NONSENSE", LC_CTYPE = "POSIX");
+ERROR: conflicting or redundant options
+LINE 1: ...ATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_COLLATE...
+ ^
+DETAIL: Option "lc_collate" specified more than once.
+-- LC_CTYPE
+CREATE COLLATION coll_dup_chk (LC_CTYPE = "POSIX", LC_CTYPE = "NONSENSE", LC_COLLATE = "POSIX");
+ERROR: conflicting or redundant options
+LINE 1: ...REATE COLLATION coll_dup_chk (LC_CTYPE = "POSIX", LC_CTYPE =...
+ ^
+DETAIL: Option "lc_ctype" specified more than once.
+-- PROVIDER
+CREATE COLLATION coll_dup_chk (PROVIDER = icu, PROVIDER = NONSENSE, LC_COLLATE = "POSIX", LC_CTYPE = "POSIX");
+ERROR: conflicting or redundant options
+LINE 1: CREATE COLLATION coll_dup_chk (PROVIDER = icu, PROVIDER = NO...
+ ^
+DETAIL: Option "provider" specified more than once.
+-- LOCALE
+CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONSENSE");
+ERROR: conflicting or redundant options
+LINE 1: CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONS...
+ ^
+DETAIL: Option "locale" specified more than once.
+-- DETERMINISTIC
+CREATE COLLATION coll_dup_chk (DETERMINISTIC = TRUE, DETERMINISTIC = NONSENSE, LOCALE = '');
+ERROR: conflicting or redundant options
+LINE 1: ...ATE COLLATION coll_dup_chk (DETERMINISTIC = TRUE, DETERMINIS...
+ ^
+DETAIL: Option "deterministic" specified more than once.
+-- VERSION
+CREATE COLLATION coll_dup_chk (VERSION = '1', VERSION = "NONSENSE", LOCALE = '');
+ERROR: conflicting or redundant options
+LINE 1: CREATE COLLATION coll_dup_chk (VERSION = '1', VERSION = "NON...
+ ^
+DETAIL: Option "version" specified more than once.
+-- LOCALE conflicts with LC_COLLATE and LC_CTYPE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_CTYPE = "POSIX", LOCALE = '');
+ERROR: conflicting or redundant options
+DETAIL: LOCALE cannot be specified together with LC_COLLATE or LC_CTYPE.
+-- LOCALE conflicts with LC_COLLATE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LOCALE = '');
+ERROR: conflicting or redundant options
+DETAIL: LOCALE cannot be specified together with LC_COLLATE or LC_CTYPE.
+-- LOCALE conflicts with LC_CTYPE
+CREATE COLLATION coll_dup_chk (LC_CTYPE = "POSIX", LOCALE = '');
+ERROR: conflicting or redundant options
+DETAIL: LOCALE cannot be specified together with LC_COLLATE or LC_CTYPE.
--
-- Clean up. Many of these table names will be re-used if the user is
-- trying to run any platform-specific collation tests later, so we
diff --git a/src/test/regress/sql/collate.sql b/src/test/regress/sql/collate.sql
new file mode 100644
index 30f811b..87c31d4
--- a/src/test/regress/sql/collate.sql
+++ b/src/test/regress/sql/collate.sql
@@ -272,6 +272,29 @@ SELECT c1+1 AS c1p FROM
(SELECT ('4' COLLATE "C")::INT AS c1) ss;
\d+ collate_on_int
+-- Check conflicting or duplicate options in CREATE COLLATION
+-- FROM
+CREATE COLLATION coll_dup_chk (FROM = "C", FROM = "X");
+-- FROM conflicts with any other option
+CREATE COLLATION coll_dup_chk (FROM = "C", VERSION = "1");
+-- LC_COLLATE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_COLLATE = "NONSENSE", LC_CTYPE = "POSIX");
+-- LC_CTYPE
+CREATE COLLATION coll_dup_chk (LC_CTYPE = "POSIX", LC_CTYPE = "NONSENSE", LC_COLLATE = "POSIX");
+-- PROVIDER
+CREATE COLLATION coll_dup_chk (PROVIDER = icu, PROVIDER = NONSENSE, LC_COLLATE = "POSIX", LC_CTYPE = "POSIX");
+-- LOCALE
+CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONSENSE");
+-- DETERMINISTIC
+CREATE COLLATION coll_dup_chk (DETERMINISTIC = TRUE, DETERMINISTIC = NONSENSE, LOCALE = '');
+-- VERSION
+CREATE COLLATION coll_dup_chk (VERSION = '1', VERSION = "NONSENSE", LOCALE = '');
+-- LOCALE conflicts with LC_COLLATE and LC_CTYPE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_CTYPE = "POSIX", LOCALE = '');
+-- LOCALE conflicts with LC_COLLATE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LOCALE = '');
+-- LOCALE conflicts with LC_CTYPE
+CREATE COLLATION coll_dup_chk (LC_CTYPE = "POSIX", LOCALE = '');
--
-- Clean up. Many of these table names will be re-used if the user is
