Thread

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

Tender Wang <tndrwang@gmail.com> — 2025-11-17T03:36:48Z

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

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

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