Thread

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

Tender Wang <tndrwang@gmail.com> — 2025-11-16T12:41:26Z

Tom Lane <tgl@sss.pgh.pa.us> 于2025年11月16日周日 04:45写道：

> Tender Wang <tndrwang@gmail.com> writes:
> > Tom Lane <tgl@sss.pgh.pa.us> 于2025年9月20日周六 00:16写道：
> >> I don't understand what purpose this check serves at all.
> >> We are looking at an arm of an OR clause, so it had better
> >> yield boolean.
>
> > Yeah, this check doesn't need any more. I removed this check in the
> > attached patch.
>
> > In match_index_to_operand(), the indexExpr has been ignored, so I removed
> > below check, too.
> > - if (IsA(indexExpr, RelabelType))
> > - indexExpr = (Node *) ((RelabelType *) indexExpr)->arg;
>
> 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.


>
> So I removed both of those in v2, attached.
>
> >> In general, this code looks like a mess.  There are a lot of
> >> Asserts that might be okay in development but probably should
> >> not have got committed, and the comments need work.
>
> > These assertions were removed by me, too.
> > I didn’t modify these code comments since English isn’t my native
> language,
> > and I’d appreciate your help with them.
>
> Here's a v2 with some further cleanup work.  One notable item
> is that I moved the type_is_rowtype checks, which don't seem
> to need to be done more than once.
>
> 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.
The v2 patch LGTM.

-- 
Thanks,
Tender Wang