Hi,
While reviewing [1], I found that the CREATE COLLATION doesn't throw an
error if duplicate options are specified, see [2] for testing a few cases
on HEAD. This may end up accepting some of the weird cases, see [2]. It's
against other option checking code in the server where the duplicate option
is detected and an error thrown if found one. Attached a patch doing that.
I chose to have the error message "option \"%s\" specified more than once"
and parser_errposition because that's kind of agreed in [3].
Thoughts?
[1]
https://www.postgresql.org/message-id/CALj2ACWVd%3D-E6uG5AdHD0MvHY6e4mVzkapT%3DvLDnJJseGjaJLQ%40mail.gmail.com
[2]
CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_COLLATE =
"NONSENSE", LC_CTYPE = "POSIX"); -- ERROR
CREATE COLLATION coll_dup_chk (LC_COLLATE = "NONSENSE", LC_COLLATE =
"POSIX", LC_CTYPE = "POSIX"); -- OK but it's weird
CREATE COLLATION coll_dup_chk (LC_CTYPE = "POSIX", LC_CTYPE = "NONSENSE",
LC_COLLATE = "POSIX"); -- ERROR
CREATE COLLATION coll_dup_chk (LC_CTYPE = "NONSENSE", LC_CTYPE = "POSIX",
LC_COLLATE = "POSIX",); -- OK but it's weird
CREATE COLLATION coll_dup_chk (PROVIDER = icu, PROVIDER = NONSENSE,
LC_COLLATE = "POSIX", LC_CTYPE = "POSIX"); -- ERROR
CREATE COLLATION coll_dup_chk (PROVIDER = NONSENSE, PROVIDER = icu,
LC_COLLATE = "POSIX", LC_CTYPE = "POSIX"); -- OK but it's weird
CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONSENSE"); -- ERROR
CREATE COLLATION coll_dup_chk (LOCALE = "NONSENSE", LOCALE = ''); -- OK but
it's weird
CREATE COLLATION coll_dup_chk (DETERMINISTIC = TRUE, DETERMINISTIC =
NONSENSE, LOCALE = ''); -- ERROR
CREATE COLLATION coll_dup_chk (DETERMINISTIC = NONSENSE, DETERMINISTIC =
TRUE, LOCALE = ''); -- OK but it's weird
[3]
https://www.postgresql.org/message-id/CALj2ACUa%3DZM8QtOLPCHc7%3DWgFrx9P6-AgKQs8cmKLvNCvu7arQ%40mail.gmail.com
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 555ac67a94d899b107e27a691f3a2a7340a14f64 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Tue, 27 Apr 2021 12:01:44 +0530
Subject: [PATCH v1] Check duplicate options and error out for CREATE COLLATION
command
CREATE COLLATION doesn't throw an error if duplicate options are
specified. Modify the option parsing loop in DefineCollation to
check for duplicate options and error out if found one.
---
src/backend/commands/collationcmds.c | 35 +++++++++++++++++++++++++++
src/test/regress/expected/collate.out | 35 +++++++++++++++++++++++++++
src/test/regress/sql/collate.sql | 17 +++++++++++++
3 files changed, 87 insertions(+)
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
index b8ec6f5756..8d07b21eda 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -86,15 +86,50 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
if (strcmp(defel->defname, "from") == 0)
defelp = &fromEl;
else if (strcmp(defel->defname, "locale") == 0)
+ {
+ if (localeEl)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", "locale"),
+ parser_errposition(pstate, defel->location)));
defelp = &localeEl;
+ }
else if (strcmp(defel->defname, "lc_collate") == 0)
+ {
+ if (lccollateEl)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", "lc_collate"),
+ parser_errposition(pstate, defel->location)));
defelp = &lccollateEl;
+ }
else if (strcmp(defel->defname, "lc_ctype") == 0)
+ {
+ if (lcctypeEl)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", "lc_ctype"),
+ parser_errposition(pstate, defel->location)));
defelp = &lcctypeEl;
+ }
else if (strcmp(defel->defname, "provider") == 0)
+ {
+ if (providerEl)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", "provider"),
+ parser_errposition(pstate, defel->location)));
defelp = &providerEl;
+ }
else if (strcmp(defel->defname, "deterministic") == 0)
+ {
+ if (deterministicEl)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", "deterministic"),
+ parser_errposition(pstate, defel->location)));
defelp = &deterministicEl;
+ }
else
{
ereport(ERROR,
diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
index 0b60b55514..85a10c8198 100644
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -701,6 +701,41 @@ View definition:
SELECT ss.c1 + 1 AS c1p
FROM ( SELECT 4 AS c1) ss;
+-- Check duplicate options in CREATE COLLATION command and throw error
+-- LC_COLLATE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_COLLATE = "NONSENSE", LC_CTYPE = "POSIX");
+ERROR: option "lc_collate" specified more than once
+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: option "lc_ctype" specified more than once
+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: option "provider" specified more than once
+LINE 1: CREATE COLLATION coll_dup_chk (PROVIDER = icu, PROVIDER = NO...
+ ^
+-- LOCALE
+CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONSENSE");
+ERROR: option "locale" specified more than once
+LINE 1: CREATE COLLATION case_sensitive (LOCALE = '', LOCALE = "NONS...
+ ^
+-- DETERMINISTIC
+CREATE COLLATION coll_dup_chk (DETERMINISTIC = TRUE, DETERMINISTIC = NONSENSE, LOCALE = '');
+ERROR: option "deterministic" specified more than once
+LINE 1: ...ATE COLLATION coll_dup_chk (DETERMINISTIC = TRUE, DETERMINIS...
+ ^
+-- LOCALE conflicting with LC_COLLATE and LC_CTYPE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_CTYPE = "POSIX", LOCALE = '');
+ERROR: conflicting or redundant options
+-- LOCALE conflicting with LC_COLLATE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LOCALE = '');
+ERROR: conflicting or redundant options
+-- LOCALE conflicting with LC_CTYPE
+CREATE COLLATION coll_dup_chk (LC_CTYPE = "POSIX", LOCALE = '');
+ERROR: conflicting or redundant 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
index 30f811ba89..8a9cca1e35 100644
--- a/src/test/regress/sql/collate.sql
+++ b/src/test/regress/sql/collate.sql
@@ -272,6 +272,23 @@ SELECT c1+1 AS c1p FROM
(SELECT ('4' COLLATE "C")::INT AS c1) ss;
\d+ collate_on_int
+-- Check duplicate options in CREATE COLLATION command and throw error
+-- 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 = '');
+-- LOCALE conflicting with LC_COLLATE and LC_CTYPE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LC_CTYPE = "POSIX", LOCALE = '');
+-- LOCALE conflicting with LC_COLLATE
+CREATE COLLATION coll_dup_chk (LC_COLLATE = "POSIX", LOCALE = '');
+-- LOCALE conflicting 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
--
2.25.1
