Re: Collation versioning - Mailing list pgsql-hackers

On Tue, Mar 17, 2020 at 03:37:49PM +0900, Michael Paquier wrote:
> 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.


Versions shouldn't be that long usually, but there were some previous
discussions on how to try to come up with some workaround on systems that don't
provide a version, using a hash of the underlying file or something like that.
Using a hash value big enough to require toasting wouldn't make much sense, but
it feels safer to be ready to handle any later use, whether for collation or
other kind of objects


> >> 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"');


Agreed, I'll add that, but using a name that looks less like a typo :)


>    <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?


That's a mistake, will fix.


> 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.


Ah good catch, I missed that during the NameData/text refactoring.  I'll fix it
anyway, better to have clean history.