On Fri, 16 Jul 2021 at 06:40, Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
>
> 1) Duplicate option check for "FROM" option is unnecessary and will be
> a dead code. Because the syntaxer anyways would catch if FROM is
> specified more than once, something like CREATE COLLATION mycoll1 FROM
> FROM "C";.
Hmm, it is possible to type CREATE COLLATION mycoll1 (FROM = "C", FROM
= "POSIX") though. It will still be caught by the check at the bottom
though, so I guess it's OK to skip the duplicate check in that case.
Also, it's not a documented syntax, so it's unlikely to occur in
practice anyway.
> 2) I don't understand the difference between errorConflictingDefElem
> and errorDuplicateDefElem.
>
> I personally don't like the new function errorDuplicateDefElem as it
> doesn't add any value given the presence of pstate.
Yeah, there was a lot of discussion on that other thread about using
defel->defname in these kinds of errors, and that was still on my
mind. In general there are a number of cases where defel->defname
isn't quite right, because it doesn't match the user-entered text,
though in this case it always does. You're right though, it's a bit
redundant with the position indicator pointing to the offending
option, so it's probably not worth the effort to include it here when
we don't anywhere else. That makes the patch much simpler, and
consistent with option-checking elsewhere -- v5 attached (which is now
much more like your v3).
Regards,
Dean
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
new file mode 100644
index ebb0994..c6a6ed3
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -88,17 +88,41 @@ DefineCollation(ParseState *pstate, List
if (strcmp(defel->defname, "from") == 0)
defelp = &fromEl;
else if (strcmp(defel->defname, "locale") == 0)
+ {
+ if (localeEl)
+ errorConflictingDefElem(defel, pstate);
defelp = &localeEl;
+ }
else if (strcmp(defel->defname, "lc_collate") == 0)
+ {
+ if (lccollateEl)
+ errorConflictingDefElem(defel, pstate);
defelp = &lccollateEl;
+ }
else if (strcmp(defel->defname, "lc_ctype") == 0)
+ {
+ if (lcctypeEl)
+ errorConflictingDefElem(defel, pstate);
defelp = &lcctypeEl;
+ }
else if (strcmp(defel->defname, "provider") == 0)
+ {
+ if (providerEl)
+ errorConflictingDefElem(defel, pstate);
defelp = &providerEl;
+ }
else if (strcmp(defel->defname, "deterministic") == 0)
+ {
+ if (deterministicEl)
+ errorConflictingDefElem(defel, pstate);
defelp = &deterministicEl;
+ }
else if (strcmp(defel->defname, "version") == 0)
+ {
+ if (versionEl)
+ errorConflictingDefElem(defel, pstate);
defelp = &versionEl;
+ }
else
{
ereport(ERROR,
@@ -112,11 +136,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/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
new file mode 100644
index 0b60b55..2468325
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -701,6 +701,53 @@ View definition:
SELECT ss.c1 + 1 AS c1p
FROM ( SELECT 4 AS c1) ss;
+-- Check conflicting or redundant options in CREATE COLLATION
+-- 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...
+ ^
+-- 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 =...
+ ^
+-- 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...
+ ^
+-- LOCALE
+CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONSENSE");
+ERROR: conflicting or redundant options
+LINE 1: CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONS...
+ ^
+-- 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...
+ ^
+-- 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...
+ ^
+-- 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.
+-- 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.
--
-- 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..c3d40fc
--- a/src/test/regress/sql/collate.sql
+++ b/src/test/regress/sql/collate.sql
@@ -272,6 +272,27 @@ SELECT c1+1 AS c1p FROM
(SELECT ('4' COLLATE "C")::INT AS c1) ss;
\d+ collate_on_int
+-- Check conflicting or redundant options in CREATE COLLATION
+-- 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 = '');
+-- FROM conflicts with any other option
+CREATE COLLATION coll_dup_chk (FROM = "C", VERSION = "1");
--
-- Clean up. Many of these table names will be re-used if the user is
