pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feeduseless RangeIOData->typiofunc
6+ messages / 3 participants
[nested] [flat]
* useless RangeIOData->typiofunc
@ 2020-03-04 21:57 Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 2 replies; 6+ messages in thread
From: Alvaro Herrera @ 2020-03-04 21:57 UTC (permalink / raw)
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>
I noticed while going over the multirange types patch that it adds a
pointless typiofunc cached OID to a struct used for I/O functions'
fn_extra. It seems to go completely unused, so I checked range types
(which this was cribbed from) and indeed, it is completely unused there
either. My guess is that it was in turn cribbed from array's
ArrayMetaState, which is considerably more sophisticated; I suspect
nobody noticed that caching it was pointless.
Here's a patch to remove it from rangetypes.c. It doesn't really waste
much memory anyway, but removing it lessens the cognitive load by one or
two bits.
--
Álvaro Herrera
Attachments:
[text/x-diff] range-typiofunc-cleanup.patch (1.5K, ../../20200304215711.GA8732@alvherre.pgsql/2-range-typiofunc-cleanup.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/rangetypes.c b/src/backend/utils/adt/rangetypes.c
index 490bc2ae81..412322c6fa 100644
--- a/src/backend/utils/adt/rangetypes.c
+++ b/src/backend/utils/adt/rangetypes.c
@@ -49,7 +49,6 @@
typedef struct RangeIOData
{
TypeCacheEntry *typcache; /* range type's typcache entry */
- Oid typiofunc; /* element type's I/O function */
Oid typioparam; /* element type's I/O parameter */
FmgrInfo proc; /* lookup result for typiofunc */
} RangeIOData;
@@ -309,6 +308,7 @@ get_range_io_data(FunctionCallInfo fcinfo, Oid rngtypid, IOFuncSelector func)
bool typbyval;
char typalign;
char typdelim;
+ Oid typiofunc;
cache = (RangeIOData *) MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
sizeof(RangeIOData));
@@ -324,9 +324,9 @@ get_range_io_data(FunctionCallInfo fcinfo, Oid rngtypid, IOFuncSelector func)
&typalign,
&typdelim,
&cache->typioparam,
- &cache->typiofunc);
+ &typiofunc);
- if (!OidIsValid(cache->typiofunc))
+ if (!OidIsValid(typiofunc))
{
/* this could only happen for receive or send */
if (func == IOFunc_receive)
@@ -340,7 +340,7 @@ get_range_io_data(FunctionCallInfo fcinfo, Oid rngtypid, IOFuncSelector func)
errmsg("no binary output function available for type %s",
format_type_be(cache->typcache->rngelemtype->type_id))));
}
- fmgr_info_cxt(cache->typiofunc, &cache->proc,
+ fmgr_info_cxt(typiofunc, &cache->proc,
fcinfo->flinfo->fn_mcxt);
fcinfo->flinfo->fn_extra = (void *) cache;
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: useless RangeIOData->typiofunc
@ 2020-03-04 22:34 Paul Jungwirth <pj@illuminatedcomputing.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 0 replies; 6+ messages in thread
From: Paul Jungwirth @ 2020-03-04 22:34 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
On 3/4/20 1:57 PM, Alvaro Herrera wrote:
> I noticed while going over the multirange types patch that it adds a
> pointless typiofunc cached OID to a struct used for I/O functions'
> fn_extra. It seems to go completely unused, so I checked range types
> (which this was cribbed from) and indeed, it is completely unused there
> either. My guess is that it was in turn cribbed from array's
> ArrayMetaState, which is considerably more sophisticated; I suspect
> nobody noticed that caching it was pointless.
I didn't believe it at first but I think you're right. :-)
> Here's a patch to remove it from rangetypes.c. It doesn't really waste
> much memory anyway, but removing it lessens the cognitive load by one or
> two bits.
Looks good to me, and it seems okay to make the same edits to
multirangetypes.c
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: useless RangeIOData->typiofunc
@ 2020-03-04 22:34 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 6+ messages in thread
From: Tom Lane @ 2020-03-04 22:34 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I noticed while going over the multirange types patch that it adds a
> pointless typiofunc cached OID to a struct used for I/O functions'
> fn_extra. It seems to go completely unused, so I checked range types
> (which this was cribbed from) and indeed, it is completely unused there
> either. My guess is that it was in turn cribbed from array's
> ArrayMetaState, which is considerably more sophisticated; I suspect
> nobody noticed that caching it was pointless.
> Here's a patch to remove it from rangetypes.c. It doesn't really waste
> much memory anyway, but removing it lessens the cognitive load by one or
> two bits.
Hm, I'm not sure that really lessens the cognitive load any, but
if you do commit this please fix the dangling reference you left
in the nearby comment:
{
TypeCacheEntry *typcache; /* range type's typcache entry */
- Oid typiofunc; /* element type's I/O function */
Oid typioparam; /* element type's I/O parameter */
FmgrInfo proc; /* lookup result for typiofunc */
^^^^^^^^^^^^^^^^^^^^^^^^^^^
} RangeIOData;
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: useless RangeIOData->typiofunc
@ 2020-03-05 14:18 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Alvaro Herrera @ 2020-03-05 14:18 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>; Paul Jungwirth <pj@illuminatedcomputing.com>
On 2020-Mar-04, Tom Lane wrote:
> Hm, I'm not sure that really lessens the cognitive load any, but
> if you do commit this please fix the dangling reference you left
> in the nearby comment:
>
> {
> TypeCacheEntry *typcache; /* range type's typcache entry */
> - Oid typiofunc; /* element type's I/O function */
> Oid typioparam; /* element type's I/O parameter */
> FmgrInfo proc; /* lookup result for typiofunc */
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> } RangeIOData;
Thanks -- ISTM it makes more sense to put the FmgrInfo before the
typioparam too:
typedef struct RangeIOData
{
TypeCacheEntry *typcache; /* range type's typcache entry */
FmgrInfo proc; /* element type's I/O function */
Oid typioparam; /* element type's I/O parameter */
} RangeIOData;
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
[text/x-diff] range-typiofunc-cleanup-2.patch (1.6K, ../../20200305141859.GA6736@alvherre.pgsql/2-range-typiofunc-cleanup-2.patch)
download | inline diff:
diff --git a/src/backend/utils/adt/rangetypes.c b/src/backend/utils/adt/rangetypes.c
index 490bc2ae81..c6bb21219b 100644
--- a/src/backend/utils/adt/rangetypes.c
+++ b/src/backend/utils/adt/rangetypes.c
@@ -49,9 +49,8 @@
typedef struct RangeIOData
{
TypeCacheEntry *typcache; /* range type's typcache entry */
- Oid typiofunc; /* element type's I/O function */
+ FmgrInfo proc; /* element type's I/O function */
Oid typioparam; /* element type's I/O parameter */
- FmgrInfo proc; /* lookup result for typiofunc */
} RangeIOData;
@@ -309,6 +308,7 @@ get_range_io_data(FunctionCallInfo fcinfo, Oid rngtypid, IOFuncSelector func)
bool typbyval;
char typalign;
char typdelim;
+ Oid typiofunc;
cache = (RangeIOData *) MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
sizeof(RangeIOData));
@@ -324,9 +324,9 @@ get_range_io_data(FunctionCallInfo fcinfo, Oid rngtypid, IOFuncSelector func)
&typalign,
&typdelim,
&cache->typioparam,
- &cache->typiofunc);
+ &typiofunc);
- if (!OidIsValid(cache->typiofunc))
+ if (!OidIsValid(typiofunc))
{
/* this could only happen for receive or send */
if (func == IOFunc_receive)
@@ -340,7 +340,7 @@ get_range_io_data(FunctionCallInfo fcinfo, Oid rngtypid, IOFuncSelector func)
errmsg("no binary output function available for type %s",
format_type_be(cache->typcache->rngelemtype->type_id))));
}
- fmgr_info_cxt(cache->typiofunc, &cache->proc,
+ fmgr_info_cxt(typiofunc, &cache->proc,
fcinfo->flinfo->fn_mcxt);
fcinfo->flinfo->fn_extra = (void *) cache;
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: useless RangeIOData->typiofunc
@ 2020-03-05 14:26 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2020-03-05 14:26 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>; Paul Jungwirth <pj@illuminatedcomputing.com>
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Thanks -- ISTM it makes more sense to put the FmgrInfo before the
> typioparam too:
> typedef struct RangeIOData
> {
> TypeCacheEntry *typcache; /* range type's typcache entry */
> FmgrInfo proc; /* element type's I/O function */
> Oid typioparam; /* element type's I/O parameter */
> } RangeIOData;
Yeah, WFM. Maybe even rename the FmgrInfo to "typioproc"
or the like?
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: useless RangeIOData->typiofunc
@ 2020-03-05 15:02 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 6+ messages in thread
From: Alvaro Herrera @ 2020-03-05 15:02 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>; Paul Jungwirth <pj@illuminatedcomputing.com>
On 2020-Mar-05, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Thanks -- ISTM it makes more sense to put the FmgrInfo before the
> > typioparam too:
>
> > typedef struct RangeIOData
> > {
> > TypeCacheEntry *typcache; /* range type's typcache entry */
> > FmgrInfo proc; /* element type's I/O function */
> > Oid typioparam; /* element type's I/O parameter */
> > } RangeIOData;
>
> Yeah, WFM. Maybe even rename the FmgrInfo to "typioproc"
> or the like?
Good idea, thanks! Pushed with that change.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2020-03-05 15:02 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-04 21:57 useless RangeIOData->typiofunc Alvaro Herrera <alvherre@2ndquadrant.com>
2020-03-04 22:34 ` Paul Jungwirth <pj@illuminatedcomputing.com>
2020-03-04 22:34 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-05 14:18 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-03-05 14:26 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-05 15:02 ` Alvaro Herrera <alvherre@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox