Re: Collation versioning

On Mon, Mar 16, 2020 at 03:05:20PM +0100, Julien Rouhaud wrote:
> On Mon, Mar 16, 2020 at 04:57:38PM +0900, Michael Paquier wrote:
>> This comes from a regression test doing a sanity check to look for
>> catalogs which have a toastable column but no toast tables. As an
>> exception, it should be documented in the test's comment. Actually,
>> does it need to be an exception? This does not depend on
>> relation-level facilities so there should be no risk of recursive
>> dependencies, though I have not looked in details at this part.
>
> I totally missed that, and I agree that there's no need for an exception, so
> fixed.

How long can actually be collation version strings? Note also
96cdeae, which makes sense for pg_depend to have one.

>> Regarding patch 0003, it would be nice to include some tests
>> independent on the rest and making use of the new functions. These
>> normally go in regproc.sql. For example with a collation that needs
>> double quotes as this is not obvious:
>> =# select regcollation('"POSIX"');
>> regcollation
>> --------------
>> "POSIX"
>> (1 row)
>>
>> On top of that, this needs tests with to_regcollation() and tests with
>> schema-qualified collations.
>
>
> Done too, using the same collation name, for both with and without schema
> qualification.

It seems to me that you could add an extra test with a catalog that
does not exist, making sure that NULL is returned:
SELECT to_regtype('ng_catalog."POSIX"');

The other two cases are not really doable in regproc.sql as they would
show up the encoding used in the error message, but there could be a
point to include them in collate.icu.utf8.sql or equivalent.

> Indeed. I found missing reference in datatype.sgml; func.sgml and
> pgupgrade.sgml.

That looks right.

<entry>
<indexterm><primary>pg_collation_actual_version</primary></indexterm>
- <literal><function>pg_collation_actual_version(<type>oid</type>)</function></literal>
+ <literal><function>pg_collation_actual_version(<type>regcollation</type>)</function></literal>
</entry>
The function's input argument is not changed, why?

Patch 0003 is visibly getting in shape, and that's an independent
piece. I guess that Thomas is looking at that, so let's wait for his
input.

Note that patch 0002 fails to compile because it is missing to include
utils/builtins.h for CStringGetTextDatum(), and that you cannot pass
down a NameData to this API, because it needs a simple char string or
you would need NameStr() or such. Anyway, it happens that you don't
need recordDependencyOnVersion() at all, because it is removed by
patch 0004 in the set, so you could just let it go.

I am still looking at the rest as of 0004~0007, the largest pieces.
--
Michael