Thread: 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
Attachment
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
Attachment
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