useless RangeIOData->typiofunc
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:
range-typiofunc-cleanup.patchtext/x-diff; charset=us-asciiDownload
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;
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
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
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:
range-typiofunc-cleanup-2.patchtext/x-diff; charset=us-asciiDownload
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;
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
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