Horton

Thread

Commits

  1. Doc: Update notes about libc collation versions.

  2. Revert per-index collation version tracking feature.

  3. Rethink extraction of collation dependencies.

  4. Fix bogus collation-version-recording logic.

  5. Rename the "point is strictly above/below point" comparison operators.

  6. Move catalog toast table declarations

  1. Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-14T17:18:07Z

    I noticed some broken-looking logic in recordMultipleDependencies
concerning how it records collation versions.  It was a bit harder
than I expected to demonstrate the bugs, but I eventually succeeded
with

u8=# create function foo(varchar) returns bool language sql return false;
CREATE FUNCTION
u8=# create collation mycoll from "en_US";
CREATE COLLATION
u8=# CREATE DOMAIN d4 AS character varying(3) COLLATE "aa_DJ"
    CONSTRAINT yes_or_no_check CHECK (value = 'YES' collate mycoll or foo(value));
CREATE DOMAIN
u8=# select objid, pg_describe_object(classid,objid,objsubid) as obj, pg_describe_object(refclassid,refobjid,refobjsubid) as ref, deptype, refobjversion from pg_depend where objid = 'd4'::regtype;
 objid |   obj   |        ref        | deptype | refobjversion 
-------+---------+-------------------+---------+---------------
 37421 | type d4 | schema public     | n       | 
 37421 | type d4 | collation "aa_DJ" | n       | 
(2 rows)

u8=# select objid, pg_describe_object(classid,objid,objsubid) as obj, pg_describe_object(refclassid,refobjid,refobjsubid) as ref, deptype, refobjversion from pg_depend where refobjid = 'd4'::regtype;
 objid |            obj             |   ref   | deptype | refobjversion 
-------+----------------------------+---------+---------+---------------
 37420 | type d4[]                  | type d4 | i       | 
 37422 | constraint yes_or_no_check | type d4 | a       | 
(2 rows)

u8=# select objid, pg_describe_object(classid,objid,objsubid) as obj, pg_describe_object(refclassid,refobjid,refobjsubid) as ref, deptype, refobjversion from pg_depend where objid = 37422;
 objid |            obj             |               ref               | deptype | refobjversion 
-------+----------------------------+---------------------------------+---------+---------------
 37422 | constraint yes_or_no_check | type d4                         | a       | 
 37422 | constraint yes_or_no_check | collation mycoll                | n       | 2.28
 37422 | constraint yes_or_no_check | function foo(character varying) | n       | 2.28
 37422 | constraint yes_or_no_check | collation "default"             | n       | 
(4 rows)

(This is in a glibc-based build, with C as the database's default
collation.)

One question here is whether it's correct that the domain's dependency
on collation "aa_DJ" is unversioned.  Maybe that's intentional, but it
seems worth asking.

Anyway, there are two pretty obvious bugs in the dependencies for the
domain's CHECK constraint: the version for collation mycoll leaks
into the entry for function foo, and an entirely useless (because
unversioned) dependency is recorded on the default collation.

... well, it's almost entirely useless.  If we fix things to not do that
(as per patch 0001 below), the results of the create_index regression
test become unstable, because there's two queries that inquire into the
dependencies of indexes, and their results change depending on whether
the default collation has a version or not.  I'd be inclined to just
take out the portions of that test that depend on that question, but
maybe somebody will complain that there's a loss of useful coverage.
I don't agree, but maybe I'll be overruled.

If we do feel we need to stay bug-compatible with that behavior, then
the alternate 0002 patch just fixes the version-leakage-across-entries
problem, while still removing the unnecessary assumption that C, POSIX,
and DEFAULT are the only pinned collations.

(To be clear: 0002 passes check-world as-is, while 0001 is not
committable without some regression-test fiddling.)

Thoughts?

			regards, tom lane

  2. Re: Bogus collation version recording in recordMultipleDependencies

    Julien Rouhaud <rjuju123@gmail.com> — 2021-04-15T10:56:47Z

    On Wed, Apr 14, 2021 at 01:18:07PM -0400, Tom Lane wrote:
> 
> One question here is whether it's correct that the domain's dependency
> on collation "aa_DJ" is unversioned.  Maybe that's intentional, but it
> seems worth asking.

This is intentional I think, we should record collation version only for object
that might break if the collation version is updated.  So creating an index on
that domain would record the collation version.

> Anyway, there are two pretty obvious bugs in the dependencies for the
> domain's CHECK constraint: the version for collation mycoll leaks
> into the entry for function foo, and an entirely useless (because
> unversioned) dependency is recorded on the default collation.

Agreed.

> (To be clear: 0002 passes check-world as-is, while 0001 is not
> committable without some regression-test fiddling.)

I'm probably missing something obvious but both 0001 and 0002 pass check-world
for me, on a glibc box and --with-icu.

> Thoughts?

I think this is an open item, so I added one for now.

  3. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-15T14:06:24Z

    Julien Rouhaud <rjuju123@gmail.com> writes:
> On Wed, Apr 14, 2021 at 01:18:07PM -0400, Tom Lane wrote:
>> (To be clear: 0002 passes check-world as-is, while 0001 is not
>> committable without some regression-test fiddling.)

> I'm probably missing something obvious but both 0001 and 0002 pass check-world
> for me, on a glibc box and --with-icu.

0001 fails for me :-(.  I think that requires default collation to be C.

			regards, tom lane

  4. Re: Bogus collation version recording in recordMultipleDependencies

    Julien Rouhaud <rjuju123@gmail.com> — 2021-04-16T02:56:05Z

    On Thu, Apr 15, 2021 at 10:06:24AM -0400, Tom Lane wrote:
> Julien Rouhaud <rjuju123@gmail.com> writes:
> > On Wed, Apr 14, 2021 at 01:18:07PM -0400, Tom Lane wrote:
> >> (To be clear: 0002 passes check-world as-is, while 0001 is not
> >> committable without some regression-test fiddling.)
> 
> > I'm probably missing something obvious but both 0001 and 0002 pass check-world
> > for me, on a glibc box and --with-icu.
> 
> 0001 fails for me :-(.  I think that requires default collation to be C.

Oh right, adding --no-locale to the regress opts I see that create_index is
failing, and that's not the one I was expecting.

We could change create_index test to create c2 with a C collation, in order to
test that we don't track dependency on unversioned locales, and add an extra
test in collate.linux.utf8 to check that we do track a dependency on the
default collation as this test isn't run in the --no-locale case.  The only
case not tested would be default unversioned collation, but I'm not sure where
to properly test that.  Maybe a short leading test in collate.linux.utf8 that
would be run on linux in that case (when getdatabaseencoding() != 'UTF8')?  It
would require an extra alternate file but it wouldn't cause too much
maintenance problem as there should be only one test.

  5. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-16T14:03:42Z

    Julien Rouhaud <rjuju123@gmail.com> writes:
> On Thu, Apr 15, 2021 at 10:06:24AM -0400, Tom Lane wrote:
>> 0001 fails for me :-(.  I think that requires default collation to be C.

> Oh right, adding --no-locale to the regress opts I see that create_index is
> failing, and that's not the one I was expecting.

> We could change create_index test to create c2 with a C collation, in order to
> test that we don't track dependency on unversioned locales, and add an extra
> test in collate.linux.utf8 to check that we do track a dependency on the
> default collation as this test isn't run in the --no-locale case.  The only
> case not tested would be default unversioned collation, but I'm not sure where
> to properly test that.  Maybe a short leading test in collate.linux.utf8 that
> would be run on linux in that case (when getdatabaseencoding() != 'UTF8')?  It
> would require an extra alternate file but it wouldn't cause too much
> maintenance problem as there should be only one test.

Since the proposed patch removes the dependency code's special-case
handling of the default collation, I don't feel like we need to jump
through hoops to prove that the default collation is tracked the
same as other collations.  A regression test with alternative outputs
is a significant ongoing maintenance burden, and I do not see where
we're getting a commensurate improvement in test coverage.  Especially
since, AFAICS, the two alternative outputs would essentially have to
accept both the "it works" and "it doesn't work" outcomes.

So I propose that we do 0001 below, which is my first patch plus your
suggestion about fixing up create_index.sql.  This passes check-world
for me under both C and en_US.utf8 prevailing locales.

			regards, tom lane

  6. Re: Bogus collation version recording in recordMultipleDependencies

    Julien Rouhaud <rjuju123@gmail.com> — 2021-04-16T15:55:35Z

    On Fri, Apr 16, 2021 at 10:03:42AM -0400, Tom Lane wrote:
> 
> Since the proposed patch removes the dependency code's special-case
> handling of the default collation, I don't feel like we need to jump
> through hoops to prove that the default collation is tracked the
> same as other collations.  A regression test with alternative outputs
> is a significant ongoing maintenance burden, and I do not see where
> we're getting a commensurate improvement in test coverage.  Especially
> since, AFAICS, the two alternative outputs would essentially have to
> accept both the "it works" and "it doesn't work" outcomes.

Fine by me, I was mentioning those if we wanted to keep some extra coverage for
that by I agree it doesn't add much value.

> So I propose that we do 0001 below, which is my first patch plus your
> suggestion about fixing up create_index.sql.  This passes check-world
> for me under both C and en_US.utf8 prevailing locales.

That's what I ended up with too, so LGTM!

  7. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-16T16:27:56Z

    Julien Rouhaud <rjuju123@gmail.com> writes:
> On Fri, Apr 16, 2021 at 10:03:42AM -0400, Tom Lane wrote:
>> So I propose that we do 0001 below, which is my first patch plus your
>> suggestion about fixing up create_index.sql.  This passes check-world
>> for me under both C and en_US.utf8 prevailing locales.

> That's what I ended up with too, so LGTM!

Pushed, thanks for review!  (and I'll update the open items list in a
sec)

			regards, tom lane

  8. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-16T16:55:28Z

    I wrote:
>> That's what I ended up with too, so LGTM!

> Pushed, thanks for review!  (and I'll update the open items list in a
> sec)

... or maybe not just yet.  Andres' buildfarm critters seem to have
a different opinion than my machine about what the output of
collate.icu.utf8 ought to be.  I wonder what the prevailing LANG
setting is for them, and which ICU version they're using.

			regards, tom lane

  9. Re: Bogus collation version recording in recordMultipleDependencies

    Andres Freund <andres@anarazel.de> — 2021-04-16T17:04:16Z

    Hi,

On 2021-04-16 12:55:28 -0400, Tom Lane wrote:
> I wrote:
> >> That's what I ended up with too, so LGTM!
> 
> > Pushed, thanks for review!  (and I'll update the open items list in a
> > sec)
> 
> ... or maybe not just yet.  Andres' buildfarm critters seem to have
> a different opinion than my machine about what the output of
> collate.icu.utf8 ought to be.  I wonder what the prevailing LANG
> setting is for them, and which ICU version they're using.

andres@andres-pg-buildfarm-valgrind:~/src/pgbuildfarm-client-stock$ grep calliph *.conf
build-farm-copyparse.conf:    animal => "calliphoridae",
build-farm-copyparse.conf:    build_root => '/mnt/resource/andres/bf/calliphoridae',

andres@andres-pg-buildfarm-valgrind:~/src/pgbuildfarm-client-stock$ dpkg -l|grep icu
ii  icu-devtools                         67.1-6                         amd64        Development utilities for International Components for Unicode
ii  libicu-dev:amd64                     67.1-6                         amd64        Development files for International Components for Unicode
ii  libicu67:amd64                       67.1-6                         amd64        International Components for Unicode

andres@andres-pg-buildfarm-valgrind:~/src/pgbuildfarm-client-stock$ locale
LANG=C.UTF-8
LANGUAGE=
LC_CTYPE="C.UTF-8"
LC_NUMERIC="C.UTF-8"
LC_TIME="C.UTF-8"
LC_COLLATE="C.UTF-8"
LC_MONETARY="C.UTF-8"
LC_MESSAGES="C.UTF-8"
LC_PAPER="C.UTF-8"
LC_NAME="C.UTF-8"
LC_ADDRESS="C.UTF-8"
LC_TELEPHONE="C.UTF-8"
LC_MEASUREMENT="C.UTF-8"
LC_IDENTIFICATION="C.UTF-8"
LC_ALL=

Greetings,

Andres Freund

  10. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-16T17:07:52Z

    I wrote:
> ... or maybe not just yet.  Andres' buildfarm critters seem to have
> a different opinion than my machine about what the output of
> collate.icu.utf8 ought to be.  I wonder what the prevailing LANG
> setting is for them, and which ICU version they're using.

Oh, I bet it's "C.utf8", because I can reproduce the failure with that.
This crystallizes a nagging feeling I'd had that you were misdescribing
the collate.icu.utf8 test as not being run under --no-locale.  Actually,
it's only skipped if the encoding isn't UTF8, not the same thing.
I think we need to remove the default-collation cases from that test too.

			regards, tom lane

  11. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-16T17:13:54Z

    Andres Freund <andres@anarazel.de> writes:
> On 2021-04-16 12:55:28 -0400, Tom Lane wrote:
>> ... or maybe not just yet.  Andres' buildfarm critters seem to have
>> a different opinion than my machine about what the output of
>> collate.icu.utf8 ought to be.  I wonder what the prevailing LANG
>> setting is for them, and which ICU version they're using.

> LANG=C.UTF-8

I'd guessed that shortly later, but thanks for confirming.

			regards, tom lane

  12. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-16T17:45:49Z

    I wrote:
> Oh, I bet it's "C.utf8", because I can reproduce the failure with that.
> This crystallizes a nagging feeling I'd had that you were misdescribing
> the collate.icu.utf8 test as not being run under --no-locale.  Actually,
> it's only skipped if the encoding isn't UTF8, not the same thing.
> I think we need to remove the default-collation cases from that test too.

Hmm ... this is more subtle than it seemed.

I tried to figure out where the default-collation dependencies were coming
from, and it's quite non-obvious, at least for some of them.  Observe:

u8de=# create table t1 (f1 text collate "fr_FR");
CREATE TABLE
u8de=# create index on t1(f1) where f1 > 'foo';
CREATE INDEX
u8de=# SELECT objid::regclass, refobjid::regcollation, refobjversion
FROM pg_depend d
LEFT JOIN pg_class c ON c.oid = d.objid
WHERE refclassid = 'pg_collation'::regclass
AND coalesce(relkind, 'i') = 'i'
AND relname LIKE 't1_%';
   objid   | refobjid  | refobjversion 
-----------+-----------+---------------
 t1_f1_idx | "fr_FR"   | 2.28
 t1_f1_idx | "fr_FR"   | 2.28
 t1_f1_idx | "default" | 2.28
(3 rows)

(The "default" item doesn't show up if default collation is C,
which is what's causing the buildfarm instability.)

Now, it certainly looks like that index definition ought to only
have fr_FR dependencies.  I dug into it and discovered that the
reason we're coming up with a dependency on "default" is that
the WHERE clause looks like

	         {OPEXPR 
	         :opno 666 
	         :opfuncid 742 
	         :opresulttype 16 
	         :opretset false 
	         :opcollid 0 
	         :inputcollid 14484 
	         :args (
	            {VAR 
	            :varno 1 
	            :varattno 1 
	            :vartype 25 
	            :vartypmod -1 
	            :varcollid 14484 
	            :varlevelsup 0 
	            :varnosyn 1 
	            :varattnosyn 1 
	            :location 23
	            }
	            {CONST 
	            :consttype 25 
	            :consttypmod -1 
	            :constcollid 100 
	            :constlen -1 
	            :constbyval false 
	            :constisnull false 
	            :location 28 
	            :constvalue 7 [ 28 0 0 0 102 111 111 ]
	            }
	         )
	         :location 26
	         }

So sure enough, the comparison operator's inputcollid is
fr_FR, but the 'foo' constant has constcollid = "default".
That will have exactly zero impact on the semantics of the
expression, but dependency.c doesn't realize that and
reports it as a dependency anyway.

I feel like this is telling us that there's a fundamental
misunderstanding in find_expr_references_walker about which
collation dependencies to report.  It's reporting all the
leaf-node collations, and ignoring the ones that actually
count semantically, that is the inputcollid fields of
function and operator nodes.

Not sure what's the best thing to do here.  Redesigning
this post-feature-freeze doesn't seem terribly appetizing,
but on the other hand, this index collation recording
feature has put a premium on not overstating the collation
dependencies of an expression.  We don't want to tell users
that an index is broken when it isn't really.

			regards, tom lane

  13. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-16T20:39:39Z

    I wrote:
> I feel like this is telling us that there's a fundamental
> misunderstanding in find_expr_references_walker about which
> collation dependencies to report.  It's reporting all the
> leaf-node collations, and ignoring the ones that actually
> count semantically, that is the inputcollid fields of
> function and operator nodes.
> Not sure what's the best thing to do here.  Redesigning
> this post-feature-freeze doesn't seem terribly appetizing,
> but on the other hand, this index collation recording
> feature has put a premium on not overstating the collation
> dependencies of an expression.  We don't want to tell users
> that an index is broken when it isn't really.

I felt less hesitant to modify find_expr_references_walker's
behavior w.r.t. collations after realizing that most of it
was not of long standing, but came in with 257836a75.
So here's a draft patch that redesigns it as suggested above.
Along the way I discovered that GetTypeCollations was quite
broken for ranges and arrays, so this fixes that too.

Per the changes in collate.icu.utf8.out, this gets rid of
a lot of imaginary collation dependencies, but it also gets
rid of some arguably-real ones.  In particular, calls of
record_eq and its siblings will be considered not to have
any collation dependencies, although we know that internally
those will look up per-column collations of their input types.
We could imagine special-casing record_eq etc here, but that
sure seems like a hack.

I"m starting to have a bad feeling about 257836a75 overall.
As I think I've complained before, I do not like anything about
what it's done to pg_depend; it's forcing that relation to serve
two masters, neither one well.  We now see that the same remark
applies to find_expr_references(), because the semantics of
"which collations does this expression's behavior depend on" aren't
identical to "which collations need to be recorded as direct
dependencies of this expression", especially not if you'd prefer
to minimize either list.  (Which is important.)  Moreover, for all
the complexity it's introducing, it's next door to useless for
glibc collations --- we might as well tell people "reindex
everything when your glibc version changes", which could be done
with a heck of a lot less infrastructure.  The situation on Windows
looks pretty user-unfriendly as well, per the other thread.

So I wonder if, rather than continuing to pursue this right now,
we shouldn't revert 257836a75 and try again later with a new design
that doesn't try to commandeer the existing dependency infrastructure.
We might have a better idea about what to do on Windows by the time
that's done, too.

			regards, tom lane

  14. Re: Bogus collation version recording in recordMultipleDependencies

    Thomas Munro <thomas.munro@gmail.com> — 2021-04-16T22:01:53Z

    On Sat, Apr 17, 2021 at 8:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Per the changes in collate.icu.utf8.out, this gets rid of
> a lot of imaginary collation dependencies, but it also gets
> rid of some arguably-real ones.  In particular, calls of
> record_eq and its siblings will be considered not to have
> any collation dependencies, although we know that internally
> those will look up per-column collations of their input types.
> We could imagine special-casing record_eq etc here, but that
> sure seems like a hack.

Thanks for looking into all this.  Hmm.

> I"m starting to have a bad feeling about 257836a75 overall.
> As I think I've complained before, I do not like anything about
> what it's done to pg_depend; it's forcing that relation to serve
> two masters, neither one well. ...

We did worry about (essentially) this question quite a bit in the
discussion thread, but we figured that you'd otherwise have to create
a parallel infrastructure that would look almost identical (for
example [1]).

> ...  We now see that the same remark
> applies to find_expr_references(), because the semantics of
> "which collations does this expression's behavior depend on" aren't
> identical to "which collations need to be recorded as direct
> dependencies of this expression", especially not if you'd prefer
> to minimize either list.  (Which is important.) ...

Bugs in the current analyser code aside, if we had a second catalog
and a second analyser for this stuff, then you'd still have the union
of both minimised sets in total, with some extra duplication because
you'd have some rows in both places that are currently handled by one
row, no?

> ... Moreover, for all
> the complexity it's introducing, it's next door to useless for
> glibc collations --- we might as well tell people "reindex
> everything when your glibc version changes", which could be done
> with a heck of a lot less infrastructure. ...

You do gain reliable tracking of which indexes remain to be rebuilt,
and warnings for common hazards like hot standbys with mismatched
glibc, so I think it's pretty useful.  As for the poverty of
information from glibc, I don't see why it should hold ICU, Windows,
FreeBSD users back.  In fact I am rather hoping that by shipping this,
glibc developers will receive encouragement to add the trivial
interface we need to do better.

> ... The situation on Windows
> looks pretty user-unfriendly as well, per the other thread.

That is unfortunate, it seems like such a stupid problem.  Restating
here for the sake of the list:  initdb just needs to figure out how to
ask for the current environment's locale in BCP 47 format ("en-US")
when setting the default for your template databases, not the
traditional format ("English_United States.1252") that Microsoft
explicitly tells us not to store in databases and that doesn't work in
the versioning API, but since we're mostly all Unix hackers we don't
know how.

> So I wonder if, rather than continuing to pursue this right now,
> we shouldn't revert 257836a75 and try again later with a new design
> that doesn't try to commandeer the existing dependency infrastructure.
> We might have a better idea about what to do on Windows by the time
> that's done, too.

It seems to me that there are two things that would be needed to
salvage this for PG14: (1) deciding that we're unlikely to come up
with a better idea than using pg_depend for this (following the
argument that it'd only create duplication to have a parallel
dedicated catalog), (2) fixing any remaining flaws in the dependency
analyser code.  I'll look into the details some more on Monday.

[1] https://www.postgresql.org/message-id/e9e22c5e-c018-f4ea-24c8-5b6d6fdacf30%402ndquadrant.com

  15. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-16T22:47:04Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> I'll look into the details some more on Monday.

Fair enough.

Although there are only a few buildfarm members complaining, I don't
really want to leave them red all weekend.  I could either commit the
patch I just presented, or revert ef387bed8 ... got a preference?

			regards, tom lane

  16. Re: Bogus collation version recording in recordMultipleDependencies

    Thomas Munro <thomas.munro@gmail.com> — 2021-04-17T01:34:28Z

    On Sat, Apr 17, 2021 at 10:47 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > I'll look into the details some more on Monday.
>
> Fair enough.
>
> Although there are only a few buildfarm members complaining, I don't
> really want to leave them red all weekend.  I could either commit the
> patch I just presented, or revert ef387bed8 ... got a preference?

+1 for committing the new patch for now.  I will look into to the
record problem.  More in a couple of days.

  17. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-17T02:24:21Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Sat, Apr 17, 2021 at 10:47 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Although there are only a few buildfarm members complaining, I don't
>> really want to leave them red all weekend.  I could either commit the
>> patch I just presented, or revert ef387bed8 ... got a preference?

> +1 for committing the new patch for now.  I will look into to the
> record problem.  More in a couple of days.

OK, done.

			regards, tom lane

  18. Re: Bogus collation version recording in recordMultipleDependencies

    Julien Rouhaud <rjuju123@gmail.com> — 2021-04-17T09:23:09Z

    On Fri, Apr 16, 2021 at 01:07:52PM -0400, Tom Lane wrote:
> I wrote:
> > ... or maybe not just yet.  Andres' buildfarm critters seem to have
> > a different opinion than my machine about what the output of
> > collate.icu.utf8 ought to be.  I wonder what the prevailing LANG
> > setting is for them, and which ICU version they're using.
> 
> Oh, I bet it's "C.utf8", because I can reproduce the failure with that.
> This crystallizes a nagging feeling I'd had that you were misdescribing
> the collate.icu.utf8 test as not being run under --no-locale.  Actually,
> it's only skipped if the encoding isn't UTF8, not the same thing.
> I think we need to remove the default-collation cases from that test too.

IIUC pg_regress --no-locale will call initdb --no-locale which force the locale
to C, and in that case pg_get_encoding_from_locale() does force SQL_ASCII as
encoding.  But yes I clearly didn't think at all that you could set the various
env variables to C.utf8 which can then run the collate.icu.utf8 or linux.utf8
:(

  19. Re: Bogus collation version recording in recordMultipleDependencies

    Julien Rouhaud <rjuju123@gmail.com> — 2021-04-17T09:24:43Z

    On Fri, Apr 16, 2021 at 10:24:21PM -0400, Tom Lane wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > On Sat, Apr 17, 2021 at 10:47 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Although there are only a few buildfarm members complaining, I don't
> >> really want to leave them red all weekend.  I could either commit the
> >> patch I just presented, or revert ef387bed8 ... got a preference?
> 
> > +1 for committing the new patch for now.  I will look into to the
> > record problem.  More in a couple of days.
> 
> OK, done.

Thanks for the fixes!  I'll also look at the problem.

  20. Re: Bogus collation version recording in recordMultipleDependencies

    Julien Rouhaud <rjuju123@gmail.com> — 2021-04-18T11:23:33Z

    On Sat, Apr 17, 2021 at 10:01:53AM +1200, Thomas Munro wrote:
> On Sat, Apr 17, 2021 at 8:39 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Per the changes in collate.icu.utf8.out, this gets rid of
> > a lot of imaginary collation dependencies, but it also gets
> > rid of some arguably-real ones.  In particular, calls of
> > record_eq and its siblings will be considered not to have
> > any collation dependencies, although we know that internally
> > those will look up per-column collations of their input types.
> > We could imagine special-casing record_eq etc here, but that
> > sure seems like a hack.
> 
> [...]
> 
> > So I wonder if, rather than continuing to pursue this right now,
> > we shouldn't revert 257836a75 and try again later with a new design
> > that doesn't try to commandeer the existing dependency infrastructure.
> > We might have a better idea about what to do on Windows by the time
> > that's done, too.
> 
> It seems to me that there are two things that would be needed to
> salvage this for PG14: (1) deciding that we're unlikely to come up
> with a better idea than using pg_depend for this (following the
> argument that it'd only create duplication to have a parallel
> dedicated catalog), (2) fixing any remaining flaws in the dependency
> analyser code.  I'll look into the details some more on Monday.

So IIUC the issue here is that the code could previously record useless
collation version dependencies in somes cases, which could lead to false
positive possible corruption messages (and of course additional bloat on
pg_depend).  False positive messages can't be avoided anyway, as a collation
version update may not corrupt the actually indexed set of data, especially for
glibc.  But with the infrastructure as-is advanced user can look into the new
version changes and choose to ignore changes for a specific set of collation,
which is way easier to do with the recorded dependencies.

The new situation is now that the code can record too few version dependencies
leading to false negative detection, which is way more problematic.

This was previously discussed around [1].  Quoting Thomas:

> To state more explicitly what's happening here, we're searching the
> expression trees for subexpresions that have a collation as part of
> their static type.  We don't know which functions or operators are
> actually affected by the collation, though.  For example, if an
> expression says "x IS NOT NULL" and x happens to be a subexpression of
> a type with a particular collation, we don't now that this
> expression's value can't possibly be affected by the collation version
> changing.  So, the system will nag you to rebuild an index just
> because you mentioned it, even though the index can't be corrupted.
> To do better than that, I suppose we'd need declarations in the
> catalog to say which functions/operators are collation sensitive.

We agreed that having possible false positive dependencies was acceptable for
the initial implementation and that we will improve it in later versions, as
otherwise the alternative is to reindex everything without getting any warning,
which clearly isn't better anyway.

FTR was had the same agreement to not handle specific AMs that don't care about
collation (like hash or bloom) in [2], even though I provided a patch to handle
that case ([3]) which was dropped later on ([4]).

Properly and correctly handling collation version dependency in expressions is
a hard problem and will definitely require additional fields in pg_proc, so we
clearly can't add that in pg14.  So yes we have to decide whether we want to
keep the feature in pg14 with the known limitations (and in that case probably
revert f24b15699, possibly improving documentation on the possibility of false
positive) or revert it entirely.

Unsurprisingly, I think that the feature as-is is already a significant
improvement, which can be easily improved, so my vote is to keep it in pg14.
And just to be clear I'm volunteering to work on the expression problem and all
other related improvements for the next version, whether the current feature is
reverted or not.

[1]: https://www.postgresql.org/message-id/CA%2BhUKGK8CwBcTcXWL2kUjpHT%2B6t2hEFCzkcZ-Z7xXbz%3DC4NLCQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/13b0c950-80f9-4c10-7e0f-f59feac56a98%402ndquadrant.com
[3]: https://www.postgresql.org/message-id/20200908144507.GA57691%40nol
[4]: https://www.postgresql.org/message-id/CA%2BhUKGKHj4aYmmwKZdZjkD%3DCWRmn%3De6UsS7S%2Bu6oLrrp0orgsw%40mail.gmail.com

  21. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-18T15:29:42Z

    Julien Rouhaud <rjuju123@gmail.com> writes:
> On Sat, Apr 17, 2021 at 10:01:53AM +1200, Thomas Munro wrote:
>> It seems to me that there are two things that would be needed to
>> salvage this for PG14: (1) deciding that we're unlikely to come up
>> with a better idea than using pg_depend for this (following the
>> argument that it'd only create duplication to have a parallel
>> dedicated catalog), (2) fixing any remaining flaws in the dependency
>> analyser code.  I'll look into the details some more on Monday.

> So IIUC the issue here is that the code could previously record useless
> collation version dependencies in somes cases, ...
> The new situation is now that the code can record too few version dependencies
> leading to false negative detection, which is way more problematic.

I'm not sure that an error in this direction is all that much more
problematic than the other direction.  If it's okay to claim that
indexes need to be rebuilt when they don't really, then we could just
drop this entire overcomplicated infrastructure and report that all
indexes need to be rebuilt after any collation version change.

But in any case you're oversimplifying tremendously.  The previous code is
just as capable of errors of omission, because it was inquiring into the
wrong composite types, ie those of leaf expression nodes.  The ones we'd
need to look at are the immediate inputs of record_eq and siblings.  Here
are a couple of examples where the leaf types are unhelpful:

... where row(a,b,c)::composite_type < row(d,e,f)::composite_type;
... where function_returning_composite(...) < function_returning_composite(...);

And even if we do this, we're not entirely in the clear in an abstract
sense, because this only covers cases in which an immediate input is
of a known named composite type.  Cases dealing in anonymous RECORD
types simply can't be resolved statically.  It might be that that
can't occur in the specific situation of CREATE INDEX expressions,
but I'm not 100% sure of it.  The apparent counterexample of

... where row(a,b) < row(a,c)

isn't one because we parse that as RowCompareExpr not an application
of record_lt.

> We agreed that having possible false positive dependencies was acceptable for
> the initial implementation and that we will improve it in later versions, as
> otherwise the alternative is to reindex everything without getting any warning,
> which clearly isn't better anyway.

[ shrug... ] You have both false positives and false negatives in the
thing as it stood before f24b15699.  I'm not convinced that it's possible
to completely avoid either issue via static analysis.  I'm inclined to
think that false negatives around record_eq-like functions are not such a
problem for real index definitions, and we'd be better off with fewer
false positives.  But it's all judgment calls.

			regards, tom lane

  22. Re: Bogus collation version recording in recordMultipleDependencies

    Andres Freund <andres@anarazel.de> — 2021-04-19T17:36:32Z

    Hi,

On 2021-04-18 11:29:42 -0400, Tom Lane wrote:
> I'm not sure that an error in this direction is all that much more
> problematic than the other direction.  If it's okay to claim that
> indexes need to be rebuilt when they don't really, then we could just
> drop this entire overcomplicated infrastructure and report that all
> indexes need to be rebuilt after any collation version change.

That doesn't ring true to me. There's a huge difference between needing
to rebuild all indexes, especially primary key indexes which often are
over int8 etc, and unnecessarily needing to rebuild indexes doing
comparatively rare things.

Greetings,

Andres Freund

  23. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-19T17:52:59Z

    Andres Freund <andres@anarazel.de> writes:
> On 2021-04-18 11:29:42 -0400, Tom Lane wrote:
>> I'm not sure that an error in this direction is all that much more
>> problematic than the other direction.  If it's okay to claim that
>> indexes need to be rebuilt when they don't really, then we could just
>> drop this entire overcomplicated infrastructure and report that all
>> indexes need to be rebuilt after any collation version change.

> That doesn't ring true to me. There's a huge difference between needing
> to rebuild all indexes, especially primary key indexes which often are
> over int8 etc, and unnecessarily needing to rebuild indexes doing
> comparatively rare things.

It would not be that hard to exclude indexes on int8, or other cases
that clearly have zero collation dependencies.  And I think I might
have some faith in such a solution.  Right now I have zero faith
that the patch as it stands gives trustworthy answers.

I think that the real fundamental bug is supposing that static analysis
can give 100% correct answers.  Even if it did do so in a given state
of the database, consider this counterexample:

create type myrow as (f1 int, f2 int);
create table mytable (id bigint, r1 myrow, r2 myrow);
create index myindex on mytable(id) where r1 < r2;
alter type myrow add attribute f3 text;

myindex is recorded as having no collation dependency, but that is
now wrong.

			regards, tom lane

  24. Re: Bogus collation version recording in recordMultipleDependencies

    Peter Geoghegan <pg@bowt.ie> — 2021-04-19T18:04:26Z

    On Mon, Apr 19, 2021 at 10:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think that the real fundamental bug is supposing that static analysis
> can give 100% correct answers.  Even if it did do so in a given state
> of the database, consider this counterexample:
>
> create type myrow as (f1 int, f2 int);
> create table mytable (id bigint, r1 myrow, r2 myrow);
> create index myindex on mytable(id) where r1 < r2;
> alter type myrow add attribute f3 text;
>
> myindex is recorded as having no collation dependency, but that is
> now wrong.

Is it really the case that static analysis of the kind that you'd need
to make this 100% robust is fundamentally impossible? I find that
proposition hard to believe.

I'm not sure that you were making a totally general statement, rather
than a statement about the patch/implementation, so perhaps I just
missed the point.

-- 
Peter Geoghegan

  25. Re: Bogus collation version recording in recordMultipleDependencies

    Peter Geoghegan <pg@bowt.ie> — 2021-04-19T18:13:37Z

    On Sun, Apr 18, 2021 at 4:23 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> So IIUC the issue here is that the code could previously record useless
> collation version dependencies in somes cases, which could lead to false
> positive possible corruption messages (and of course additional bloat on
> pg_depend).  False positive messages can't be avoided anyway, as a collation
> version update may not corrupt the actually indexed set of data, especially for
> glibc.

This argument seems completely absurd to me.

-- 
Peter Geoghegan

  26. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-19T18:49:05Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Mon, Apr 19, 2021 at 10:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think that the real fundamental bug is supposing that static analysis
>> can give 100% correct answers.

> Is it really the case that static analysis of the kind that you'd need
> to make this 100% robust is fundamentally impossible? I find that
> proposition hard to believe.

I didn't mean to imply that it's necessarily theoretically impossible,
but given our lack of visibility into what a function or operator
will do, plus the way that the collation feature was bolted on
with minimal system-level redesign, it's sure pretty darn hard.
Code like record_eq is doing a lot at runtime that we can't really
see from static analysis.

Anyway, given the ALTER TYPE ADD ATTRIBUTE counterexample, I'm
definitely starting to lean towards "revert and try again in v15".
I feel we'd be best off to consider functions/operators that
operate on container types to be "maybe"s rather than certainly
safe or certainly not safe.  I think that such things appear
sufficiently rarely in index specifications that it's not worth it
to try to do an exact analysis of them, even if we were sure we
could get that 100% right.  But that doesn't seem to be an idea that
can trivially be added to the current design.

			regards, tom lane

  27. Re: Bogus collation version recording in recordMultipleDependencies

    Peter Geoghegan <pg@bowt.ie> — 2021-04-19T19:38:27Z

    On Mon, Apr 19, 2021 at 11:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I didn't mean to imply that it's necessarily theoretically impossible,
> but given our lack of visibility into what a function or operator
> will do, plus the way that the collation feature was bolted on
> with minimal system-level redesign, it's sure pretty darn hard.
> Code like record_eq is doing a lot at runtime that we can't really
> see from static analysis.

It's worth pointing out that code like record_eq is not (or at least
should not be) fundamentally unpredictable and unruly. The fact that
record_eq does typecache lookups and whatnot seems to me to be an
implementation detail. What record_eq is entitled to assume about
collations could be formalized by some general high-level
specification. It ought to be possible to do this, just as it ought to
be possible for us to statically determine if a composite type is safe
to use with B-Tree deduplication.

Whether or not it's worth the trouble is another matter, but it might
be if a single effort solved a bunch of related problems, not just the
collation dependency problem.

> Anyway, given the ALTER TYPE ADD ATTRIBUTE counterexample, I'm
> definitely starting to lean towards "revert and try again in v15".

The counterexample concerns me because it seems to indicate a lack of
sophistication in how dependencies are managed with corner cases -- I
don't think that it's okay to leave the behavior unspecified in a
stable release. But I also think that we should consider if code like
record_eq is in fact the real problem (or just the lack of any general
specification that constrains code like it in useful ways, perhaps).
This probably won't affect whether or not the patch gets reverted now,
but it still matters.

-- 
Peter Geoghegan

  28. Re: Bogus collation version recording in recordMultipleDependencies

    Thomas Munro <thomas.munro@gmail.com> — 2021-04-19T19:42:42Z

    On Tue, Apr 20, 2021 at 5:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think that the real fundamental bug is supposing that static analysis
> can give 100% correct answers.  ...

Well, the goal was to perform analysis to the extent possible
statically since that would cover the vast majority of cases and is
practically all you can do.  Clearly there is always going to be a
category of invisible dependencies inside procedural code in general
(halting problem).  We did think about the idea of using new
declarations about functions/operators to know which ones actually
care about collation, rather than assuming that they all do (bugs
aside), as an optimisation, and then that mechanism could in theory
also be used to say that functions that don't appear to depend on
collations actually do internal, but that all seemed like vast
overkill, so we left it for possible later improvements.  The question
on my mind is whether reverting the feature and trying again for 15
could produce anything fundamentally better at a design level, or
would just fix problems in the analyser code that we could fix right
now.  For example, if you think there actually is a potential better
plan than using pg_depend for this, that'd definitely be good to know
about.

> ... Even if it did do so in a given state
> of the database, consider this counterexample:
>
> create type myrow as (f1 int, f2 int);
> create table mytable (id bigint, r1 myrow, r2 myrow);
> create index myindex on mytable(id) where r1 < r2;
> alter type myrow add attribute f3 text;
>
> myindex is recorded as having no collation dependency, but that is
> now wrong.

Hrmph.  Yeah.  We didn't consider types that change later like this,
and handling those correctly does seem to warrant some more thought
and work than we perhaps have time for.  My first thought is that we'd
need to teach it to trigger reanalysis.

  29. Re: Bogus collation version recording in recordMultipleDependencies

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-19T20:21:17Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> ... The question
> on my mind is whether reverting the feature and trying again for 15
> could produce anything fundamentally better at a design level, or
> would just fix problems in the analyser code that we could fix right
> now.

Well, as I said, I think what we ought to do is treat any record-accepting
functions/operators as "don't know, better assume it's collation
dependent".  What's not clear to me is how that concept could be
shoehorned into the existing design.

> For example, if you think there actually is a potential better
> plan than using pg_depend for this, that'd definitely be good to know
> about.

I really dislike using pg_depend, for a couple of reasons:

* You've broken the invariant that dependencies on pinned objects
are never recorded.  Now, some of them exist, for reasons having
nothing to do with the primary goals of pg_depend.  If that's not
a sign of bad relational design, I don't know what is.  I didn't
look at the code, but I wonder if you didn't have to lobotomize
some error checks in dependency.c because of that.  (Perhaps
some sort of special-case representation for the default
collation would help here?)

* pg_depend used to always be all-not-null.  Now, most rows in it
will need a nulls bitmap, adding 8 bytes per row (on maxalign=8
hardware) to what had been fairly narrow rows.  By my arithmetic
that's 13.3% bloat in what is already one of our largest
catalogs.  That's quite unpleasant.  (It would actually be
cheaper to store an empty-string refobjversion for non-collation
entries; a single-byte string would fit into the pad space
after deptype, adding nothing to the row width.)

> Hrmph.  Yeah.  We didn't consider types that change later like this,
> and handling those correctly does seem to warrant some more thought
> and work than we perhaps have time for.  My first thought is that we'd
> need to teach it to trigger reanalysis.

That seems like a nonstarter, even before you think about race
conditions.

			regards, tom lane

  30. Re: Bogus collation version recording in recordMultipleDependencies

    Thomas Munro <thomas.munro@gmail.com> — 2021-04-20T00:05:27Z

    On Tue, Apr 20, 2021 at 8:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:>
> Thomas Munro <thomas.munro@gmail.com> writes:
> > For example, if you think there actually is a potential better
> > plan than using pg_depend for this, that'd definitely be good to know
> > about.
>
> I really dislike using pg_depend, for a couple of reasons:
>
> * You've broken the invariant that dependencies on pinned objects
> are never recorded.  Now, some of them exist, for reasons having
> nothing to do with the primary goals of pg_depend.  If that's not
> a sign of bad relational design, I don't know what is.  I didn't
> look at the code, but I wonder if you didn't have to lobotomize
> some error checks in dependency.c because of that.  (Perhaps
> some sort of special-case representation for the default
> collation would help here?)

Hmm, OK, thanks, that's something to go back and think about.

> * pg_depend used to always be all-not-null.  Now, most rows in it
> will need a nulls bitmap, adding 8 bytes per row (on maxalign=8
> hardware) to what had been fairly narrow rows.  By my arithmetic
> that's 13.3% bloat in what is already one of our largest
> catalogs.  That's quite unpleasant.  (It would actually be
> cheaper to store an empty-string refobjversion for non-collation
> entries; a single-byte string would fit into the pad space
> after deptype, adding nothing to the row width.)

That seems like a good idea.

> > Hrmph.  Yeah.  We didn't consider types that change later like this,
> > and handling those correctly does seem to warrant some more thought
> > and work than we perhaps have time for.  My first thought is that we'd
> > need to teach it to trigger reanalysis.
>
> That seems like a nonstarter, even before you think about race
> conditions.

Yeah, that runs directly into non-trivial locking problems.  I felt
like some of the other complaints could conceivably be addressed in
time, including dumb stuff like Windows default locale string format
and hopefully some expression analysis problems, but not this.  I'll
hold off reverting for a few more days to see if anyone has any other
thoughts on that, because there doesn't seem to be any advantage in
being too hasty about it.

  31. Re: Bogus collation version recording in recordMultipleDependencies

    Julien Rouhaud <rjuju123@gmail.com> — 2021-04-20T01:46:07Z

    On Mon, Apr 19, 2021 at 11:13:37AM -0700, Peter Geoghegan wrote:
> On Sun, Apr 18, 2021 at 4:23 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > So IIUC the issue here is that the code could previously record useless
> > collation version dependencies in somes cases, which could lead to false
> > positive possible corruption messages (and of course additional bloat on
> > pg_depend).  False positive messages can't be avoided anyway, as a collation
> > version update may not corrupt the actually indexed set of data, especially for
> > glibc.
> 
> This argument seems completely absurd to me.

I'm not sure why?  For glibc at least, I don't see how we could not end up
raising false positive as you have a single glibc version for all its
collations.  If a user has say en_US and fr_FR, or any quite stable collation,
most of the glibc upgrades (except 2.28 of course) won't corrupt your indexes.

  32. Re: Bogus collation version recording in recordMultipleDependencies

    Julien Rouhaud <rjuju123@gmail.com> — 2021-04-20T01:49:08Z

    On Tue, Apr 20, 2021 at 12:05:27PM +1200, Thomas Munro wrote:
> 
> Yeah, that runs directly into non-trivial locking problems.  I felt
> like some of the other complaints could conceivably be addressed in
> time, including dumb stuff like Windows default locale string format
> and hopefully some expression analysis problems, but not this.  I'll
> hold off reverting for a few more days to see if anyone has any other
> thoughts on that, because there doesn't seem to be any advantage in
> being too hasty about it.

I also feel that the ALTER TYPE example Tom showed earlier isn't something
trivial to fix and cannot be done in pg14 :(

  33. Re: Bogus collation version recording in recordMultipleDependencies

    Peter Geoghegan <pg@bowt.ie> — 2021-04-20T02:27:24Z

    On Mon, Apr 19, 2021 at 6:45 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > This argument seems completely absurd to me.
>
> I'm not sure why?  For glibc at least, I don't see how we could not end up
> raising false positive as you have a single glibc version for all its
> collations.  If a user has say en_US and fr_FR, or any quite stable collation,
> most of the glibc upgrades (except 2.28 of course) won't corrupt your indexes.

If the versions differ and your index happens to not be corrupt
because it just so happened to not depend on any of the rules that
have changed, then a complaint about the collation versions changing
is not what I'd call a false positive. You can call it that if you
want, I suppose -- it's just a question of semantics. But I don't
think you should conflate two very different things. You seem to be
suggesting that they're equivalent just because you can refer to both
of them using the same term.

It's obvious that you could have an absence of index corruption even
in the presence of a collation incompatibility. Especially when there
is only 1 tuple in the index, say -- obviously the core idea is to
manage the dependency on versioned collations, which isn't magic. Do
you really think that's equivalent to having incorrect version
dependencies?

-- 
Peter Geoghegan

  34. Re: Bogus collation version recording in recordMultipleDependencies

    Julien Rouhaud <rjuju123@gmail.com> — 2021-04-20T03:02:31Z

    On Mon, Apr 19, 2021 at 07:27:24PM -0700, Peter Geoghegan wrote:
> On Mon, Apr 19, 2021 at 6:45 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > > This argument seems completely absurd to me.
> >
> > I'm not sure why?  For glibc at least, I don't see how we could not end up
> > raising false positive as you have a single glibc version for all its
> > collations.  If a user has say en_US and fr_FR, or any quite stable collation,
> > most of the glibc upgrades (except 2.28 of course) won't corrupt your indexes.
> 
> If the versions differ and your index happens to not be corrupt
> because it just so happened to not depend on any of the rules that
> have changed, then a complaint about the collation versions changing
> is not what I'd call a false positive. You can call it that if you
> want, I suppose -- it's just a question of semantics. But I don't
> think you should conflate two very different things. You seem to be
> suggesting that they're equivalent just because you can refer to both
> of them using the same term.
> 
> It's obvious that you could have an absence of index corruption even
> in the presence of a collation incompatibility. Especially when there
> is only 1 tuple in the index, say 

Yes, and technically you could still have corruption on indexes containing 1 or
even 0 rows in case of collation provider upgrade, eg if you have a WHERE
clause on the index that does depend on a collation.

> -- obviously the core idea is to
> manage the dependency on versioned collations, which isn't magic. Do
> you really think that's equivalent to having incorrect version
> dependencies?

No I don't think that's equivalent.  What I wanted to say that it's impossible
to raise a WARNING only if the index can really be corrupted (corner cases like
empty tables or similar apart) for instance because of how glibc report
versions, so raising WARNING in some limited corner cases that definitely can't
be corrupted (like because the index expression itself doesn't depend on the
ordering), which clearly isn't the same thing,  was in my opinion an acceptable
trade-off in a first version.  Sorry if that was (or still is) poorly worded.

In any case it was proven that the current approach has way bigger deficiencies
so it's probably not relevant anymore.

  35. Re: Bogus collation version recording in recordMultipleDependencies

    Thomas Munro <thomas.munro@gmail.com> — 2021-04-20T03:07:16Z

    On Tue, Apr 20, 2021 at 1:48 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> On Tue, Apr 20, 2021 at 12:05:27PM +1200, Thomas Munro wrote:
> > Yeah, that runs directly into non-trivial locking problems.  I felt
> > like some of the other complaints could conceivably be addressed in
> > time, including dumb stuff like Windows default locale string format
> > and hopefully some expression analysis problems, but not this.  I'll
> > hold off reverting for a few more days to see if anyone has any other
> > thoughts on that, because there doesn't seem to be any advantage in
> > being too hasty about it.
>
> I also feel that the ALTER TYPE example Tom showed earlier isn't something
> trivial to fix and cannot be done in pg14 :(

Just an idea:  It might be possible to come up with a scheme where
ALTER TYPE ADD ATTRIBUTE records versions somewhere at column add
time, and index_check_collation_versions() finds and checks those when
they aren't superseded by index->collation versions created by
REINDEX, or already present due to other dependencies on the same
collation.  Of course, the opposite problem applies when you ALTER
TYPE DROP ATTRIBUTE: you might have some zombie refobjversions you
don't need anymore, but that would seem to be the least of your
worries if you drop attributes from composite types used in indexes:

create type myrow as (f1 int, f2 int);
create table mytable (r1 myrow primary key);
insert into mytable
select row(generate_series(1, 10), generate_series(10, 1, -1))::myrow;
select * from mytable;
alter type myrow drop attribute f1;
select * from mytable;
select * from mytable where r1 = row(6); -- !!!
reindex table mytable;
select * from mytable where r1 = row(6);

  36. Re: Bogus collation version recording in recordMultipleDependencies

    Andres Freund <andres@anarazel.de> — 2021-04-21T20:28:08Z

    Hi,

On 2021-04-20 12:05:27 +1200, Thomas Munro wrote:
> I'll hold off reverting for a few more days to see if anyone has any
> other thoughts on that, because there doesn't seem to be any advantage
> in being too hasty about it.

I'm not really convinced that this is warranted, and that it isn't
better addressed by reducing the scope of the feature:

When using index collation versions to decide whether to reindex
individual indexes it is important to not have any false negatives -
otherwise the feature could trigger corruption.

However, the feature has a second, IMO more crucial, aspect: Preventing
silent corruption due to collation changes. There are regular reports of
people corrupting their indexes (and subsequently constraints) due to
collation changes (or collation differences between primary/replica).
To be effective detecting such cases it is not required to catch 100% of
all dangerous cases, just that a high fraction of cases is caught.

And handling the composite type case doesn't seem like it'd impact the
percentage of detected collation issues all that much. For one, indexes
on composite types aren't all that common, and additing new columns to
those composite types is likely even rarer. For another, I'd expect that
nearly all databases that have indexes on composite types also have
indexes on non-composite text columns - which'd be likely to catch the
issue.

Given that this is a regularly occurring source of corruption for users,
and not one just negligent operators run into (we want people to upgrade
OS versions), I think we ought to factor that into our decision what to
do.

Greetings,

Andres Freund

  37. Re: Bogus collation version recording in recordMultipleDependencies

    Andrew Dunstan <andrew@dunslane.net> — 2021-05-05T20:58:08Z

    On 4/21/21 4:28 PM, Andres Freund wrote:
> Hi,
>
> On 2021-04-20 12:05:27 +1200, Thomas Munro wrote:
>> I'll hold off reverting for a few more days to see if anyone has any
>> other thoughts on that, because there doesn't seem to be any advantage
>> in being too hasty about it.
> I'm not really convinced that this is warranted, and that it isn't
> better addressed by reducing the scope of the feature:
>
> When using index collation versions to decide whether to reindex
> individual indexes it is important to not have any false negatives -
> otherwise the feature could trigger corruption.
>
> However, the feature has a second, IMO more crucial, aspect: Preventing
> silent corruption due to collation changes. There are regular reports of
> people corrupting their indexes (and subsequently constraints) due to
> collation changes (or collation differences between primary/replica).
> To be effective detecting such cases it is not required to catch 100% of
> all dangerous cases, just that a high fraction of cases is caught.
>
> And handling the composite type case doesn't seem like it'd impact the
> percentage of detected collation issues all that much. For one, indexes
> on composite types aren't all that common, and additing new columns to
> those composite types is likely even rarer. For another, I'd expect that
> nearly all databases that have indexes on composite types also have
> indexes on non-composite text columns - which'd be likely to catch the
> issue.
>
> Given that this is a regularly occurring source of corruption for users,
> and not one just negligent operators run into (we want people to upgrade
> OS versions), I think we ought to factor that into our decision what to
> do.
>


Hi,


this is an open item for release 14 . The discussion seems to have gone
silent for a couple of weeks. Are we in a position to make any
decisions? I hear what Andres says, but is anyone acting on it?


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  38. Re: Bogus collation version recording in recordMultipleDependencies

    Thomas Munro <thomas.munro@gmail.com> — 2021-05-05T21:12:18Z

    On Thu, May 6, 2021 at 8:58 AM Andrew Dunstan <andrew@dunslane.net> wrote:
> this is an open item for release 14 . The discussion seems to have gone
> silent for a couple of weeks. Are we in a position to make any
> decisions? I hear what Andres says, but is anyone acting on it?

I'm going to revert this and resubmit for 15.  That'll give proper
time to reconsider the question of whether pg_depend is right for
this, and come up with a non-rushed response to the composite type
problem etc.

  39. Re: Bogus collation version recording in recordMultipleDependencies

    Andrew Dunstan <andrew@dunslane.net> — 2021-05-05T21:23:16Z

    On 5/5/21 5:12 PM, Thomas Munro wrote:
> On Thu, May 6, 2021 at 8:58 AM Andrew Dunstan <andrew@dunslane.net> wrote:
>> this is an open item for release 14 . The discussion seems to have gone
>> silent for a couple of weeks. Are we in a position to make any
>> decisions? I hear what Andres says, but is anyone acting on it?
> I'm going to revert this and resubmit for 15.  That'll give proper
> time to reconsider the question of whether pg_depend is right for
> this, and come up with a non-rushed response to the composite type
> problem etc.


OK, thanks.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  40. Re: Bogus collation version recording in recordMultipleDependencies

    Thomas Munro <thomas.munro@gmail.com> — 2021-05-07T10:01:40Z

    On Thu, May 6, 2021 at 9:23 AM Andrew Dunstan <andrew@dunslane.net> wrote:
> On 5/5/21 5:12 PM, Thomas Munro wrote:
> > On Thu, May 6, 2021 at 8:58 AM Andrew Dunstan <andrew@dunslane.net> wrote:
> >> this is an open item for release 14 . The discussion seems to have gone
> >> silent for a couple of weeks. Are we in a position to make any
> >> decisions? I hear what Andres says, but is anyone acting on it?
> > I'm going to revert this and resubmit for 15.  That'll give proper
> > time to reconsider the question of whether pg_depend is right for
> > this, and come up with a non-rushed response to the composite type
> > problem etc.
>
> OK, thanks.

Reverted.  Rebasing notes:

1.  Commit b4c9695e moved toast table declarations so I adapted to the
new scheme, but commit 0cc99327 had taken the OIDs that pg_collation
was previously using, so I had to pick some new ones from the
temporary range for later reassignment.

2.  It took me quite a while to figure out that the collversion column
now needs BKI_DEFAULT(_null_), or the perl script wouldn't accept the
contents of pg_collation.dat.

3.  In a separate commit, I rescued a few sentences of text from the
documentation about libc collation versions and reinstated them in the
most obvious place, because although the per-index tracking has been
reverted, the per-collation version tracking (limited as it is) is now
back and works on more OSes than before.