| Lists: | pgsql-hackers |
|---|
| From: | zengman <zengman(at)halodbtech(dot)com> |
|---|---|
| To: | pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | [PATCH] Simplify ExecWithoutOverlapsNotEmpty by removing unused parameter |
| Date: | 2026-02-24 15:09:07 |
| Message-ID: | tencent_6AF712884E8D163F7F46073C@qq.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Lists: | pgsql-hackers |
Hi all,
I noticed that ExecWithoutOverlapsNotEmpty() accepts an atttypid parameter that isn't actually used. The function only needs typtype to distinguish between range and multirange types.
Currently lookup_type_cache() is called just to extract typtype, but I think using get_typtype() directly seems more appropriate.
```
static void ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum attval,
- char typtype, Oid atttypid);
+ char typtype);
/* ----------------------------------------------------------------
* ExecOpenIndices
@@ -753,11 +754,10 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index,
{
TupleDesc tupdesc = RelationGetDescr(heap);
Form_pg_attribute att = TupleDescAttr(tupdesc, attno - 1);
- TypeCacheEntry *typcache = lookup_type_cache(att->atttypid, 0);
ExecWithoutOverlapsNotEmpty(heap, att->attname,
values[indnkeyatts - 1],
- typcache->typtype, att->atttypid);
+ get_typtype(att->atttypid));
}
}
@@ -1149,7 +1149,7 @@ index_expression_changed_walker(Node *node, Bitmapset *allUpdatedCols)
* range or multirange in the given attribute.
*/
static void
-ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum attval, char typtype, Oid atttypid)
+ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum attval, char typtype)
{
bool isempty;
RangeType *r;
```
--
regards,
Man Zeng
| Attachment | Content-Type | Size |
|---|---|---|
| 0001-refactor-Simplify-ExecWithoutOverlapsNotEmpty-functi.patch | application/octet-stream | 2.1 KB |
| From: | Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> |
|---|---|
| To: | zengman <zengman(at)halodbtech(dot)com> |
| Cc: | pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: [PATCH] Simplify ExecWithoutOverlapsNotEmpty by removing unused parameter |
| Date: | 2026-02-24 17:13:09 |
| Message-ID: | CAN4CZFOE54zW6=PKmiqeNH+K7P_nkBLW9F8omk1hdcsiLOz5pQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Lists: | pgsql-hackers |
Hello!
Looks good to me!
My only comment is that it could use a proper commit message
explaining the changes.
| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | zengman <zengman(at)halodbtech(dot)com> |
| Cc: | pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: [PATCH] Simplify ExecWithoutOverlapsNotEmpty by removing unused parameter |
| Date: | 2026-02-24 22:42:58 |
| Message-ID: | 2422E6F1-685E-4349-AF8A-684C801567B1@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Lists: | pgsql-hackers |
> On Feb 24, 2026, at 23:09, zengman <zengman(at)halodbtech(dot)com> wrote:
>
> Hi all,
>
> I noticed that ExecWithoutOverlapsNotEmpty() accepts an atttypid parameter that isn't actually used. The function only needs typtype to distinguish between range and multirange types.
> Currently lookup_type_cache() is called just to extract typtype, but I think using get_typtype() directly seems more appropriate.
>
> ```
> static void ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum attval,
> - char typtype, Oid atttypid);
> + char typtype);
>
> /* ----------------------------------------------------------------
> * ExecOpenIndices
> @@ -753,11 +754,10 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index,
> {
> TupleDesc tupdesc = RelationGetDescr(heap);
> Form_pg_attribute att = TupleDescAttr(tupdesc, attno - 1);
> - TypeCacheEntry *typcache = lookup_type_cache(att->atttypid, 0);
>
> ExecWithoutOverlapsNotEmpty(heap, att->attname,
> values[indnkeyatts - 1],
> - typcache->typtype, att->atttypid);
> + get_typtype(att->atttypid));
> }
> }
>
> @@ -1149,7 +1149,7 @@ index_expression_changed_walker(Node *node, Bitmapset *allUpdatedCols)
> * range or multirange in the given attribute.
> */
> static void
> -ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum attval, char typtype, Oid atttypid)
> +ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum attval, char typtype)
> {
> bool isempty;
> RangeType *r;
> ```
>
> --
> regards,
> Man Zeng<0001-refactor-Simplify-ExecWithoutOverlapsNotEmpty-functi.patch>
Removing the parameter atttypid from ExecWithoutOverlapsNotEmpty looks okay as it’s a static function and is only called once.
For the other change, I see a difference between lookup_type_cache and get_typtype, where lookup_type_cache never returns NULL but ereport(ERROR) when oid is invalid; while get_typtype will return ‘\0'. Though ExecWithoutOverlapsNotEmpty() will end up also elog(ERROR), the log message is changed.
I am not sure if there could be some edge cases where att->atttypid could be invalid. If yes, then this change will lead to a small behavior change.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| From: | shihao zhong <zhong950419(at)gmail(dot)com> |
|---|---|
| To: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Cc: | zengman <zengman(at)halodbtech(dot)com>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: [PATCH] Simplify ExecWithoutOverlapsNotEmpty by removing unused parameter |
| Date: | 2026-03-11 16:50:14 |
| Message-ID: | CAGRkXqR0AXy6pRqp2D0D6h8rjoOZ7xm8mKWXfpDk4zwrqxwCCg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Lists: | pgsql-hackers |
On Tue, Feb 24, 2026 at 5:43 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
> > On Feb 24, 2026, at 23:09, zengman <zengman(at)halodbtech(dot)com> wrote:
> >
> > Hi all,
> >
> > I noticed that ExecWithoutOverlapsNotEmpty() accepts an atttypid parameter that isn't actually used. The function only needs typtype to distinguish between range and multirange types.
> > Currently lookup_type_cache() is called just to extract typtype, but I think using get_typtype() directly seems more appropriate.
> >
> > ```
> > static void ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum attval,
> > - char typtype, Oid atttypid);
> > + char typtype);
> >
> > /* ----------------------------------------------------------------
> > * ExecOpenIndices
> > @@ -753,11 +754,10 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index,
> > {
> > TupleDesc tupdesc = RelationGetDescr(heap);
> > Form_pg_attribute att = TupleDescAttr(tupdesc, attno - 1);
> > - TypeCacheEntry *typcache = lookup_type_cache(att->atttypid, 0);
> >
> > ExecWithoutOverlapsNotEmpty(heap, att->attname,
> > values[indnkeyatts - 1],
> > - typcache->typtype, att->atttypid);
> > + get_typtype(att->atttypid));
> > }
> > }
> >
> > @@ -1149,7 +1149,7 @@ index_expression_changed_walker(Node *node, Bitmapset *allUpdatedCols)
> > * range or multirange in the given attribute.
> > */
> > static void
> > -ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum attval, char typtype, Oid atttypid)
> > +ExecWithoutOverlapsNotEmpty(Relation rel, NameData attname, Datum attval, char typtype)
> > {
> > bool isempty;
> > RangeType *r;
> > ```
> >
> > --
> > regards,
> > Man Zeng<0001-refactor-Simplify-ExecWithoutOverlapsNotEmpty-functi.patch>
>
> Removing the parameter atttypid from ExecWithoutOverlapsNotEmpty looks okay as it’s a static function and is only called once.
>
> For the other change, I see a difference between lookup_type_cache and get_typtype, where lookup_type_cache never returns NULL but ereport(ERROR) when oid is invalid; while get_typtype will return ‘\0'. Though ExecWithoutOverlapsNotEmpty() will end up also elog(ERROR), the log message is changed.
>
> I am not sure if there could be some edge cases where att->atttypid could be invalid. If yes, then this change will lead to a small behavior change.
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
>
>
That patch looks good to me.
Thanks,
Shihao