Thread: type of some table storage params on doc

On Mon, Mar 16, 2020 at 11:07:37AM +0900, Atsushi Torikoshi wrote:
> As far as I read the reloptions.c, autovacuum_vacuum_cost_delay,
> autovacuum_vacuum_scale_factor and autovacuum_analyze_scale_factor
> are the members of relopt_real, so their type seems the same, real.

In this case, the parsing uses parse_real(), which is exactly the same
code path as what real GUCs use.

> But the manual about storage parameters[1] says two of their type
> are float4 and the other is floating point.
>
> I think using float4 on storage parameters[1] are not consistent
> so far, how about changing these parameters type from float4 to
> floating point if there are no specific reasons using float4?

That's a good idea, so I am fine to apply your patch as float4 is a
data type.  However, let's see first if others have more comments or
objections.
--
Michael

On 2020-Mar-18, Michael Paquier wrote:

> On Mon, Mar 16, 2020 at 11:07:37AM +0900, Atsushi Torikoshi wrote:
> > As far as I read the reloptions.c, autovacuum_vacuum_cost_delay,
> > autovacuum_vacuum_scale_factor and autovacuum_analyze_scale_factor
> > are the members of relopt_real, so their type seems the same, real.
> 
> In this case, the parsing uses parse_real(), which is exactly the same
> code path as what real GUCs use.
> 
> > But the manual about storage parameters[1] says two of their type
> > are float4 and the other is floating point.
> >
> > I think using float4 on storage parameters[1] are not consistent
> > so far, how about changing these parameters type from float4 to
> > floating point if there are no specific reasons using float4?
> 
> That's a good idea, so I am fine to apply your patch as float4 is a
> data type.  However, let's see first if others have more comments or
> objections.

Hmm.  So unadorned 'floating point' seems to refer to float8; you have
to use float(24) in order to get a float4.  The other standards-mandated
name for float4 seems to be REAL.  (I had a look around but was unable
to figure out whether the standard mandates exact bit widths other than
the precision spec).  Since they're not doubles, what about we use REAL
rather than FLOATING POINT?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2020-Mar-18, Michael Paquier wrote:
>> On Mon, Mar 16, 2020 at 11:07:37AM +0900, Atsushi Torikoshi wrote:
>>>> In this case, the parsing uses parse_real(), which is exactly the same
>>>> code path as what real GUCs use.

> Hmm.  So unadorned 'floating point' seems to refer to float8; you have
> to use float(24) in order to get a float4.  The other standards-mandated
> name for float4 seems to be REAL.  (I had a look around but was unable
> to figure out whether the standard mandates exact bit widths other than
> the precision spec).  Since they're not doubles, what about we use REAL
> rather than FLOATING POINT?

Isn't this whole argument based on a false premise?  What parse_real
returns is double, not float.  Also notice that config.sgml consistently
documents those GUCs as <type>floating point</type>.  (I recall having
recently whacked some GUC descriptions that were randomly out of line
with that.)

            regards, tom lane

On 2020-Mar-18, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2020-Mar-18, Michael Paquier wrote:
> >> On Mon, Mar 16, 2020 at 11:07:37AM +0900, Atsushi Torikoshi wrote:
> >>>> In this case, the parsing uses parse_real(), which is exactly the same
> >>>> code path as what real GUCs use.
> 
> > Hmm.  So unadorned 'floating point' seems to refer to float8; you have
> > to use float(24) in order to get a float4.  The other standards-mandated
> > name for float4 seems to be REAL.  (I had a look around but was unable
> > to figure out whether the standard mandates exact bit widths other than
> > the precision spec).  Since they're not doubles, what about we use REAL
> > rather than FLOATING POINT?
> 
> Isn't this whole argument based on a false premise?  What parse_real
> returns is double, not float.  Also notice that config.sgml consistently
> documents those GUCs as <type>floating point</type>.  (I recall having
> recently whacked some GUC descriptions that were randomly out of line
> with that.)

Ah, I hadn't checked -- I was taking the function and struct names at
face value, but it turns out that they're lies as well -- parse_real,
relopt_real all parsing/storing doubles *is* confusing.

That being the case, I agree that "float4" is the wrong thing and
"floating point" is what to use.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Ah, I hadn't checked -- I was taking the function and struct names at
> face value, but it turns out that they're lies as well -- parse_real,
> relopt_real all parsing/storing doubles *is* confusing.

Yeah, that's certainly true.  I wonder if we could rename them
without causing a lot of pain for extensions?

            regards, tom lane

On 2020-Mar-18, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Ah, I hadn't checked -- I was taking the function and struct names at
> > face value, but it turns out that they're lies as well -- parse_real,
> > relopt_real all parsing/storing doubles *is* confusing.
> 
> Yeah, that's certainly true.  I wonder if we could rename them
> without causing a lot of pain for extensions?

I don't think it will, directly; debian.codesearch.net says only patroni
and slony1-2 contain the "parse_real", and both have their own
implementations (patroni is Python anyway).  I didn't find any
relopt_real anywhere.

However, if we were to rename DefineCustomRealVariable() to match, that
would no doubt hurt a lot of people.  We also have GucRealCheckHook and
GucRealAssignHook typedefs, but those appear to hit no Debian package.
(In guc.c, the fallout rabbit hole goes pretty deep, but that seems well
localized.)

I don't think the last pg13 CF is when to be spending time on this,
though.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Mar 18, 2020 at 02:29:17PM -0300, Alvaro Herrera wrote:
> I don't think it will, directly; debian.codesearch.net says only patroni
> and slony1-2 contain the "parse_real", and both have their own
> implementations (patroni is Python anyway).  I didn't find any
> relopt_real anywhere.

Reloptions in general are not used much in extensions, and one could
assume that reloptions of type real (well, double) are even less.

> However, if we were to rename DefineCustomRealVariable() to match, that
> would no doubt hurt a lot of people.  We also have GucRealCheckHook and
> GucRealAssignHook typedefs, but those appear to hit no Debian package.
> (In guc.c, the fallout rabbit hole goes pretty deep, but that seems well
> localized.)

I make use of this API myself, for some personal stuff, and even some
internal company stuff.  And I am ready to bet that it is much more
popular than its reloption cousin mainly for bgworkers.  Hence a
rename would need a compatibility layer remaining around.  Honestly, I
am not sure that a rename is worth it.

> I don't think the last pg13 CF is when to be spending time on this,
> though.

Indeed.

Do any of you have any arguments against the patch proposed upthread
switching "float4" to "floating point" then?  Better be sure..
--
Michael

On 2020-Mar-19, Michael Paquier wrote:

> Do any of you have any arguments against the patch proposed upthread
> switching "float4" to "floating point" then?  Better be sure..

None here.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Mar 19, 2020 at 12:04:45AM -0300, Alvaro Herrera wrote:
> None here.

Thanks Álvaro.  Applied and back-patched down to 9.5 then.
--
Michael