Subject: Re: Built-in CTYPE provider

From: Noah Misch <[email-masked]>
Date: Tue, 2 Jul 2024 09:51:45 -0700

On Mon, Jul 01, 2024 at 06:19:08PM -0700, Jeff Davis wrote:
> On Mon, 2024-07-01 at 16:03 -0700, Noah Misch wrote:
> >   An alternative would be to make pg_upgrade reject
> > operating on a cluster that contains use of $SUBJECT.
> 
> That wouldn't help anyone.

Can you say more about that?  For the last decade at least, I think our
standard for new features has been to error rather than allow an operation
that creates a known path to wrong query results.  I think that's a helpful
standard that we should continue to follow.

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 3 Jul 2024 00:05:09 +0200

> standard that we should continue to follow.

I don't think the builtin locale provider is any different in this 
respect from the other providers:  The locale data might change and 
there is a version mechanism to track that.  We don't prevent pg_upgrade 
in scenarios like that for other providers.

---

From: Noah Misch <[email-masked]>
Date: Tue, 2 Jul 2024 16:03:33 -0700

On Wed, Jul 03, 2024 at 12:05:09AM +0200, Peter Eisentraut wrote:
> I don't think the builtin locale provider is any different in this respect
> from the other providers:  The locale data might change and there is a
> version mechanism to track that.  We don't prevent pg_upgrade in scenarios
> like that for other providers.

Each packager can choose their dependencies so the v16 providers don't have
the problem.  With the $SUBJECT provider, a packager won't have that option.

---

From: Jeff Davis <[email-masked]>
Date: Wed, 03 Jul 2024 14:19:07 -0700

> the problem.  With the $SUBJECT provider, a packager won't have that

While nothing needs to be changed for 17, I agree that we may need to
be careful in future releases not to break things.

Broadly speaking, you are right that we may need to freeze Unicode
updates or be more precise about versioning. But there's a lot of
nuance to the problem, so I don't think we should pre-emptively promise
either of those things right now.

Consider:

* Unless I made a mistake, the last three releases of Unicode (14.0,
15.0, and 15.1) all have the exact same behavior for UPPER() and
LOWER() -- even for unassigned code points. It would be silly to
promise to stay with 15.1 and then realize that moving to 16.0 doesn't
create any actual problem.

* Unicode also offers "case folding", which has even stronger stability
guarantees, and I plan to propose that soon. When implemented, it would
be preferred over LOWER()/UPPER() in index expressions for most use
cases.

* While someone can pin libc+ICU to particular versions, it's
impossible when using the official packages, and additionally requires
using something like [1], which just became available last year. I
don't think it's reasonable to put it forth as a matter-of-fact
solution.

* Let's keep some perspective: we've lived for a long time with ALL
text indexes at serious risk of breakage. In contrast, the concerns you
are raising now are about certain kinds of expression indexes over data
containing certain unassigned code points. I am not dismissing that
concern, but the builtin provider moves us in the right direction and
let's not lose sight of that.

Given that no code changes for v17 are proposed, I suggest that we
refrain from making any declarations until the next version of Unicode
is released. If the pattern holds, that will be around September, which
still leaves time to make reasonable decisions for v18.

Regards,
	Jeff Davis

[1] https://github.com/awslabs/compat-collation-for-glibc

---

From: Noah Misch <[email-masked]>
Date: Thu, 4 Jul 2024 14:26:41 -0700

> create any actual problem.

I think you're saying that if some Unicode update changes the results of a
STABLE function but does not change the result of any IMMUTABLE function, we
may as well import that update.  Is that about right?  If so, I agree.

In addition to the options I listed earlier (error in pg_upgrade or document
that IMMUTABLE stands) I would be okay with a third option.  Decide here that
we'll not adopt a Unicode update in a way that changes a v17 IMMUTABLE
function result of the new provider.  We don't need to write that in the
documentation, since it's implicit in IMMUTABLE.  Delete the "stable within a
<productname>Postgres</productname> major version" documentation text.

> let's not lose sight of that.

I see you're trying to help users get less breakage, and that's a good goal.
I agree $SUBJECT eliminates libc+ICU breakage, and libc+ICU breakage has hurt
plenty.  However, you proposed to update Unicode data and give REINDEX as the
solution to breakage this causes.  Unlike libc+ICU breakage, the packager has
no escape from that.  That's a different kind of breakage proposition, and no
new PostgreSQL feature should do that.  It's on a different axis from helping
users avoid libc+ICU breakage, and a feature doesn't get to credit helping on
one axis against a regression on the other axis.  What am I missing here?

> still leaves time to make reasonable decisions for v18.

Soon enough, a Unicode release will add one character to regexp [[:alpha:]].
PostgreSQL will then need to decide what IMMUTABLE is going to mean.  How does
that get easier in September?

Thanks,
nm

---

From: Jeff Davis <[email-masked]>
Date: Fri, 05 Jul 2024 14:38:45 -0700

> may as well import that update.  Is that about right?  If so, I

If you are proposing that Unicode updates should not be performed if
they affect the results of any IMMUTABLE function, then that's a new
policy.

For instance, the results of NORMALIZE() changed from PG15 to PG16 due
to commit 1091b48cd7:

  SELECT NORMALIZE(U&'\+01E030',nfkc)::bytea;

  Version 15: \xf09e80b0

  Version 16: \xd0b0

I am neither endorsing nor opposing the new policy you propose at this
time, but deep in the sub-thread of one particular feature is not the
right place to discuss it.

Please start a new thread for the proposed PG18 policy change and CC
me. I happen to think that around the release of the next version of
Unicode (in a couple months) would be the most productive time to have
that discussion, but you can start the discussion now if you like.

Regards,
	Jeff Davis

---

From: Noah Misch <[email-masked]>
Date: Sat, 6 Jul 2024 12:51:29 -0700

>   SELECT NORMALIZE(U&'\+01E030',nfkc)::bytea;

As a released feature, NORMALIZE() has a different set of remedies to choose
from, and I'm not proposing one.  I may have sidetracked this thread by
talking about remedies without an agreement that pg_c_utf8 has a problem.  My
question for the PostgreSQL maintainers is this:

  textregexeq(... COLLATE pg_c_utf8, '[[:alpha:]]') and lower(), despite being
  IMMUTABLE, will change behavior in some major releases.  pg_upgrade does not
  have a concept of IMMUTABLE functions changing, so index scans will return
  wrong query results after upgrade.  Is it okay for v17 to release a
  pg_c_utf8 planned to behave that way when upgrading v17 to v18+?

If the answer is yes, the open item closes.  If the answer is no, determining
the remedy can come next.

Lest concrete details help anyone reading, here are some affected objects:

  CREATE TABLE t (s text COLLATE pg_c_utf8);
  INSERT INTO t VALUES (U&'\+00a7dc'), (U&'\+001dd3');
  CREATE INDEX iexpr ON t ((lower(s)));
  CREATE INDEX ipred ON t (s) WHERE s ~ '[[:alpha:]]';

v17 can simulate the Unicode aspect of a v18 upgrade, like this:

  sed -i 's/^UNICODE_VERSION.*/UNICODE_VERSION = 16.0.0/' src/Makefile.global.in
  # ignore test failures (your ICU likely doesn't have the Unicode 16.0.0 draft)
  make -C src/common/unicode update-unicode
  make
  make install
  pg_ctl restart

Behavior after that:

-- 2 rows w/ seq scan, 0 rows w/ index scan
SELECT 1 FROM t WHERE s ~ '[[:alpha:]]';
SET enable_seqscan = off;
SELECT 1 FROM t WHERE s ~ '[[:alpha:]]';

-- ERROR:  heap tuple (0,1) from table "t" lacks matching index tuple within index "iexpr"
SELECT bt_index_parent_check('iexpr', heapallindexed => true);
-- ERROR:  heap tuple (0,1) from table "t" lacks matching index tuple within index "ipred"
SELECT bt_index_parent_check('ipred', heapallindexed => true);

---

From: Tom Lane <[email-masked]>
Date: Sat, 06 Jul 2024 16:19:21 -0400

>   pg_c_utf8 planned to behave that way when upgrading v17 to v18+?

I do not think it is realistic to define "IMMUTABLE" as meaning that
the function will never change behavior until the heat death of the
universe.  As a counterexample, we've not worried about applying
bug fixes or algorithm improvements that change the behavior of
"immutable" numeric computations.  It might be unwise to do that
in a minor release, but we certainly do it in major releases.

I'd say a realistic policy is "immutable means we don't intend to
change it within a major release".  If we do change the behavior,
either as a bug fix or a major-release improvement, that should
be release-noted so that people know they have to rebuild dependent
indexes and matviews.

It gets stickier for behaviors that aren't fully under our control,
which is the case for a lot of locale-related things.  We cannot then
promise "no changes within major releases".  But I do not think it
is helpful to react to that fact by refusing to label such things
immutable.  Then we'd just need another mutability classification,
and it would effectively act the same as immutable does now, because
people will certainly wish to use these functions in indexes etc.

			regards, tom lane

---

From: Noah Misch <[email-masked]>
Date: Mon, 8 Jul 2024 18:05:45 -0700

On Sat, Jul 06, 2024 at 04:19:21PM -0400, Tom Lane wrote:
> "immutable" numeric computations.

True.  There's a continuum from "releases can change any IMMUTABLE function"
to "index integrity always wins, even if a function is as wrong as 1+1=3".
I'm less concerned about the recent "Incorrect results from numeric round"
thread, even though it's proposing to back-patch.  I'm thinking about these
aggravating factors for $SUBJECT:

- $SUBJECT is planning an annual cadence of this kind of change.

- We already have ICU providing collation support for the same functions.
  Unlike $SUBJECT, ICU integration gives packagers control over when to accept
  corruption at pg_upgrade time.

- SQL Server, DB2 and Oracle do their Unicode updates in a non-corrupting way.
  (See Jeremy Schneider's reply concerning DB2 and Oracle.)

- lower() and regexp are more popular in index expressions than
  high-digit-count numeric calculations.

> be release-noted so that people know they have to rebuild dependent

It sounds like you're very comfortable with $SUBJECT proceeding in its current
form.  Is that right?

---

From: Tom Lane <[email-masked]>
Date: Mon, 08 Jul 2024 21:17:52 -0400

> It sounds like you're very comfortable with $SUBJECT proceeding in its current

I don't have an opinion on whether the overall feature design
is well-chosen.  But the mere fact that Unicode updates will
from time to time change the behavior (presumably only in edge
cases or for previously-unassigned code points) doesn't strike
me as a big enough problem to justify saying these functions
can't be marked immutable anymore.  Especially since we have been
faced with that problem all along anyway; we just didn't have a way
to track or quantify it before, because locale changes happened
outside code we control.

			regards, tom lane

---

From: Jeff Davis <[email-masked]>
Date: Tue, 09 Jul 2024 16:20:12 -0700

On Mon, 2024-07-08 at 18:05 -0700, Noah Misch wrote:
> I'm thinking about these

This is still marked as an open item for 17, but you've already
acknowledged[1] that no code changes are necessary in version 17.
Upgrades of Unicode take an active step from a committer, so it's not a
pressing problem for 18, either.

The idea that you're arguing against is "stability within a PG major
version". There's no new discovery here: it was listed under the
heading of "Benefits" near the top of my initial proposal[2], and known
to all reviewers.

This is not an Open Item for 17, and new policy discussions should not
happen deep in this subthread. Please resolve the Open Item, and feel
free to start a thread about policy changes in 18.

Regards,
	Jeff Davis

[1]
https://www.postgresql.org/message-id/[email-masked]
[2]
https://www.postgresql.org/message-id/[email-masked]

---

From: Noah Misch <[email-masked]>
Date: Thu, 11 Jul 2024 05:50:40 -0700

> acknowledged[1] that no code changes are necessary in version 17.

Later posts on the thread made that obsolete.  The next step is to settle the
question at https://postgr.es/m/[email-masked].  If that
conclusion entails a remedy, v17 code changes may be part of that remedy.

> heading of "Benefits" near the top of my initial proposal[2]

Thanks for that distillation.  More specifically, I'm arguing against the lack
of choice about instability across major versions:

                                  | ICU collations    | pg_c_utf8
----------------------------------|-------------------|----------
Corruption within a major version | packager's choice | no
Corruption at pg_upgrade time     | packager's choice | yes

I am a packager who chooses no-corruption (chooses stability).  As a packager,
the pg_c_utf8 stability within major versions is never a bad thing, but it
does not compensate for instability across major versions.  I don't want a
future in which pg_c_utf8 is the one provider that integrity-demanding
pg_upgrade users should not use.

> and known to all reviewers.

If after https://postgr.es/m/[email-masked] and
https://postgr.es/m/[email-masked] they think $SUBJECT
should continue as-committed, they should vote that way.  Currently, we have
multiple votes in one direction and multiple votes in the other direction.  If
all three reviewers were to vote in the same direction (assuming no other new
votes), I argue that such a count would render whichever way they vote as the
conclusion.  Does that match your count?