Re: Use opresulttype instead of calling SearchSysCache1() in match_orclause_to_indexcol() - Mailing list pgsql-hackers

Tender Wang <tndrwang@gmail.com> writes:
> Tom Lane <tgl@sss.pgh.pa.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.

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]))

>> 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.