Обсуждение: max_slot_wal_keep_size comment in postgresql.conf

At Tue, 26 May 2020 09:10:40 -0400, Jeff Janes <jeff.janes@gmail.com> wrote in 
> In postgresql.conf, it says:
> 
> #max_slot_wal_keep_size = -1  # measured in bytes; -1 disables
> 
> I don't know if that is describing the dimension of this parameter or the
> units of it, but the default units for it are megabytes, not individual
> bytes, so I think it is pretty confusing.

Agreed. It should be a leftover at the time the unit was changed
(before committed) to MB from bytes.  The default value makes the
confusion worse.

Is the following works?

#max_slot_wal_keep_size = -1  # in MB; -1 disables

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 0bd9a7360caaf8a31d71ba6392d9e4b7496a9c02 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Wed, 27 May 2020 10:42:24 +0900
Subject: [PATCH] Change a comment in postgresql.conf.sample

Fix a lame comment for max_slot_wal_keep_size so that it suggests the
unit for the value.
---
 src/backend/utils/misc/postgresql.conf.sample | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 995b6ca155..43340eee71 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -289,7 +289,7 @@
 #max_wal_senders = 10        # max number of walsender processes
                 # (change requires restart)
 #wal_keep_segments = 0        # in logfile segments; 0 disables
-#max_slot_wal_keep_size = -1    # measured in bytes; -1 disables
+#max_slot_wal_keep_size = -1    # in MB; -1 disables
 #wal_sender_timeout = 60s    # in milliseconds; 0 disables
 
 #max_replication_slots = 10    # max number of replication slots
-- 
2.18.2

On Wed, May 27, 2020 at 10:46:27AM +0900, Kyotaro Horiguchi wrote:
> Agreed. It should be a leftover at the time the unit was changed
> (before committed) to MB from bytes.  The default value makes the
> confusion worse.
>
> Is the following works?
>
> #max_slot_wal_keep_size = -1  # in MB; -1 disables

Indeed, better to fix that.  The few GUCs using memory units that have
such a mention in their comments use the actual name of the memory
unit, and not its abbreviation (see log_temp_files).  So it seems more
logic to me to just use "in megabytes; -1 disables", that would be
also more consistent with the time-unit-based ones.
--
Michael

At Tue, 26 May 2020 22:56:39 -0400, Isaac Morland <isaac.morland@gmail.com> wrote in 
> On Tue, 26 May 2020 at 21:46, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> wrote:
> 
> > At Tue, 26 May 2020 09:10:40 -0400, Jeff Janes <jeff.janes@gmail.com>
> > wrote in
> > > In postgresql.conf, it says:
> > >
> > > #max_slot_wal_keep_size = -1  # measured in bytes; -1 disables
> > >
> > > I don't know if that is describing the dimension of this parameter or the
> > > units of it, but the default units for it are megabytes, not individual
> > > bytes, so I think it is pretty confusing.
> >
> > Agreed. It should be a leftover at the time the unit was changed
> > (before committed) to MB from bytes.  The default value makes the
> > confusion worse.
> >
> > Is the following works?
> >
> > #max_slot_wal_keep_size = -1  # in MB; -1 disables
> 
> 
> Extreme pedant question: Is it MB (10^6 bytes) or MiB (2^20 bytes)?


GUC variables for file/memory sizes are in a traditional
representation, that is, a power of two represented by
SI-prefixes. AFAICS PostgreSQL doesn't use binary-prefixed units.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2020/05/27 15:11, Michael Paquier wrote:
> On Wed, May 27, 2020 at 10:46:27AM +0900, Kyotaro Horiguchi wrote:
>> Agreed. It should be a leftover at the time the unit was changed
>> (before committed) to MB from bytes.  The default value makes the
>> confusion worse.
>>
>> Is the following works?
>>
>> #max_slot_wal_keep_size = -1  # in MB; -1 disables
> 
> Indeed, better to fix that.  The few GUCs using memory units that have
> such a mention in their comments use the actual name of the memory
> unit, and not its abbreviation (see log_temp_files).  So it seems more
> logic to me to just use "in megabytes; -1 disables", that would be
> also more consistent with the time-unit-based ones.

+1

#temp_file_limit = -1            # limits per-process temp file space
                    # in kB, or -1 for no limit

BTW, the abbreviation "in kB" is used in temp_file_limit.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Wed, 27 May 2020 15:11:00 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Wed, May 27, 2020 at 10:46:27AM +0900, Kyotaro Horiguchi wrote:
> > Agreed. It should be a leftover at the time the unit was changed
> > (before committed) to MB from bytes.  The default value makes the
> > confusion worse.
> > 
> > Is the following works?
> > 
> > #max_slot_wal_keep_size = -1  # in MB; -1 disables
> 
> Indeed, better to fix that.  The few GUCs using memory units that have
> such a mention in their comments use the actual name of the memory
> unit, and not its abbreviation (see log_temp_files).  So it seems more

I was not sure which is preferable.  Does that mean we will fix the
following, too?

> #temp_file_limit = -1           # limits per-process temp file space
>                     # in kB, or -1 for no limit

> logic to me to just use "in megabytes; -1 disables", that would be
> also more consistent with the time-unit-based ones.

I don't oppose to full-spelling.  How about the attached?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 0389911da7b2af708d518423e2c5cacebc4d5f31 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Wed, 27 May 2020 10:42:24 +0900
Subject: [PATCH v2] Change some comments in postgresql.conf.sample

Fix a lame comment for max_slot_wal_keep_size so that it suggests the
unit for the value.  On the way fixing that, fixed another instacne of
unit symbol (kB) used as unit name to its unit name (kilobytes).
---
 src/backend/utils/misc/postgresql.conf.sample | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 995b6ca155..81055edde7 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -149,7 +149,7 @@
 # - Disk -
 
 #temp_file_limit = -1            # limits per-process temp file space
-                    # in kB, or -1 for no limit
+                    # in kilobytes, or -1 for no limit
 
 # - Kernel Resources -
 
@@ -289,7 +289,7 @@
 #max_wal_senders = 10        # max number of walsender processes
                 # (change requires restart)
 #wal_keep_segments = 0        # in logfile segments; 0 disables
-#max_slot_wal_keep_size = -1    # measured in bytes; -1 disables
+#max_slot_wal_keep_size = -1    # in megabytes; -1 disables
 #wal_sender_timeout = 60s    # in milliseconds; 0 disables
 
 #max_replication_slots = 10    # max number of replication slots
-- 
2.18.2

On Wed, May 27, 2020 at 04:21:59PM +0900, Kyotaro Horiguchi wrote:
> I don't oppose to full-spelling.  How about the attached?

No problem from me.
--
Michael

On Wed, May 27, 2020 at 04:35:51PM +0900, Michael Paquier wrote:
> On Wed, May 27, 2020 at 04:21:59PM +0900, Kyotaro Horiguchi wrote:
> > I don't oppose to full-spelling.  How about the attached?
>
> No problem from me.

And applied this one as of 55ca50d.
--
Michael

At Thu, 28 May 2020 15:44:26 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Wed, May 27, 2020 at 04:35:51PM +0900, Michael Paquier wrote:
> > On Wed, May 27, 2020 at 04:21:59PM +0900, Kyotaro Horiguchi wrote:
> > > I don't oppose to full-spelling.  How about the attached?
> > 
> > No problem from me.
> 
> And applied this one as of 55ca50d.

Thanks!

-- 
Kyotaro Horiguchi
NTT Open Source Software Center