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

On Tue, Sep 15, 2026 at 4:05 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Tue, Sep 15, 2026 at 4:48 AM Richard Guo <guofenglinux@gmail.com> wrote:
> >
> > On Mon, Sep 14, 2026 at 5:24 PM Richard Guo <guofenglinux@gmail.com> wrote:
> > > On Sat, Sep 12, 2026 at 10:57 PM Alexander Korotkov
> > > <aekorotkov@gmail.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.

Please, let me know if you're OK with this change.  As this is major
change for the SQL tests of this patch.

------
Regards,
Alexander Korotkov
Supabase