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.
