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

Hi, Richard!

On Tue, Sep 15, 2026 at 4:48 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> On Mon, Sep 14, 2026 at 5:24 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> > On Sat, Sep 12, 2026 at 10:57 PM Alexander Korotkov
> > <aekorotkov(at)gmail(dot)com> wrote:
> > > Any objections if I push (and backpatch) this?
>
> > I'll have a look ASAP.
>
> I looked at v6. The fix itself looks right to me. I have some
> comments on the rest of the patch, though.
>
> * Overall, the new comments in this patch are overly verbose. I think
> we should keep them in line with the style of the existing comments.
> Also, the new comment in compute_semijoin_info() talks only about the
> collation, but the call also keeps the expression's original type,
> which I think the comment should mention as well.
>
> * In compute_semijoin_info(), the new comment and the relabeling call
> are inserted between the two appends, right after
>
> /* so far so good, keep building lists */
>
> That comment is about building both lists, so it reads oddly with the
> appends split apart. I'd rather relabel right_expr as a separate step
> before that comment, and then keep the two appends together under it.
>
> * The tests alter tables that other test cases in this file rely on,
> which is really not a good practice.
>
> INSERT INTO test2cs VALUES ('abc');
> ALTER TABLE test2cs ALTER COLUMN x SET (n_distinct = 1);
> ANALYZE test1ci, test2cs;
> ...
> ANALYZE test1cs;
>
> The INSERT is only there so that test2cs holds both 'abc' and 'ABC',
> but test3cs already holds exactly that, and it also has the unique
> index under case_sensitive, so it covers the unique-index case as
> well. We don't need the n_distinct hack or the ANALYZEs either:
> disabling seqscan and material is enough to make the planner
> unique-ify test3cs, since the semijoin's inner index-only scan on
> test3cs can't use the case_insensitive join clause as an index
> condition and would have to be rescanned for every outer row.
>
> * The same query
>
> SELECT count(*) FROM test1ci
> WHERE x COLLATE case_insensitive IN
> (SELECT x FROM test2cs WHERE x IN ('abc', 'ABC'));
>
> is run three times, once under the default settings without an EXPLAIN
> and once under each forced strategy. The first run adds nothing, as
> its plan is one of the other two and we can't even tell which.
>
> * The last test, the one with "ANALYZE test1cs", pins a Hash Right
> Semi Join, which is a plain semijoin that never unique-ifies, so on
> master it doesn't exercise this patch at all. What it does exercise
> is reduce_unique_semijoins(), which was fixed in 5a55ea507 and is
> already covered by the "Ensure that semijoin is not reduced to
> innerjoin" tests earlier in this file. The case the comment describes
> only exists in the back branches, where create_unique_path() still
> consults relation_has_unique_index_for() with semi_rhs_exprs, and
> using test3cs as the RHS covers that with the same query.
>
> * One more thing for the commit message. In the back branches
> create_unique_path() also has the DISTINCT-subquery shortcut, which
> goes through the collation-blind two-list query_is_distinct_for()
> wrapper. This patch happens to close that too, because
> translate_sub_tlist() punts on the RelabelType, so it's worth a
> sentence there.
>
> Attached is v7 with these changes.

Thank you for your corrections. I accept all of them.

I suggest we could change the test query as follows.

SELECT count(*) FROM test1ci
WHERE x COLLATE case_insensitive IN (SELECT x FROM test3cs);

This shape allows to achieve the desired plan both with
enable_groupagg = off and with enable_sort = off. Therefore sql part
for pg19- will only differ with this GUC name. I've checked it works
down to pg14 and keeps distinguishing the fix.

Attached v8 with this change.

------
Regards,
Alexander Korotkov
Supabase