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

Hi Tender,

Although this would not improve performance, it can save some CPU time.
Any thoughts?

The patch looks correct to me. The correctness can also be rechecked like this:

```
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -3372,6 +3372,9 @@ match_orclause_to_indexcol(PlannerInfo *root,
                /* Only the operator returning a boolean suit the
transformation. */
                if (get_op_rettype(opno) != BOOLOID)
                        break;
+               else {
+                       Assert(subClause->opresulttype == BOOLOID);
+               }
```

--
Best regards,
Aleksander Alekseev

Tender Wang <tndrwang@gmail.com> writes:

While working on another issue, I stepped into match_orclause_to_indexcol().
I found below codes:

/* Only the operator returning a boolean suit the transformation. */
if (get_op_rettype(opno) != BOOLOID)
break;

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.

If anything, this is actively wrong, because for example it'd
reject a polymorphic operator that yields boolean in-context.

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.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> 于2025年9月20日周六 00:16写道：

Tender Wang <tndrwang@gmail.com> writes:

While working on another issue, I stepped into

match_orclause_to_indexcol().

I found below codes:

/* Only the operator returning a boolean suit the transformation. */
if (get_op_rettype(opno) != BOOLOID)
break;

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;

If anything, this is actively wrong, because for example it'd
reject a polymorphic operator that yields boolean in-context.

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.

--
Thanks,
Tender Wang

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.

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.

regards, tom lane

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

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.

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]: /messages/by-id/CACJufxFrZS07oBHMk1_c8P3A84VZ3ysXiZV8NeU6gAnvu+HsVA@mail.gmail.com

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.

regards, tom lane

[1]: /messages/by-id/CACJufxFrZS07oBHMk1_c8P3A84VZ3ysXiZV8NeU6gAnvu+HsVA@mail.gmail.com

On Sun, Nov 16, 2025 at 7:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

Thank you so much for the clarification of this subject with examples.

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.

I think another email to reference is [1]. It analyses the problems
with row expressions, but finally it mistakenly generalizes that for
composite types. So, yes, thread didn't show any problems with
composites.

So I end with v3, now with a full-dress commit message.

It looks very good, thank you so much for dedicating your time on fixing this.

Links.
1. /messages/by-id/567ED6CA.2040504@sigaev.ru

------
Regards,
Alexander Korotkov
Supabase

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

Alexander Korotkov <aekorotkov@gmail.com> writes:

On Sun, Nov 16, 2025 at 7:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

I think another email to reference is [1]. It analyses the problems
with row expressions, but finally it mistakenly generalizes that for
composite types. So, yes, thread didn't show any problems with
composites.

Thanks for confirming that.

So I end with v3, now with a full-dress commit message.

It looks very good, thank you so much for dedicating your time on fixing this.

Pushed v3, thank you for looking at it.

regards, tom lane