max_slot_wal_keep_size and wal_keep_segments

Started by Fujii Masaoover 5 years ago24 messages
#1Fujii Masao
masao.fujii@oss.nttdata.com

Hi,

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

There seem to be several options to do this.

(1) Rename max_slot_wal_keep_size to max_slot_wal_keep_segments,
and make users specify the number of WAL segments in it instead of
WAL size.

(2) Rename wal_keep_segments to wal_keep_size, and make users specify
the WAL size in it instead of the number of WAL segments.

(3) Don't rename the parameters, and allow users to specify not only
the number of WAL segments but also the WAL size in wal_keep_segments.

Since we have been moving away from measuring in segments, e.g.,
max_wal_size, I don't think (1) is good idea.

For backward compatibility, (3) is better. But which needs more
(maybe a bit complicated) code in guc.c. Also the parameter names
are not consistent yet (i.e., _segments and _size).

So for now I like (2).

Thought?

Regards,

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

#2Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Fujii Masao (#1)
Re: max_slot_wal_keep_size and wal_keep_segments

At Tue, 30 Jun 2020 23:51:40 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

Hi,

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments
different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

We are moving the units for amount of WAL from segments to MB. The
variable is affected by the movement. I'm not sure wal_keep_segments
is going to die soon but we may change it to wal_keep_size(_mb) sooner
or later if it going to stay alive.

There seem to be several options to do this.

(1) Rename max_slot_wal_keep_size to max_slot_wal_keep_segments,
and make users specify the number of WAL segments in it instead of
WAL size.

I don't think this is not the way.

(2) Rename wal_keep_segments to wal_keep_size, and make users specify
the WAL size in it instead of the number of WAL segments.

Yes. I agree to this (as I wrote above before reading this).

(3) Don't rename the parameters, and allow users to specify not only
the number of WAL segments but also the WAL size in wal_keep_segments.

Possible in a short term, but not for a long term.

Since we have been moving away from measuring in segments, e.g.,
max_wal_size, I don't think (1) is good idea.

For backward compatibility, (3) is better. But which needs more
(maybe a bit complicated) code in guc.c. Also the parameter names
are not consistent yet (i.e., _segments and _size).

So for now I like (2).

Thought?

I agree to you. If someone found that wal_keep_segment is no longer
usable, the alternative would easily be found by searching config file
for "wal_keep". Or we could have a default config line like this:

wal_keep_size = 0 # in megabytes: 0 disables (formerly wal_keep_segments)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#3Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Fujii Masao (#1)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020-Jun-30, Fujii Masao wrote:

Hi,

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

Do we still need wal_keep_segments for anything? Maybe we should
consider removing that functionality instead ... and even if we don't
remove it in 13, then what is the point of renaming it only to remove it
shortly after?

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

#4Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Alvaro Herrera (#3)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020/07/01 12:26, Alvaro Herrera wrote:

On 2020-Jun-30, Fujii Masao wrote:

Hi,

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

Do we still need wal_keep_segments for anything?

Yeah, personally I like wal_keep_segments because its setting is very
simple and no extra operations on replication slots are necessary.

Regards,

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

#5Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Fujii Masao (#4)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020-Jul-01, Fujii Masao wrote:

On 2020/07/01 12:26, Alvaro Herrera wrote:

On 2020-Jun-30, Fujii Masao wrote:

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

Do we still need wal_keep_segments for anything?

Yeah, personally I like wal_keep_segments because its setting is very
simple and no extra operations on replication slots are necessary.

Okay. In that case I +1 the idea of renaming to wal_keep_size.

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

#6David Steele
david@pgmasters.net
In reply to: Alvaro Herrera (#5)
Re: max_slot_wal_keep_size and wal_keep_segments

On 7/1/20 10:54 AM, Alvaro Herrera wrote:

On 2020-Jul-01, Fujii Masao wrote:

On 2020/07/01 12:26, Alvaro Herrera wrote:

On 2020-Jun-30, Fujii Masao wrote:

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

Do we still need wal_keep_segments for anything?

Yeah, personally I like wal_keep_segments because its setting is very
simple and no extra operations on replication slots are necessary.

Okay. In that case I +1 the idea of renaming to wal_keep_size.

+1 for renaming to wal_keep_size.

--
-David
david@pgmasters.net

#7Bruce Momjian
bruce@momjian.us
In reply to: David Steele (#6)
Re: max_slot_wal_keep_size and wal_keep_segments

On Wed, Jul 1, 2020 at 01:18:06PM -0400, David Steele wrote:

On 7/1/20 10:54 AM, Alvaro Herrera wrote:

On 2020-Jul-01, Fujii Masao wrote:

On 2020/07/01 12:26, Alvaro Herrera wrote:

On 2020-Jun-30, Fujii Masao wrote:

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

Do we still need wal_keep_segments for anything?

Yeah, personally I like wal_keep_segments because its setting is very
simple and no extra operations on replication slots are necessary.

Okay. In that case I +1 the idea of renaming to wal_keep_size.

+1 for renaming to wal_keep_size.

We have the following wal*size GUC settings:

SELECT name FROM pg_settings WHERE name LIKE '%wal%size%';
name
------------------------
max_slot_wal_keep_size
max_wal_size
min_wal_size
wal_block_size
wal_segment_size

Does wal_keep_size make sense here?

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

#8Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Bruce Momjian (#7)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020-Jul-01, Bruce Momjian wrote:

We have the following wal*size GUC settings:

SELECT name FROM pg_settings WHERE name LIKE '%wal%size%';
name
------------------------
max_slot_wal_keep_size
max_wal_size
min_wal_size
wal_block_size
wal_segment_size

Does wal_keep_size make sense here?

I think it does. What do you think?

Are you suggesting that "keep_wal_size" is better, since it's more in
line with min/max? I lean towards no.

(I think it's okay to conceptually separate these three options from
wal_block_size, since that's a compile time option and thus it's an
introspective GUC rather than actual configuration, but as I recall that
argument does not hold for wal_segment_size. But at one point, even that
one was an introspective GUC too.)

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

#9Bruce Momjian
bruce@momjian.us
In reply to: Alvaro Herrera (#8)
Re: max_slot_wal_keep_size and wal_keep_segments

On Wed, Jul 1, 2020 at 04:25:35PM -0400, Alvaro Herrera wrote:

On 2020-Jul-01, Bruce Momjian wrote:

We have the following wal*size GUC settings:

SELECT name FROM pg_settings WHERE name LIKE '%wal%size%';
name
------------------------
max_slot_wal_keep_size
max_wal_size
min_wal_size
wal_block_size
wal_segment_size

Does wal_keep_size make sense here?

I think it does. What do you think?

Are you suggesting that "keep_wal_size" is better, since it's more in
line with min/max? I lean towards no.

No, I am more just asking since I saw wal_keep_size as a special version
of wal_size. I don't have a firm opinion.

(I think it's okay to conceptually separate these three options from
wal_block_size, since that's a compile time option and thus it's an
introspective GUC rather than actual configuration, but as I recall that
argument does not hold for wal_segment_size. But at one point, even that
one was an introspective GUC too.)

Yep, just asking.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

#10Masahiko Sawada
masahiko.sawada@2ndquadrant.com
In reply to: David Steele (#6)
Re: max_slot_wal_keep_size and wal_keep_segments

On Thu, 2 Jul 2020 at 02:18, David Steele <david@pgmasters.net> wrote:

On 7/1/20 10:54 AM, Alvaro Herrera wrote:

On 2020-Jul-01, Fujii Masao wrote:

On 2020/07/01 12:26, Alvaro Herrera wrote:

On 2020-Jun-30, Fujii Masao wrote:

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

Do we still need wal_keep_segments for anything?

Yeah, personally I like wal_keep_segments because its setting is very
simple and no extra operations on replication slots are necessary.

Okay. In that case I +1 the idea of renaming to wal_keep_size.

+1 for renaming to wal_keep_size.

+1 from me, too.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#11Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: David Steele (#6)
1 attachment(s)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020/07/02 2:18, David Steele wrote:

On 7/1/20 10:54 AM, Alvaro Herrera wrote:

On 2020-Jul-01, Fujii Masao wrote:

On 2020/07/01 12:26, Alvaro Herrera wrote:

On 2020-Jun-30, Fujii Masao wrote:

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

Do we still need wal_keep_segments for anything?

Yeah, personally I like wal_keep_segments because its setting is very
simple and no extra operations on replication slots are necessary.

Okay.  In that case I +1 the idea of renaming to wal_keep_size.

+1 for renaming to wal_keep_size.

I attached the patch that renames wal_keep_segments to wal_keep_size.

Regards,

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

Attachments:

wal_keep_size_v1.patchtext/plain; charset=UTF-8; name=wal_keep_size_v1.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e9cdff4864..3d973a5c8e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -11249,7 +11249,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
          <para><literal>extended</literal> means
           that <varname>max_wal_size</varname> is exceeded but the files are
           still retained, either by the replication slot or
-          by <varname>wal_keep_segments</varname>.
+          by <varname>wal_keep_size</varname>.
          </para>
         </listitem>
         <listitem>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 02909b1e66..33c3ccb923 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3124,7 +3124,7 @@ include_dir 'conf.d'
         checkpoints. This is a soft limit; WAL size can exceed
         <varname>max_wal_size</varname> under special circumstances, such as
         heavy load, a failing <varname>archive_command</varname>, or a high
-        <varname>wal_keep_segments</varname> setting.
+        <varname>wal_keep_size</varname> setting.
         If this value is specified without units, it is taken as megabytes.
         The default is 1 GB.
         Increasing this parameter can increase the amount of time needed for
@@ -3751,21 +3751,21 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
        </listitem>
       </varlistentry>
 
-      <varlistentry id="guc-wal-keep-segments" xreflabel="wal_keep_segments">
-       <term><varname>wal_keep_segments</varname> (<type>integer</type>)
+      <varlistentry id="guc-wal-keep-size" xreflabel="wal_keep_size">
+       <term><varname>wal_keep_size</varname> (<type>integer</type>)
        <indexterm>
-        <primary><varname>wal_keep_segments</varname> configuration parameter</primary>
+        <primary><varname>wal_keep_size</varname> configuration parameter</primary>
        </indexterm>
        </term>
        <listitem>
        <para>
-        Specifies the minimum number of past log file segments kept in the
+        Specifies the minimum size of past log file segments kept in the
         <filename>pg_wal</filename>
         directory, in case a standby server needs to fetch them for streaming
-        replication. Each segment is normally 16 megabytes. If a standby
+        replication. If a standby
         server connected to the sending server falls behind by more than
-        <varname>wal_keep_segments</varname> segments, the sending server might remove
-        a WAL segment still needed by the standby, in which case the
+        <varname>wal_keep_size</varname> megabytes, the sending server might
+        remove a WAL segment still needed by the standby, in which case the
         replication connection will be terminated.  Downstream connections
         will also eventually fail as a result.  (However, the standby
         server can recover by fetching the segment from archive, if WAL
@@ -3773,14 +3773,15 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
        </para>
 
        <para>
-        This sets only the minimum number of segments retained in
+        This sets only the minimum size of segments retained in
         <filename>pg_wal</filename>; the system might need to retain more segments
         for WAL archival or to recover from a checkpoint. If
-        <varname>wal_keep_segments</varname> is zero (the default), the system
+        <varname>wal_keep_size</varname> is zero (the default), the system
         doesn't keep any extra segments for standby purposes, so the number
         of old WAL segments available to standby servers is a function of
         the location of the previous checkpoint and status of WAL
         archiving.
+        If this value is specified without units, it is taken as megabytes.
         This parameter can only be set in the
         <filename>postgresql.conf</filename> file or on the server command line.
        </para>
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 65c3fc62a9..2d4f9f03c7 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -785,7 +785,7 @@ archive_cleanup_command = 'pg_archivecleanup /path/to/archive %r'
     archiving, the server might recycle old WAL segments before the standby
     has received them.  If this occurs, the standby will need to be
     reinitialized from a new base backup.  You can avoid this by setting
-    <varname>wal_keep_segments</varname> to a value large enough to ensure that
+    <varname>wal_keep_size</varname> to a value large enough to ensure that
     WAL segments are not recycled too early, or by configuring a replication
     slot for the standby.  If you set up a WAL archive that's accessible from
     the standby, these solutions are not required, since the standby can
@@ -929,7 +929,7 @@ primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
    </para>
    <para>
     In lieu of using replication slots, it is possible to prevent the removal
-    of old WAL segments using <xref linkend="guc-wal-keep-segments"/>, or by
+    of old WAL segments using <xref linkend="guc-wal-keep-size"/>, or by
     storing the segments in an archive using
     <xref linkend="guc-archive-command"/>.
     However, these methods often result in retaining more WAL segments than
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index db480be674..91cfd92ab2 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -305,7 +305,7 @@ PostgreSQL documentation
            <para>
             The write-ahead log files are collected at the end of the backup.
             Therefore, it is necessary for the
-            <xref linkend="guc-wal-keep-segments"/> parameter to be set high
+            <xref linkend="guc-wal-keep-size"/> parameter to be set high
              enough that the log is not removed before the end of the backup.
              If the log has been rotated when it's time to transfer it, the
              backup will fail and be unusable.
diff --git a/doc/src/sgml/wal.sgml b/doc/src/sgml/wal.sgml
index bd9fae544c..34af9961b4 100644
--- a/doc/src/sgml/wal.sgml
+++ b/doc/src/sgml/wal.sgml
@@ -578,7 +578,8 @@
 
   <para>
    Independently of <varname>max_wal_size</varname>,
-   <xref linkend="guc-wal-keep-segments"/> + 1 most recent WAL files are
+   most recent <xref linkend="guc-wal-keep-size"/> megabytes
+   WAL files plus one WAL file are
    kept at all times. Also, if WAL archiving is used, old segments can not be
    removed or recycled until they are archived. If WAL archiving cannot keep up
    with the pace that WAL is generated, or if <varname>archive_command</varname>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 91d99c113c..f1dfeeb765 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -88,7 +88,7 @@ extern uint32 bootstrap_data_checksum_version;
 /* User-settable parameters */
 int			max_wal_size_mb = 1024; /* 1 GB */
 int			min_wal_size_mb = 80;	/* 80 MB */
-int			wal_keep_segments = 0;
+int			wal_keep_size_mb = 0;
 int			XLOGbuffers = -1;
 int			XLogArchiveTimeout = 0;
 int			XLogArchiveMode = ARCHIVE_MODE_OFF;
@@ -9525,7 +9525,7 @@ GetWALAvailability(XLogRecPtr targetLSN)
 
 	/*
 	 * calculate the oldest segment currently reserved by all slots,
-	 * considering wal_keep_segments and max_slot_wal_keep_size
+	 * considering wal_keep_size and max_slot_wal_keep_size
 	 */
 	XLByteToSeg(targetLSN, targetSeg, wal_segment_size);
 	KeepLogSeg(currpos, &oldestSlotSeg);
@@ -9571,9 +9571,9 @@ GetWALAvailability(XLogRecPtr targetLSN)
 
 /*
  * Retreat *logSegNo to the last segment that we need to retain because of
- * either wal_keep_segments or replication slots.
+ * either wal_keep_size or replication slots.
  *
- * This is calculated by subtracting wal_keep_segments from the given xlog
+ * This is calculated by subtracting wal_keep_size from the given xlog
  * location, recptr and by making sure that that result is below the
  * requirement of replication slots.  For the latter criterion we do consider
  * the effects of max_slot_wal_keep_size: reserve at most that much space back
@@ -9611,14 +9611,20 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
 		}
 	}
 
-	/* but, keep at least wal_keep_segments if that's set */
-	if (wal_keep_segments > 0 && currSegNo - segno < wal_keep_segments)
+	/* but, keep at least wal_keep_size if that's set */
+	if (wal_keep_size_mb > 0)
 	{
-		/* avoid underflow, don't go below 1 */
-		if (currSegNo <= wal_keep_segments)
-			segno = 1;
-		else
-			segno = currSegNo - wal_keep_segments;
+		uint64		keep_segs;
+
+		keep_segs = ConvertToXSegs(wal_keep_size_mb, wal_segment_size);
+		if (currSegNo - segno < keep_segs)
+		{
+			/* avoid underflow, don't go below 1 */
+			if (currSegNo <= keep_segs)
+				segno = 1;
+			else
+				segno = currSegNo - keep_segs;
+		}
 	}
 
 	/* don't delete WAL segments newer than the calculated segment */
@@ -11323,7 +11329,7 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 	 * If archiving is enabled, wait for all the required WAL files to be
 	 * archived before returning. If archiving isn't enabled, the required WAL
 	 * needs to be transported via streaming replication (hopefully with
-	 * wal_keep_segments set high enough), or some more exotic mechanism like
+	 * wal_keep_size set high enough), or some more exotic mechanism like
 	 * polling and copying files from pg_wal with script. We have no knowledge
 	 * of those mechanisms, so it's up to the user to ensure that he gets all
 	 * the required WAL.
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 9fe147bf44..03525eda35 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -413,19 +413,20 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 		else
 		{
 			XLogSegNo   targetSeg;
-			XLogSegNo   keepSegs;
+			uint64   slotKeepSegs;
+			uint64   keepSegs;
 			XLogSegNo   failSeg;
 			XLogRecPtr  failLSN;
 
 			XLByteToSeg(slot_contents.data.restart_lsn, targetSeg, wal_segment_size);
 
 			/* determine how many segments slots can be kept by slots ... */
-			keepSegs = XLogMBVarToSegs(max_slot_wal_keep_size_mb, wal_segment_size);
-			/* ... and override by wal_keep_segments as needed */
-			keepSegs = Max(keepSegs, wal_keep_segments);
+			slotKeepSegs = XLogMBVarToSegs(max_slot_wal_keep_size_mb, wal_segment_size);
+			/* ... and override by wal_keep_size as needed */
+			keepSegs = XLogMBVarToSegs(wal_keep_size_mb, wal_segment_size);
 
 			/* if currpos reaches failLSN, we lose our segment */
-			failSeg = targetSeg + keepSegs + 1;
+			failSeg = targetSeg + Max(slotKeepSegs, keepSegs) + 1;
 			XLogSegNoOffsetToRecPtr(failSeg, 0, wal_segment_size, failLSN);
 
 			values[i++] = Int64GetDatum(failLSN - currlsn);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 3a802d8627..498d0d991c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2631,12 +2631,13 @@ static struct config_int ConfigureNamesInt[] =
 	},
 
 	{
-		{"wal_keep_segments", PGC_SIGHUP, REPLICATION_SENDING,
-			gettext_noop("Sets the number of WAL files held for standby servers."),
-			NULL
+		{"wal_keep_size", PGC_SIGHUP, REPLICATION_SENDING,
+			gettext_noop("Sets the size of WAL files held for standby servers."),
+			NULL,
+			GUC_UNIT_MB
 		},
-		&wal_keep_segments,
-		0, 0, INT_MAX,
+		&wal_keep_size_mb,
+		0, 0, MAX_KILOBYTES,
 		NULL, NULL, NULL
 	},
 
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 0d98e546a6..3558ba3eff 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -288,7 +288,7 @@
 
 #max_wal_senders = 10		# max number of walsender processes
 				# (change requires restart)
-#wal_keep_segments = 0		# in logfile segments; 0 disables
+#wal_keep_size = 0		# in megabytes; 0 disables
 #max_slot_wal_keep_size = -1	# in megabytes; -1 disables
 #wal_sender_timeout = 60s	# in milliseconds; 0 disables
 
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 7dabf395e1..9b01f60cbb 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -135,11 +135,11 @@ sub setup_cluster
 		extra            => $extra,
 		auth_extra       => [ '--create-role', 'rewind_user' ]);
 
-	# Set wal_keep_segments to prevent WAL segment recycling after enforced
+	# Set wal_keep_size to prevent WAL segment recycling after enforced
 	# checkpoints in the tests.
 	$node_master->append_conf(
 		'postgresql.conf', qq(
-wal_keep_segments = 20
+wal_keep_size = 320MB
 ));
 	return;
 }
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 77ac4e785f..bb4c6d1d71 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -107,7 +107,7 @@ extern bool reachedConsistency;
 extern int	wal_segment_size;
 extern int	min_wal_size_mb;
 extern int	max_wal_size_mb;
-extern int	wal_keep_segments;
+extern int	wal_keep_size_mb;
 extern int	max_slot_wal_keep_size_mb;
 extern int	XLOGbuffers;
 extern int	XLogArchiveTimeout;
@@ -272,7 +272,7 @@ typedef enum WALAvailability
 	WALAVAIL_INVALID_LSN,		/* parameter error */
 	WALAVAIL_RESERVED,			/* WAL segment is within max_wal_size */
 	WALAVAIL_EXTENDED,			/* WAL segment is reserved by a slot or
-								 * wal_keep_segments */
+								 * wal_keep_size */
 	WALAVAIL_UNRESERVED,		/* no longer reserved, but not removed yet */
 	WALAVAIL_REMOVED			/* WAL segment has been removed */
 } WALAvailability;
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index af656c6902..5ddb3d6c3f 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -116,19 +116,19 @@ $start_lsn = $node_master->lsn('write');
 $node_master->wait_for_catchup($node_standby, 'replay', $start_lsn);
 $node_standby->stop;
 
-# wal_keep_segments overrides max_slot_wal_keep_size
+# wal_keep_size overrides max_slot_wal_keep_size
 $result = $node_master->safe_psql('postgres',
-	"ALTER SYSTEM SET wal_keep_segments to 8; SELECT pg_reload_conf();");
+	"ALTER SYSTEM SET wal_keep_size to '128MB'; SELECT pg_reload_conf();");
 # Advance WAL again then checkpoint, reducing remain by 6 MB.
 advance_wal($node_master, 6);
 $result = $node_master->safe_psql('postgres',
 	"SELECT wal_status as remain FROM pg_replication_slots WHERE slot_name = 'rep1'"
 );
 is($result, "extended",
-	'check that wal_keep_segments overrides max_slot_wal_keep_size');
-# restore wal_keep_segments
+	'check that wal_keep_size overrides max_slot_wal_keep_size');
+# restore wal_keep_size
 $result = $node_master->safe_psql('postgres',
-	"ALTER SYSTEM SET wal_keep_segments to 0; SELECT pg_reload_conf();");
+	"ALTER SYSTEM SET wal_keep_size to 0; SELECT pg_reload_conf();");
 
 # The standby can reconnect to master
 $node_standby->start;
#12Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Fujii Masao (#11)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020-Jul-09, Fujii Masao wrote:

I attached the patch that renames wal_keep_segments to wal_keep_size.

Looks good in a quick once-over. Just two small wording comments:

<para>
Independently of <varname>max_wal_size</varname>,
-   <xref linkend="guc-wal-keep-segments"/> + 1 most recent WAL files are
+   most recent <xref linkend="guc-wal-keep-size"/> megabytes
+   WAL files plus one WAL file are
kept at all times. Also, if WAL archiving is used, old segments can not be
removed or recycled until they are archived. If WAL archiving cannot keep up
with the pace that WAL is generated, or if <varname>archive_command</varname>

This reads a little strange to me. Maybe "the N most recent megabytes
plus ..."

/* determine how many segments slots can be kept by slots ... */
-			keepSegs = XLogMBVarToSegs(max_slot_wal_keep_size_mb, wal_segment_size);
-			/* ... and override by wal_keep_segments as needed */
-			keepSegs = Max(keepSegs, wal_keep_segments);
+			slotKeepSegs = XLogMBVarToSegs(max_slot_wal_keep_size_mb, wal_segment_size);
+			/* ... and override by wal_keep_size as needed */
+			keepSegs = XLogMBVarToSegs(wal_keep_size_mb, wal_segment_size);

Since you change the way these two variables are used, I would not say
"override" in the above comment, nor keep the ellipses; perhaps just
change them to "determine how many segments can be kept by slots" and
"ditto for wal_keep_size".

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

#13Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Fujii Masao (#11)
Re: max_slot_wal_keep_size and wal_keep_segments

At Thu, 9 Jul 2020 00:37:57 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

On 2020/07/02 2:18, David Steele wrote:

On 7/1/20 10:54 AM, Alvaro Herrera wrote:

On 2020-Jul-01, Fujii Masao wrote:

On 2020/07/01 12:26, Alvaro Herrera wrote:

On 2020-Jun-30, Fujii Masao wrote:

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments
different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

Do we still need wal_keep_segments for anything?

Yeah, personally I like wal_keep_segments because its setting is very
simple and no extra operations on replication slots are necessary.

Okay.  In that case I +1 the idea of renaming to wal_keep_size.

+1 for renaming to wal_keep_size.

I attached the patch that renames wal_keep_segments to wal_keep_size.

It fails on 019_replslot_limit.pl for uncertain reason to me..

@@ -11323,7 +11329,7 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 	 * If archiving is enabled, wait for all the required WAL files to be
 	 * archived before returning. If archiving isn't enabled, the required WAL
 	 * needs to be transported via streaming replication (hopefully with
-	 * wal_keep_segments set high enough), or some more exotic mechanism like
+	 * wal_keep_size set high enough), or some more exotic mechanism like
 	 * polling and copying files from pg_wal with script. We have no knowledge

Isn't this time a good chance to mention replication slots?

-	"ALTER SYSTEM SET wal_keep_segments to 8; SELECT pg_reload_conf();");
+	"ALTER SYSTEM SET wal_keep_size to '128MB'; SELECT pg_reload_conf();");

wal_segment_size to 1MB here so, that conversion is not correct.
(However, that test works as long as it is more than
max_slot_wal_keep_size so it's practically no problem.)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#14Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Alvaro Herrera (#12)
1 attachment(s)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020/07/09 1:20, Alvaro Herrera wrote:

On 2020-Jul-09, Fujii Masao wrote:

I attached the patch that renames wal_keep_segments to wal_keep_size.

Looks good in a quick once-over. Just two small wording comments:

Thanks for review comments!

<para>
Independently of <varname>max_wal_size</varname>,
-   <xref linkend="guc-wal-keep-segments"/> + 1 most recent WAL files are
+   most recent <xref linkend="guc-wal-keep-size"/> megabytes
+   WAL files plus one WAL file are
kept at all times. Also, if WAL archiving is used, old segments can not be
removed or recycled until they are archived. If WAL archiving cannot keep up
with the pace that WAL is generated, or if <varname>archive_command</varname>

This reads a little strange to me. Maybe "the N most recent megabytes
plus ..."

Yes, fixed.

/* determine how many segments slots can be kept by slots ... */
-			keepSegs = XLogMBVarToSegs(max_slot_wal_keep_size_mb, wal_segment_size);
-			/* ... and override by wal_keep_segments as needed */
-			keepSegs = Max(keepSegs, wal_keep_segments);
+			slotKeepSegs = XLogMBVarToSegs(max_slot_wal_keep_size_mb, wal_segment_size);
+			/* ... and override by wal_keep_size as needed */
+			keepSegs = XLogMBVarToSegs(wal_keep_size_mb, wal_segment_size);

Since you change the way these two variables are used, I would not say
"override" in the above comment, nor keep the ellipses; perhaps just
change them to "determine how many segments can be kept by slots" and
"ditto for wal_keep_size".

Yes, fixed.

Attached is the updated version of the patch.

Regards,

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

Attachments:

wal_keep_size_v2.patchtext/plain; charset=UTF-8; name=wal_keep_size_v2.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e9cdff4864..3d973a5c8e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -11249,7 +11249,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
          <para><literal>extended</literal> means
           that <varname>max_wal_size</varname> is exceeded but the files are
           still retained, either by the replication slot or
-          by <varname>wal_keep_segments</varname>.
+          by <varname>wal_keep_size</varname>.
          </para>
         </listitem>
         <listitem>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b353c61683..3384a5197d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3124,7 +3124,7 @@ include_dir 'conf.d'
         checkpoints. This is a soft limit; WAL size can exceed
         <varname>max_wal_size</varname> under special circumstances, such as
         heavy load, a failing <varname>archive_command</varname>, or a high
-        <varname>wal_keep_segments</varname> setting.
+        <varname>wal_keep_size</varname> setting.
         If this value is specified without units, it is taken as megabytes.
         The default is 1 GB.
         Increasing this parameter can increase the amount of time needed for
@@ -3751,21 +3751,21 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
        </listitem>
       </varlistentry>
 
-      <varlistentry id="guc-wal-keep-segments" xreflabel="wal_keep_segments">
-       <term><varname>wal_keep_segments</varname> (<type>integer</type>)
+      <varlistentry id="guc-wal-keep-size" xreflabel="wal_keep_size">
+       <term><varname>wal_keep_size</varname> (<type>integer</type>)
        <indexterm>
-        <primary><varname>wal_keep_segments</varname> configuration parameter</primary>
+        <primary><varname>wal_keep_size</varname> configuration parameter</primary>
        </indexterm>
        </term>
        <listitem>
        <para>
-        Specifies the minimum number of past log file segments kept in the
+        Specifies the minimum size of past log file segments kept in the
         <filename>pg_wal</filename>
         directory, in case a standby server needs to fetch them for streaming
-        replication. Each segment is normally 16 megabytes. If a standby
+        replication. If a standby
         server connected to the sending server falls behind by more than
-        <varname>wal_keep_segments</varname> segments, the sending server might remove
-        a WAL segment still needed by the standby, in which case the
+        <varname>wal_keep_size</varname> megabytes, the sending server might
+        remove a WAL segment still needed by the standby, in which case the
         replication connection will be terminated.  Downstream connections
         will also eventually fail as a result.  (However, the standby
         server can recover by fetching the segment from archive, if WAL
@@ -3773,14 +3773,15 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
        </para>
 
        <para>
-        This sets only the minimum number of segments retained in
+        This sets only the minimum size of segments retained in
         <filename>pg_wal</filename>; the system might need to retain more segments
         for WAL archival or to recover from a checkpoint. If
-        <varname>wal_keep_segments</varname> is zero (the default), the system
+        <varname>wal_keep_size</varname> is zero (the default), the system
         doesn't keep any extra segments for standby purposes, so the number
         of old WAL segments available to standby servers is a function of
         the location of the previous checkpoint and status of WAL
         archiving.
+        If this value is specified without units, it is taken as megabytes.
         This parameter can only be set in the
         <filename>postgresql.conf</filename> file or on the server command line.
        </para>
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 6a9184f314..89f6d6eda6 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -785,7 +785,7 @@ archive_cleanup_command = 'pg_archivecleanup /path/to/archive %r'
     archiving, the server might recycle old WAL segments before the standby
     has received them.  If this occurs, the standby will need to be
     reinitialized from a new base backup.  You can avoid this by setting
-    <varname>wal_keep_segments</varname> to a value large enough to ensure that
+    <varname>wal_keep_size</varname> to a value large enough to ensure that
     WAL segments are not recycled too early, or by configuring a replication
     slot for the standby.  If you set up a WAL archive that's accessible from
     the standby, these solutions are not required, since the standby can
@@ -929,7 +929,7 @@ primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
    </para>
    <para>
     In lieu of using replication slots, it is possible to prevent the removal
-    of old WAL segments using <xref linkend="guc-wal-keep-segments"/>, or by
+    of old WAL segments using <xref linkend="guc-wal-keep-size"/>, or by
     storing the segments in an archive using
     <xref linkend="guc-archive-command"/>.
     However, these methods often result in retaining more WAL segments than
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index aa41fb444f..e246efbdb5 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -305,7 +305,7 @@ PostgreSQL documentation
            <para>
             The write-ahead log files are collected at the end of the backup.
             Therefore, it is necessary for the
-            <xref linkend="guc-wal-keep-segments"/> parameter to be set high
+            <xref linkend="guc-wal-keep-size"/> parameter to be set high
              enough that the log is not removed before the end of the backup.
              If the log has been rotated when it's time to transfer it, the
              backup will fail and be unusable.
diff --git a/doc/src/sgml/wal.sgml b/doc/src/sgml/wal.sgml
index 1902f36291..ff3625f855 100644
--- a/doc/src/sgml/wal.sgml
+++ b/doc/src/sgml/wal.sgml
@@ -578,7 +578,8 @@
 
   <para>
    Independently of <varname>max_wal_size</varname>,
-   <xref linkend="guc-wal-keep-segments"/> + 1 most recent WAL files are
+   the <xref linkend="guc-wal-keep-size"/> most recent megabytes
+   WAL files plus one WAL file are
    kept at all times. Also, if WAL archiving is used, old segments can not be
    removed or recycled until they are archived. If WAL archiving cannot keep up
    with the pace that WAL is generated, or if <varname>archive_command</varname>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 28daf72a50..c7a59b6116 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -88,7 +88,7 @@ extern uint32 bootstrap_data_checksum_version;
 /* User-settable parameters */
 int			max_wal_size_mb = 1024; /* 1 GB */
 int			min_wal_size_mb = 80;	/* 80 MB */
-int			wal_keep_segments = 0;
+int			wal_keep_size_mb = 0;
 int			XLOGbuffers = -1;
 int			XLogArchiveTimeout = 0;
 int			XLogArchiveMode = ARCHIVE_MODE_OFF;
@@ -9527,7 +9527,7 @@ GetWALAvailability(XLogRecPtr targetLSN)
 
 	/*
 	 * calculate the oldest segment currently reserved by all slots,
-	 * considering wal_keep_segments and max_slot_wal_keep_size
+	 * considering wal_keep_size and max_slot_wal_keep_size
 	 */
 	XLByteToSeg(targetLSN, targetSeg, wal_segment_size);
 	KeepLogSeg(currpos, &oldestSlotSeg);
@@ -9573,9 +9573,9 @@ GetWALAvailability(XLogRecPtr targetLSN)
 
 /*
  * Retreat *logSegNo to the last segment that we need to retain because of
- * either wal_keep_segments or replication slots.
+ * either wal_keep_size or replication slots.
  *
- * This is calculated by subtracting wal_keep_segments from the given xlog
+ * This is calculated by subtracting wal_keep_size from the given xlog
  * location, recptr and by making sure that that result is below the
  * requirement of replication slots.  For the latter criterion we do consider
  * the effects of max_slot_wal_keep_size: reserve at most that much space back
@@ -9613,14 +9613,20 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
 		}
 	}
 
-	/* but, keep at least wal_keep_segments if that's set */
-	if (wal_keep_segments > 0 && currSegNo - segno < wal_keep_segments)
+	/* but, keep at least wal_keep_size if that's set */
+	if (wal_keep_size_mb > 0)
 	{
-		/* avoid underflow, don't go below 1 */
-		if (currSegNo <= wal_keep_segments)
-			segno = 1;
-		else
-			segno = currSegNo - wal_keep_segments;
+		uint64		keep_segs;
+
+		keep_segs = ConvertToXSegs(wal_keep_size_mb, wal_segment_size);
+		if (currSegNo - segno < keep_segs)
+		{
+			/* avoid underflow, don't go below 1 */
+			if (currSegNo <= keep_segs)
+				segno = 1;
+			else
+				segno = currSegNo - keep_segs;
+		}
 	}
 
 	/* don't delete WAL segments newer than the calculated segment */
@@ -11325,7 +11331,7 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 	 * If archiving is enabled, wait for all the required WAL files to be
 	 * archived before returning. If archiving isn't enabled, the required WAL
 	 * needs to be transported via streaming replication (hopefully with
-	 * wal_keep_segments set high enough), or some more exotic mechanism like
+	 * wal_keep_size set high enough), or some more exotic mechanism like
 	 * polling and copying files from pg_wal with script. We have no knowledge
 	 * of those mechanisms, so it's up to the user to ensure that he gets all
 	 * the required WAL.
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 9fe147bf44..f88694672f 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -413,19 +413,20 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 		else
 		{
 			XLogSegNo   targetSeg;
-			XLogSegNo   keepSegs;
+			uint64   slotKeepSegs;
+			uint64   keepSegs;
 			XLogSegNo   failSeg;
 			XLogRecPtr  failLSN;
 
 			XLByteToSeg(slot_contents.data.restart_lsn, targetSeg, wal_segment_size);
 
-			/* determine how many segments slots can be kept by slots ... */
-			keepSegs = XLogMBVarToSegs(max_slot_wal_keep_size_mb, wal_segment_size);
-			/* ... and override by wal_keep_segments as needed */
-			keepSegs = Max(keepSegs, wal_keep_segments);
+			/* determine how many segments slots can be kept by slots */
+			slotKeepSegs = XLogMBVarToSegs(max_slot_wal_keep_size_mb, wal_segment_size);
+			/* ditto for wal_keep_size */
+			keepSegs = XLogMBVarToSegs(wal_keep_size_mb, wal_segment_size);
 
 			/* if currpos reaches failLSN, we lose our segment */
-			failSeg = targetSeg + keepSegs + 1;
+			failSeg = targetSeg + Max(slotKeepSegs, keepSegs) + 1;
 			XLogSegNoOffsetToRecPtr(failSeg, 0, wal_segment_size, failLSN);
 
 			values[i++] = Int64GetDatum(failLSN - currlsn);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 031ca0327f..8b71d16f92 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2631,12 +2631,13 @@ static struct config_int ConfigureNamesInt[] =
 	},
 
 	{
-		{"wal_keep_segments", PGC_SIGHUP, REPLICATION_SENDING,
-			gettext_noop("Sets the number of WAL files held for standby servers."),
-			NULL
+		{"wal_keep_size", PGC_SIGHUP, REPLICATION_SENDING,
+			gettext_noop("Sets the size of WAL files held for standby servers."),
+			NULL,
+			GUC_UNIT_MB
 		},
-		&wal_keep_segments,
-		0, 0, INT_MAX,
+		&wal_keep_size_mb,
+		0, 0, MAX_KILOBYTES,
 		NULL, NULL, NULL
 	},
 
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index e430e33c7b..90beb87061 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -288,7 +288,7 @@
 
 #max_wal_senders = 10		# max number of walsender processes
 				# (change requires restart)
-#wal_keep_segments = 0		# in logfile segments; 0 disables
+#wal_keep_size = 0		# in megabytes; 0 disables
 #max_slot_wal_keep_size = -1	# in megabytes; -1 disables
 #wal_sender_timeout = 60s	# in milliseconds; 0 disables
 
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 7516af7a01..41ed7d4b3b 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -135,11 +135,11 @@ sub setup_cluster
 		extra            => $extra,
 		auth_extra       => [ '--create-role', 'rewind_user' ]);
 
-	# Set wal_keep_segments to prevent WAL segment recycling after enforced
+	# Set wal_keep_size to prevent WAL segment recycling after enforced
 	# checkpoints in the tests.
 	$node_primary->append_conf(
 		'postgresql.conf', qq(
-wal_keep_segments = 20
+wal_keep_size = 320MB
 ));
 	return;
 }
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 5b14334887..456f0d1321 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -107,7 +107,7 @@ extern bool reachedConsistency;
 extern int	wal_segment_size;
 extern int	min_wal_size_mb;
 extern int	max_wal_size_mb;
-extern int	wal_keep_segments;
+extern int	wal_keep_size_mb;
 extern int	max_slot_wal_keep_size_mb;
 extern int	XLOGbuffers;
 extern int	XLogArchiveTimeout;
@@ -272,7 +272,7 @@ typedef enum WALAvailability
 	WALAVAIL_INVALID_LSN,		/* parameter error */
 	WALAVAIL_RESERVED,			/* WAL segment is within max_wal_size */
 	WALAVAIL_EXTENDED,			/* WAL segment is reserved by a slot or
-								 * wal_keep_segments */
+								 * wal_keep_size */
 	WALAVAIL_UNRESERVED,		/* no longer reserved, but not removed yet */
 	WALAVAIL_REMOVED			/* WAL segment has been removed */
 } WALAvailability;
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index 1fced12fca..20f4a1bbc3 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -116,19 +116,19 @@ $start_lsn = $node_primary->lsn('write');
 $node_primary->wait_for_catchup($node_standby, 'replay', $start_lsn);
 $node_standby->stop;
 
-# wal_keep_segments overrides max_slot_wal_keep_size
+# wal_keep_size overrides max_slot_wal_keep_size
 $result = $node_primary->safe_psql('postgres',
-	"ALTER SYSTEM SET wal_keep_segments to 8; SELECT pg_reload_conf();");
+	"ALTER SYSTEM SET wal_keep_size to '8MB'; SELECT pg_reload_conf();");
 # Advance WAL again then checkpoint, reducing remain by 6 MB.
 advance_wal($node_primary, 6);
 $result = $node_primary->safe_psql('postgres',
 	"SELECT wal_status as remain FROM pg_replication_slots WHERE slot_name = 'rep1'"
 );
 is($result, "extended",
-	'check that wal_keep_segments overrides max_slot_wal_keep_size');
-# restore wal_keep_segments
+	'check that wal_keep_size overrides max_slot_wal_keep_size');
+# restore wal_keep_size
 $result = $node_primary->safe_psql('postgres',
-	"ALTER SYSTEM SET wal_keep_segments to 0; SELECT pg_reload_conf();");
+	"ALTER SYSTEM SET wal_keep_size to 0; SELECT pg_reload_conf();");
 
 # The standby can reconnect to primary
 $node_standby->start;
#15Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Kyotaro Horiguchi (#13)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020/07/09 13:47, Kyotaro Horiguchi wrote:

At Thu, 9 Jul 2020 00:37:57 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

On 2020/07/02 2:18, David Steele wrote:

On 7/1/20 10:54 AM, Alvaro Herrera wrote:

On 2020-Jul-01, Fujii Masao wrote:

On 2020/07/01 12:26, Alvaro Herrera wrote:

On 2020-Jun-30, Fujii Masao wrote:

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments
different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

Do we still need wal_keep_segments for anything?

Yeah, personally I like wal_keep_segments because its setting is very
simple and no extra operations on replication slots are necessary.

Okay.  In that case I +1 the idea of renaming to wal_keep_size.

+1 for renaming to wal_keep_size.

I attached the patch that renames wal_keep_segments to wal_keep_size.

It fails on 019_replslot_limit.pl for uncertain reason to me..

I could not reproduce this...

@@ -11323,7 +11329,7 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
* If archiving is enabled, wait for all the required WAL files to be
* archived before returning. If archiving isn't enabled, the required WAL
* needs to be transported via streaming replication (hopefully with
-	 * wal_keep_segments set high enough), or some more exotic mechanism like
+	 * wal_keep_size set high enough), or some more exotic mechanism like
* polling and copying files from pg_wal with script. We have no knowledge

Isn't this time a good chance to mention replication slots?

+1 to do that. But I found there are other places where replication slots
need to be mentioned. So I think it's better to do this as separate patch.

-	"ALTER SYSTEM SET wal_keep_segments to 8; SELECT pg_reload_conf();");
+	"ALTER SYSTEM SET wal_keep_size to '128MB'; SELECT pg_reload_conf();");

wal_segment_size to 1MB here so, that conversion is not correct.
(However, that test works as long as it is more than
max_slot_wal_keep_size so it's practically no problem.)

So I changed 128MB to 8MB. Is this OK?
I attached the updated version of the patch upthread.

Regards,

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

#16Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Fujii Masao (#15)
Re: max_slot_wal_keep_size and wal_keep_segments

At Mon, 13 Jul 2020 14:14:30 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

On 2020/07/09 13:47, Kyotaro Horiguchi wrote:

At Thu, 9 Jul 2020 00:37:57 +0900, Fujii Masao
<masao.fujii@oss.nttdata.com> wrote in

On 2020/07/02 2:18, David Steele wrote:

On 7/1/20 10:54 AM, Alvaro Herrera wrote:

On 2020-Jul-01, Fujii Masao wrote:

On 2020/07/01 12:26, Alvaro Herrera wrote:

On 2020-Jun-30, Fujii Masao wrote:

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments
different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

Do we still need wal_keep_segments for anything?

Yeah, personally I like wal_keep_segments because its setting is very
simple and no extra operations on replication slots are necessary.

Okay.  In that case I +1 the idea of renaming to wal_keep_size.

+1 for renaming to wal_keep_size.

I attached the patch that renames wal_keep_segments to wal_keep_size.

It fails on 019_replslot_limit.pl for uncertain reason to me..

I could not reproduce this...

Sorry for the ambiguity. The patch didn't applied on the file, and I
noticed that the reason is the wording change from master to
primary. So no problem in the latest patch.

@@ -11323,7 +11329,7 @@ do_pg_stop_backup(char *labelfile, bool
waitforarchive, TimeLineID *stoptli_p)
* If archiving is enabled, wait for all the required WAL files to be
* archived before returning. If archiving isn't enabled, the required
* WAL
* needs to be transported via streaming replication (hopefully with
-	 * wal_keep_segments set high enough), or some more exotic mechanism like
+ * wal_keep_size set high enough), or some more exotic mechanism like
* polling and copying files from pg_wal with script. We have no
* knowledge
Isn't this time a good chance to mention replication slots?

+1 to do that. But I found there are other places where replication
slots
need to be mentioned. So I think it's better to do this as separate
patch.

Agreed.

-	"ALTER SYSTEM SET wal_keep_segments to 8; SELECT pg_reload_conf();");
+ "ALTER SYSTEM SET wal_keep_size to '128MB'; SELECT
pg_reload_conf();");
wal_segment_size to 1MB here so, that conversion is not correct.
(However, that test works as long as it is more than
max_slot_wal_keep_size so it's practically no problem.)

So I changed 128MB to 8MB. Is this OK?
I attached the updated version of the patch upthread.

That change looks find by me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#17Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Kyotaro Horiguchi (#16)
1 attachment(s)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020/07/13 16:01, Kyotaro Horiguchi wrote:

At Mon, 13 Jul 2020 14:14:30 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

On 2020/07/09 13:47, Kyotaro Horiguchi wrote:

At Thu, 9 Jul 2020 00:37:57 +0900, Fujii Masao
<masao.fujii@oss.nttdata.com> wrote in

On 2020/07/02 2:18, David Steele wrote:

On 7/1/20 10:54 AM, Alvaro Herrera wrote:

On 2020-Jul-01, Fujii Masao wrote:

On 2020/07/01 12:26, Alvaro Herrera wrote:

On 2020-Jun-30, Fujii Masao wrote:

When I talked about max_slot_wal_keep_size as new feature in v13
at the conference, I received the question like "Why are the units of
setting values in max_slot_wal_keep_size and wal_keep_segments
different?"
from audience. That difference looks confusing for users and
IMO it's better to use the same unit for them. Thought?

Do we still need wal_keep_segments for anything?

Yeah, personally I like wal_keep_segments because its setting is very
simple and no extra operations on replication slots are necessary.

Okay.  In that case I +1 the idea of renaming to wal_keep_size.

+1 for renaming to wal_keep_size.

I attached the patch that renames wal_keep_segments to wal_keep_size.

It fails on 019_replslot_limit.pl for uncertain reason to me..

I could not reproduce this...

Sorry for the ambiguity. The patch didn't applied on the file, and I
noticed that the reason is the wording change from master to
primary. So no problem in the latest patch.

@@ -11323,7 +11329,7 @@ do_pg_stop_backup(char *labelfile, bool
waitforarchive, TimeLineID *stoptli_p)
* If archiving is enabled, wait for all the required WAL files to be
* archived before returning. If archiving isn't enabled, the required
* WAL
* needs to be transported via streaming replication (hopefully with
-	 * wal_keep_segments set high enough), or some more exotic mechanism like
+ * wal_keep_size set high enough), or some more exotic mechanism like
* polling and copying files from pg_wal with script. We have no
* knowledge
Isn't this time a good chance to mention replication slots?

+1 to do that. But I found there are other places where replication
slots
need to be mentioned. So I think it's better to do this as separate
patch.

Agreed.

-	"ALTER SYSTEM SET wal_keep_segments to 8; SELECT pg_reload_conf();");
+ "ALTER SYSTEM SET wal_keep_size to '128MB'; SELECT
pg_reload_conf();");
wal_segment_size to 1MB here so, that conversion is not correct.
(However, that test works as long as it is more than
max_slot_wal_keep_size so it's practically no problem.)

So I changed 128MB to 8MB. Is this OK?
I attached the updated version of the patch upthread.

That change looks find by me.

Thanks for the review!

The patch was no longer applied cleanly because of recent commit.
So I updated the patch. Attached.

Barring any objection, I will commit this patch.

Regards,

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

Attachments:

wal_keep_size_v3.patchtext/plain; charset=UTF-8; name=wal_keep_size_v3.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e9cdff4864..3d973a5c8e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -11249,7 +11249,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
          <para><literal>extended</literal> means
           that <varname>max_wal_size</varname> is exceeded but the files are
           still retained, either by the replication slot or
-          by <varname>wal_keep_segments</varname>.
+          by <varname>wal_keep_size</varname>.
          </para>
         </listitem>
         <listitem>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b353c61683..3384a5197d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3124,7 +3124,7 @@ include_dir 'conf.d'
         checkpoints. This is a soft limit; WAL size can exceed
         <varname>max_wal_size</varname> under special circumstances, such as
         heavy load, a failing <varname>archive_command</varname>, or a high
-        <varname>wal_keep_segments</varname> setting.
+        <varname>wal_keep_size</varname> setting.
         If this value is specified without units, it is taken as megabytes.
         The default is 1 GB.
         Increasing this parameter can increase the amount of time needed for
@@ -3751,21 +3751,21 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
        </listitem>
       </varlistentry>
 
-      <varlistentry id="guc-wal-keep-segments" xreflabel="wal_keep_segments">
-       <term><varname>wal_keep_segments</varname> (<type>integer</type>)
+      <varlistentry id="guc-wal-keep-size" xreflabel="wal_keep_size">
+       <term><varname>wal_keep_size</varname> (<type>integer</type>)
        <indexterm>
-        <primary><varname>wal_keep_segments</varname> configuration parameter</primary>
+        <primary><varname>wal_keep_size</varname> configuration parameter</primary>
        </indexterm>
        </term>
        <listitem>
        <para>
-        Specifies the minimum number of past log file segments kept in the
+        Specifies the minimum size of past log file segments kept in the
         <filename>pg_wal</filename>
         directory, in case a standby server needs to fetch them for streaming
-        replication. Each segment is normally 16 megabytes. If a standby
+        replication. If a standby
         server connected to the sending server falls behind by more than
-        <varname>wal_keep_segments</varname> segments, the sending server might remove
-        a WAL segment still needed by the standby, in which case the
+        <varname>wal_keep_size</varname> megabytes, the sending server might
+        remove a WAL segment still needed by the standby, in which case the
         replication connection will be terminated.  Downstream connections
         will also eventually fail as a result.  (However, the standby
         server can recover by fetching the segment from archive, if WAL
@@ -3773,14 +3773,15 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
        </para>
 
        <para>
-        This sets only the minimum number of segments retained in
+        This sets only the minimum size of segments retained in
         <filename>pg_wal</filename>; the system might need to retain more segments
         for WAL archival or to recover from a checkpoint. If
-        <varname>wal_keep_segments</varname> is zero (the default), the system
+        <varname>wal_keep_size</varname> is zero (the default), the system
         doesn't keep any extra segments for standby purposes, so the number
         of old WAL segments available to standby servers is a function of
         the location of the previous checkpoint and status of WAL
         archiving.
+        If this value is specified without units, it is taken as megabytes.
         This parameter can only be set in the
         <filename>postgresql.conf</filename> file or on the server command line.
        </para>
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 6a9184f314..89f6d6eda6 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -785,7 +785,7 @@ archive_cleanup_command = 'pg_archivecleanup /path/to/archive %r'
     archiving, the server might recycle old WAL segments before the standby
     has received them.  If this occurs, the standby will need to be
     reinitialized from a new base backup.  You can avoid this by setting
-    <varname>wal_keep_segments</varname> to a value large enough to ensure that
+    <varname>wal_keep_size</varname> to a value large enough to ensure that
     WAL segments are not recycled too early, or by configuring a replication
     slot for the standby.  If you set up a WAL archive that's accessible from
     the standby, these solutions are not required, since the standby can
@@ -929,7 +929,7 @@ primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
    </para>
    <para>
     In lieu of using replication slots, it is possible to prevent the removal
-    of old WAL segments using <xref linkend="guc-wal-keep-segments"/>, or by
+    of old WAL segments using <xref linkend="guc-wal-keep-size"/>, or by
     storing the segments in an archive using
     <xref linkend="guc-archive-command"/>.
     However, these methods often result in retaining more WAL segments than
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index aa41fb444f..e246efbdb5 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -305,7 +305,7 @@ PostgreSQL documentation
            <para>
             The write-ahead log files are collected at the end of the backup.
             Therefore, it is necessary for the
-            <xref linkend="guc-wal-keep-segments"/> parameter to be set high
+            <xref linkend="guc-wal-keep-size"/> parameter to be set high
              enough that the log is not removed before the end of the backup.
              If the log has been rotated when it's time to transfer it, the
              backup will fail and be unusable.
diff --git a/doc/src/sgml/wal.sgml b/doc/src/sgml/wal.sgml
index 1902f36291..ff3625f855 100644
--- a/doc/src/sgml/wal.sgml
+++ b/doc/src/sgml/wal.sgml
@@ -578,7 +578,8 @@
 
   <para>
    Independently of <varname>max_wal_size</varname>,
-   <xref linkend="guc-wal-keep-segments"/> + 1 most recent WAL files are
+   the <xref linkend="guc-wal-keep-size"/> most recent megabytes
+   WAL files plus one WAL file are
    kept at all times. Also, if WAL archiving is used, old segments can not be
    removed or recycled until they are archived. If WAL archiving cannot keep up
    with the pace that WAL is generated, or if <varname>archive_command</varname>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 0a97b1d37f..184c6672f3 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -88,7 +88,7 @@ extern uint32 bootstrap_data_checksum_version;
 /* User-settable parameters */
 int			max_wal_size_mb = 1024; /* 1 GB */
 int			min_wal_size_mb = 80;	/* 80 MB */
-int			wal_keep_segments = 0;
+int			wal_keep_size_mb = 0;
 int			XLOGbuffers = -1;
 int			XLogArchiveTimeout = 0;
 int			XLogArchiveMode = ARCHIVE_MODE_OFF;
@@ -9525,7 +9525,7 @@ GetWALAvailability(XLogRecPtr targetLSN)
 
 	/*
 	 * Calculate the oldest segment currently reserved by all slots,
-	 * considering wal_keep_segments and max_slot_wal_keep_size.  Initialize
+	 * considering wal_keep_size and max_slot_wal_keep_size.  Initialize
 	 * oldestSlotSeg to the current segment.
 	 */
 	currpos = GetXLogWriteRecPtr();
@@ -9576,9 +9576,9 @@ GetWALAvailability(XLogRecPtr targetLSN)
 
 /*
  * Retreat *logSegNo to the last segment that we need to retain because of
- * either wal_keep_segments or replication slots.
+ * either wal_keep_size or replication slots.
  *
- * This is calculated by subtracting wal_keep_segments from the given xlog
+ * This is calculated by subtracting wal_keep_size from the given xlog
  * location, recptr and by making sure that that result is below the
  * requirement of replication slots.  For the latter criterion we do consider
  * the effects of max_slot_wal_keep_size: reserve at most that much space back
@@ -9616,14 +9616,20 @@ KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
 		}
 	}
 
-	/* but, keep at least wal_keep_segments if that's set */
-	if (wal_keep_segments > 0 && currSegNo - segno < wal_keep_segments)
+	/* but, keep at least wal_keep_size if that's set */
+	if (wal_keep_size_mb > 0)
 	{
-		/* avoid underflow, don't go below 1 */
-		if (currSegNo <= wal_keep_segments)
-			segno = 1;
-		else
-			segno = currSegNo - wal_keep_segments;
+		uint64		keep_segs;
+
+		keep_segs = ConvertToXSegs(wal_keep_size_mb, wal_segment_size);
+		if (currSegNo - segno < keep_segs)
+		{
+			/* avoid underflow, don't go below 1 */
+			if (currSegNo <= keep_segs)
+				segno = 1;
+			else
+				segno = currSegNo - keep_segs;
+		}
 	}
 
 	/* don't delete WAL segments newer than the calculated segment */
@@ -11328,7 +11334,7 @@ do_pg_stop_backup(char *labelfile, bool waitforarchive, TimeLineID *stoptli_p)
 	 * If archiving is enabled, wait for all the required WAL files to be
 	 * archived before returning. If archiving isn't enabled, the required WAL
 	 * needs to be transported via streaming replication (hopefully with
-	 * wal_keep_segments set high enough), or some more exotic mechanism like
+	 * wal_keep_size set high enough), or some more exotic mechanism like
 	 * polling and copying files from pg_wal with script. We have no knowledge
 	 * of those mechanisms, so it's up to the user to ensure that he gets all
 	 * the required WAL.
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 9fe147bf44..f88694672f 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -413,19 +413,20 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
 		else
 		{
 			XLogSegNo   targetSeg;
-			XLogSegNo   keepSegs;
+			uint64   slotKeepSegs;
+			uint64   keepSegs;
 			XLogSegNo   failSeg;
 			XLogRecPtr  failLSN;
 
 			XLByteToSeg(slot_contents.data.restart_lsn, targetSeg, wal_segment_size);
 
-			/* determine how many segments slots can be kept by slots ... */
-			keepSegs = XLogMBVarToSegs(max_slot_wal_keep_size_mb, wal_segment_size);
-			/* ... and override by wal_keep_segments as needed */
-			keepSegs = Max(keepSegs, wal_keep_segments);
+			/* determine how many segments slots can be kept by slots */
+			slotKeepSegs = XLogMBVarToSegs(max_slot_wal_keep_size_mb, wal_segment_size);
+			/* ditto for wal_keep_size */
+			keepSegs = XLogMBVarToSegs(wal_keep_size_mb, wal_segment_size);
 
 			/* if currpos reaches failLSN, we lose our segment */
-			failSeg = targetSeg + keepSegs + 1;
+			failSeg = targetSeg + Max(slotKeepSegs, keepSegs) + 1;
 			XLogSegNoOffsetToRecPtr(failSeg, 0, wal_segment_size, failLSN);
 
 			values[i++] = Int64GetDatum(failLSN - currlsn);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 031ca0327f..8b71d16f92 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2631,12 +2631,13 @@ static struct config_int ConfigureNamesInt[] =
 	},
 
 	{
-		{"wal_keep_segments", PGC_SIGHUP, REPLICATION_SENDING,
-			gettext_noop("Sets the number of WAL files held for standby servers."),
-			NULL
+		{"wal_keep_size", PGC_SIGHUP, REPLICATION_SENDING,
+			gettext_noop("Sets the size of WAL files held for standby servers."),
+			NULL,
+			GUC_UNIT_MB
 		},
-		&wal_keep_segments,
-		0, 0, INT_MAX,
+		&wal_keep_size_mb,
+		0, 0, MAX_KILOBYTES,
 		NULL, NULL, NULL
 	},
 
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index e430e33c7b..90beb87061 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -288,7 +288,7 @@
 
 #max_wal_senders = 10		# max number of walsender processes
 				# (change requires restart)
-#wal_keep_segments = 0		# in logfile segments; 0 disables
+#wal_keep_size = 0		# in megabytes; 0 disables
 #max_slot_wal_keep_size = -1	# in megabytes; -1 disables
 #wal_sender_timeout = 60s	# in milliseconds; 0 disables
 
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 7516af7a01..41ed7d4b3b 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -135,11 +135,11 @@ sub setup_cluster
 		extra            => $extra,
 		auth_extra       => [ '--create-role', 'rewind_user' ]);
 
-	# Set wal_keep_segments to prevent WAL segment recycling after enforced
+	# Set wal_keep_size to prevent WAL segment recycling after enforced
 	# checkpoints in the tests.
 	$node_primary->append_conf(
 		'postgresql.conf', qq(
-wal_keep_segments = 20
+wal_keep_size = 320MB
 ));
 	return;
 }
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 5b14334887..456f0d1321 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -107,7 +107,7 @@ extern bool reachedConsistency;
 extern int	wal_segment_size;
 extern int	min_wal_size_mb;
 extern int	max_wal_size_mb;
-extern int	wal_keep_segments;
+extern int	wal_keep_size_mb;
 extern int	max_slot_wal_keep_size_mb;
 extern int	XLOGbuffers;
 extern int	XLogArchiveTimeout;
@@ -272,7 +272,7 @@ typedef enum WALAvailability
 	WALAVAIL_INVALID_LSN,		/* parameter error */
 	WALAVAIL_RESERVED,			/* WAL segment is within max_wal_size */
 	WALAVAIL_EXTENDED,			/* WAL segment is reserved by a slot or
-								 * wal_keep_segments */
+								 * wal_keep_size */
 	WALAVAIL_UNRESERVED,		/* no longer reserved, but not removed yet */
 	WALAVAIL_REMOVED			/* WAL segment has been removed */
 } WALAvailability;
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index 1fced12fca..20f4a1bbc3 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -116,19 +116,19 @@ $start_lsn = $node_primary->lsn('write');
 $node_primary->wait_for_catchup($node_standby, 'replay', $start_lsn);
 $node_standby->stop;
 
-# wal_keep_segments overrides max_slot_wal_keep_size
+# wal_keep_size overrides max_slot_wal_keep_size
 $result = $node_primary->safe_psql('postgres',
-	"ALTER SYSTEM SET wal_keep_segments to 8; SELECT pg_reload_conf();");
+	"ALTER SYSTEM SET wal_keep_size to '8MB'; SELECT pg_reload_conf();");
 # Advance WAL again then checkpoint, reducing remain by 6 MB.
 advance_wal($node_primary, 6);
 $result = $node_primary->safe_psql('postgres',
 	"SELECT wal_status as remain FROM pg_replication_slots WHERE slot_name = 'rep1'"
 );
 is($result, "extended",
-	'check that wal_keep_segments overrides max_slot_wal_keep_size');
-# restore wal_keep_segments
+	'check that wal_keep_size overrides max_slot_wal_keep_size');
+# restore wal_keep_size
 $result = $node_primary->safe_psql('postgres',
-	"ALTER SYSTEM SET wal_keep_segments to 0; SELECT pg_reload_conf();");
+	"ALTER SYSTEM SET wal_keep_size to 0; SELECT pg_reload_conf();");
 
 # The standby can reconnect to primary
 $node_standby->start;
#18David Steele
david@pgmasters.net
In reply to: Fujii Masao (#17)
Re: max_slot_wal_keep_size and wal_keep_segments

On 7/14/20 12:00 AM, Fujii Masao wrote:

The patch was no longer applied cleanly because of recent commit.
So I updated the patch. Attached.

Barring any objection, I will commit this patch.

This doesn't look right:

+   the <xref linkend="guc-wal-keep-size"/> most recent megabytes
+   WAL files plus one WAL file are

How about:

+   <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one WAL file are

Other than that, looks good to me.

Regards,
--
-David
david@pgmasters.net

#19Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: David Steele (#18)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020/07/14 20:30, David Steele wrote:

On 7/14/20 12:00 AM, Fujii Masao wrote:

The patch was no longer applied cleanly because of recent commit.
So I updated the patch. Attached.

Barring any objection, I will commit this patch.

This doesn't look right:

+   the <xref linkend="guc-wal-keep-size"/> most recent megabytes
+   WAL files plus one WAL file are

How about:

+   <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one WAL file are

Thanks for the comment! Isn't it better to keep "most recent" part?
If so, what about either of the followings?

1. <xref linkend="guc-wal-keep-size"/> megabytes of WAL files plus
one WAL file that were most recently generated are kept all time.

2. <xref linkend="guc-wal-keep-size"/> megabytes + <xref linkend="guc-wal-segment-size"> bytes
of WAL files that were most recently generated are kept all time.

Regards,

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

#20David Steele
david@pgmasters.net
In reply to: Fujii Masao (#19)
Re: max_slot_wal_keep_size and wal_keep_segments

On 7/17/20 5:11 AM, Fujii Masao wrote:

On 2020/07/14 20:30, David Steele wrote:

On 7/14/20 12:00 AM, Fujii Masao wrote:

The patch was no longer applied cleanly because of recent commit.
So I updated the patch. Attached.

Barring any objection, I will commit this patch.

This doesn't look right:

+   the <xref linkend="guc-wal-keep-size"/> most recent megabytes
+   WAL files plus one WAL file are

How about:

+   <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one WAL file are

Thanks for the comment! Isn't it better to keep "most recent" part?
If so, what about either of the followings?

1. <xref linkend="guc-wal-keep-size"/> megabytes of WAL files plus
    one WAL file that were most recently generated are kept all time.

2. <xref linkend="guc-wal-keep-size"/> megabytes + <xref
linkend="guc-wal-segment-size"> bytes
    of WAL files that were most recently generated are kept all time.

"most recent" seemed implied to me, but I see your point.

How about:

+   the most recent <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one additional WAL file are

Regards,
--
-David
david@pgmasters.net

#21Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: David Steele (#20)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020/07/17 20:24, David Steele wrote:

On 7/17/20 5:11 AM, Fujii Masao wrote:

On 2020/07/14 20:30, David Steele wrote:

On 7/14/20 12:00 AM, Fujii Masao wrote:

The patch was no longer applied cleanly because of recent commit.
So I updated the patch. Attached.

Barring any objection, I will commit this patch.

This doesn't look right:

+   the <xref linkend="guc-wal-keep-size"/> most recent megabytes
+   WAL files plus one WAL file are

How about:

+   <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one WAL file are

Thanks for the comment! Isn't it better to keep "most recent" part?
If so, what about either of the followings?

1. <xref linkend="guc-wal-keep-size"/> megabytes of WAL files plus
     one WAL file that were most recently generated are kept all time.

2. <xref linkend="guc-wal-keep-size"/> megabytes + <xref linkend="guc-wal-segment-size"> bytes
     of WAL files that were most recently generated are kept all time.

"most recent" seemed implied to me, but I see your point.

How about:

+   the most recent <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one additional WAL file are

I adopted this and pushed the patch. Thanks!

Also we need to update the release note for v13. What about adding the following?

------------------------------------
Rename configuration parameter wal_keep_segments to wal_keep_size.

This allows how much WAL files to retain for the standby server, by bytes instead of the number of files.
If you previously used wal_keep_segments, the following formula will give you an approximately equivalent setting:

wal_keep_size = wal_keep_segments * wal_segment_size (typically 16MB)
------------------------------------

Regards,

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

#22Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Fujii Masao (#21)
1 attachment(s)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020/07/20 13:48, Fujii Masao wrote:

On 2020/07/17 20:24, David Steele wrote:

On 7/17/20 5:11 AM, Fujii Masao wrote:

On 2020/07/14 20:30, David Steele wrote:

On 7/14/20 12:00 AM, Fujii Masao wrote:

The patch was no longer applied cleanly because of recent commit.
So I updated the patch. Attached.

Barring any objection, I will commit this patch.

This doesn't look right:

+   the <xref linkend="guc-wal-keep-size"/> most recent megabytes
+   WAL files plus one WAL file are

How about:

+   <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one WAL file are

Thanks for the comment! Isn't it better to keep "most recent" part?
If so, what about either of the followings?

1. <xref linkend="guc-wal-keep-size"/> megabytes of WAL files plus
     one WAL file that were most recently generated are kept all time.

2. <xref linkend="guc-wal-keep-size"/> megabytes + <xref linkend="guc-wal-segment-size"> bytes
     of WAL files that were most recently generated are kept all time.

"most recent" seemed implied to me, but I see your point.

How about:

+   the most recent <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one additional WAL file are

I adopted this and pushed the patch. Thanks!

Also we need to update the release note for v13. What about adding the following?

------------------------------------
Rename configuration parameter wal_keep_segments to wal_keep_size.

This allows how much WAL files to retain for the standby server, by bytes instead of the number of files.
If you previously used wal_keep_segments, the following formula will give you an approximately equivalent setting:

wal_keep_size = wal_keep_segments * wal_segment_size (typically 16MB)
------------------------------------

Patch attached.

Regards,

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

Attachments:

wal_keep_size_release_note_v1.patchtext/plain; charset=UTF-8; name=wal_keep_size_release_note_v1.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/doc/src/sgml/release-13.sgml b/doc/src/sgml/release-13.sgml
index 22363a186b..c3698c1401 100644
--- a/doc/src/sgml/release-13.sgml
+++ b/doc/src/sgml/release-13.sgml
@@ -209,6 +209,28 @@ Author: Fujii Masao <fujii@postgresql.org>
 
     <listitem>
 <!--
+Author: Fujii Masao <fujii@postgresql.org>
+2020-07-20 [f5dff45962] Rename wal_keep_segments to wal_keep_size.
+-->
+
+     <para>
+      Rename configuration parameter <varname>wal_keep_segments</varname>
+      to <xref linkend="guc-wal-keep-size"/> (Fujii Masao)
+     </para>
+
+     <para>
+      This allows how much WAL files to retain for the standby server,
+      to be specified by bytes instead of the number of files. If you
+      previously used <varname>wal_keep_segments</varname>,
+      the following formula will give you an approximately equivalent setting:
+<programlisting>
+wal_keep_size = wal_keep_segments * wal_segment_size (typically 16MB)
+</programlisting>
+     </para>
+    </listitem>
+
+    <listitem>
+<!--
 Author: Tom Lane <tgl@sss.pgh.pa.us>
 2020-03-05 [84eca14bc] Remove ancient hacks to ignore certain opclass names in
 -->
#23David Steele
david@pgmasters.net
In reply to: Fujii Masao (#22)
Re: max_slot_wal_keep_size and wal_keep_segments

On 7/20/20 6:02 AM, Fujii Masao wrote:

On 2020/07/20 13:48, Fujii Masao wrote:

On 2020/07/17 20:24, David Steele wrote:

On 7/17/20 5:11 AM, Fujii Masao wrote:

On 2020/07/14 20:30, David Steele wrote:

On 7/14/20 12:00 AM, Fujii Masao wrote:

The patch was no longer applied cleanly because of recent commit.
So I updated the patch. Attached.

Barring any objection, I will commit this patch.

This doesn't look right:

+   the <xref linkend="guc-wal-keep-size"/> most recent megabytes
+   WAL files plus one WAL file are

How about:

+   <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one WAL file are

Thanks for the comment! Isn't it better to keep "most recent" part?
If so, what about either of the followings?

1. <xref linkend="guc-wal-keep-size"/> megabytes of WAL files plus
     one WAL file that were most recently generated are kept all time.

2. <xref linkend="guc-wal-keep-size"/> megabytes + <xref
linkend="guc-wal-segment-size"> bytes
     of WAL files that were most recently generated are kept all time.

"most recent" seemed implied to me, but I see your point.

How about:

+   the most recent <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one additional WAL file are

I adopted this and pushed the patch. Thanks!

Also we need to update the release note for v13. What about adding the
following?

------------------------------------
Rename configuration parameter wal_keep_segments to wal_keep_size.

This allows how much WAL files to retain for the standby server, by
bytes instead of the number of files.
If you previously used wal_keep_segments, the following formula will
give you an approximately equivalent setting:

wal_keep_size = wal_keep_segments * wal_segment_size (typically 16MB)
------------------------------------

I would rework that first sentence a bit. How about:

+ This determines how much WAL to retain for the standby server,
+ specified in megabytes rather than number of files.

The rest looks fine to me.

Regards,
--
-David
david@pgmasters.net

#24Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: David Steele (#23)
Re: max_slot_wal_keep_size and wal_keep_segments

On 2020/07/20 21:21, David Steele wrote:

On 7/20/20 6:02 AM, Fujii Masao wrote:

On 2020/07/20 13:48, Fujii Masao wrote:

On 2020/07/17 20:24, David Steele wrote:

On 7/17/20 5:11 AM, Fujii Masao wrote:

On 2020/07/14 20:30, David Steele wrote:

On 7/14/20 12:00 AM, Fujii Masao wrote:

The patch was no longer applied cleanly because of recent commit.
So I updated the patch. Attached.

Barring any objection, I will commit this patch.

This doesn't look right:

+   the <xref linkend="guc-wal-keep-size"/> most recent megabytes
+   WAL files plus one WAL file are

How about:

+   <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one WAL file are

Thanks for the comment! Isn't it better to keep "most recent" part?
If so, what about either of the followings?

1. <xref linkend="guc-wal-keep-size"/> megabytes of WAL files plus
     one WAL file that were most recently generated are kept all time.

2. <xref linkend="guc-wal-keep-size"/> megabytes + <xref linkend="guc-wal-segment-size"> bytes
     of WAL files that were most recently generated are kept all time.

"most recent" seemed implied to me, but I see your point.

How about:

+   the most recent <xref linkend="guc-wal-keep-size"/> megabytes of
+   WAL files plus one additional WAL file are

I adopted this and pushed the patch. Thanks!

Also we need to update the release note for v13. What about adding the following?

------------------------------------
Rename configuration parameter wal_keep_segments to wal_keep_size.

This allows how much WAL files to retain for the standby server, by bytes instead of the number of files.
If you previously used wal_keep_segments, the following formula will give you an approximately equivalent setting:

wal_keep_size = wal_keep_segments * wal_segment_size (typically 16MB)
------------------------------------

I would rework that first sentence a bit. How about:

+ This determines how much WAL to retain for the standby server,
+ specified in megabytes rather than number of files.

The rest looks fine to me.

Thanks for the review!
I adopted your suggestion in the updated version of the patch and pushed it.

Regards,

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