Re: BUG #19641: Unexpected results on an SP-GiST indexed column with a non-deterministic collation

Andrey Rachitskiy <pl0h0yp1(at)gmail(dot)com> wrote:
> I have a draft of "A" and "B" ready, but I decided not to publish it
> until an agreement on the direction is reached.
>
> Thoughts?

Some data for that choice, all on master (374522aa63a).

First, the scope. With the 340-row table from the report, the column
under the nondeterministic collation, and the same equality query, each
index type against a sequential scan (227 rows):

- SP-GiST text_ops: 113
- btree, hash, BRIN, GiST (btree_gist): 227

Every plan used its index, so SP-GiST's text_ops is the only one of
them that gets this wrong. It also does so at any size, one row
included.

Second, what A does to indexes that already exist. The precedent,
281039631, went in before v12's rc1, when no such index could exist
yet, while an SP-GiST index under a nondeterministic collation has been
accepted since v12. So I built a prototype of A with the same check as
the pattern_ops one in index.c, for SP-GiST text_ops, created such an
index on an unpatched master cluster, and took it to the prototype:

- a pg_dump restored with psql loads the table (340 rows) and skips
the index with one ERROR; psql exits with 0, so a restore script
that does not stop on errors ends up without the index and says
nothing;
- pg_upgrade fails, during the schema restore, with the same error.

So A cannot be back-patched, and in master it would need a pg_upgrade
check that reports these indexes before the upgrade, as pg_upgrade
does for other objects it cannot carry over.

B has neither problem. If I read your description right, it changes
only how the scan uses the tree, not how the tree is built, so an
existing index returns correct results after a minor update, without a
REINDEX. That seems to me the one that can go to all the branches.
Your point that B does not make the index a good accelerator for this
equality stands; that seems like something for the documentation to
say (a btree index serves it better), rather than a reason to break
existing schemas.

If you post the B draft, I am happy to test it on the back branches:
the existing-index case above, and the other text_ops operators under
the same collation.

Regards,
Manu