On 05.03.22 09:38, Julien Rouhaud wrote:
@@ -168,18 +175,6 @@ DefineCollation(ParseState *pstate, List *names, List
*parameters, bool if_not_e
errmsg("collation \"default\" cannot be copied")));
}
- if (localeEl)
- {
- collcollate = defGetString(localeEl);
- collctype = defGetString(localeEl);
- }
[...]
I tried to read the function and quickly got confused about whether possible
problematic conditions could be reached or not and had protection or not. I
think that DefineCollation is becoming more and more unreadable, with a mix of
fromEl, !fromEl and either. Maybe it would be a good time to refactor the code
a bit and have have something like
if (fromEl)
{
// initialize all specific things
}
else
{
// initialize all specific things
}
// handle defaults and sanity checks
What do you think?
How about the attached?
From c7b408ba646c5ee5f8c6ae84bec04ca4059ed08f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Thu, 10 Mar 2022 10:49:49 +0100
Subject: [PATCH] DefineCollation() code cleanup
Reorganize the code in DefineCollation() so that the parts using the
FROM clause and the parts not doing so are more cleanly separated. No
functionality change intended.
Reported-by: Julien Rouhaud <rjuju...@gmail.com>
---
src/backend/commands/collationcmds.c | 109 ++++++++++++++-------------
1 file changed, 57 insertions(+), 52 deletions(-)
diff --git a/src/backend/commands/collationcmds.c
b/src/backend/commands/collationcmds.c
index 12fc2316f9..93df1d366c 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -63,12 +63,11 @@ DefineCollation(ParseState *pstate, List *names, List
*parameters, bool if_not_e
DefElem *providerEl = NULL;
DefElem *deterministicEl = NULL;
DefElem *versionEl = NULL;
- char *collcollate = NULL;
- char *collctype = NULL;
- char *collproviderstr = NULL;
- bool collisdeterministic = true;
- int collencoding = 0;
- char collprovider = 0;
+ char *collcollate;
+ char *collctype;
+ bool collisdeterministic;
+ int collencoding;
+ char collprovider;
char *collversion = NULL;
Oid newoid;
ObjectAddress address;
@@ -167,65 +166,71 @@ DefineCollation(ParseState *pstate, List *names, List
*parameters, bool if_not_e
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("collation \"default\" cannot
be copied")));
}
-
- if (localeEl)
+ else
{
- collcollate = defGetString(localeEl);
- collctype = defGetString(localeEl);
- }
+ char *collproviderstr = NULL;
- if (lccollateEl)
- collcollate = defGetString(lccollateEl);
+ collcollate = NULL;
+ collctype = NULL;
- if (lcctypeEl)
- collctype = defGetString(lcctypeEl);
+ if (localeEl)
+ {
+ collcollate = defGetString(localeEl);
+ collctype = defGetString(localeEl);
+ }
- if (providerEl)
- collproviderstr = defGetString(providerEl);
+ if (lccollateEl)
+ collcollate = defGetString(lccollateEl);
- if (deterministicEl)
- collisdeterministic = defGetBoolean(deterministicEl);
+ if (lcctypeEl)
+ collctype = defGetString(lcctypeEl);
- if (versionEl)
- collversion = defGetString(versionEl);
+ if (providerEl)
+ collproviderstr = defGetString(providerEl);
- if (collproviderstr)
- {
- if (pg_strcasecmp(collproviderstr, "icu") == 0)
- collprovider = COLLPROVIDER_ICU;
- else if (pg_strcasecmp(collproviderstr, "libc") == 0)
- collprovider = COLLPROVIDER_LIBC;
+ if (deterministicEl)
+ collisdeterministic = defGetBoolean(deterministicEl);
+ else
+ collisdeterministic = true;
+
+ if (versionEl)
+ collversion = defGetString(versionEl);
+
+ if (collproviderstr)
+ {
+ if (pg_strcasecmp(collproviderstr, "icu") == 0)
+ collprovider = COLLPROVIDER_ICU;
+ else if (pg_strcasecmp(collproviderstr, "libc") == 0)
+ collprovider = COLLPROVIDER_LIBC;
+ else
+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("unrecognized collation
provider: %s",
+
collproviderstr)));
+ }
else
+ collprovider = COLLPROVIDER_LIBC;
+
+ if (!collcollate)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("unrecognized collation
provider: %s",
- collproviderstr)));
- }
- else if (!fromEl)
- collprovider = COLLPROVIDER_LIBC;
-
- if (!collcollate)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("parameter \"lc_collate\" must be
specified")));
+ errmsg("parameter \"lc_collate\" must
be specified")));
- if (!collctype)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("parameter \"lc_ctype\" must be
specified")));
+ if (!collctype)
+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("parameter \"lc_ctype\" must be
specified")));
- /*
- * Nondeterministic collations are currently only supported with ICU
- * because that's the only case where it can actually make a difference.
- * So we can save writing the code for the other providers.
- */
- if (!collisdeterministic && collprovider != COLLPROVIDER_ICU)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("nondeterministic collations not
supported with this provider")));
+ /*
+ * Nondeterministic collations are currently only supported
with ICU
+ * because that's the only case where it can actually make a
difference.
+ * So we can save writing the code for the other providers.
+ */
+ if (!collisdeterministic && collprovider != COLLPROVIDER_ICU)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("nondeterministic collations
not supported with this provider")));
- if (!fromEl)
- {
if (collprovider == COLLPROVIDER_ICU)
{
#ifdef USE_ICU
--
2.35.1
