CREATE COLLATION - check for duplicate options and error out if found one
Hi,
While reviewing [1]/messages/by-id/CALj2ACWVd=-E6uG5AdHD0MvHY6e4mVzkapT=vLDnJJseGjaJLQ@mail.gmail.com, I found that the CREATE COLLATION doesn't throw an
error if duplicate options are specified, see [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 for testing a few cases
on HEAD. This may end up accepting some of the weird cases, see [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. 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]/messages/by-id/CALj2ACUa=ZM8QtOLPCHc7=WgFrx9P6-AgKQs8cmKLvNCvu7arQ@mail.gmail.com.
Thoughts?
[1]: /messages/by-id/CALj2ACWVd=-E6uG5AdHD0MvHY6e4mVzkapT=vLDnJJseGjaJLQ@mail.gmail.com
/messages/by-id/CALj2ACWVd=-E6uG5AdHD0MvHY6e4mVzkapT=vLDnJJseGjaJLQ@mail.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
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]: /messages/by-id/CALj2ACUa=ZM8QtOLPCHc7=WgFrx9P6-AgKQs8cmKLvNCvu7arQ@mail.gmail.com
/messages/by-id/CALj2ACUa=ZM8QtOLPCHc7=WgFrx9P6-AgKQs8cmKLvNCvu7arQ@mail.gmail.com
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v1-0001-Check-duplicate-options-and-error-out-for-CREATE-.patchapplication/x-patch; name=v1-0001-Check-duplicate-options-and-error-out-for-CREATE-.patchDownload
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
On Tue, Apr 27, 2021 at 3:21 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
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] /messages/by-id/CALj2ACWVd=-E6uG5AdHD0MvHY6e4mVzkapT=vLDnJJseGjaJLQ@mail.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] /messages/by-id/CALj2ACUa=ZM8QtOLPCHc7=WgFrx9P6-AgKQs8cmKLvNCvu7arQ@mail.gmail.com
+1 for fixing this issue, we have handled this error in other places.
The patch does not apply on head, could you rebase the patch on head
and post a new patch.
Regards,
Vignesh
On Wed, May 26, 2021 at 7:18 PM vignesh C <vignesh21@gmail.com> wrote:
+1 for fixing this issue, we have handled this error in other places.
The patch does not apply on head, could you rebase the patch on head
and post a new patch.
Thanks. I thought of rebasing once the other patch (which reorganizes
"...specified more than once" error) gets committed. Anyways, I've
rebased for now on the latest master. Please review v2 patch.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v2-0001-Check-duplicate-options-and-error-out-for-CREATE-.patchapplication/octet-stream; name=v2-0001-Check-duplicate-options-and-error-out-for-CREATE-.patchDownload
From 510224af089b1b81b5c990283de88f9952db163f Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Wed, 26 May 2021 19:43:42 +0530
Subject: [PATCH v2] Check duplicate options and error out for CREATE COLLATION
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 | 42 +++++++++++++++++++++++++++
src/test/regress/expected/collate.out | 40 +++++++++++++++++++++++++
src/test/regress/sql/collate.sql | 19 ++++++++++++
3 files changed, 101 insertions(+)
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
index ebb0994db3..4620069aba 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -88,17 +88,59 @@ 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", defel->defname),
+ 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", defel->defname),
+ 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", defel->defname),
+ 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", defel->defname),
+ 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", defel->defname),
+ parser_errposition(pstate, defel->location)));
defelp = &deterministicEl;
+ }
else if (strcmp(defel->defname, "version") == 0)
+ {
+ if (versionEl)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", defel->defname),
+ parser_errposition(pstate, defel->location)));
defelp = &versionEl;
+ }
else
{
ereport(ERROR,
diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
index 0b60b55514..edced86af2 100644
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -701,6 +701,46 @@ 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...
+ ^
+-- VERSION
+CREATE COLLATION coll_dup_chk (VERSION = '1', VERSION = "NONSENSE", LOCALE = '');
+ERROR: option "version" specified more than once
+LINE 1: CREATE COLLATION coll_dup_chk (VERSION = '1', VERSION = "NON...
+ ^
+-- 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..d691fae9d4 100644
--- a/src/test/regress/sql/collate.sql
+++ b/src/test/regress/sql/collate.sql
@@ -272,6 +272,25 @@ 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 = '');
+-- VERSION
+CREATE COLLATION coll_dup_chk (VERSION = '1', VERSION = "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
On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
On Wed, May 26, 2021 at 7:18 PM vignesh C <vignesh21@gmail.com> wrote:
+1 for fixing this issue, we have handled this error in other places.
The patch does not apply on head, could you rebase the patch on head
and post a new patch.Thanks. I thought of rebasing once the other patch (which reorganizes
"...specified more than once" error) gets committed. Anyways, I've
rebased for now on the latest master. Please review v2 patch.
The test changes look good to me, I liked the idea of rebasing the
source changes once "specified more than once" patch gets committed. I
will review the code changes after that.
Regards,
Vignesh
On Thu, May 27, 2021 at 8:36 PM vignesh C <vignesh21@gmail.com> wrote:
On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:On Wed, May 26, 2021 at 7:18 PM vignesh C <vignesh21@gmail.com> wrote:
+1 for fixing this issue, we have handled this error in other places.
The patch does not apply on head, could you rebase the patch on head
and post a new patch.Thanks. I thought of rebasing once the other patch (which reorganizes
"...specified more than once" error) gets committed. Anyways, I've
rebased for now on the latest master. Please review v2 patch.The test changes look good to me, I liked the idea of rebasing the
source changes once "specified more than once" patch gets committed. I
will review the code changes after that.
I'm not sure which patch goes first. I think the review can be
finished and see which one will be picked up first by the committer.
Based on that, the other patch can be rebased.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On Wed, May 26, 2021 at 7:44 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
On Wed, May 26, 2021 at 7:18 PM vignesh C <vignesh21@gmail.com> wrote:
+1 for fixing this issue, we have handled this error in other places.
The patch does not apply on head, could you rebase the patch on head
and post a new patch.Thanks. I thought of rebasing once the other patch (which reorganizes
"...specified more than once" error) gets committed. Anyways, I've
rebased for now on the latest master. Please review v2 patch.
Thanks for the updated patch.
One minor comment:
You can remove the brackets around errcode, You could change:
+ if (localeEl)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", defel->defname),
+ parser_errposition(pstate, defel->location)));
to:
+ if (localeEl)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", defel->defname),
+ parser_errposition(pstate, defel->location));Regards,
Vignesh
On Sat, May 29, 2021 at 9:08 PM vignesh C <vignesh21@gmail.com> wrote:
One minor comment: You can remove the brackets around errcode, You could change: + if (localeEl) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("option \"%s\" specified more than once", defel->defname), + parser_errposition(pstate, defel->location))); to: + if (localeEl) + ereport(ERROR, + errcode(ERRCODE_SYNTAX_ERROR), + errmsg("option \"%s\" specified more than once", defel->defname), + parser_errposition(pstate, defel->location));
Thanks. PSA v3 patch.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
v3-0001-Check-duplicate-options-and-error-out-for-CREATE-.patchapplication/octet-stream; name=v3-0001-Check-duplicate-options-and-error-out-for-CREATE-.patchDownload
From 83e1ae0c3fd6c1b733e4589e3f486c0afada92c3 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Sat, 29 May 2021 21:18:26 +0530
Subject: [PATCH v3] Check duplicate options and error out for CREATE COLLATION
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 | 42 +++++++++++++++++++++++++++
src/test/regress/expected/collate.out | 40 +++++++++++++++++++++++++
src/test/regress/sql/collate.sql | 19 ++++++++++++
3 files changed, 101 insertions(+)
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
index ebb0994db3..440067ef74 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -88,17 +88,59 @@ 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", defel->defname),
+ 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", defel->defname),
+ 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", defel->defname),
+ 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", defel->defname),
+ 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", defel->defname),
+ parser_errposition(pstate, defel->location));
defelp = &deterministicEl;
+ }
else if (strcmp(defel->defname, "version") == 0)
+ {
+ if (versionEl)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("option \"%s\" specified more than once", defel->defname),
+ parser_errposition(pstate, defel->location));
defelp = &versionEl;
+ }
else
{
ereport(ERROR,
diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
index 0b60b55514..edced86af2 100644
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -701,6 +701,46 @@ 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...
+ ^
+-- VERSION
+CREATE COLLATION coll_dup_chk (VERSION = '1', VERSION = "NONSENSE", LOCALE = '');
+ERROR: option "version" specified more than once
+LINE 1: CREATE COLLATION coll_dup_chk (VERSION = '1', VERSION = "NON...
+ ^
+-- 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..d691fae9d4 100644
--- a/src/test/regress/sql/collate.sql
+++ b/src/test/regress/sql/collate.sql
@@ -272,6 +272,25 @@ 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 = '');
+-- VERSION
+CREATE COLLATION coll_dup_chk (VERSION = '1', VERSION = "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
On Sat, May 29, 2021 at 9:20 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
On Sat, May 29, 2021 at 9:08 PM vignesh C <vignesh21@gmail.com> wrote:
One minor comment: You can remove the brackets around errcode, You could change: + if (localeEl) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("option \"%s\" specified more than once", defel->defname), + parser_errposition(pstate, defel->location))); to: + if (localeEl) + ereport(ERROR, + errcode(ERRCODE_SYNTAX_ERROR), + errmsg("option \"%s\" specified more than once", defel->defname), + parser_errposition(pstate, defel->location));Thanks. PSA v3 patch.
Thanks for the updated patch, the changes look good to me.
Regards,
Vignesh
On Mon, 31 May 2021 at 15:10, vignesh C <vignesh21@gmail.com> wrote:
On Sat, May 29, 2021 at 9:20 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Thanks. PSA v3 patch.
Thanks for the updated patch, the changes look good to me.
Hi,
Having pushed [1]/messages/by-id/CAEZATCXHWa9OoSAetiZiGQy1eM2raa9q-b3K4ZYDwtcARypCcA@mail.gmail.com, 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]/messages/by-id/CAEZATCXHWa9OoSAetiZiGQy1eM2raa9q-b3K4ZYDwtcARypCcA@mail.gmail.com.
So, attached is what I propose.
Regards,
Dean
[1]: /messages/by-id/CAEZATCXHWa9OoSAetiZiGQy1eM2raa9q-b3K4ZYDwtcARypCcA@mail.gmail.com
Attachments:
v4-check-CREATE-COLLATION-options.patchtext/x-patch; charset=US-ASCII; name=v4-check-CREATE-COLLATION-options.patchDownload
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
On Fri, Jul 16, 2021 at 1:04 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
Having pushed [1], I started looking at this, and I think it's mostly
in good shape.
Thanks a lot for taking a look at this.
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.
Here are some comments:
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";.
+ {
+ if (fromEl)
+ errorDuplicateDefElem(defel, pstate);
defelp = &fromEl;
And we might think to catch below errors:
postgres=# CREATE COLLATION coll_dup_chk (FROM = "C", FROM = "C",
VERSION = "1");
ERROR: conflicting or redundant options
LINE 1: CREATE COLLATION coll_dup_chk (FROM = "C", FROM = "C", VERSI...
^
DETAIL: Option "from" specified more than once.
But IMO, the above should fail with:
postgres=# CREATE COLLATION coll_dup_chk (FROM = "C", FROM = "C",
VERSION = "1");
ERROR: conflicting or redundant options
DETAIL: FROM cannot be specified together with any other options.
2) I don't understand the difference between errorConflictingDefElem
and errorDuplicateDefElem. Isn't the following statement "This should
only be used if defel->defname is guaranteed to match the user-entered
option name"
true with errorConflictingDefElem? I mean, aren't we calling
errorConflictingDefElem only if the defel->defname is guaranteed to
match the user-entered option name? I don't see much use of
errdetail("Option \"%s\" specified more than once.", defel->defname),
in errorDuplicateDefElem when we have pstate and that sort of points
to the option that's specified more than once.
+
+/*
+ * 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.
+ */I personally don't like the new function errorDuplicateDefElem as it
doesn't add any value given the presence of pstate.
Regards,
Bharath Rupireddy.
On Fri, 16 Jul 2021 at 06:40, Bharath Rupireddy
<bharath.rupireddyforpostgres@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
Attachments:
v5-check-CREATE-COLLATION-options.patchtext/x-patch; charset=US-ASCII; name=v5-check-CREATE-COLLATION-options.patchDownload
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
On Fri, Jul 16, 2021 at 1:32 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Fri, 16 Jul 2021 at 06:40, Bharath Rupireddy
<bharath.rupireddyforpostgres@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).
Thanks. The v5 patch LGTM.
Comment on errorConflictingDefElem:
I think that the message in errorConflictingDefElem should say
<<option \"%s\'' specified more than once>>. I'm not sure why it
wasn't done. Almost, all the cases where errorConflictingDefElem is
called from, def->defname would give the correct user specified option
name right, as errorConflictingDefElem called in if
(strcmp(def->defname, "foo") == 0) clause.
Is there any location the function errorConflictingDefElem gets called
when def->defname isn't a name that's specified by the user? Please
point me to that location. If there's a scenario, then the function
can be something like below:
void
errorConflictingDefElem(DefElem *defel, ParseState *pstate, bool
show_option_name)
{
if (show_option_name)
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("option \"%s\" specified more than once",
defel->defname),
pstate ? parser_errposition(pstate, defel->location) : 0);
else
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options"),
pstate ? parser_errposition(pstate, defel->location) : 0);
}
Regards,
Bharath Rupireddy.
On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
Thanks. The v5 patch LGTM.
OK, I'll push that in a while.
Comment on errorConflictingDefElem:
I think that the message in errorConflictingDefElem should say
<<option \"%s\'' specified more than once>>. I'm not sure why it
wasn't done. Almost, all the cases where errorConflictingDefElem is
called from, def->defname would give the correct user specified option
name right, as errorConflictingDefElem called in if
(strcmp(def->defname, "foo") == 0) clause.Is there any location the function errorConflictingDefElem gets called
when def->defname isn't a name that's specified by the user?
There are a few cases where def->defname isn't necessarily the name
that was specified by the user (e.g., "volatility", "strict",
"format", and probably more cases not spotted in the other thread,
which was only a cursory review), and it would be quite onerous to go
through every one of the 100+ places in the code where this error is
raised to check them all. 2bfb50b3df was more about making pstate
available in more places to add location information to existing
errors, and did not want the risk of changing and possibly worsening
existing errors.
If there's a scenario, then the function
can be something like below:
void
errorConflictingDefElem(DefElem *defel, ParseState *pstate, bool
show_option_name)
{
if (show_option_name)
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("option \"%s\" specified more than once",
defel->defname),
pstate ? parser_errposition(pstate, defel->location) : 0);
else
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
errmsg("conflicting or redundant options"),
pstate ? parser_errposition(pstate, defel->location) : 0);
}
I think it's preferable to have a single consistent primary error
message for all these cases. I wouldn't really want "CREATE FUNCTION
... STRICT STRICT" to throw a different error from "CREATE FUNCTION
... LEAKPROOF LEAKPROOF", but saying "option \"strict\" specified more
than once" would be odd for "CREATE FUNCTION ... CALLED ON NULL INPUT
RETURNS NULL ON NULL INPUT", which is indistinguishable from "STRICT
STRICT" in the code.
In any case, as you said before, the location is sufficient to
identify the offending option. Making this kind of change would
require going through all 100+ callers quite carefully, and a lot more
testing. I'm not really convinced that it's worth it.
Regards,
Dean
On Fri, Jul 16, 2021 at 4:47 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Thanks. The v5 patch LGTM.
OK, I'll push that in a while.
Thanks.
There are a few cases where def->defname isn't necessarily the name
that was specified by the user (e.g., "volatility", "strict",
"format", and probably more cases not spotted in the other thread,
which was only a cursory review), and it would be quite onerous to go
through every one of the 100+ places in the code where this error is
raised to check them all. 2bfb50b3df was more about making pstate
available in more places to add location information to existing
errors, and did not want the risk of changing and possibly worsening
existing errors.I think it's preferable to have a single consistent primary error
message for all these cases. I wouldn't really want "CREATE FUNCTION
... STRICT STRICT" to throw a different error from "CREATE FUNCTION
... LEAKPROOF LEAKPROOF", but saying "option \"strict\" specified more
than once" would be odd for "CREATE FUNCTION ... CALLED ON NULL INPUT
RETURNS NULL ON NULL INPUT", which is indistinguishable from "STRICT
STRICT" in the code.In any case, as you said before, the location is sufficient to
identify the offending option. Making this kind of change would
require going through all 100+ callers quite carefully, and a lot more
testing. I'm not really convinced that it's worth it.
Thanks for the examples. I get it. It doesn't make sense to show
"option "foo" specified more than once" for the cases where "foo" is
not necessarily an option that's specified in a WITH clause of a
statement, but it can be something like CREATE FUNCTION ... foo foo,
like you quoted above.
I also think that if it is specified as CREATE FUNCTION ... STRICT
STRICT, CREATE FUNCTION ... CALLED ON NULL INPUT RETURNS NULL ON NULL
INPUT etc. isn't the syntaxer catching that error while parsing the
SQL text, similar to CREATE COLLATION mycoll1 FROM FROM "C";? If we
don't want to go dig why the syntaxer isn't catching such errors, I
tend to agree to keep the errorConflictingDefElem as is given the
effort that one needs to put in identifying, fixing and testing the
changes.
Regards,
Bharath Rupireddy.
On Fri, 16 Jul 2021 at 12:17, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Fri, 16 Jul 2021 at 10:26, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:Thanks. The v5 patch LGTM.
OK, I'll push that in a while.
Pushed, with some additional tidying up.
In particular, I decided it was neater to follow the style of the code
in typecmds.c, and just do a single check for duplicates at the end of
the loop, since that makes for a significantly smaller patch, with
less code duplication. That, of course, means duplicate "from" options
are handled the same as any other option, but that's arguably more
consistent, and not particularly important anyway, since it's not a
documented syntax.
Regards,
Dean
On Sat, 17 Jul 2021 at 05:24, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
I also think that if it is specified as CREATE FUNCTION ... STRICT
STRICT, CREATE FUNCTION ... CALLED ON NULL INPUT RETURNS NULL ON NULL
INPUT etc. isn't the syntaxer catching that error while parsing the
SQL text, similar to CREATE COLLATION mycoll1 FROM FROM "C";?
No, they're processed quite differently. The initial parsing of CREATE
FUNCTION allows an arbitrary list of things like STRICT, CALLED ON
NULL INPUT, etc., which it turns into a list of DefElem that is only
checked later on. That's the most natural way to do it, since this is
really just a list of options that can appear in any order, much like
the version of CREATE COLLATION that allows options in parentheses,
which is quite different from the version that takes a single FROM.
Reading the relevant portions of gram.y is probably the easiest way to
understand it.
It's actually quite instructive to search for "makeDefElem" in gram.y,
and see all the places that create a DefElem that doesn't match the
user-entered syntax. There are quite a few of them, and there may be
others elsewhere.
Regards,
Dean