On 1/9/17 10:04 PM, Euler Taveira wrote:
> On 18-12-2016 18:30, Peter Eisentraut wrote:
>> Updated patch with that fix.
>>
> Peter, I reviewed and improved your patch.
>
> * I document the new function. Since collation is a database object, I
> chose "Database Object Management Functions" section.
OK
> * I've added a check to any-encoding database because I got 'FATAL:
> collation "C" already exists' on Debian 8, although, I didn't get on
> CentOS 7. The problem is that Debian has two locales for C (C and
> C.UTF-8) and CentOS has just one (C).
OK
> * I've added OidIsValid to test the new collation row.
OK
> * I've changed the parameter order. Schema seems more important than
> if_not_exists. Also, we generally leave those boolean parameters for the
> end of list. I don't turn if_not_exists optional but IMO it would be a
> good idea (default = true).
I put them that way because in an SQL command the "IF NOT EXISTS" comes
before the schema, but I can see how it is weird that way in a function.
> * You removed some #if and #ifdef while moving things around. I put it back.
> * You didn't pgident some lines of code but I'm sure you didn't for a
> small patch footprint.
I had left the #if in initdb, but I think your changes are better.
> I'm attaching the complete and also a patch at the top of your last patch.
Thanks. If there are no more comments, I will proceed with that.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
