Re: BUG #19633: Unexpected results of IN (subquery) with a non-deterministic collation

Hi!

On Fri, Aug 21, 2026 at 8:02 AM Andrey Rachitskiy <pl0h0yp1(at)gmail(dot)com> wrote:
> пт, 21 авг. 2026 г. в 06:54, Tender Wang <tndrwang(at)gmail(dot)com>:
>>
>> The approach looks good to me. I'd suggest adjusting the comment as follows:
>> ...
>> /* so far so good, keep building lists */
>> semi_operators = lappend_oid(semi_operators, opno);
>>
>> /*
>> * Ensure that the RHS expression exposes the join operator's input
>> * collation. The expression will later be used as a grouping key when
>> * unique-ifying the RHS, so its collation must agree with the semijoin
>> * equality semantics.
>> */
>> semi_rhs_exprs =
>> lappend(semi_rhs_exprs,
>> canonicalize_ec_expression((Expr *) copyObject(right_expr),
>> exprType(right_expr),
>> op->inputcollid));
>>
> Dear Tender,
>
> Thanks for the review.
> Made the changes - v2 attached.

I think this approach is right. Everybody who consumes
SpecialJoinInfo.semi_rhs_exprs takes the collation from the expression
itself, because SortGroupClause does not carry one and neither does
the pathkey machinery. So labelling the expression once, where it is
recorded, fixes every consumer at the same time; fixing it in the
consumers would mean doing the same thing in three places and getting
it right in each. The call is also exactly what process_equivalence()
already does for equivalence class members, with the same two
arguments and for the same reason, so this is not a new trick.

I've revised the patch. The v3 changes are:
1. The test now covers the hash path as well. v2 turned
enable_hashagg off, which is precisely the plan shape that was not
broken in isolation; both strategies are now exercised, each with its
plan and its result.
2. Test also checks results rows, not only the count. The bug is the
disagreement between the two: the predicate is true for one row while
filtering on it returned two.
3. Added a test for unique-ification being skipped because of a unique
index. Getting real coverage from it needs a little care: with only
'a' and 'A' in the RHS the planner keeps an ordinary semijoin and
answers correctly, so the test passes with or without the fix. Two
filler rows are enough to make unique-ification look worthwhile and
put the decision on the table. The plan is checked as well, so that a
future costing change cannot quietly turn this into a test of nothing.
4. Added a comment saying why the INSERT of 'A' comes after ANALYZE.
5. Reworded the code comment to say why rather than what:
unique-ification groups on this expression and takes the collation
from it, since SortGroupClause carries none.
6. Commit message now mentions HashAggregate, which v2 did not.

Now the part that is missing from the thread entirely. This is not a v19
regression. The line

semi_rhs_exprs = lappend(semi_rhs_exprs, copyObject(right_expr));

is identical in every supported branch back to 14, and the consumer
has the same shape there: REL_18's create_unique_path() builds a
SortGroupClause with no collation and takes the key from uniqexpr. So
this is a live wrong-results bug in all of them (I've checked it), and
I intend to backpatch it through 14.

------
Regards,
Alexander Korotkov
Supabase