type of some table storage params on doc
Hi,
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.
But the manual about storage parameters[1]https://www.postgresql.org/docs/devel/sql-createtable.htm says two of their type
are float4 and the other is floating point.
autovacuum_vacuum_cost_delay, toast.autovacuum_vacuum_cost_delay
(floating point)
autovacuum_vacuum_scale_factor, toast.autovacuum_vacuum_scale_factor
(float4)
autovacuum_analyze_scale_factor (float4)
And the manual about GUC says all these parameters are floating
point.
autovacuum_vacuum_cost_delay (floating point)
autovacuum_vacuum_scale_factor (floating point)
autovacuum_analyze_scale_factor (floating point)
Also other members of relopt_real such as seq_page_cost,
random_page_cost and vacuum_cleanup_index_scale_factor are
documented as floating point.
I think using float4 on storage parameters[1]https://www.postgresql.org/docs/devel/sql-createtable.htm are not consistent
so far, how about changing these parameters type from float4 to
floating point if there are no specific reasons using float4?
Attached a patch.
Any thought?
[1]: https://www.postgresql.org/docs/devel/sql-createtable.htm
[2]: https://www.postgresql.org/docs/devel/runtime-config-autovacuum.html
Regards,
--
Torikoshi Atsushi
Attachments:
fix_type_in_createtable_storage-params_v1.patchapplication/octet-stream; name=fix_type_in_createtable_storage-params_v1.patchDownload
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 4a2b6f0..15b50c5 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -1461,7 +1461,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
</varlistentry>
<varlistentry id="reloption-autovacuum-vauum-scale-factor" xreflabel="autovacuum_vacuum_scale_factor">
- <term><literal>autovacuum_vacuum_scale_factor</literal>, <literal>toast.autovacuum_vacuum_scale_factor</literal> (<type>float4</type>)
+ <term><literal>autovacuum_vacuum_scale_factor</literal>, <literal>toast.autovacuum_vacuum_scale_factor</literal> (<type>floating point</type>)
<indexterm>
<primary><varname>autovacuum_vacuum_scale_factor</varname> </primary>
<secondary>storage parameter</secondary>
@@ -1491,7 +1491,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
</varlistentry>
<varlistentry id="reloption-autovacuum-analyze-scale-factor" xreflabel="autovacuum_analyze_scale_factor">
- <term><literal>autovacuum_analyze_scale_factor</literal> (<type>float4</type>)
+ <term><literal>autovacuum_analyze_scale_factor</literal> (<type>floating point</type>)
<indexterm>
<primary><varname>autovacuum_analyze_scale_factor</varname></primary>
<secondary>storage parameter</secondary>
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
On Mon, Mar 23, 2020 at 1:58 PM Michael Paquier <michael@paquier.xyz> wrote:
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
Thanks for applying the patch.
Regards,
--
Atsushi Torikoshi