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] 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] 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 fix_type_in_createtable_storage-params_v1.patch Description: Binary data
Re: 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 signature.asc Description: PGP signature
Re: type of some table storage params on doc
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: type of some table storage params on doc
Alvaro Herrera 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 floating point. (I recall having recently whacked some GUC descriptions that were randomly out of line with that.) regards, tom lane
Re: type of some table storage params on doc
On 2020-Mar-18, Tom Lane wrote: > Alvaro Herrera 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 floating point. (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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: type of some table storage params on doc
Alvaro Herrera 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
Re: type of some table storage params on doc
On 2020-Mar-18, Tom Lane wrote: > Alvaro Herrera 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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: type of some table storage params on doc
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 signature.asc Description: PGP signature
Re: type of some table storage params on doc
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: type of some table storage params on doc
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 signature.asc Description: PGP signature
Re: type of some table storage params on doc
On Mon, Mar 23, 2020 at 1:58 PM Michael Paquier 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