Re: Use opresulttype instead of calling SearchSysCache1() in match_orclause_to_indexcol()

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 于2025年11月17日周一 01:27写道：

> Tender Wang <tndrwang(at)gmail(dot)com> writes:
> > Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 于2025年11月16日周日 04:45写道：
> >> Yeah. In fact, I think it's outright wrong to do that here.
> >> It'd result in building a SAOP expression that lacks the RelabelType,
> >> which seems incorrect since the operator is one that expects the
> >> relabeled type.
> >>
> >> The RelabelType-stripping logic for the constExpr seems unnecessary as
> >> well, if not outright wrong. It won't matter for an actual Const,
> >> because eval_const_expressions would have flattened the relabeled type
> >> into the Const node. However, if we have some non-Const right-hand
> >> sides, the effect of stripping RelabelTypes could easily be to fail the
> >> transformation unnecessarily. That'd happen if the parser had coerced
> >> all the RHS values to be the same type for application of the operator,
> >> but then we stripped some RelabelTypes and mistakenly decided that
> >> the resulting RHSes didn't match in type.
>
> > Thank you for pointing that out. I hadn’t been aware of these problems
> > earlier.
>
> I made a test script (attached) that demonstrates that these problems
> are real. In HEAD, if you look at the logged plan tree for the first
> query, you'll see that we have a SAOP with operator texteq whose first
> input is a bare varchar-type Var, unlike what you get with a plain
> indexqual such as "vc1 = '23'". Now texteq() doesn't care, but there
> are polymorphic functions that do care because they look at the
> exposed types of their input arguments. Also, HEAD fails to optimize
> the second test case into a SAOP because it's fooled itself by
> stripping the RelabelType from the outer-side Var.
>

Yeah, the plan of the second test case should be like below:
postgres=# explain
select t1.* from t1, t2
where t2.vc1 = '66' and (t1.vc1 = t2.x or t1.vc1 = '99');
QUERY PLAN

-----------------------------------------------------------------------------
Nested Loop (cost=0.83..17.32 rows=2 width=5)
-> Index Scan using t2_pkey on t2 (cost=0.42..8.44 rows=1 width=5)
Index Cond: ((vc1)::text = '66'::text)
-> Index Only Scan using t1_pkey on t1 (cost=0.42..8.87 rows=2 width=5)
Index Cond: (vc1 = ANY (ARRAY[(t2.x)::text, '99'::text]))
(5 rows)

>
> >> I'm not very convinced that the type_is_rowtype checks are correct
> >> either. I can see that we'd better forbid RECORD, because we've got
> >> no way to be sure that all the RHSes are actually the same record
> >> type. But I don't see why it's necessary or appropriate to forbid
> >> named composite types. I didn't change that here; maybe we should
> >> look into the discussion leading up to d4378c000.
>
> > Agree.
>
> I dug into the history a little and could not find anything except
> [1], which says
>
> I have made some changes (attachment).
> * if the operator expression left or right side type category is
> {array | domain | composite}, then don't do the transformation.
> (i am not 10% sure with composite)
>
> with no further justification than that. There were even messages
> later in the thread questioning the need for it, but nobody did
> anything about it. The transformation does work perfectly well
> if enabled, as shown by the second part of the attached test script.
>
> So I end with v3, now with a full-dress commit message.
>

The v3 LGTM.

--
Thanks,
Tender Wang