Feedback-ID: i131946ab:Fastmail
Message-ID: <f0ca9ad8-67f9-6d18-ec15-9525f864fa12@enterprisedb.com>
Date: Mon, 31 Oct 2022 15:09:36 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.3.1
Subject: Re: WIN32 pg_import_system_collations
Content-Language: en-US
To: =?UTF-8?Q?Juan_Jos=c3=a9_Santamar=c3=ada_Flecha?=
 <juanjo.santamaria@gmail.com>
Cc: Dmitry Koval <d.koval@postgrespro.ru>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <0050ec23-34d9-2765-9015-98c04f0e18ac@postgrespro.ru>
 <639e0abd-ea82-b751-bfba-5166a7a1b304@enterprisedb.com>
 <CAC+AXB0kd+YgF408sy5c2Bsa3PQdiSS6YJZLq6TMx33_WTJ-rA@mail.gmail.com>
 <20220322010057.w7mzoahyi3jy5dyf@alap3.anarazel.de>
 <CAC+AXB1v_H68VQoXK=Pdnc7VwUC-9ifdQEtvyx_qEkMiBGJKbw@mail.gmail.com>
 <CAC+AXB1ha0tnWcqMdjaBq_Lkw346E+h_5UqeYK5NZ29WR8a-bQ@mail.gmail.com>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: 
 <CAC+AXB1ha0tnWcqMdjaBq_Lkw346E+h_5UqeYK5NZ29WR8a-bQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/f0ca9ad8-67f9-6d18-ec15-9525f864fa12%40enterprisedb.com>
Precedence: bulk

On 12.07.22 21:32, Juan José Santamaría Flecha wrote:
> Please find attached a rebased version. I have split the patch into two 
> parts trying to make it easier to review, one with the code changes and 
> the other with the test.
> 
> Other than that, there are minimal changes from the previous version to 
> the code due to the update of _WIN32_WINNT and enabling the test on cirrus.

I'm not familiar with Windows, so I'm just looking at the overall 
structure of this patch.  I think it pretty much makes sense.  But we 
need to consider that this operates on the confluence of various 
different operating system interfaces that not all people will be 
familiar with, so we need to really get the documentation done well.

Consider this function you are introducing:

+/*
+ * Create a collation if the input locale is valid for so.
+ * Also keeps track of the number of valid locales and collations created.
+ */
+static int
+CollationFromLocale(char *isolocale, char *localebuf, int *nvalid,
+                                       int *ncreated, int nspid)

This declaration is incomprehensible without studying all the callers 
and the surrounding code.

Start with the name: What does "collation from locale" mean?  Does it 
make a collation?  Does it convert one?  Does it find one?  There should 
be a verb in there.

(I think in the context of this file, a lower case name would be more 
appropriate for a static function.)

Then the arguments.  The input arguments should be "const".  All the 
arguments should be documented.  What is "isolocale", what is 
"localebuf", how are they different?  What is being counted by "valid" 
(collatons?, locales?), and what makes a thing valid and invalid?  What 
is being "created"?  What is nspid?  What is the return value?

Please make another pass over this.

Also consider describing in the commit message what you are doing in 
more detail, including some of the things that have been discussed in 
this thread.