[Peter Eisentraut <peter.eisentraut@enterprisedb.com>]

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?

Attachments:

0001-DefineCollation-code-cleanup.patch (text/plain, 4.8K) download
From c7b408ba646c5ee5f8c6ae84bec04ca4059ed08f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@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 <rjuju123@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