Re: Collation versioning

On Tue, Jan 28, 2020 at 1:06 PM Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>
> On 2019-12-17 14:25, Julien Rouhaud wrote:
> > PFA rebased v6, with the following changes:
>
> Some thoughts on this patch set.

Thanks for looking at it!

> I think we are all agreed on the general idea.
>
> 0001-0003 seem pretty much OK. Why is pg_depend.refobjversion of type
> "name" whereas the previous pg_collation.collversion was type "text"?
> Related to that, we previously used null to indicate an unknown
> collation version, and now it's an empty string.

That's what Thomas implemented when he started to work on it and I
simply kept it that way until now. I'm assuming that it was simply to
avoid wasting time on the varlena stuff while working on the
prototype, so barring any objections I'll change to nullable text
column in the next revision.

> For 0005, if the new commands are only to be used in binary upgrades,
> then they should be implemented as built-in functions instead of full
> DDL commands. There is precedent for that.

Oh, I wasn't aware of that. I can definitely use built-in functions
instead, but some people previously argued that those command should
be available even in non binary upgrade and I'm not clear on whether
this is wanted or not. Do you have any thoughts on that?

> The documentation snippet for this patch talks about upgrades from PG12
> to later. But what about upgrades on platforms where we currently don't
> have collation versioning but might introduce it later (FreeBSD,
> Windows)? This needs to be generalized.

Good point, I'll try to improve that.

> For 0006 ("Add a new ALTER INDEX name DEPENDS ON COLLATION name
> CURRENT VERSION"), I find the syntax misleading. This command doesn't
> (or shouldn't) add a new dependency, it only changes the version of an
> existing dependency. The previously used syntax ALTER COLLATION /
> REFRESH VERSION is a better vocabulary, I think.

I agree and also complained about that syntax too. I'm however
struggling on coming up with a syntax that makes it clear it's
refreshing the version of a collation the index already depends on.
E.g.:

ALTER INDEX name ALTER COLLATION name REFRESH VERSION

is still quite poor, but I don't have anything better. Do you have
some better suggestion or should I go with that?

> I think this whole thing needs more tests. We are designing this
> delicate mechanism but have no real tests for it. We at least need to
> come up with a framework for how to test this automatically, so that we
> can add more test cases over time as people will invariably report
> issues when this hits the real world.

Indeed. I have some unlikely index test cases I'm for now using to
validate the behavior, but didn't start a real test infrastructure.
I'll also work on that for the next revision, although I'll need some
more thinking on how to properly test the pg_upgrade part.