Re: [HACKERS] Restricting maximum keep segments by repslots

Hello.

Although replication slot is helpful to avoid unwanted WAL
deletion, on the other hand it can cause a disastrous situation
by keeping WAL segments without a limit. Removing the causal
repslot will save this situation but it is not doable if the
standby is active. We should do a rather complex and forcible
steps to relieve the situation especially in an automatic
manner. (As for me, specifically in an HA cluster.)

This patch adds a GUC to put a limit to the number of segments
that replication slots can keep. Hitting the limit during
checkpoint shows a warining and the segments older than the limit
are removed.

> WARNING: restart LSN of replication slots is ignored by checkpoint
> DETAIL: Some replication slots lose required WAL segnents to continue.

Another measure would be automatic deletion or inactivation of
the culprit slot but it seems too complex for the problem.

As we have already postponed some patches by the triage for the
last commit fest, this might should be postponed to PG11.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Feb 28, 2017 at 12:27 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Although replication slot is helpful to avoid unwanted WAL
> deletion, on the other hand it can cause a disastrous situation
> by keeping WAL segments without a limit. Removing the causal
> repslot will save this situation but it is not doable if the
> standby is active. We should do a rather complex and forcible
> steps to relieve the situation especially in an automatic
> manner. (As for me, specifically in an HA cluster.)
>
> This patch adds a GUC to put a limit to the number of segments
> that replication slots can keep. Hitting the limit during
> checkpoint shows a warining and the segments older than the limit
> are removed.
>
>> WARNING: restart LSN of replication slots is ignored by checkpoint
>> DETAIL: Some replication slots lose required WAL segnents to continue.
>
> Another measure would be automatic deletion or inactivation of
> the culprit slot but it seems too complex for the problem.
>
>
> As we have already postponed some patches by the triage for the
> last commit fest, this might should be postponed to PG11.

Please no. Replication slots are designed the current way because we
don't want to have to use something like wal_keep_segments as it is a
wart, and this applies as well for replication slots in my opinion. If
a slot is bloating WAL and you care about your Postgres instance, I
would recommend instead that you use a background worker that does
monitoring of the situation based on max_wal_size for example, killing
the WAL sender associated to the slot if there is something connected
but it is frozen or it cannot keep up the pace of WAL generation, and
then dropping the slot. You may want to issue a checkpoint in this
case as well to ensure that segments get recycled. But anyway, if you
reach this point of WAL bloat, perhaps that's for the best as users
would know about it because backups would get in danger. For some
applications, that is acceptable, but you could always rely on
monitoring slots and kill them on sight if needed. That's as well more
flexible than having a parameter that basically is just a synonym of
max_wal_size.
--
Michael

Thank you for the opinion.

At Tue, 28 Feb 2017 12:42:32 +0900, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote in <CAB7nPqQm0QetoShggQnn4bLFd9oXKKHG7RafBP3Krno62=ORww(at)mail(dot)gmail(dot)com>
> Please no. Replication slots are designed the current way because we
> don't want to have to use something like wal_keep_segments as it is a
> wart, and this applies as well for replication slots in my opinion. If
> a slot is bloating WAL and you care about your Postgres instance, I
> would recommend instead that you use a background worker that does
> monitoring of the situation based on max_wal_size for example, killing
> the WAL sender associated to the slot if there is something connected
> but it is frozen or it cannot keep up the pace of WAL generation, and
> then dropping the slot.

It is doable without a plugin and currently we are planning to do
in the way (Maybe such plugin would be unacceptable..). Killing
walsender (which one?), removing the slot and if failed.. This is
the 'steps rather complex' and fragile.

> You may want to issue a checkpoint in this
> case as well to ensure that segments get recycled. But anyway, if you
> reach this point of WAL bloat, perhaps that's for the best as users
> would know about it because backups would get in danger.

Yes, but at the end it is better than that a server just stops
with a PANIC.

> For some applications, that is acceptable, but you could always
> rely on monitoring slots and kill them on sight if
> needed.

Another solution would be that removing a slot kills
corresponding walsender. What do you think about this?

pg_drop_replication_slot(name, *force*)

force = true kills the walsender runs on the slot.

> That's as well more flexible than having a parameter
> that basically is just a synonym of max_wal_size.

I thought the same thing first, max_wal_size_hard, that limits
the wal size including extra (other than them for the two
checkpoig cycles) segments.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 28/02/17 04:27, Kyotaro HORIGUCHI wrote:
> Hello.
>
> Although replication slot is helpful to avoid unwanted WAL
> deletion, on the other hand it can cause a disastrous situation
> by keeping WAL segments without a limit. Removing the causal
> repslot will save this situation but it is not doable if the
> standby is active. We should do a rather complex and forcible
> steps to relieve the situation especially in an automatic
> manner. (As for me, specifically in an HA cluster.)
>

I agree that that it should be possible to limit how much WAL slot keeps.

> This patch adds a GUC to put a limit to the number of segments
> that replication slots can keep. Hitting the limit during
> checkpoint shows a warining and the segments older than the limit
> are removed.
>
>> WARNING: restart LSN of replication slots is ignored by checkpoint
>> DETAIL: Some replication slots lose required WAL segnents to continue.
>

However this is dangerous as logical replication slot does not consider
it error when too old LSN is requested so we'd continue replication,
hiding data loss.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Feb 28, 2017 at 1:16 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> It is doable without a plugin and currently we are planning to do
> in the way (Maybe such plugin would be unacceptable..). Killing
> walsender (which one?), removing the slot and if failed..

The PID and restart_lsn associated to each slot offer enough
information for monitoring.

> This is the 'steps rather complex' and fragile.

The handling of slot drop is not complex. The insurance that WAL
segments get recycled on time and avoid a full bloat is though.

>> That's as well more flexible than having a parameter
>> that basically is just a synonym of max_wal_size.
>
> I thought the same thing first, max_wal_size_hard, that limits
> the wal size including extra (other than them for the two
> checkpoig cycles) segments.

It would make more sense to just switch max_wal_size from a soft to a
hard limit. The current behavior is not cool with activity spikes.
--
Michael

On Tue, Feb 28, 2017 at 10:04 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> It would make more sense to just switch max_wal_size from a soft to a
> hard limit. The current behavior is not cool with activity spikes.

Having a hard limit on WAL size would be nice, but that's a different
problem from the one being discussed here. If max_wal_size becomes a
hard limit, and a standby with a replication slot dies, then the
master eventually starts refusing all writes. I guess that's better
than a PANIC, but it's not likely to make users very happy. I think
it's entirely reasonable to want a behavior where the master is
willing to retain up to X amount of extra WAL for the benefit of some
standby, but after that the health of the master takes priority.

You can't really get that behavior today. Either you can retain as
much WAL as might be necessary through archiving or a slot, or you can
retain a fixed amount of WAL whether it's actually needed or not.
There's currently no approach that retains min(wal_needed,
configured_value).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2017-02-28 12:42:32 +0900, Michael Paquier wrote:
> Please no. Replication slots are designed the current way because we
> don't want to have to use something like wal_keep_segments as it is a
> wart, and this applies as well for replication slots in my opinion.

I think a per-slot option to limit the amount of retention would make
sense.

- Andres

On 2/27/17 23:27, Petr Jelinek wrote:
>>> WARNING: restart LSN of replication slots is ignored by checkpoint
>>> DETAIL: Some replication slots lose required WAL segnents to continue.
> However this is dangerous as logical replication slot does not consider
> it error when too old LSN is requested so we'd continue replication,
> hiding data loss.

In general, we would need a much more evident and strict way to discover
when this condition is hit. Like a "full" column in
pg_stat_replication_slot, and refusing connections to the slot until it
is cleared.

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

On 2/27/17 22:27, Kyotaro HORIGUCHI wrote:
> This patch adds a GUC to put a limit to the number of segments
> that replication slots can keep.

Please measure it in size, not in number of segments.

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

At Wed, 1 Mar 2017 08:06:10 -0800, Andres Freund <andres(at)anarazel(dot)de> wrote in <20170301160610(dot)wc7ez3vihmialntd(at)alap3(dot)anarazel(dot)de>
> On 2017-02-28 12:42:32 +0900, Michael Paquier wrote:
> > Please no. Replication slots are designed the current way because we
> > don't want to have to use something like wal_keep_segments as it is a
> > wart, and this applies as well for replication slots in my opinion.
>
> I think a per-slot option to limit the amount of retention would make
> sense.

I started from that but I found that all slots refer to the same
location as the origin of the distance, that is, the last segment
number that KeepLogSeg returns. As the result the whole logic
became as the following. This is one reason of the proposed pach.

- Take the maximum value of the maximum-retain-LSN-amount per slot.
- Apply the maximum value during the calcuation in KeepLogSeg.
- (These steps runs only when at least one slot exists)

The another reason was, as Robert retold, I thought that this is
a matter of system (or a DB cluster) wide health and works in a
bit different way from what the name "max_wal_size_hard"
suggests.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Wed, 1 Mar 2017 12:17:43 -0500, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <dc7faead-61c4-402e-a6dc-534192833d77(at)2ndquadrant(dot)com>
> On 2/27/17 23:27, Petr Jelinek wrote:
> >>> WARNING: restart LSN of replication slots is ignored by checkpoint
> >>> DETAIL: Some replication slots lose required WAL segnents to continue.
> > However this is dangerous as logical replication slot does not consider
> > it error when too old LSN is requested so we'd continue replication,
> > hiding data loss.
>
> In general, we would need a much more evident and strict way to discover
> when this condition is hit. Like a "full" column in
> pg_stat_replication_slot, and refusing connections to the slot until it
> is cleared.

Anyway, if preserving WAL to replicate has priority to the
master's health, this doesn't nothing by leaving
'max_wal_keep_segments' to 0.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Wed, 1 Mar 2017 12:18:07 -0500, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <98538b00-42ae-6a6b-f852-50b3c937ade4(at)2ndquadrant(dot)com>
> On 2/27/17 22:27, Kyotaro HORIGUCHI wrote:
> > This patch adds a GUC to put a limit to the number of segments
> > that replication slots can keep.
>
> Please measure it in size, not in number of segments.

It was difficult to dicide which is reaaonable but I named it
after wal_keep_segments because it has the similar effect.

In bytes(or LSN)
max_wal_size
min_wal_size
wal_write_flush_after

In segments
wal_keep_segments

But surely max_slot_wal_keep_segments works to keep disk space so
bytes would be reasonable.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 3/1/17 19:54, Kyotaro HORIGUCHI wrote:
>> Please measure it in size, not in number of segments.
> It was difficult to dicide which is reaaonable but I named it
> after wal_keep_segments because it has the similar effect.
>
> In bytes(or LSN)
> max_wal_size
> min_wal_size
> wal_write_flush_after
>
> In segments
> wal_keep_segments

We have been moving away from measuring in segments. For example,
checkpoint_segments was replaced by max_wal_size.

Also, with the proposed patch that allows changing the segment size more
easily, this will become more important. (I wonder if that will require
wal_keep_segments to change somehow.)

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

Thank you for the comment.

At Fri, 3 Mar 2017 14:47:20 -0500, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <ac510b45-7805-7ccc-734c-1b38a6645f3e(at)2ndquadrant(dot)com>
> On 3/1/17 19:54, Kyotaro HORIGUCHI wrote:
> >> Please measure it in size, not in number of segments.
> > It was difficult to dicide which is reaaonable but I named it
> > after wal_keep_segments because it has the similar effect.
> >
> > In bytes(or LSN)
> > max_wal_size
> > min_wal_size
> > wal_write_flush_after
> >
> > In segments
> > wal_keep_segments
>
> We have been moving away from measuring in segments. For example,
> checkpoint_segments was replaced by max_wal_size.
>
> Also, with the proposed patch that allows changing the segment size more
> easily, this will become more important. (I wonder if that will require
> wal_keep_segments to change somehow.)

Agreed. It is 'max_slot_wal_keep_size' in the new version.

wal_keep_segments might should be removed someday.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 28 February 2017 at 12:27, Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:

>> This patch adds a GUC to put a limit to the number of segments
>> that replication slots can keep. Hitting the limit during
>> checkpoint shows a warining and the segments older than the limit
>> are removed.
>>
>>> WARNING: restart LSN of replication slots is ignored by checkpoint
>>> DETAIL: Some replication slots lose required WAL segnents to continue.
>>
>
> However this is dangerous as logical replication slot does not consider
> it error when too old LSN is requested so we'd continue replication,
> hiding data loss.

That skipping only happens if you request a startpoint older than
confirmed_flush_lsn . It doesn't apply to this situation.

The client cannot control where we start decoding, it's always
restart_lsn, and if we can't find a needed WAL segment we'll ERROR. So
this is safe, though the error will be something about being unable to
find a wal segment that users might not directly associate with having
set this option. It won't say "slot disabled because needed WAL has
been discarded due to [setting]" or anything.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hello,

I'll add this to CF2017-09.

At Mon, 06 Mar 2017 18:20:06 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20170306(dot)182006(dot)172683338(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> Thank you for the comment.
>
> At Fri, 3 Mar 2017 14:47:20 -0500, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <ac510b45-7805-7ccc-734c-1b38a6645f3e(at)2ndquadrant(dot)com>
> > On 3/1/17 19:54, Kyotaro HORIGUCHI wrote:
> > >> Please measure it in size, not in number of segments.
> > > It was difficult to dicide which is reaaonable but I named it
> > > after wal_keep_segments because it has the similar effect.
> > >
> > > In bytes(or LSN)
> > > max_wal_size
> > > min_wal_size
> > > wal_write_flush_after
> > >
> > > In segments
> > > wal_keep_segments
> >
> > We have been moving away from measuring in segments. For example,
> > checkpoint_segments was replaced by max_wal_size.
> >
> > Also, with the proposed patch that allows changing the segment size more
> > easily, this will become more important. (I wonder if that will require
> > wal_keep_segments to change somehow.)
>
> Agreed. It is 'max_slot_wal_keep_size' in the new version.
>
> wal_keep_segments might should be removed someday.

- Following to min/max_wal_size, the variable was renamed to
"max_slot_wal_keep_size_mb" and used as ConvertToXSegs(x)"

- Stopped warning when checkpoint doesn't flush segments required
by slots even if max_slot_wal_keep_size have worked.

- Avoided subraction that may be negative.

regards,

I'm still concerned about how the critical situation is handled. Your
patch just prints a warning to the log and then goes on -- doing what?
The warning rolls off the log, and then you have no idea what happened,
or how to recover.

I would like a flag in pg_replication_slots, and possibly also a
numerical column that indicates how far away from the critical point
each slot is. That would be great for a monitoring system.

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

Hello,

At Fri, 1 Sep 2017 23:49:21 -0400, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <751e09c4-93e0-de57-edd2-e64c4950f5e3(at)2ndquadrant(dot)com>
> I'm still concerned about how the critical situation is handled. Your
> patch just prints a warning to the log and then goes on -- doing what?
>
> The warning rolls off the log, and then you have no idea what happened,
> or how to recover.

The victims should be complaining in their log files, but, yes, I
must admit that it's extremely resembles /dev/null. And the
catastrophe comes suddenly.

> I would like a flag in pg_replication_slots, and possibly also a
> numerical column that indicates how far away from the critical point
> each slot is. That would be great for a monitoring system.

Great! I'll do that right now.

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

Thanks.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Thu, 07 Sep 2017 14:12:12 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20170907(dot)141212(dot)227032666(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > I would like a flag in pg_replication_slots, and possibly also a
> > numerical column that indicates how far away from the critical point
> > each slot is. That would be great for a monitoring system.
>
> Great! I'll do that right now.

Done.

In the attached patch on top of the previous patch, I added two
columns in pg_replication_slots, "live" and "distance". The first
indicates the slot will "live" after the next checkpoint. The
second shows the how many bytes checkpoint lsn can advance before
the slot will "die", or how many bytes the slot have lost after
"death".

Setting wal_keep_segments = 1 and max_slot_wal_keep_size = 16MB.

=# select slot_name, restart_lsn, pg_current_wal_lsn(), live, distance from pg_replication_slots;

slot_name | restart_lsn | pg_current_wal_lsn | live | distance
-----------+-------------+--------------------+------+-----------
s1 | 0/162D388 | 0/162D3C0 | t | 0/29D2CE8

This shows that checkpoint can advance 0x29d2ce8 bytes before the
slot will die even if the connection stalls.

s1 | 0/4001180 | 0/6FFF2B8 | t | 0/DB8

Just before the slot loses sync.

s1 | 0/4001180 | 0/70008A8 | f | 0/FFEE80

The checkpoint after this removes some required segments.

2017-09-07 19:04:07.677 JST [13720] WARNING: restart LSN of replication slots is ignored by checkpoint
2017-09-07 19:04:07.677 JST [13720] DETAIL: Some replication slots have lost required WAL segnents to continue by up to 1 segments.

If max_slot_wal_keep_size if not set (0), live is always true and
distance is NULL.

slot_name | restart_lsn | pg_current_wal_lsn | live | distance
-----------+-------------+--------------------+------+-----------
s1 | 0/4001180 | 0/73117A8 | t |

- The name (or its content) of the new columns should be arguable.

- pg_replication_slots view takes LWLock on ControlFile and
spinlock on XLogCtl for every slot. But seems difficult to
reduce it..

- distance seems mitakenly becomes 0/0 for certain condition..

- The result seems almost right but more precise check needed.
(Anyway it cannot be perfectly exact.);

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Thu, 07 Sep 2017 21:59:56 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20170907(dot)215956(dot)110216588(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> Hello,
>
> At Thu, 07 Sep 2017 14:12:12 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20170907(dot)141212(dot)227032666(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > > I would like a flag in pg_replication_slots, and possibly also a
> > > numerical column that indicates how far away from the critical point
> > > each slot is. That would be great for a monitoring system.
> >
> > Great! I'll do that right now.
>
> Done.

The CF status of this patch turned into "Waiting on Author".
This is because the second patch is posted separately from the
first patch. I repost them together after rebasing to the current
master.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Wed, 13 Sep 2017 11:43:06 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20170913(dot)114306(dot)67844218(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
horiguchi.kyotaro> At Thu, 07 Sep 2017 21:59:56 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20170907(dot)215956(dot)110216588(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > Hello,
> >
> > At Thu, 07 Sep 2017 14:12:12 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20170907(dot)141212(dot)227032666(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > > > I would like a flag in pg_replication_slots, and possibly also a
> > > > numerical column that indicates how far away from the critical point
> > > > each slot is. That would be great for a monitoring system.
> > >
> > > Great! I'll do that right now.
> >
> > Done.
>
> The CF status of this patch turned into "Waiting on Author".
> This is because the second patch is posted separately from the
> first patch. I repost them together after rebasing to the current
> master.

Hmm. I was unconsciously careless of regression test since it is
in a tentative shape. This must pass the regression..

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello, this is a rebased version.

It gets a change of the meaning of monitoring value along with
rebasing.

In previous version, the "live" column mysteriously predicts the
necessary segments will be kept or lost by the next checkpoint
and the "distance" offered a still more mysterious value.

In this version the meaning of the two columns became clear and
informative.

pg_replication_slots
- live :
true the slot have not lost necessary segments.

- distance:
how many bytes LSN can advance before the margin defined by
max_slot_wal_keep_size (and wal_keep_segments) is exhasuted,
or how many bytes this slot have lost xlog from restart_lsn.

There is a case where live = t and distance = 0. The slot is
currently having all the necessary segments but will start to
lose them at most two checkpoint passes.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Oct 31, 2017 at 10:43 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello, this is a rebased version.

Hello Horiguchi-san,

I think the "ddl" test under contrib/test_decoding also needs to be
updated because it looks at pg_replication_slots and doesn't expect
your new columns.

--
Thomas Munro
http://www.enterprisedb.com

On 31 October 2017 at 17:43, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello, this is a rebased version.
>
> It gets a change of the meaning of monitoring value along with
> rebasing.
>
> In previous version, the "live" column mysteriously predicts the
> necessary segments will be kept or lost by the next checkpoint
> and the "distance" offered a still more mysterious value.

Would it make sense to teach xlogreader how to fetch from WAL archive,
too? That way if there's an archive, slots could continue to be used
even after we purge from local pg_xlog, albeit at a performance cost.

I'm thinking of this mainly for logical slots.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2017-11-06 11:07:04 +0800, Craig Ringer wrote:
> Would it make sense to teach xlogreader how to fetch from WAL archive,
> too? That way if there's an archive, slots could continue to be used
> even after we purge from local pg_xlog, albeit at a performance cost.
>
> I'm thinking of this mainly for logical slots.

That seems more like a page read callback's job than xlogreader's.

Greetings,

Andres Freund

Hi,

On 2017-10-31 18:43:10 +0900, Kyotaro HORIGUCHI wrote:
> - distance:
> how many bytes LSN can advance before the margin defined by
> max_slot_wal_keep_size (and wal_keep_segments) is exhasuted,
> or how many bytes this slot have lost xlog from restart_lsn.

I don't think 'distance' is a good metric - that's going to continually
change. Why not store the LSN that's available and provide a function
that computes this? Or just rely on the lsn - lsn operator?

- Andres

Hello,

At Mon, 6 Nov 2017 05:20:50 -0800, Andres Freund <andres(at)anarazel(dot)de> wrote in <20171106132050(dot)6apzynxrqrzghb4r(at)alap3(dot)anarazel(dot)de>
> Hi,
>
> On 2017-10-31 18:43:10 +0900, Kyotaro HORIGUCHI wrote:
> > - distance:
> > how many bytes LSN can advance before the margin defined by
> > max_slot_wal_keep_size (and wal_keep_segments) is exhasuted,
> > or how many bytes this slot have lost xlog from restart_lsn.
>
> I don't think 'distance' is a good metric - that's going to continually
> change. Why not store the LSN that's available and provide a function
> that computes this? Or just rely on the lsn - lsn operator?

It seems reasonable.,The 'secured minimum LSN' is common among
all slots so showing it in the view may look a bit stupid but I
don't find another suitable place for it. distance = 0 meant the
state that the slot is living but insecured in the previous patch
and that information is lost by changing 'distance' to
'min_secure_lsn'.

Thus I changed the 'live' column to 'status' and show that staus
in text representation.

status: secured | insecured | broken

So this looks like the following (max_slot_wal_keep_size = 8MB,
which is a half of the default segment size)

-- slots that required WAL is surely available
select restart_lsn, status, min_secure_lsn, pg_current_wal_lsn() from pg_replication_slots;
restart_lsn | status | min_recure_lsn | pg_current_wal_lsn
------------+---------+----------------+--------------------
0/1A000060 | secured | 0/1A000000 | 0/1B42BC78

-- slots that required WAL is still available but insecured
restart_lsn | status | min_recure_lsn | pg_current_wal_lsn
------------+-----------+----------------+--------------------
0/1A000060 | insecured | 0/1C000000 | 0/1D76C948

-- slots that required WAL is lost
# We should have seen the log 'Some replication slots have lost...'

restart_lsn | status | min_recure_lsn | pg_current_wal_lsn
------------+--------+----------------+--------------------
0/1A000060 | broken | 0/1C000000 | 0/1D76C9F0

I noticed that I abandoned the segment fragment of
max_slot_wal_keep_size in calculating in the routines. The
current patch honors the frament part of max_slot_wal_keep_size.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Oops! The previous patch is forgetting the default case and crashes.

At Wed, 08 Nov 2017 13:14:31 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20171108(dot)131431(dot)170534842(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > I don't think 'distance' is a good metric - that's going to continually
> > change. Why not store the LSN that's available and provide a function
> > that computes this? Or just rely on the lsn - lsn operator?
>
> It seems reasonable.,The 'secured minimum LSN' is common among
> all slots so showing it in the view may look a bit stupid but I
> don't find another suitable place for it. distance = 0 meant the
> state that the slot is living but insecured in the previous patch
> and that information is lost by changing 'distance' to
> 'min_secure_lsn'.
>
> Thus I changed the 'live' column to 'status' and show that staus
> in text representation.
>
> status: secured | insecured | broken
>
> So this looks like the following (max_slot_wal_keep_size = 8MB,
> which is a half of the default segment size)
>
> -- slots that required WAL is surely available
> select restart_lsn, status, min_secure_lsn, pg_current_wal_lsn() from pg_replication_slots;
> restart_lsn | status | min_recure_lsn | pg_current_wal_lsn
> ------------+---------+----------------+--------------------
> 0/1A000060 | secured | 0/1A000000 | 0/1B42BC78
>
> -- slots that required WAL is still available but insecured
> restart_lsn | status | min_recure_lsn | pg_current_wal_lsn
> ------------+-----------+----------------+--------------------
> 0/1A000060 | insecured | 0/1C000000 | 0/1D76C948
>
> -- slots that required WAL is lost
> # We should have seen the log 'Some replication slots have lost...'
>
> restart_lsn | status | min_recure_lsn | pg_current_wal_lsn
> ------------+--------+----------------+--------------------
> 0/1A000060 | broken | 0/1C000000 | 0/1D76C9F0
>
>
> I noticed that I abandoned the segment fragment of
> max_slot_wal_keep_size in calculating in the routines. The
> current patch honors the frament part of max_slot_wal_keep_size.

I changed IsLsnStillAvailable to return meaningful values
regardless whether max_slot_wal_keep_size is set or not.

# I had been forgetting to count the version for latestst several
# patches. I give the version '4' - as the next of the last
# numbered patch.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Nov 9, 2017 at 5:31 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> # I had been forgetting to count the version for latestst several
> # patches. I give the version '4' - as the next of the last
> # numbered patch.

With all the changes that have happened in the documentation lately, I
suspect that this is going to need a rework.. Moved to next CF per
lack of reviews, with waiting on author as status.
--
Michael

At Thu, 30 Nov 2017 12:44:16 +0900, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote in <CAB7nPqS4bhSsDm_47GVjQno=iU6thx13MQVwwXXKBHQwfwwNCA(at)mail(dot)gmail(dot)com>
> On Thu, Nov 9, 2017 at 5:31 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > # I had been forgetting to count the version for latestst several
> > # patches. I give the version '4' - as the next of the last
> > # numbered patch.
>
> With all the changes that have happened in the documentation lately, I
> suspect that this is going to need a rework.. Moved to next CF per
> lack of reviews, with waiting on author as status.

I refactored this patch so that almost-same don't appear
twice. And added recovery TAP test for this.

New function GetMinSecuredSegment() calculates the segment number
considering wal_keep_segments and
max_slot_wal_keep_size. KeepLogSeg and IsLsnStillAvailable no
longer have the code block that should be in "sync".
I think the new code is far understandable than the previous one.

The new third patch contains a TAP test to check
max_slot_wal_keep_size and relevant stats view are surely
working.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello
I think limit wal in replication slots is useful in some cases. But first time i was confused with proposed terminology secured/insecured/broken/unknown state.

patch -p1 gives some "Stripping trailing CRs from patch" messages for me, but applied to current HEAD and builds. After little testing i understood the difference in secured/insecured/broken terminology. Secured means garantee to keep wal, insecure - wal may be deleted with next checkpoint, broken - wal already deleted.
I think, we may split "secure" to "streaming" and... hmm... "waiting"? "keeping"? - according active flag for clarify and readable "status" field.

regards, Sergei

Hello. Thank you for the comment.

(And sorry for the absense.)

At Fri, 22 Dec 2017 15:04:20 +0300, Sergei Kornilov <sk(at)zsrv(dot)org> wrote in <337571513944260(at)web55j(dot)yandex(dot)ru>
> Hello
> I think limit wal in replication slots is useful in some cases. But first time i was confused with proposed terminology secured/insecured/broken/unknown state.

I'm not confident on the terminology. Suggestions are welcome on
the wording that makes more sense.

> patch -p1 gives some "Stripping trailing CRs from patch"
> messages for me, but applied to current HEAD and builds. After

Hmm. I wonder why I get that complaint so often. (It's rather
common? or caused by the MIME format of my mail?) I'd say with
confidence that it is because you retrieved the patch file on
Windows mailer.

> little testing i understood the difference in
> secured/insecured/broken terminology. Secured means garantee to
> keep wal, insecure - wal may be deleted with next checkpoint,
> broken - wal already deleted.

Right. I'm sorry that I haven't written that clearly anywhere and
bothered you confirming that. I added documentation as the forth
patch.

> I think, we may split "secure" to "streaming"
> and... hmm... "waiting"? "keeping"? - according active flag for
> clarify and readable "status" field.

streaming / keeping and lost? (and unknown) Also "status" is
surely offers somewhat obscure meaning. wal_status (or
(wal_)availability) and min_keep_lsn maeke more sense?

The additional fields in pg_replication_slots have been changed
as the follows in the attached patch.

confirmed_flush_lsn:
+ wal_status : (streaming | keeping | lost | unknown)
+ min_keep_lsn : <The oldest LSN that is available in WAL files>

The changes of documentation are seen in the following html files.

doc/src/sgml/html/warm-standby.html#STREAMING-REPLICATION-SLOTS
doc/src/sgml/html/runtime-config-replication.html#GUC-MAX-SLOT-WAL-KEEP-SIZE
doc/src/sgml/html/view-pg-replication-slots.html

One annoyance is that the min_keep_lsn always has the same value
among all slots.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello

>> patch -p1 gives some "Stripping trailing CRs from patch"
>> messages for me, but applied to current HEAD and builds. After
>
> Hmm. I wonder why I get that complaint so often. (It's rather
> common? or caused by the MIME format of my mail?) I'd say with
> confidence that it is because you retrieved the patch file on
> Windows mailer.
I use Debian and web based mailer. Hm, i wget patches from links here /message-id/flat/20180111.155910.26212237.horiguchi.kyotaro%40lab.ntt.co.jp - applies clean both last and previous messages. Its strange.

Updated patches builds ok, but i found one failed test in make check-world: contrib/test_decoding/sql/ddl.sql at the end makes SELECT * FROM pg_replication_slots; which result of course was changed
And i still have no better ideas for naming. I think on something like
if (min_keep_lsn <= restart_lsn)
if (active_pid != 0)
status = "streaming";
else
status = "keeping";
else
status = "may_lost";
This duplicates an existing active field, but I think it's useful as slot status description.
wal_status streaming/keeping/lost/unknown as described in docs patch is also acceptable for me. Maybe anyone else has better idea?

Regards, Sergei

On 11 January 2018 at 09:55, Sergei Kornilov <sk(at)zsrv(dot)org> wrote:
> if (active_pid != 0)
> status = "streaming";
> else
> status = "keeping";

Perhaps "idle" by analogy to a pg_stat_activity entry for a backend
that's connected but not doing anything.

> status = "may_lost";

Perhaps "stale" or "expired"?

Is this patch in bike-shed territory? Are there any questions about
whether we want the basic shape to look like this?

Fwiw I think there's a real need for this feature so I would like to
get it in for Postgres 11.

--
greg

Hello,

At Thu, 11 Jan 2018 12:55:27 +0300, Sergei Kornilov <sk(at)zsrv(dot)org> wrote in <2798121515664527(at)web40g(dot)yandex(dot)ru>
> Hello
>
> >> patch -p1 gives some "Stripping trailing CRs from patch"
> >> messages for me, but applied to current HEAD and builds. After
> >
> > Hmm. I wonder why I get that complaint so often. (It's rather
> > common? or caused by the MIME format of my mail?) I'd say with
> > confidence that it is because you retrieved the patch file on
> > Windows mailer.
> I use Debian and web based mailer. Hm, i wget patches from links here /message-id/flat/20180111.155910.26212237.horiguchi.kyotaro%40lab.ntt.co.jp - applies clean both last and previous messages. Its strange.

Thanks for the information. The cause I suppose is that *I*
attached the files in *text* MIME type. I taught my mailer
application to use "Application/Octet-stream" instead and that
should make most (or all) people here happy.

> Updated patches builds ok, but i found one failed test in make check-world: contrib/test_decoding/sql/ddl.sql at the end makes SELECT * FROM pg_replication_slots; which result of course was changed

Mmm. Good catch. check-world (contribs) was out of my sight.
It is fixed locally.

> And i still have no better ideas for naming. I think on something like
> if (min_keep_lsn <= restart_lsn)
> if (active_pid != 0)
> status = "streaming";
> else
> status = "keeping";
> else
> status = "may_lost";
> This duplicates an existing active field, but I think it's useful as slot status description.
> wal_status streaming/keeping/lost/unknown as described in docs patch is also acceptable for me. Maybe anyone else has better idea?

I'll fix this after the discussion.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Thu, 11 Jan 2018 13:56:14 +0000, Greg Stark <stark(at)mit(dot)edu> wrote in <CAM-w4HOVYZkCbCdFt8N8zwAAcuETFimwOB_Db+jgFajn-iYHEQ(at)mail(dot)gmail(dot)com>
> On 11 January 2018 at 09:55, Sergei Kornilov <sk(at)zsrv(dot)org> wrote:
> > if (active_pid != 0)
> > status = "streaming";
> > else
> > status = "keeping";
>
> Perhaps "idle" by analogy to a pg_stat_activity entry for a backend
> that's connected but not doing anything.

The state "keeping" is "some segments that are needed by a slot
are still existing but to be removed by the next checkpoint". The
three states are alogogous to green/yellow/red in traffic
lights. "idle" doesn't feel right.

> > status = "may_lost";
>
> Perhaps "stale" or "expired"?

Some random thoughts on this topic:

Reading the field as "WAL record at restrat_lsn is/has been
$(status)", "expired" fits there. "safe"/"crtical"/("stale" and
"expired") would fit "restart_lsn is $(status)"?

If we merge the second sate to the red-side, a boolean column
with the names "wal_preserved" or "wal_available" might work. But
I believe the second state is crucial.

> Is this patch in bike-shed territory? Are there any questions about
> whether we want the basic shape to look like this?

FWIW the summary history of this patch follows.

- added monitoring feature,
- GUC in bytes not in segments,
- show the "min_keep_lsn" instead of "spare amount of avalable
WAL(distance)" (*1)
- changed the words to show the status. (still under discussion)
- added documentation.

I didn't adopt "setting per slot" since the keep amount is not
measured from slot's restart_lsn, but from checkpoint LSN.

*1: As I mentioned upthread, I think that at least the
"pg_replication_slots.min_keep_lsn" is arguable since it shows
the same value for all slots and I haven't found no other
appropriate place.

> Fwiw I think there's a real need for this feature so I would like to
> get it in for Postgres 11.

It encourages me a lot. Thanks.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jan 15, 2018 at 1:05 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> >> patch -p1 gives some "Stripping trailing CRs from patch"
>> >> messages for me, but applied to current HEAD and builds. After
>> >
>> > Hmm. I wonder why I get that complaint so often. (It's rather
>> > common? or caused by the MIME format of my mail?) I'd say with
>> > confidence that it is because you retrieved the patch file on
>> > Windows mailer.
>> I use Debian and web based mailer. Hm, i wget patches from links here /message-id/flat/20180111.155910.26212237.horiguchi.kyotaro%40lab.ntt.co.jp - applies clean both last and previous messages. Its strange.
>
> Thanks for the information. The cause I suppose is that *I*
> attached the files in *text* MIME type. I taught my mailer
> application to use "Application/Octet-stream" instead and that
> should make most (or all) people here happy.

Since the "Stripping trailing CRs from patch" message is totally
harmless, I'm not sure why you should need to devote any effort to
avoiding it. Anyone who gets it should just ignore it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Since the "Stripping trailing CRs from patch" message is totally
> harmless, I'm not sure why you should need to devote any effort to
> avoiding it. Anyone who gets it should just ignore it.

Not sure, but that might be another situation in which "patch"
works and "git apply" doesn't. (Feeling too lazy to test it...)

regards, tom lane

I'm digressing...

At Mon, 15 Jan 2018 21:45:34 -0500, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in <26718(dot)1516070734(at)sss(dot)pgh(dot)pa(dot)us>
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > Since the "Stripping trailing CRs from patch" message is totally
> > harmless, I'm not sure why you should need to devote any effort to
> > avoiding it. Anyone who gets it should just ignore it.

I know that and totally agree to Robert but still I wonder why
(and am annoyed by) I sometimes receive such complain or even an
accusation that I sent an out-of-the-convention patch and I was
afraid that it is not actually common.

For thie reason I roughly counted up CT/CTE's that people here is
using for patches in my mail box this time and got the following
numbers. (Counted on attachments with a name "*.patch/diff".)

Rank : Freq : CT/CTE
1: 3308: application/octet-stream:base64
2: 1642: text/x-patch;charset=us-ascii:base64
3: 1286: text/x-diff;charset=us-ascii:7bit
* 4: 997: text/x-patch;charset=us-ascii:7bit
5: 497: text/x-diff;charset=us-ascii:base64
6: 406: text/x-diff:quoted-printable
7: 403: text/plain;charset=us-ascii:7bit
8: 389: text/x-diff:base64
9: 321: application/x-gzip:base64
10: 281: text/plain;charset=us-ascii:base64
<snip>
Total: attachments=11461 / mails=158121

The most common setting is application/octet-stream:base64 but
text/x-patch;charset=us-ascii:7bit is also one of ... the majority?

I'm convinced that my original setting is not so problematic so I
reverted it.

> Not sure, but that might be another situation in which "patch"
> works and "git apply" doesn't. (Feeling too lazy to test it...)

I was also afraid of that as I wrote upthread but it seems also a
needless fear.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Jan 11, 2018 at 7:59 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> [new patch set]

FYI this is still broken:

test ddl ... FAILED

You could see that like this:

cd contrib/test_decoding
make check

--
Thomas Munro
http://www.enterprisedb.com

Thank you for kindly noticing me of that.

At Mon, 29 Jan 2018 11:07:31 +1300, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> wrote in <CAEepm=3nOUqNWyKQ83StGUeCB9LUsTw66w=Sy6H+xKfSbcRu3Q(at)mail(dot)gmail(dot)com>
> On Thu, Jan 11, 2018 at 7:59 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > [new patch set]
>
> FYI this is still broken:
>
> test ddl ... FAILED
>
> You could see that like this:
>
> cd contrib/test_decoding
> make check

I guess I might somehow have sent a working version of 0002.
While rechecking the patch, I fixed the message issued on losing
segments in 0001, revised the TAP test since I found that it was
unstable.

The attached files are the correct version of the latest patch.

Thanks.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello,

At Mon, 29 Jan 2018 19:26:34 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20180129(dot)192634(dot)217484965(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> While rechecking the patch, I fixed the message issued on losing
> segments in 0001, revised the TAP test since I found that it was
> unstable.
>
> The attached files are the correct version of the latest patch.

The name of the new function GetMinKeepSegment seems giving wrong
meaning. I renamed it to GetOlestKeepSegment.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Mon, 29 Jan 2018 19:40:23 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20180129(dot)194023(dot)228030941(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> Hello,
>
> At Mon, 29 Jan 2018 19:26:34 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20180129(dot)192634(dot)217484965(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > While rechecking the patch, I fixed the message issued on losing
> > segments in 0001, revised the TAP test since I found that it was
> > unstable.
> >
> > The attached files are the correct version of the latest patch.
>
> The name of the new function GetMinKeepSegment seems giving wrong
> meaning. I renamed it to GetOlestKeepSegment.

I found that fd1a421fe6 and df411e7c66 hit this . Rebased to the
current HEAD.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello. This is the reabased version of slot-limit feature.

This patch limits maximum WAL segments to be kept by replication
slots. Replication slot is useful to avoid desync with replicas
after temporary disconnection but it is dangerous when some of
replicas are lost. The WAL space can be exhausted and server can
PANIC in the worst case. This can prevent the worst case having a
benefit from replication slots using a new GUC variable
max_slot_wal_keep_size.

This is a feature mentioned in the documentation.

/docs/current/static/warm-standby.html#STREAMING-REPLICATION-SLOTS

> In lieu of using replication slots, it is possible to prevent the
> removal of old WAL segments using wal_keep_segments, or by
> storing the segments in an archive using
> archive_command. However, these methods often result in retaining
> more WAL segments than required, whereas replication slots retain
> only the number of segments known to be needed. An advantage of
> these methods is that they bound the space requirement for
> pg_wal; there is currently no way to do this using replication
> slots.

The previous patche files doesn't have version number so I let
the attached latest version be v2.

v2-0001-Add-WAL-releaf-vent-for-replication-slots.patch
The body of the limiting feature

v2-0002-Add-monitoring-aid-for-max_replication_slots.patch
Shows the status of WAL rataining in pg_replication_slot view

v2-0003-TAP-test-for-the-slot-limit-feature.patch
TAP test for this feature

v2-0004-Documentation-for-slot-limit-feature.patch
Documentation, as the name.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello.

At Tue, 26 Jun 2018 16:26:59 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20180626(dot)162659(dot)223208514(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> The previous patche files doesn't have version number so I let
> the attached latest version be v2.
>
>
> v2-0001-Add-WAL-releaf-vent-for-replication-slots.patch
> The body of the limiting feature
>
> v2-0002-Add-monitoring-aid-for-max_replication_slots.patch
> Shows the status of WAL rataining in pg_replication_slot view
>
> v2-0003-TAP-test-for-the-slot-limit-feature.patch
> TAP test for this feature
>
> v2-0004-Documentation-for-slot-limit-feature.patch
> Documentation, as the name.

Travis (test_decoding test) showed that GetOldestXLogFileSegNo
added by 0002 forgets to close temporarily opened pg_wal
directory. This is the fixed version v3.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Jul 4, 2018 at 5:28 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello.
>
> At Tue, 26 Jun 2018 16:26:59 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20180626(dot)162659(dot)223208514(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
>> The previous patche files doesn't have version number so I let
>> the attached latest version be v2.
>>
>>
>> v2-0001-Add-WAL-releaf-vent-for-replication-slots.patch
>> The body of the limiting feature
>>
>> v2-0002-Add-monitoring-aid-for-max_replication_slots.patch
>> Shows the status of WAL rataining in pg_replication_slot view
>>
>> v2-0003-TAP-test-for-the-slot-limit-feature.patch
>> TAP test for this feature
>>
>> v2-0004-Documentation-for-slot-limit-feature.patch
>> Documentation, as the name.
>
> Travis (test_decoding test) showed that GetOldestXLogFileSegNo
> added by 0002 forgets to close temporarily opened pg_wal
> directory. This is the fixed version v3.
>

Thank you for updating the patch! I looked at v3 patches. Here is
review comments.

---
+ {"max_slot_wal_keep_size", PGC_SIGHUP, REPLICATION_SENDING,
+ gettext_noop("Sets the maximum size of extra
WALs kept by replication slots."),
+ NULL,
+ GUC_UNIT_MB
+ },
+ &max_slot_wal_keep_size_mb,
+ 0, 0, INT_MAX,
+ NULL, NULL, NULL
+ },

Maybe MAX_KILOBYTES would be better instead of INT_MAX like wal_max_size.

---
Once the following WARNING emitted this message is emitted whenever we
execute CHECKPOINT even if we don't change anything. Is that expected
behavior? I think it would be better to emit this message only when we
remove WAL segements that are required by slots.

WARNING: some replication slots have lost required WAL segments
DETAIL: The mostly affected slot has lost 153 segments.

---
+ Assert(wal_keep_segments >= 0);
+ Assert(max_slot_wal_keep_size_mb >= 0);

These assertions are not meaningful because these parameters are
ensured >= 0 by those definition.

---
+ /* slots aren't useful, consider only wal_keep_segments */
+ if (slotpos == InvalidXLogRecPtr)
+ {

Perhaps XLogRecPtrIsInvalid(slotpos) is better.

---
+ if (slotpos != InvalidXLogRecPtr && currSeg <= slotSeg + wal_keep_segments)
+ slotpos = InvalidXLogRecPtr;
+
+ /* slots aren't useful, consider only wal_keep_segments */
+ if (slotpos == InvalidXLogRecPtr)
+ {

This logic is hard to read to me. The slotpos can be any of: valid,
valid but then become invalid in halfway or invalid from beginning of
this function. Can we convert this logic to following?

if (XLogRecPtrIsInvalid(slotpos) ||
currSeg <= slotSeg + wal_keep_segments)

---
+ keepSegs = wal_keep_segments +
+ ConvertToXSegs(max_slot_wal_keep_size_mb, wal_segment_size);

Why do we need to keep (wal_keep_segment + max_slot_wal_keep_size) WAL
segments? I think what this feature does is, if wal_keep_segments is
not useful (that is, slotSeg < (currSeg - wal_keep_segment) then we
normally choose slotSeg as lower boundary but max_slot_wal_keep_size
restrict the lower boundary so that it doesn't get lower than the
threshold. So I thought what this function should do is to calculate
min(currSeg - wal_keep_segment, max(currSeg - max_slot_wal_keep_size,
slotSeg)), I might be missing something though.

---
+ SpinLockAcquire(&XLogCtl->info_lck);
+ oldestSeg = XLogCtl->lastRemovedSegNo;
+ SpinLockRelease(&XLogCtl->info_lck);

We can use XLogGetLastRemovedSegno() instead.

---
+ xldir = AllocateDir(XLOGDIR);
+ if (xldir == NULL)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not open write-ahead log directory \"%s\": %m",
+ XLOGDIR)));

Looking at other code allocating a directory we don't check xldir ==
NULL because it will be detected by ReadDir() function and raise an
error in that function. So maybe we don't need to check it just after
allocation.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Hello. Sawada-san.

Thank you for the comments.

At Thu, 5 Jul 2018 15:43:56 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoDiiA4qHj0thqw80Bt=vefSQ9yGpZnr0kuLTXszbrV9iQ(at)mail(dot)gmail(dot)com>
> On Wed, Jul 4, 2018 at 5:28 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > Hello.
> >
> > At Tue, 26 Jun 2018 16:26:59 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20180626(dot)162659(dot)223208514(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> >> The previous patche files doesn't have version number so I let
> >> the attached latest version be v2.
> >>
> >>
> >> v2-0001-Add-WAL-releaf-vent-for-replication-slots.patch
> >> The body of the limiting feature
> >>
> >> v2-0002-Add-monitoring-aid-for-max_replication_slots.patch
> >> Shows the status of WAL rataining in pg_replication_slot view
> >>
> >> v2-0003-TAP-test-for-the-slot-limit-feature.patch
> >> TAP test for this feature
> >>
> >> v2-0004-Documentation-for-slot-limit-feature.patch
> >> Documentation, as the name.
> >
> > Travis (test_decoding test) showed that GetOldestXLogFileSegNo
> > added by 0002 forgets to close temporarily opened pg_wal
> > directory. This is the fixed version v3.
> >
>
> Thank you for updating the patch! I looked at v3 patches. Here is
> review comments.
>
> ---
> + {"max_slot_wal_keep_size", PGC_SIGHUP, REPLICATION_SENDING,
> + gettext_noop("Sets the maximum size of extra
> WALs kept by replication slots."),
> + NULL,
> + GUC_UNIT_MB
> + },
> + &max_slot_wal_keep_size_mb,
> + 0, 0, INT_MAX,
> + NULL, NULL, NULL
> + },
>
> Maybe MAX_KILOBYTES would be better instead of INT_MAX like wal_max_size.

The MAX_KILOBYTES is maximum value of size in kB, which fits long
or Size/size_t variables after convreted into bytes. Applying the
limit there means that we assume that the _mb variable can be
converted not into bytes but kB. So applying it to
max/min_wal_size seems somewhat wrong but doesn't harm since they
are not acutually converted into bytes.

max_slot_wal_keep_size is not converted into bytes so capping
with INT_MAX is no problem. However it doesn't need to be larger
than MAX_KILOBYTES, I follow that in order to make it same with
max/min_wal_size.

> ---
> Once the following WARNING emitted this message is emitted whenever we
> execute CHECKPOINT even if we don't change anything. Is that expected
> behavior? I think it would be better to emit this message only when we
> remove WAL segements that are required by slots.
>
> WARNING: some replication slots have lost required WAL segments
> DETAIL: The mostly affected slot has lost 153 segments.

I didn't consider the situation the number of lost segments
doesn't change. Changed to mute the message when the number of
lost segments is not changed.

> ---
> + Assert(wal_keep_segments >= 0);
> + Assert(max_slot_wal_keep_size_mb >= 0);
>
> These assertions are not meaningful because these parameters are
> ensured >= 0 by those definition.

Yeah, that looks a bit being paranoid. Removed.

> ---
> + /* slots aren't useful, consider only wal_keep_segments */
> + if (slotpos == InvalidXLogRecPtr)
> + {
>
> Perhaps XLogRecPtrIsInvalid(slotpos) is better.

Agreed. It is changed to "slotpos != InvalidXLogRecPtr" after
changing the function by the comments below. I think that the
double negation !XLogRecPtrInvalid() is not fine.

> ---
> + if (slotpos != InvalidXLogRecPtr && currSeg <= slotSeg + wal_keep_segments)
> + slotpos = InvalidXLogRecPtr;
> +
> + /* slots aren't useful, consider only wal_keep_segments */
> + if (slotpos == InvalidXLogRecPtr)
> + {
>
> This logic is hard to read to me. The slotpos can be any of: valid,
> valid but then become invalid in halfway or invalid from beginning of
> this function. Can we convert this logic to following?
>
> if (XLogRecPtrIsInvalid(slotpos) ||
> currSeg <= slotSeg + wal_keep_segments)

Right. But it is removed.

> ---
> + keepSegs = wal_keep_segments +
> + ConvertToXSegs(max_slot_wal_keep_size_mb, wal_segment_size);
>
> Why do we need to keep (wal_keep_segment + max_slot_wal_keep_size) WAL
> segments? I think what this feature does is, if wal_keep_segments is
> not useful (that is, slotSeg < (currSeg - wal_keep_segment) then we
> normally choose slotSeg as lower boundary but max_slot_wal_keep_size
> restrict the lower boundary so that it doesn't get lower than the
> threshold. So I thought what this function should do is to calculate
> min(currSeg - wal_keep_segment, max(currSeg - max_slot_wal_keep_size,
> slotSeg)), I might be missing something though.

You're right that wal_keep_segments should not been added, but
should give lower limit of segments to keep as the current
KeepLogSeg() does. Fixed that.

Since the amount is specified in mega bytes, silently rounding
down to segment bounds may not be proper in general and this
feature used to use the fragments to show users something. But
there's no loner a place where the fragments are perceptible to
users and anyway the fragments are way smaller than the expected
total WAL size.

As the result, I removed the fragment calculation at all as you
suggested. It gets way smaller and simpler.

> ---
> + SpinLockAcquire(&XLogCtl->info_lck);
> + oldestSeg = XLogCtl->lastRemovedSegNo;
> + SpinLockRelease(&XLogCtl->info_lck);
>
> We can use XLogGetLastRemovedSegno() instead.

It is because I thought that it is for external usage,
spcifically by slot.c since CheckXLogRemoved() is reading it
directly. I leave it alone and they would have to be fixed at
once if we decide to use it internally.

> ---
> + xldir = AllocateDir(XLOGDIR);
> + if (xldir == NULL)
> + ereport(ERROR,
> + (errcode_for_file_access(),
> + errmsg("could not open write-ahead log directory \"%s\": %m",
> + XLOGDIR)));
>
> Looking at other code allocating a directory we don't check xldir ==
> NULL because it will be detected by ReadDir() function and raise an
> error in that function. So maybe we don't need to check it just after
> allocation.

Thanks. I found that in the comment of ReadDir(). This doesn't
need a special error handling so I leave it to ReadDir there.

Addition to that, documentation is fixed.

Attached is the v4 files.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jul 9, 2018 at 2:47 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello. Sawada-san.
>
> Thank you for the comments.
>

Thank you for updating the patch!

> At Thu, 5 Jul 2018 15:43:56 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoDiiA4qHj0thqw80Bt=vefSQ9yGpZnr0kuLTXszbrV9iQ(at)mail(dot)gmail(dot)com>
>> On Wed, Jul 4, 2018 at 5:28 PM, Kyotaro HORIGUCHI
>> ---
>> + SpinLockAcquire(&XLogCtl->info_lck);
>> + oldestSeg = XLogCtl->lastRemovedSegNo;
>> + SpinLockRelease(&XLogCtl->info_lck);
>>
>> We can use XLogGetLastRemovedSegno() instead.
>
> It is because I thought that it is for external usage,
> spcifically by slot.c since CheckXLogRemoved() is reading it
> directly. I leave it alone and they would have to be fixed at
> once if we decide to use it internally.

Agreed. I noticed that after commented.

Here is review comments of v4 patches.

+ if (minKeepLSN)
+ {
+ XLogRecPtr slotPtr = XLogGetReplicationSlotMinimumLSN();
+ Assert(!XLogRecPtrIsInvalid(slotPtr));
+
+ tailSeg = GetOldestKeepSegment(currpos, slotPtr);
+
+ XLogSegNoOffsetToRecPtr(tailSeg, 0, *minKeepLSN,
wal_segment_size);
+ }

The usage of XLogSegNoOffsetToRecPtr is wrong. Since we specify the
destination at 4th argument the wal_segment_size will be changed in
the above expression. The regression tests by PostgreSQL Patch Tester
seem passed but I got the following assertion failure in
recovery/t/010_logical_decoding_timelines.pl

TRAP: FailedAssertion("!(XLogRecPtrToBytePos(*StartPos) ==
startbytepos)", File: "xlog.c", Line: 1277)
----
+ XLByteToSeg(restartLSN, restartSeg, wal_segment_size);
+
+
+ if (minKeepLSN)

There is an extra empty line.

----
+ /* but, keep larger than wal_segment_size if any*/
+ if (wal_keep_segments > 0 && keepSegs < wal_keep_segments)
+ keepSegs = wal_keep_segments;

You meant wal_keep_segments in the above comment rather than
wal_segment_size? Also, the above comment need a whitespace just after
"any".

----
+bool
+IsLsnStillAvaiable(XLogRecPtr restartLSN, XLogRecPtr *minKeepLSN)
+{

I think restartLSN is a word used for replication slots. Since this
function is defined in xlog.c it would be better to change the
argument name to more generic name, for example recptr.

----
+ /*
+ * Calcualte keep segments by slots first. The second term of the
+ * condition is just a sanity check.
+ */

s/calcualte/calculate/

----
+ /* get minimum segment ignorig timeline ID */

s/ignorig/ignoring/

----
min_keep_lsn in pg_replication_slots currently shows the same value in
every slots but I felt that the value seems not easy to understand
intuitively for users because users will have to confirm that value
and to compare the current LSN in order to check if replication slots
will become the "lost" status. So how about showing values that
indicate how far away from the point where we become "lost" for
individual slots?

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Hello.

At Wed, 11 Jul 2018 15:09:23 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoCFtW6+SN_eVTszDAjQeeU2sSea2VpCEx08ejNafk8H9w(at)mail(dot)gmail(dot)com>
> On Mon, Jul 9, 2018 at 2:47 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
..
> Here is review comments of v4 patches.
>
> + if (minKeepLSN)
> + {
> + XLogRecPtr slotPtr = XLogGetReplicationSlotMinimumLSN();
> + Assert(!XLogRecPtrIsInvalid(slotPtr));
> +
> + tailSeg = GetOldestKeepSegment(currpos, slotPtr);
> +
> + XLogSegNoOffsetToRecPtr(tailSeg, 0, *minKeepLSN,
> wal_segment_size);
> + }
>
> The usage of XLogSegNoOffsetToRecPtr is wrong. Since we specify the
> destination at 4th argument the wal_segment_size will be changed in
> the above expression. The regression tests by PostgreSQL Patch Tester

I'm not sure I get this correctly, the definition of the macro is
as follows.

| #define XLogSegNoOffsetToRecPtr(segno, offset, dest, wal_segsz_bytes) \
| (dest) = (segno) * (wal_segsz_bytes) + (offset)

The destination is the *3rd* parameter and the forth is segment
size which is not to be written.

> seem passed but I got the following assertion failure in
> recovery/t/010_logical_decoding_timelines.pl
>
> TRAP: FailedAssertion("!(XLogRecPtrToBytePos(*StartPos) ==
> startbytepos)", File: "xlog.c", Line: 1277)

Hmm. I don't see a relation with this patch, but how did you
cause the failure? The failure means inconsistency between
existing XLogBytePosToRecPtr and XLogRecPtrToBytePos, which
doesn't seem to happen without modifying the two functions.

> ----
> + XLByteToSeg(restartLSN, restartSeg, wal_segment_size);
> +
> +
> + if (minKeepLSN)
> There is an extra empty line.
>
> ----
> + /* but, keep larger than wal_segment_size if any*/
> + if (wal_keep_segments > 0 && keepSegs < wal_keep_segments)
> + keepSegs = wal_keep_segments;
>
> You meant wal_keep_segments in the above comment rather than
> wal_segment_size? Also, the above comment need a whitespace just after
> "any".

Ouch! You're right. Fixed.

> ----
> +bool
> +IsLsnStillAvaiable(XLogRecPtr restartLSN, XLogRecPtr *minKeepLSN)
> +{
>
> I think restartLSN is a word used for replication slots. Since this
> function is defined in xlog.c it would be better to change the
> argument name to more generic name, for example recptr.

Agreed. I used "target" instead.

> ----
> + /*
> + * Calcualte keep segments by slots first. The second term of the
> + * condition is just a sanity check.
> + */
>
> s/calcualte/calculate/

Fixed.

> ----
> + /* get minimum segment ignorig timeline ID */
>
> s/ignorig/ignoring/

Fixed.

# One of my fingers is literally fatter with bandaid than usual..

> ----
> min_keep_lsn in pg_replication_slots currently shows the same value in
> every slots but I felt that the value seems not easy to understand
> intuitively for users because users will have to confirm that value
> and to compare the current LSN in order to check if replication slots
> will become the "lost" status. So how about showing values that
> indicate how far away from the point where we become "lost" for
> individual slots?

Yeah, that is what I did in the first cut of this patch from the
same thought. pg_replication_slots have two additional columns
"live" and "distance".

/message-id/20171031.184310.182012625.horiguchi.kyotaro@lab.ntt.co.jp

Thre current design is changed following a comment.

/message-id/20171108.131431.170534842.horiguchi.kyotaro%40lab.ntt.co.jp

> > I don't think 'distance' is a good metric - that's going to continually
> > change. Why not store the LSN that's available and provide a function
> > that computes this? Or just rely on the lsn - lsn operator?
>
> It seems reasonable.,The 'secured minimum LSN' is common among
> all slots so showing it in the view may look a bit stupid but I
> don't find another suitable place for it. distance = 0 meant the
> state that the slot is living but insecured in the previous patch
> and that information is lost by changing 'distance' to
> 'min_secure_lsn'.

As I reconsidered this, I noticed that "lsn - lsn" doesn't make
sense here. The correct formula for the value is
"max_slot_wal_keep_size * 1024 * 1024 - ((oldest LSN to keep) -
restart_lsn). It is not a simple formula to write by hand but
doesn't seem general enough. I re-changed my mind to show the
"distance" there again.

pg_replication_slots now has the column "remain" instaed of
"min_keep_lsn", which shows an LSN when wal_status is "streaming"
and otherwise "0/0". In a special case, "remain" can be "0/0"
while "wal_status" is "streaming". It is the reason for the
tristate return value of IsLsnStillAvaialbe().

wal_status | remain
streaming | 0/19E3C0 -- WAL is reserved
streaming | 0/0 -- Still reserved but on the boundary
keeping | 0/0 -- About to be lost.
lost | 0/0 -- Lost.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Jul 13, 2018 at 5:40 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello.
>
> At Wed, 11 Jul 2018 15:09:23 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoCFtW6+SN_eVTszDAjQeeU2sSea2VpCEx08ejNafk8H9w(at)mail(dot)gmail(dot)com>
>> On Mon, Jul 9, 2018 at 2:47 PM, Kyotaro HORIGUCHI
>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> ..
>> Here is review comments of v4 patches.
>>
>> + if (minKeepLSN)
>> + {
>> + XLogRecPtr slotPtr = XLogGetReplicationSlotMinimumLSN();
>> + Assert(!XLogRecPtrIsInvalid(slotPtr));
>> +
>> + tailSeg = GetOldestKeepSegment(currpos, slotPtr);
>> +
>> + XLogSegNoOffsetToRecPtr(tailSeg, 0, *minKeepLSN,
>> wal_segment_size);
>> + }
>>
>> The usage of XLogSegNoOffsetToRecPtr is wrong. Since we specify the
>> destination at 4th argument the wal_segment_size will be changed in
>> the above expression. The regression tests by PostgreSQL Patch Tester
>
> I'm not sure I get this correctly, the definition of the macro is
> as follows.
>
> | #define XLogSegNoOffsetToRecPtr(segno, offset, dest, wal_segsz_bytes) \
> | (dest) = (segno) * (wal_segsz_bytes) + (offset)
>
> The destination is the *3rd* parameter and the forth is segment
> size which is not to be written.

Please see commit a22445ff0b which flipped input and output arguments.
So maybe you need to rebase the patches to current HEAD.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Hello.

At Tue, 17 Jul 2018 13:37:59 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoCAdDfXNwVhoAKhBtpmrY-0tfQoQh2NiTX_Ji15msNPew(at)mail(dot)gmail(dot)com>
> >> The usage of XLogSegNoOffsetToRecPtr is wrong. Since we specify the
> >> destination at 4th argument the wal_segment_size will be changed in
> >> the above expression. The regression tests by PostgreSQL Patch Tester
> >
> > I'm not sure I get this correctly, the definition of the macro is
> > as follows.
> >
> > | #define XLogSegNoOffsetToRecPtr(segno, offset, dest, wal_segsz_bytes) \
> > | (dest) = (segno) * (wal_segsz_bytes) + (offset)
> >
> > The destination is the *3rd* parameter and the forth is segment
> > size which is not to be written.
>
> Please see commit a22445ff0b which flipped input and output arguments.
> So maybe you need to rebase the patches to current HEAD.

Mmm. Thanks. I never thought such change happned but it is
accidentially took away in the latest patch.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Jul 13, 2018 at 5:40 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello.
>
> At Wed, 11 Jul 2018 15:09:23 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoCFtW6+SN_eVTszDAjQeeU2sSea2VpCEx08ejNafk8H9w(at)mail(dot)gmail(dot)com>
>> On Mon, Jul 9, 2018 at 2:47 PM, Kyotaro HORIGUCHI
>> ----
>> min_keep_lsn in pg_replication_slots currently shows the same value in
>> every slots but I felt that the value seems not easy to understand
>> intuitively for users because users will have to confirm that value
>> and to compare the current LSN in order to check if replication slots
>> will become the "lost" status. So how about showing values that
>> indicate how far away from the point where we become "lost" for
>> individual slots?
>
> Yeah, that is what I did in the first cut of this patch from the
> same thought. pg_replication_slots have two additional columns
> "live" and "distance".
>
> /message-id/20171031.184310.182012625.horiguchi.kyotaro@lab.ntt.co.jp
>
> Thre current design is changed following a comment.
>
> /message-id/20171108.131431.170534842.horiguchi.kyotaro%40lab.ntt.co.jp
>
>> > I don't think 'distance' is a good metric - that's going to continually
>> > change. Why not store the LSN that's available and provide a function
>> > that computes this? Or just rely on the lsn - lsn operator?
>>
>> It seems reasonable.,The 'secured minimum LSN' is common among
>> all slots so showing it in the view may look a bit stupid but I
>> don't find another suitable place for it. distance = 0 meant the
>> state that the slot is living but insecured in the previous patch
>> and that information is lost by changing 'distance' to
>> 'min_secure_lsn'.
>
> As I reconsidered this, I noticed that "lsn - lsn" doesn't make
> sense here. The correct formula for the value is
> "max_slot_wal_keep_size * 1024 * 1024 - ((oldest LSN to keep) -
> restart_lsn). It is not a simple formula to write by hand but
> doesn't seem general enough. I re-changed my mind to show the
> "distance" there again.
>
> pg_replication_slots now has the column "remain" instaed of
> "min_keep_lsn", which shows an LSN when wal_status is "streaming"
> and otherwise "0/0". In a special case, "remain" can be "0/0"
> while "wal_status" is "streaming". It is the reason for the
> tristate return value of IsLsnStillAvaialbe().
>
> wal_status | remain
> streaming | 0/19E3C0 -- WAL is reserved
> streaming | 0/0 -- Still reserved but on the boundary
> keeping | 0/0 -- About to be lost.
> lost | 0/0 -- Lost.
>

The "remain" column still shows same value at all rows as follows
because you always compare between the current LSN and the minimum LSN
of replication slots. Is that you expected? My comment was to show the
distance from the restart_lsn of individual slots to the critical
point where it will lost WAL. That way, we can easily find out which
slots is about to get lost.

postgres(1:126712)=# select pg_current_wal_lsn(), slot_name,
restart_lsn, remain from pg_replication_slots ;
pg_current_wal_lsn | slot_name | restart_lsn | remain
--------------------+-----------+-------------+------------
0/4000108 | 5 | 0/1645CA0 | 0/3DFFFEF8
0/4000108 | 4 | 0/40000D0 | 0/3DFFFEF8
(2 rows)

Also, I'm not sure it's a good way to show the distance as LSN. LSN is
a monotone increasing value but in your patch, a value of the "remain"
column can get decreased. As an alternative way I'd suggest to show it
as the number of segments. Attached patch is a patch for your v5 patch
that changes it so that the column shows how many WAL segments of
individual slots are remained until they get lost WAL.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, Jul 20, 2018 at 10:13:58AM +0900, Masahiko Sawada wrote:
> Also, I'm not sure it's a good way to show the distance as LSN. LSN is
> a monotone increasing value but in your patch, a value of the "remain"
> column can get decreased.

If that can happen, I think that this is a very, very bad idea. A
couple of code paths, including segment recycling and the new WAL
advancing rely on such monotonic properties. That would be also very
confusing for any monitoring job looking at pg_replication_slots.
--
Michael

Hello.

At Fri, 20 Jul 2018 10:13:58 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoDayePWwu4t=VPP5P1QFDSBvks1d8j76bXp5rbXoPbZcA(at)mail(dot)gmail(dot)com>
> > As I reconsidered this, I noticed that "lsn - lsn" doesn't make
> > sense here. The correct formula for the value is
> > "max_slot_wal_keep_size * 1024 * 1024 - ((oldest LSN to keep) -
> > restart_lsn). It is not a simple formula to write by hand but
> > doesn't seem general enough. I re-changed my mind to show the
> > "distance" there again.
> >
> > pg_replication_slots now has the column "remain" instaed of
> > "min_keep_lsn", which shows an LSN when wal_status is "streaming"
> > and otherwise "0/0". In a special case, "remain" can be "0/0"
> > while "wal_status" is "streaming". It is the reason for the
> > tristate return value of IsLsnStillAvaialbe().
> >
> > wal_status | remain
> > streaming | 0/19E3C0 -- WAL is reserved
> > streaming | 0/0 -- Still reserved but on the boundary
> > keeping | 0/0 -- About to be lost.
> > lost | 0/0 -- Lost.
> >
>
> The "remain" column still shows same value at all rows as follows
> because you always compare between the current LSN and the minimum LSN
> of replication slots. Is that you expected? My comment was to show the

Ouch! Sorry for the silly mistake. GetOldestKeepSegment should
calculate restBytes based on the distance from the cutoff LSN to
restart_lsn, not to minSlotLSN. The attached fixed v6 correctly
shows the distance individually.

> Also, I'm not sure it's a good way to show the distance as LSN. LSN is
> a monotone increasing value but in your patch, a value of the "remain"
> column can get decreased. As an alternative way I'd suggest to show it

The LSN of WAL won't be decreased but an LSN is just a position
in a WAL stream. Since the representation of LSN is composed of
the two components 'file number' and 'offset', it's quite natural
to show the difference in the same unit. The distance between the
points at "6m" and "10m" is "4m".

> as the number of segments. Attached patch is a patch for your v5 patch
> that changes it so that the column shows how many WAL segments of
> individual slots are remained until they get lost WAL.

Segment size varies by configuration, so segment number is not
intuitive to show distance. I think it is the most significant
reason we move to "bytes" from "segments" about WAL sizings like
max_wal_size. More than anything, it's too coarse. The required
segments may be lasts for the time to consume a whole segment or
may be removed just after. We could calculate the fragment bytes
but it requires some internal knowledge.

Instead, I made the field be shown in flat "bytes" using bigint,
which can be nicely shown using pg_size_pretty;

=# select pg_current_wal_lsn(), restart_lsn, wal_status, pg_size_pretty(remain) as remain from pg_replication_slots ;
pg_current_wal_lsn | restart_lsn | wal_status | remain
--------------------+-------------+------------+--------
0/DD3B188 | 0/CADD618 | streaming | 19 MB
0/DD3B188 | 0/DD3B188 | streaming | 35 MB

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jul 23, 2018 at 4:16 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello.
>
> At Fri, 20 Jul 2018 10:13:58 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoDayePWwu4t=VPP5P1QFDSBvks1d8j76bXp5rbXoPbZcA(at)mail(dot)gmail(dot)com>
>> > As I reconsidered this, I noticed that "lsn - lsn" doesn't make
>> > sense here. The correct formula for the value is
>> > "max_slot_wal_keep_size * 1024 * 1024 - ((oldest LSN to keep) -
>> > restart_lsn). It is not a simple formula to write by hand but
>> > doesn't seem general enough. I re-changed my mind to show the
>> > "distance" there again.
>> >
>> > pg_replication_slots now has the column "remain" instaed of
>> > "min_keep_lsn", which shows an LSN when wal_status is "streaming"
>> > and otherwise "0/0". In a special case, "remain" can be "0/0"
>> > while "wal_status" is "streaming". It is the reason for the
>> > tristate return value of IsLsnStillAvaialbe().
>> >
>> > wal_status | remain
>> > streaming | 0/19E3C0 -- WAL is reserved
>> > streaming | 0/0 -- Still reserved but on the boundary
>> > keeping | 0/0 -- About to be lost.
>> > lost | 0/0 -- Lost.
>> >
>>
>> The "remain" column still shows same value at all rows as follows
>> because you always compare between the current LSN and the minimum LSN
>> of replication slots. Is that you expected? My comment was to show the
>
> Ouch! Sorry for the silly mistake. GetOldestKeepSegment should
> calculate restBytes based on the distance from the cutoff LSN to
> restart_lsn, not to minSlotLSN. The attached fixed v6 correctly
> shows the distance individually.
>
>> Also, I'm not sure it's a good way to show the distance as LSN. LSN is
>> a monotone increasing value but in your patch, a value of the "remain"
>> column can get decreased. As an alternative way I'd suggest to show it
>
> The LSN of WAL won't be decreased but an LSN is just a position
> in a WAL stream. Since the representation of LSN is composed of
> the two components 'file number' and 'offset', it's quite natural
> to show the difference in the same unit. The distance between the
> points at "6m" and "10m" is "4m".
>
>> as the number of segments. Attached patch is a patch for your v5 patch
>> that changes it so that the column shows how many WAL segments of
>> individual slots are remained until they get lost WAL.
>
> Segment size varies by configuration, so segment number is not
> intuitive to show distance. I think it is the most significant
> reason we move to "bytes" from "segments" about WAL sizings like
> max_wal_size. More than anything, it's too coarse. The required
> segments may be lasts for the time to consume a whole segment or
> may be removed just after. We could calculate the fragment bytes
> but it requires some internal knowledge.
>
> Instead, I made the field be shown in flat "bytes" using bigint,
> which can be nicely shown using pg_size_pretty;

Thank you for updating. I agree showing the remain in bytes.

Here is review comments for v6 patch.

@@ -967,9 +969,9 @@ postgres=# SELECT * FROM
pg_create_physical_replication_slot('node_a_slot');
node_a_slot |

postgres=# SELECT * FROM pg_replication_slots;
- slot_name | slot_type | datoid | database | active | xmin |
restart_lsn | confirmed_flush_lsn
--------------+-----------+--------+----------+--------+------+-------------+---------------------
- node_a_slot | physical | | | f | | |
+ slot_name | slot_type | datoid | database | active | xmin |
restart_lsn | confirmed_flush_lsn | wal_status | min_keep_lsn
+-------------+-----------+--------+----------+--------+------+-------------+---------------------+------------+--------------
+ node_a_slot | physical | | | f | |
| | unknown | 0/1000000

This funk should be updated.

-----
+/*
+ * Returns minimum segment number the next checktpoint must leave considering
+ * wal_keep_segments, replication slots and max_slot_wal_keep_size.
+ *
+ * If resetBytes is not NULL, returns remaining LSN bytes to advance until any
+ * slot loses reserving a WAL record.
+ */
+static XLogSegNo
+GetOldestKeepSegment(XLogRecPtr currLSN, XLogRecPtr minSlotLSN,
XLogRecPtr restartLSN, uint64 *restBytes)
+{

You're assuming that the minSlotLSN is the minimum LSN of replication
slots but it's not mentioned anywhere. Since you check minSlotSeg <=
currSeg but not force it, if a caller sets a wrong value to minSlotLSN
this function will return a wrong value with no complaints. Similarly
there is not explanation about the resetartLSN, so you can add it. I'm
not sure the augment name restartLSN is suitable for the function in
xlog.c but I'd defer it to committers.

Since this function assumes that both restartLSN and *restBytes are
valid or invalid (and NULL) it's better to add assertions for safety.
The current code accepts even the case where only either argment is
valid.

-----
+ if (limitSegs > 0 && currSeg <= restartSeg + limitSegs)
+ {
+ /*
+ * This slot still has all required segments.
Calculate how many
+ * LSN bytes the slot has until it loses restart_lsn.
+ */
+ fragbytes = wal_segment_size - (currLSN %
wal_segment_size);
+ *restBytes =
+ (restartSeg + limitSegs - currSeg) *
wal_segment_size
+ + fragbytes;
+ }
+ }

This code doesn't consider the case where wal_keep_segments >
max_slot_keep_size. In the case I think we should use (currSeg -
wal_keep_segments) as the lower bound in order to avoid showing
"streaming" in the wal_status although the remain is 0.

-----
+ *restBytes =
+ (restartSeg + limitSegs - currSeg) *
wal_segment_size
+ + fragbytes;

Maybe you can use XLogSegNoOffsetToRecPtr instead.

-----
+ * 0 means that WAL record at targetLSN is alredy removed.
+ * 1 means that WAL record at tagetLSN is availble.
+ * 2 means that WAL record at tagetLSN is availble but about to be removed by

s/alredy/already/
s/tagetLSN/targetLSN/
s/availble/available/

-----
+ * If resetBytes is not NULL, returns remaining LSN bytes to advance until any
+ * slot loses reserving a WAL record.

s/resetBytes/restBytes/

-----
+ Specify the maximum size of WAL files
+ that <link linkend="streaming-replication-slots">replication
+ slots</link> are allowed to reatin in the <filename>pg_wal</filename>

s/reatin/retain/

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Hello.

At Tue, 24 Jul 2018 16:47:41 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoD0rChq7wQE=_o95quopcQGjcVG9omwdH07nT5cm81hzg(at)mail(dot)gmail(dot)com>
> On Mon, Jul 23, 2018 at 4:16 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > Hello.
> >
> > At Fri, 20 Jul 2018 10:13:58 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoDayePWwu4t=VPP5P1QFDSBvks1d8j76bXp5rbXoPbZcA(at)mail(dot)gmail(dot)com>
..
> > Instead, I made the field be shown in flat "bytes" using bigint,
> > which can be nicely shown using pg_size_pretty;
>
> Thank you for updating. I agree showing the remain in bytes.
>
> Here is review comments for v6 patch.
>
> @@ -967,9 +969,9 @@ postgres=# SELECT * FROM
> pg_create_physical_replication_slot('node_a_slot');
> node_a_slot |
>
> postgres=# SELECT * FROM pg_replication_slots;
> - slot_name | slot_type | datoid | database | active | xmin |
> restart_lsn | confirmed_flush_lsn
> --------------+-----------+--------+----------+--------+------+-------------+---------------------
> - node_a_slot | physical | | | f | | |
> + slot_name | slot_type | datoid | database | active | xmin |
> restart_lsn | confirmed_flush_lsn | wal_status | min_keep_lsn
> +-------------+-----------+--------+----------+--------+------+-------------+---------------------+------------+--------------
> + node_a_slot | physical | | | f | |
> | | unknown | 0/1000000
>
> This funk should be updated.

Perhaps you need a fresh database cluster.

> -----
> +/*
> + * Returns minimum segment number the next checktpoint must leave considering
> + * wal_keep_segments, replication slots and max_slot_wal_keep_size.
> + *
> + * If resetBytes is not NULL, returns remaining LSN bytes to advance until any
> + * slot loses reserving a WAL record.
> + */
> +static XLogSegNo
> +GetOldestKeepSegment(XLogRecPtr currLSN, XLogRecPtr minSlotLSN,
> XLogRecPtr restartLSN, uint64 *restBytes)
> +{
>
> You're assuming that the minSlotLSN is the minimum LSN of replication
> slots but it's not mentioned anywhere. Since you check minSlotSeg <=

I added description for parameters in the function comment.

> currSeg but not force it, if a caller sets a wrong value to minSlotLSN
> this function will return a wrong value with no complaints. Similarly

I don't think such case can happen on a sane system. Even that
happenes it behaves in the same way as minSlotLSN being invalid
in the case. KeepLogSeg() also behaves in the same way and the
wal recycling will be performed as pg_replication_losts
predicted. Nothing can improve the behavior and I don't think
placing assertion there is overkill.

> there is not explanation about the resetartLSN, so you can add it. I'm
> not sure the augment name restartLSN is suitable for the function in
> xlog.c but I'd defer it to committers.

Done.

> Since this function assumes that both restartLSN and *restBytes are
> valid or invalid (and NULL) it's better to add assertions for safety.
> The current code accepts even the case where only either argment is
> valid.
> -----
> + if (limitSegs > 0 && currSeg <= restartSeg + limitSegs)
> + {

Even if the caller gives InvalidRecPtr as restartLSN, which is an
insane situation, the function just treats the value as zero and
reuturns the "correct" value for the restartLSN, which doesn't
harm anything.

> + /*
> + * This slot still has all required segments.
> Calculate how many
> + * LSN bytes the slot has until it loses restart_lsn.
> + */
> + fragbytes = wal_segment_size - (currLSN %
> wal_segment_size);
> + *restBytes =
> + (restartSeg + limitSegs - currSeg) *
> wal_segment_size
> + + fragbytes;
> + }
> + }
>
> This code doesn't consider the case where wal_keep_segments >
> max_slot_keep_size. In the case I think we should use (currSeg -
> wal_keep_segments) as the lower bound in order to avoid showing
> "streaming" in the wal_status although the remain is 0.

Thanks. It should use keepSegs instead of limitSegs. Fixed.

> -----
> + *restBytes =
> + (restartSeg + limitSegs - currSeg) *
> wal_segment_size
> + + fragbytes;
>
> Maybe you can use XLogSegNoOffsetToRecPtr instead.

Indeed. I'm not sure it is easier to read, though. (Maybe the
functions should use wal_segment_size out-of-band. (That is, not
passed as a parameter)).

> -----
> + * 0 means that WAL record at targetLSN is alredy removed.
> + * 1 means that WAL record at tagetLSN is availble.
> + * 2 means that WAL record at tagetLSN is availble but about to be removed by
>
> s/alredy/already/
> s/tagetLSN/targetLSN/
> s/availble/available/
> -----
> + * If resetBytes is not NULL, returns remaining LSN bytes to advance until any
> + * slot loses reserving a WAL record.
>
> s/resetBytes/restBytes/

Ugggh! Sorry that my fingers are extra-fat.. Fixed. I rechecked
through the whole patch and found one more typo.

> -----
> + Specify the maximum size of WAL files
> + that <link linkend="streaming-replication-slots">replication
> + slots</link> are allowed to reatin in the <filename>pg_wal</filename>
>
> s/reatin/retain/

Thank you. I also found other leftovers in catalogs.sgml and
high-availability.sgml.

# The latter file seems needing amendment for v11.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jun 26, 2018 at 04:26:59PM +0900, Kyotaro HORIGUCHI wrote:
> Hello. This is the reabased version of slot-limit feature.
>
> This patch limits maximum WAL segments to be kept by replication
> slots. Replication slot is useful to avoid desync with replicas
> after temporary disconnection but it is dangerous when some of
> replicas are lost. The WAL space can be exhausted and server can
> PANIC in the worst case. This can prevent the worst case having a
> benefit from replication slots using a new GUC variable
> max_slot_wal_keep_size.

Have you considered just using a boolean to control if max_wal_size
honors WAL preserved by replication slots, rather than creating the new
GUC max_slot_wal_keep_size?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On 2018-07-31 15:11:52 -0400, Bruce Momjian wrote:
> On Tue, Jun 26, 2018 at 04:26:59PM +0900, Kyotaro HORIGUCHI wrote:
> > Hello. This is the reabased version of slot-limit feature.
> >
> > This patch limits maximum WAL segments to be kept by replication
> > slots. Replication slot is useful to avoid desync with replicas
> > after temporary disconnection but it is dangerous when some of
> > replicas are lost. The WAL space can be exhausted and server can
> > PANIC in the worst case. This can prevent the worst case having a
> > benefit from replication slots using a new GUC variable
> > max_slot_wal_keep_size.
>
> Have you considered just using a boolean to control if max_wal_size
> honors WAL preserved by replication slots, rather than creating the new
> GUC max_slot_wal_keep_size?

That seems like a bad idea. max_wal_size influences checkpoint
scheduling - there's no good reason to conflate that with retention?

Greetings,

Andres Freund

Greetings,

* Andres Freund (andres(at)anarazel(dot)de) wrote:
> On 2018-07-31 15:11:52 -0400, Bruce Momjian wrote:
> > On Tue, Jun 26, 2018 at 04:26:59PM +0900, Kyotaro HORIGUCHI wrote:
> > > Hello. This is the reabased version of slot-limit feature.
> > >
> > > This patch limits maximum WAL segments to be kept by replication
> > > slots. Replication slot is useful to avoid desync with replicas
> > > after temporary disconnection but it is dangerous when some of
> > > replicas are lost. The WAL space can be exhausted and server can
> > > PANIC in the worst case. This can prevent the worst case having a
> > > benefit from replication slots using a new GUC variable
> > > max_slot_wal_keep_size.
> >
> > Have you considered just using a boolean to control if max_wal_size
> > honors WAL preserved by replication slots, rather than creating the new
> > GUC max_slot_wal_keep_size?
>
> That seems like a bad idea. max_wal_size influences checkpoint
> scheduling - there's no good reason to conflate that with retention?

I agree that we shouldn't conflate checkpointing and retention. What I
wonder about though is what value will wal_keep_segments have once this
new GUC exists..? I wonder if we could deprecate it... I wish we had
implemented repliation slots from the start with wal_keep_segments
capping the max WAL retained but that ship has sailed and changing it
now would break existing configurations.

Thanks!

Stephen

On 2018-07-31 15:21:27 -0400, Stephen Frost wrote:
> Greetings,
>
> * Andres Freund (andres(at)anarazel(dot)de) wrote:
> > On 2018-07-31 15:11:52 -0400, Bruce Momjian wrote:
> > > On Tue, Jun 26, 2018 at 04:26:59PM +0900, Kyotaro HORIGUCHI wrote:
> > > > Hello. This is the reabased version of slot-limit feature.
> > > >
> > > > This patch limits maximum WAL segments to be kept by replication
> > > > slots. Replication slot is useful to avoid desync with replicas
> > > > after temporary disconnection but it is dangerous when some of
> > > > replicas are lost. The WAL space can be exhausted and server can
> > > > PANIC in the worst case. This can prevent the worst case having a
> > > > benefit from replication slots using a new GUC variable
> > > > max_slot_wal_keep_size.
> > >
> > > Have you considered just using a boolean to control if max_wal_size
> > > honors WAL preserved by replication slots, rather than creating the new
> > > GUC max_slot_wal_keep_size?
> >
> > That seems like a bad idea. max_wal_size influences checkpoint
> > scheduling - there's no good reason to conflate that with retention?
>
> I agree that we shouldn't conflate checkpointing and retention. What I
> wonder about though is what value will wal_keep_segments have once this
> new GUC exists..? I wonder if we could deprecate it...

Don't think that's a good idea. It's entirely conceivable to have a
wal_keep_segments much lower than max_slot_wal_keep_size. For some
throwaway things it can be annoying to have to slots, and if you remove
wal_keep_segments there's no alternative.

Greetings,

Andres Freund

At Tue, 31 Jul 2018 12:24:13 -0700, Andres Freund <andres(at)anarazel(dot)de> wrote in <20180731192413(dot)7lr4qbc4qbyoim5y(at)alap3(dot)anarazel(dot)de>
> On 2018-07-31 15:21:27 -0400, Stephen Frost wrote:
> > Greetings,
> >
> > * Andres Freund (andres(at)anarazel(dot)de) wrote:
> > > On 2018-07-31 15:11:52 -0400, Bruce Momjian wrote:
> > > > On Tue, Jun 26, 2018 at 04:26:59PM +0900, Kyotaro HORIGUCHI wrote:
> > > > > Hello. This is the reabased version of slot-limit feature.
> > > > >
> > > > > This patch limits maximum WAL segments to be kept by replication
> > > > > slots. Replication slot is useful to avoid desync with replicas
> > > > > after temporary disconnection but it is dangerous when some of
> > > > > replicas are lost. The WAL space can be exhausted and server can
> > > > > PANIC in the worst case. This can prevent the worst case having a
> > > > > benefit from replication slots using a new GUC variable
> > > > > max_slot_wal_keep_size.
> > > >
> > > > Have you considered just using a boolean to control if max_wal_size
> > > > honors WAL preserved by replication slots, rather than creating the new
> > > > GUC max_slot_wal_keep_size?
> > >
> > > That seems like a bad idea. max_wal_size influences checkpoint
> > > scheduling - there's no good reason to conflate that with retention?
> >
> > I agree that we shouldn't conflate checkpointing and retention. What I
> > wonder about though is what value will wal_keep_segments have once this
> > new GUC exists..? I wonder if we could deprecate it...
>
> Don't think that's a good idea. It's entirely conceivable to have a
> wal_keep_segments much lower than max_slot_wal_keep_size. For some
> throwaway things it can be annoying to have to slots, and if you remove
> wal_keep_segments there's no alternative.

I thought it's to be deprecated for some reason so I'm leaving
wal_keep_segments in '# of segments' even though the new GUC is
in MB. I'm a bit uneasy that the two similar settings are in
different units. Couldn't we turn it into MB taking this
opportunity if we will keep wal_keep_segments, changing its name
to min_wal_keep_size? max_slot_wal_keep_size could be changed to
just max_wal_keep_size along with it.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jul 31, 2018 at 9:52 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> I thought it's to be deprecated for some reason so I'm leaving
> wal_keep_segments in '# of segments' even though the new GUC is
> in MB. I'm a bit uneasy that the two similar settings are in
> different units. Couldn't we turn it into MB taking this
> opportunity if we will keep wal_keep_segments, changing its name
> to min_wal_keep_size? max_slot_wal_keep_size could be changed to
> just max_wal_keep_size along with it.

This seems like it's a little bit of a separate topic from what this
thread about, but FWIW, +1 for standardizing on MB.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

At Thu, 2 Aug 2018 09:05:33 -0400, Robert Haas <robertmhaas(at)gmail(dot)com> wrote in <CA+TgmoYVrKY0W0jigJymFZo0ewkQoWGfLLpiTSgJLQN3tcHGTg(at)mail(dot)gmail(dot)com>
> On Tue, Jul 31, 2018 at 9:52 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > I thought it's to be deprecated for some reason so I'm leaving
> > wal_keep_segments in '# of segments' even though the new GUC is
> > in MB. I'm a bit uneasy that the two similar settings are in
> > different units. Couldn't we turn it into MB taking this
> > opportunity if we will keep wal_keep_segments, changing its name
> > to min_wal_keep_size? max_slot_wal_keep_size could be changed to
> > just max_wal_keep_size along with it.
>
> This seems like it's a little bit of a separate topic from what this
> thread about, but FWIW, +1 for standardizing on MB.

Thanks. Ok, I'll raise this after separately with this.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Thank you for updating the patch.

On Tue, Jul 31, 2018 at 6:11 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello.
>
> At Tue, 24 Jul 2018 16:47:41 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoD0rChq7wQE=_o95quopcQGjcVG9omwdH07nT5cm81hzg(at)mail(dot)gmail(dot)com>
>> On Mon, Jul 23, 2018 at 4:16 PM, Kyotaro HORIGUCHI
>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> > Hello.
>> >
>> > At Fri, 20 Jul 2018 10:13:58 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoDayePWwu4t=VPP5P1QFDSBvks1d8j76bXp5rbXoPbZcA(at)mail(dot)gmail(dot)com>
> ..
>> > Instead, I made the field be shown in flat "bytes" using bigint,
>> > which can be nicely shown using pg_size_pretty;
>>
>> Thank you for updating. I agree showing the remain in bytes.
>>
>> Here is review comments for v6 patch.
>>
>> @@ -967,9 +969,9 @@ postgres=# SELECT * FROM
>> pg_create_physical_replication_slot('node_a_slot');
>> node_a_slot |
>>
>> postgres=# SELECT * FROM pg_replication_slots;
>> - slot_name | slot_type | datoid | database | active | xmin |
>> restart_lsn | confirmed_flush_lsn
>> --------------+-----------+--------+----------+--------+------+-------------+---------------------
>> - node_a_slot | physical | | | f | | |
>> + slot_name | slot_type | datoid | database | active | xmin |
>> restart_lsn | confirmed_flush_lsn | wal_status | min_keep_lsn
>> +-------------+-----------+--------+----------+--------+------+-------------+---------------------+------------+--------------
>> + node_a_slot | physical | | | f | |
>> | | unknown | 0/1000000
>>
>> This funk should be updated.
>
> Perhaps you need a fresh database cluster.

I meant this was a doc update in 0004 patch but it's fixed in v7 patch.

While testing the v7 patch, I got the following result with
max_slot_wal_keep_size = 5GB and without wal_keep_segments setting.

=# select pg_current_wal_lsn(), slot_name, restart_lsn,
confirmed_flush_lsn, wal_status, remain, pg_size_pretty(remain) from
pg_replication_slots ;
pg_current_wal_lsn | slot_name | restart_lsn | confirmed_flush_lsn |
wal_status | remain | pg_size_pretty
--------------------+-----------+-------------+---------------------+------------+----------+----------------
2/A30000D8 | l1 | 1/AC000910 | 1/AC000948 |
streaming | 16777000 | 16 MB
(1 row)

The actual distance between the slot limit and the slot 'l1' is about
1GB(5GB - (2/A30000D8 - 1/AC000910)) but the system view says the
remain is only 16MB. For the calculation of resetBytes in
GetOldestKeepSegment(), the current patch seems to calculate the
distance between the minSlotLSN and restartLSN when (curLSN -
max_slot_wal_keep_size) < minSlotLSN. However, I think that the actual
remained bytes until the slot lost the required WAL is (restartLSN -
(currLSN - max_slot_wal_keep_size)) in that case.

Also, 0004 patch needs to be rebased on the current HEAD.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

At Mon, 3 Sep 2018 18:14:22 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBgCMc9bp2cADMFm40qoEXxbomdu1dtj5EaFSAS4BtAyw(at)mail(dot)gmail(dot)com>
> Thank you for updating the patch!
>
> On Tue, Jul 31, 2018 at 6:11 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > Hello.
> >
> > At Tue, 24 Jul 2018 16:47:41 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoD0rChq7wQE=_o95quopcQGjcVG9omwdH07nT5cm81hzg(at)mail(dot)gmail(dot)com>
> >> On Mon, Jul 23, 2018 at 4:16 PM, Kyotaro HORIGUCHI
> >> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >> > Hello.
> >> >
> >> > At Fri, 20 Jul 2018 10:13:58 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoDayePWwu4t=VPP5P1QFDSBvks1d8j76bXp5rbXoPbZcA(at)mail(dot)gmail(dot)com>
> >> This funk should be updated.
> >
> > Perhaps you need a fresh database cluster.
>
> I meant this was a doc update in 0004 patch but it's fixed in v7 patch.

Wow..

> While testing the v7 patch, I got the following result with
> max_slot_wal_keep_size = 5GB and without wal_keep_segments setting.
>
> =# select pg_current_wal_lsn(), slot_name, restart_lsn,
> confirmed_flush_lsn, wal_status, remain, pg_size_pretty(remain) from
> pg_replication_slots ;
> pg_current_wal_lsn | slot_name | restart_lsn | confirmed_flush_lsn |
> wal_status | remain | pg_size_pretty
> --------------------+-----------+-------------+---------------------+------------+----------+----------------
> 2/A30000D8 | l1 | 1/AC000910 | 1/AC000948 |
> streaming | 16777000 | 16 MB
> (1 row)
>
> The actual distance between the slot limit and the slot 'l1' is about
> 1GB(5GB - (2/A30000D8 - 1/AC000910)) but the system view says the
> remain is only 16MB. For the calculation of resetBytes in
> GetOldestKeepSegment(), the current patch seems to calculate the
> distance between the minSlotLSN and restartLSN when (curLSN -
> max_slot_wal_keep_size) < minSlotLSN. However, I think that the actual
> remained bytes until the slot lost the required WAL is (restartLSN -
> (currLSN - max_slot_wal_keep_size)) in that case.

Oops! That's a silly thinko or rather a typo. It's apparently
wrong that keepSegs instead of limitSegs is involved in making
the calculation of restBytes. Just using limitSegs makes it
sane. It's a pity that I removed the remain from regression test.

Fixed that and added an item for remain calculation in the TAP
test. I expect that pg_size_pretty() adds some robustness to the
test.

> Also, 0004 patch needs to be rebased on the current HEAD.

Done. Please find the v8 attached.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Sep 4, 2018 at 7:52 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> At Mon, 3 Sep 2018 18:14:22 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBgCMc9bp2cADMFm40qoEXxbomdu1dtj5EaFSAS4BtAyw(at)mail(dot)gmail(dot)com>
>> Thank you for updating the patch!
>>
>> On Tue, Jul 31, 2018 at 6:11 PM, Kyotaro HORIGUCHI
>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> > Hello.
>> >
>> > At Tue, 24 Jul 2018 16:47:41 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoD0rChq7wQE=_o95quopcQGjcVG9omwdH07nT5cm81hzg(at)mail(dot)gmail(dot)com>
>> >> On Mon, Jul 23, 2018 at 4:16 PM, Kyotaro HORIGUCHI
>> >> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> >> > Hello.
>> >> >
>> >> > At Fri, 20 Jul 2018 10:13:58 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoDayePWwu4t=VPP5P1QFDSBvks1d8j76bXp5rbXoPbZcA(at)mail(dot)gmail(dot)com>
>> >> This funk should be updated.
>> >
>> > Perhaps you need a fresh database cluster.
>>
>> I meant this was a doc update in 0004 patch but it's fixed in v7 patch.
>
> Wow..
>
>> While testing the v7 patch, I got the following result with
>> max_slot_wal_keep_size = 5GB and without wal_keep_segments setting.
>>
>> =# select pg_current_wal_lsn(), slot_name, restart_lsn,
>> confirmed_flush_lsn, wal_status, remain, pg_size_pretty(remain) from
>> pg_replication_slots ;
>> pg_current_wal_lsn | slot_name | restart_lsn | confirmed_flush_lsn |
>> wal_status | remain | pg_size_pretty
>> --------------------+-----------+-------------+---------------------+------------+----------+----------------
>> 2/A30000D8 | l1 | 1/AC000910 | 1/AC000948 |
>> streaming | 16777000 | 16 MB
>> (1 row)
>>
>> The actual distance between the slot limit and the slot 'l1' is about
>> 1GB(5GB - (2/A30000D8 - 1/AC000910)) but the system view says the
>> remain is only 16MB. For the calculation of resetBytes in
>> GetOldestKeepSegment(), the current patch seems to calculate the
>> distance between the minSlotLSN and restartLSN when (curLSN -
>> max_slot_wal_keep_size) < minSlotLSN. However, I think that the actual
>> remained bytes until the slot lost the required WAL is (restartLSN -
>> (currLSN - max_slot_wal_keep_size)) in that case.
>
> Oops! That's a silly thinko or rather a typo. It's apparently
> wrong that keepSegs instead of limitSegs is involved in making
> the calculation of restBytes. Just using limitSegs makes it
> sane. It's a pity that I removed the remain from regression test.
>
> Fixed that and added an item for remain calculation in the TAP
> test. I expect that pg_size_pretty() adds some robustness to the
> test.
>
>> Also, 0004 patch needs to be rebased on the current HEAD.
>
> Done. Please find the v8 attached.
>

Thank you for updating! Here is the review comment for v8 patch.

+ /*
+ * This slot still has all required segments. Calculate how many
+ * LSN bytes the slot has until it loses restart_lsn.
+ */
+ fragbytes = wal_segment_size - (currLSN % wal_segment_size);
+ XLogSegNoOffsetToRecPtr(restartSeg + limitSegs - currSeg,
fragbytes,
+ wal_segment_size, *restBytes);

For the calculation of fragbytes, I think we should calculate the
fragment bytes of restartLSN instead. The the formula "restartSeg +
limitSegs - currSeg" means the # of segment between restartLSN and the
limit by the new parameter. I don't think that the summation of it and
fragment bytes of currenLSN is correct. As the following result
(max_slot_wal_keep_size is 128MB) shows, the remain column shows the
actual remains + 16MB (get_bytes function returns the value of
max_slot_wal_keep_size in bytes).

postgres(1:29447)=# select pg_current_wal_lsn(), slot_name,
restart_lsn, wal_status, remain, pg_size_pretty(remain),
pg_size_pretty(get_bytes('max_slot_wal_keep_size') -
(pg_current_wal_lsn() - restart_lsn)) from pg_replication_slots ;
pg_current_wal_lsn | slot_name | restart_lsn | wal_status | remain
| pg_size_pretty | pg_size_pretty
--------------------+-----------+-------------+------------+-----------+----------------+----------------
0/1D0001F0 | l1 | 0/1D0001B8 | streaming | 150994448
| 144 MB | 128 MB
(1 row)

---
If the wal_keeps_segments is greater than max_slot_wal_keep_size, the
wal_keep_segments doesn't affect the value of the remain column.

postgres(1:48422)=# show max_slot_wal_keep_size ;
max_slot_wal_keep_size
------------------------
128MB
(1 row)

postgres(1:48422)=# show wal_keep_segments ;
wal_keep_segments
-------------------
5000
(1 row)

postgres(1:48422)=# select slot_name, wal_status, remain,
pg_size_pretty(remain) as remain from pg_replication_slots ;
slot_name | wal_status | remain | remain
-----------+------------+-----------+--------
l1 | streaming | 150994728 | 144 MB
(1 row)

*** After consumed over 128MB WAL ***

postgres(1:48422)=# select slot_name, wal_status, remain,
pg_size_pretty(remain) as remain from pg_replication_slots ;
slot_name | wal_status | remain | remain
-----------+------------+--------+---------
l1 | streaming | 0 | 0 bytes
(1 row)

---
For the cosmetic stuff there are code where need the line break.

static void CheckPointGuts(XLogRecPtr checkPointRedo, int flags);
+static XLogSegNo GetOldestKeepSegment(XLogRecPtr currpos, XLogRecPtr
minSlotPtr, XLogRecPtr restartLSN, uint64 *restBytes);
static void KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo);
static XLogRecPtr XLogGetReplicationSlotMinimumLSN(void);

and

+static XLogSegNo
+GetOldestKeepSegment(XLogRecPtr currLSN, XLogRecPtr minSlotLSN,
XLogRecPtr restartLSN, uint64 *restBytes)
+{

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Thank you for the comment.

At Wed, 5 Sep 2018 14:31:10 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoB-HJvL+uKsv40Gb8Dymh9uBBQUXTucqv4MDtH_AGKh4g(at)mail(dot)gmail(dot)com>
> On Tue, Sep 4, 2018 at 7:52 PM, Kyotaro HORIGUCHI
> Thank you for updating! Here is the review comment for v8 patch.
>
> + /*
> + * This slot still has all required segments. Calculate how many
> + * LSN bytes the slot has until it loses restart_lsn.
> + */
> + fragbytes = wal_segment_size - (currLSN % wal_segment_size);
> + XLogSegNoOffsetToRecPtr(restartSeg + limitSegs - currSeg,
> fragbytes,
> + wal_segment_size, *restBytes);
>
> For the calculation of fragbytes, I think we should calculate the
> fragment bytes of restartLSN instead. The the formula "restartSeg +
> limitSegs - currSeg" means the # of segment between restartLSN and the
> limit by the new parameter. I don't think that the summation of it and
> fragment bytes of currenLSN is correct. As the following result
> (max_slot_wal_keep_size is 128MB) shows, the remain column shows the
> actual remains + 16MB (get_bytes function returns the value of
> max_slot_wal_keep_size in bytes).

Since a oldest segment is removed after the current LSN moves to
the next segmen, current LSN naturally determines the fragment
bytes. Maybe you're concerning that the number of segments looks
too much by one segment.

One arguable point of the feature is how max_slot_wal_keep_size
works exactly. I assume that even though the name is named as
"max_", we actually expect that "at least that bytes are
kept". So, for example, with 16MB of segment size and 50MB of
max_s_w_k_s, I designed this so that the size of preserved WAL
doesn't go below 50MB, actually (rounding up to multples of 16MB
of 50MB), and loses the oldest segment when it reaches 64MB +
16MB = 80MB as you saw.

# I believe that the difference is not so significant since we
# have around hunderd or several hundreds of segments in common
# cases.

Do you mean that we should define the GUC parameter literally as
"we won't have exactly that many bytes of WAL segmetns"? That is,
we have at most 48MB preserved WAL records for 50MB of
max_s_w_k_s setting. This is the same to how max_wal_size is
counted but I don't think max_slot_wal_keep_size will be regarded
in the same way.

The another design would be that we remove the oldest segnent
when WAL reaches to 64MB and reduces to 48MB after deletion.

> ---
> For the cosmetic stuff there are code where need the line break.
>
> static void CheckPointGuts(XLogRecPtr checkPointRedo, int flags);
> +static XLogSegNo GetOldestKeepSegment(XLogRecPtr currpos, XLogRecPtr
> minSlotPtr, XLogRecPtr restartLSN, uint64 *restBytes);
> static void KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo);
> static XLogRecPtr XLogGetReplicationSlotMinimumLSN(void);
>
> and
>
> +static XLogSegNo
> +GetOldestKeepSegment(XLogRecPtr currLSN, XLogRecPtr minSlotLSN,
> XLogRecPtr restartLSN, uint64 *restBytes)
> +{

Thanks, I folded the parameter list in my working repository.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Sep 6, 2018 at 4:10 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Thank you for the comment.
>
> At Wed, 5 Sep 2018 14:31:10 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoB-HJvL+uKsv40Gb8Dymh9uBBQUXTucqv4MDtH_AGKh4g(at)mail(dot)gmail(dot)com>
>> On Tue, Sep 4, 2018 at 7:52 PM, Kyotaro HORIGUCHI
>> Thank you for updating! Here is the review comment for v8 patch.
>>
>> + /*
>> + * This slot still has all required segments. Calculate how many
>> + * LSN bytes the slot has until it loses restart_lsn.
>> + */
>> + fragbytes = wal_segment_size - (currLSN % wal_segment_size);
>> + XLogSegNoOffsetToRecPtr(restartSeg + limitSegs - currSeg,
>> fragbytes,
>> + wal_segment_size, *restBytes);
>>
>> For the calculation of fragbytes, I think we should calculate the
>> fragment bytes of restartLSN instead. The the formula "restartSeg +
>> limitSegs - currSeg" means the # of segment between restartLSN and the
>> limit by the new parameter. I don't think that the summation of it and
>> fragment bytes of currenLSN is correct. As the following result
>> (max_slot_wal_keep_size is 128MB) shows, the remain column shows the
>> actual remains + 16MB (get_bytes function returns the value of
>> max_slot_wal_keep_size in bytes).
>
> Since a oldest segment is removed after the current LSN moves to
> the next segmen, current LSN naturally determines the fragment
> bytes. Maybe you're concerning that the number of segments looks
> too much by one segment.
>
> One arguable point of the feature is how max_slot_wal_keep_size
> works exactly. I assume that even though the name is named as
> "max_", we actually expect that "at least that bytes are
> kept". So, for example, with 16MB of segment size and 50MB of
> max_s_w_k_s, I designed this so that the size of preserved WAL
> doesn't go below 50MB, actually (rounding up to multples of 16MB
> of 50MB), and loses the oldest segment when it reaches 64MB +
> 16MB = 80MB as you saw.
>
> # I believe that the difference is not so significant since we
> # have around hunderd or several hundreds of segments in common
> # cases.
>
> Do you mean that we should define the GUC parameter literally as
> "we won't have exactly that many bytes of WAL segmetns"? That is,
> we have at most 48MB preserved WAL records for 50MB of
> max_s_w_k_s setting. This is the same to how max_wal_size is
> counted but I don't think max_slot_wal_keep_size will be regarded
> in the same way.

I might be missing something but what I'm expecting to this feature is
to restrict the how much WAL we can keep at a maximum for replication
slots. In other words, the distance between the current LSN and the
minimum restart_lsn of replication slots doesn't over the value of
max_slot_wal_keep_size. It's similar to wal_keep_segments except for
that this feature affects only replication slots. And
wal_keep_segments cannot restrict WAL that replication slots are
holding. For example, with 16MB of segment size and 50MB of
max_slot_wal_keep_size, we can keep at most 50MB WAL for replication
slots. However, once we consumed more than 50MB WAL while not
advancing any restart_lsn the required WAL might be lost by the next
checkpoint, which depends on the min_wal_size. On the other hand, if
we mostly can advance restart_lsn to approximately the current LSN the
size of preserved WAL for replication slots can go below 50MB.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

This documentation

+ <para>
+ Specify the maximum size of WAL files
+ that <link linkend="streaming-replication-slots">replication
+ slots</link> are allowed to retain in the
<filename>pg_wal</filename>
+ directory at checkpoint time.
+ If <varname>max_slot_wal_keep_size</varname> is zero (the default),
+ replication slots retain unlimited size of WAL files.
+ </para>

doesn't say anything about what happens when the limit is exceeded.
Does the system halt until the WAL is fetched from the slots? Do the
slots get invalidated?

Also, I don't think 0 is a good value for the default behavior. 0 would
mean that a slot is not allowed to retain any more WAL than already
exists anyway. Maybe we don't want to support that directly, but it's a
valid configuration. So maybe use -1 for infinity.

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

Hello.

At Thu, 6 Sep 2018 19:55:39 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoAZCdvdMN-vG4D_653vb_FN-AaMAP5+GXgF1JRjy+LeyA(at)mail(dot)gmail(dot)com>
> On Thu, Sep 6, 2018 at 4:10 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > Thank you for the comment.
> >
> > At Wed, 5 Sep 2018 14:31:10 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoB-HJvL+uKsv40Gb8Dymh9uBBQUXTucqv4MDtH_AGKh4g(at)mail(dot)gmail(dot)com>
> >> On Tue, Sep 4, 2018 at 7:52 PM, Kyotaro HORIGUCHI
> >> Thank you for updating! Here is the review comment for v8 patch.
> >>
> >> + /*
> >> + * This slot still has all required segments. Calculate how many
> >> + * LSN bytes the slot has until it loses restart_lsn.
> >> + */
> >> + fragbytes = wal_segment_size - (currLSN % wal_segment_size);
> >> + XLogSegNoOffsetToRecPtr(restartSeg + limitSegs - currSeg,
> >> fragbytes,
> >> + wal_segment_size, *restBytes);
> >>
> >> For the calculation of fragbytes, I think we should calculate the
> >> fragment bytes of restartLSN instead. The the formula "restartSeg +
> >> limitSegs - currSeg" means the # of segment between restartLSN and the
> >> limit by the new parameter. I don't think that the summation of it and
> >> fragment bytes of currenLSN is correct. As the following result
> >> (max_slot_wal_keep_size is 128MB) shows, the remain column shows the
> >> actual remains + 16MB (get_bytes function returns the value of
> >> max_slot_wal_keep_size in bytes).
> >
> > Since a oldest segment is removed after the current LSN moves to
> > the next segmen, current LSN naturally determines the fragment
> > bytes. Maybe you're concerning that the number of segments looks
> > too much by one segment.
> >
> > One arguable point of the feature is how max_slot_wal_keep_size
> > works exactly. I assume that even though the name is named as
> > "max_", we actually expect that "at least that bytes are
> > kept". So, for example, with 16MB of segment size and 50MB of
> > max_s_w_k_s, I designed this so that the size of preserved WAL
> > doesn't go below 50MB, actually (rounding up to multples of 16MB
> > of 50MB), and loses the oldest segment when it reaches 64MB +
> > 16MB = 80MB as you saw.
> >
> > # I believe that the difference is not so significant since we
> > # have around hunderd or several hundreds of segments in common
> > # cases.
> >
> > Do you mean that we should define the GUC parameter literally as
> > "we won't have exactly that many bytes of WAL segmetns"? That is,
> > we have at most 48MB preserved WAL records for 50MB of
> > max_s_w_k_s setting. This is the same to how max_wal_size is
> > counted but I don't think max_slot_wal_keep_size will be regarded
> > in the same way.
>
> I might be missing something but what I'm expecting to this feature is
> to restrict the how much WAL we can keep at a maximum for replication
> slots. In other words, the distance between the current LSN and the
> minimum restart_lsn of replication slots doesn't over the value of
> max_slot_wal_keep_size.

Yes, it's one possible design, the same with "we won't have more
than exactly that many bytes of WAL segmetns" above ("more than"
is added, which I meant). But anyway we cannot keep the limit
strictly since WAL segments are removed only at checkpoint
time. So If doing so, we can reach the lost state before the
max_slot_wal_keep_size is filled up meanwhile WAL can exceed the
size by a WAL flood. We can define it precisely at most as "wal
segments are preserved at most aorund the value". So I choosed
the definition so that we can tell about this as "we don't
guarantee more than that bytes".

# Uuuu. sorry for possiblly hard-to-read sentence..

> It's similar to wal_keep_segments except for
> that this feature affects only replication slots. And

It defines the *extra* segments to be kept, that is, if we set it
to 2, at least 3 segments are present. If we set
max_slot_wal_keep_size to 32MB (= 2 segs here), we have at most 3
segments since 32MB range before the current LSN almost always
spans over 3 segments. Doesn't this seemingly in a similar way
with wal_keep_segments?

If the current LSN is at the very last of a segment and
restart_lsn is catching up to the current LSN, the "remain" is
equal to max_slot_wal_keep_size as the guaranteed size. If very
beginning of a segments, it gets extra 16MB.

> wal_keep_segments cannot restrict WAL that replication slots are
> holding. For example, with 16MB of segment size and 50MB of
> max_slot_wal_keep_size, we can keep at most 50MB WAL for replication
> slots. However, once we consumed more than 50MB WAL while not
> advancing any restart_lsn the required WAL might be lost by the next
> checkpoint, which depends on the min_wal_size.

I don't get the last phrase. With small min_wal_size, we don't
recycle most of the "removed" segments. If large, we recycle more
of them. It doesn't affect up to where the checkpoint removes WAL
files. But it is right that LSN advance with
max_slot_wal_keep_size bytes immediately leands to breaking a
slot and it is intended behavior.

> On the other hand, if
> we mostly can advance restart_lsn to approximately the current LSN the
> size of preserved WAL for replication slots can go below 50MB.

Y..eah.. That's right. It is just how this works. But I don't
understand how this is related to the intepretation of the "max"
of the GUC variable.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hello.

At Thu, 6 Sep 2018 22:32:21 +0200, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <29bbd79d-696b-509e-578a-0fc38a3b9405(at)2ndquadrant(dot)com>
> This documentation
>
> + <para>
> + Specify the maximum size of WAL files
> + that <link linkend="streaming-replication-slots">replication
> + slots</link> are allowed to retain in the
> <filename>pg_wal</filename>
> + directory at checkpoint time.
> + If <varname>max_slot_wal_keep_size</varname> is zero (the default),
> + replication slots retain unlimited size of WAL files.
> + </para>
>
> doesn't say anything about what happens when the limit is exceeded.
> Does the system halt until the WAL is fetched from the slots? Do the
> slots get invalidated?

Thanks for pointing that. That's a major cause of confusion. Does
the following make sense?

> Specify the maximum size of WAL files that <link
> linkend="streaming-replication-slots">replication slots</link>
> are allowed to retain in the <filename>pg_wal</filename>
> directory at checkpoint time. If
> <varname>max_slot_wal_keep_size</varname> is zero (the
> default), replication slots retain unlimited size of WAL files.
+ If restart_lsn of a replication slot gets behind more than that
+ bytes from the current LSN, the standby using the slot may not
+ be able to reconnect due to removal of required WAL records.

And the following sentense is wrong now. I'll remove it in the
coming version 9.

> <para>
> This parameter is used being rounded down to the multiples of WAL file
> size.
> </para>

> Also, I don't think 0 is a good value for the default behavior. 0 would
> mean that a slot is not allowed to retain any more WAL than already
> exists anyway. Maybe we don't want to support that directly, but it's a
> valid configuration. So maybe use -1 for infinity.

In realtion to the reply just sent to Sawada-san, remain of a
slot can be at most 16MB in the 0 case with the default segment
size. So you're right in this sense. Will fix in the coming
version. Thanks.

=# show max_slot_wal_keep_size;
max_slot_wal_keep_size
------------------------
0
(1 row)
=# select pg_current_wal_lsn(), restart_lsn, remain, pg_size_pretty(remain) as remain from pg_replication_slots ;
pg_current_wal_lsn | restart_lsn | remain | remain
--------------------+-------------+----------+--------
0/4000000 | 0/4000000 | 16777216 | 16 MB
(1 row)
....
=# select pg_current_wal_lsn(), restart_lsn, remain, pg_size_pretty(remain) as remain from pg_replication_slots ;
pg_current_wal_lsn | restart_lsn | remain | remain
--------------------+-------------+--------+--------
0/4FF46D8 | 0/4FF46D8 | 47400 | 46 kB
(1 row)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Sep 10, 2018 at 7:19 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello.
>
> At Thu, 6 Sep 2018 19:55:39 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoAZCdvdMN-vG4D_653vb_FN-AaMAP5+GXgF1JRjy+LeyA(at)mail(dot)gmail(dot)com>
>> On Thu, Sep 6, 2018 at 4:10 PM, Kyotaro HORIGUCHI
>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> > Thank you for the comment.
>> >
>> > At Wed, 5 Sep 2018 14:31:10 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoB-HJvL+uKsv40Gb8Dymh9uBBQUXTucqv4MDtH_AGKh4g(at)mail(dot)gmail(dot)com>
>> >> On Tue, Sep 4, 2018 at 7:52 PM, Kyotaro HORIGUCHI
>> >> Thank you for updating! Here is the review comment for v8 patch.
>> >>
>> >> + /*
>> >> + * This slot still has all required segments. Calculate how many
>> >> + * LSN bytes the slot has until it loses restart_lsn.
>> >> + */
>> >> + fragbytes = wal_segment_size - (currLSN % wal_segment_size);
>> >> + XLogSegNoOffsetToRecPtr(restartSeg + limitSegs - currSeg,
>> >> fragbytes,
>> >> + wal_segment_size, *restBytes);
>> >>
>> >> For the calculation of fragbytes, I think we should calculate the
>> >> fragment bytes of restartLSN instead. The the formula "restartSeg +
>> >> limitSegs - currSeg" means the # of segment between restartLSN and the
>> >> limit by the new parameter. I don't think that the summation of it and
>> >> fragment bytes of currenLSN is correct. As the following result
>> >> (max_slot_wal_keep_size is 128MB) shows, the remain column shows the
>> >> actual remains + 16MB (get_bytes function returns the value of
>> >> max_slot_wal_keep_size in bytes).
>> >
>> > Since a oldest segment is removed after the current LSN moves to
>> > the next segmen, current LSN naturally determines the fragment
>> > bytes. Maybe you're concerning that the number of segments looks
>> > too much by one segment.
>> >
>> > One arguable point of the feature is how max_slot_wal_keep_size
>> > works exactly. I assume that even though the name is named as
>> > "max_", we actually expect that "at least that bytes are
>> > kept". So, for example, with 16MB of segment size and 50MB of
>> > max_s_w_k_s, I designed this so that the size of preserved WAL
>> > doesn't go below 50MB, actually (rounding up to multples of 16MB
>> > of 50MB), and loses the oldest segment when it reaches 64MB +
>> > 16MB = 80MB as you saw.
>> >
>> > # I believe that the difference is not so significant since we
>> > # have around hunderd or several hundreds of segments in common
>> > # cases.
>> >
>> > Do you mean that we should define the GUC parameter literally as
>> > "we won't have exactly that many bytes of WAL segmetns"? That is,
>> > we have at most 48MB preserved WAL records for 50MB of
>> > max_s_w_k_s setting. This is the same to how max_wal_size is
>> > counted but I don't think max_slot_wal_keep_size will be regarded
>> > in the same way.
>>
>> I might be missing something but what I'm expecting to this feature is
>> to restrict the how much WAL we can keep at a maximum for replication
>> slots. In other words, the distance between the current LSN and the
>> minimum restart_lsn of replication slots doesn't over the value of
>> max_slot_wal_keep_size.
>
> Yes, it's one possible design, the same with "we won't have more
> than exactly that many bytes of WAL segmetns" above ("more than"
> is added, which I meant). But anyway we cannot keep the limit
> strictly since WAL segments are removed only at checkpoint
> time.

Agreed. It should be something like a soft limit.

> So If doing so, we can reach the lost state before the
> max_slot_wal_keep_size is filled up meanwhile WAL can exceed the
> size by a WAL flood. We can define it precisely at most as "wal
> segments are preserved at most aorund the value". So I choosed
> the definition so that we can tell about this as "we don't
> guarantee more than that bytes".

Agreed.

>
> # Uuuu. sorry for possiblly hard-to-read sentence..
>
>> It's similar to wal_keep_segments except for
>> that this feature affects only replication slots. And
>
> It defines the *extra* segments to be kept, that is, if we set it
> to 2, at least 3 segments are present. If we set
> max_slot_wal_keep_size to 32MB (= 2 segs here), we have at most 3
> segments since 32MB range before the current LSN almost always
> spans over 3 segments. Doesn't this seemingly in a similar way
> with wal_keep_segments

Yeah, that's fine with me. The wal_keep_segments works regardless
existence of replication slots. If we have replication slots and set
both settings we can reserve extra WAL as much as
max(wal_keep_segments, max_slot_wal_keep_size).

>
> If the current LSN is at the very last of a segment and
> restart_lsn is catching up to the current LSN, the "remain" is
> equal to max_slot_wal_keep_size as the guaranteed size. If very
> beginning of a segments, it gets extra 16MB.

Agreed.

>
>> wal_keep_segments cannot restrict WAL that replication slots are
>> holding. For example, with 16MB of segment size and 50MB of
>> max_slot_wal_keep_size, we can keep at most 50MB WAL for replication
>> slots. However, once we consumed more than 50MB WAL while not
>> advancing any restart_lsn the required WAL might be lost by the next
>> checkpoint, which depends on the min_wal_size.
>
> I don't get the last phrase. With small min_wal_size, we don't
> recycle most of the "removed" segments. If large, we recycle more
> of them. It doesn't affect up to where the checkpoint removes WAL
> files. But it is right that LSN advance with
> max_slot_wal_keep_size bytes immediately leands to breaking a
> slot and it is intended behavior.

Sorry I was wrong. Please ignore the last sentence. What I want to say
is that there is no guarantees that the required WAL is kept once the
reserved extra WAL by replication slots exceeds the threshold.

>
>> On the other hand, if
>> we mostly can advance restart_lsn to approximately the current LSN the
>> size of preserved WAL for replication slots can go below 50MB.
>
> Y..eah.. That's right. It is just how this works. But I don't
> understand how this is related to the intepretation of the "max"
> of the GUC variable.

When I wrote this I understood that the following sentence is that we
regularly keep at least max_slot_wal_keep_size byte regardless the
progress of the minimum restart_lsn, I might have been
misunderstanding though.

>> > I assume that even though the name is named as
>> > "max_", we actually expect that "at least that bytes are
>> > kept".

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Hello.

Thank you for the comments, Sawada-san, Peter.

At Mon, 10 Sep 2018 19:52:24 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20180910(dot)195224(dot)22629595(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> At Thu, 6 Sep 2018 22:32:21 +0200, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <29bbd79d-696b-509e-578a-0fc38a3b9405(at)2ndquadrant(dot)com>
> Thanks for pointing that. That's a major cause of confusion. Does
> the following make sense?
>
> > Specify the maximum size of WAL files that <link
> > linkend="streaming-replication-slots">replication slots</link>
> > are allowed to retain in the <filename>pg_wal</filename>
> > directory at checkpoint time. If
> > <varname>max_slot_wal_keep_size</varname> is zero (the
> > default), replication slots retain unlimited size of WAL files.
> + If restart_lsn of a replication slot gets behind more than that
> + bytes from the current LSN, the standby using the slot may not
> + be able to reconnect due to removal of required WAL records.
...
> > Also, I don't think 0 is a good value for the default behavior. 0 would
> > mean that a slot is not allowed to retain any more WAL than already
> > exists anyway. Maybe we don't want to support that directly, but it's a
> > valid configuration. So maybe use -1 for infinity.
>
> In realtion to the reply just sent to Sawada-san, remain of a
> slot can be at most 16MB in the 0 case with the default segment
> size. So you're right in this sense. Will fix in the coming
> version. Thanks.

I did the following thinkgs in the new version.

- Changed the disable (or infinite) and default value of
max_slot_wal_keep_size to -1 from 0.
(patch 1, 2. guc.c, xlog.c: GetOldestKeepSegment())

- Fixed documentation for max_slot_wal_keep_size tomention what
happnes when WAL exceeds the size, and additional rewrites.
(patch 4, catalogs.sgml, config.sgml)

- Folded parameter list of GetOldestKeepSegment().
(patch 1, 2. xlog.c)

- Provided the plural form of errdetail of checkpoint-time
warning. (patch 1, xlog.c: KeepLogSeg())

- Some cosmetic change and small refactor.
(patch 1, 2, 3)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Sep 13, 2018 at 6:30 PM Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> Hello.
>
> Thank you for the comments, Sawada-san, Peter.
>
> At Mon, 10 Sep 2018 19:52:24 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20180910(dot)195224(dot)22629595(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > At Thu, 6 Sep 2018 22:32:21 +0200, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote in <29bbd79d-696b-509e-578a-0fc38a3b9405(at)2ndquadrant(dot)com>
> > Thanks for pointing that. That's a major cause of confusion. Does
> > the following make sense?
> >
> > > Specify the maximum size of WAL files that <link
> > > linkend="streaming-replication-slots">replication slots</link>
> > > are allowed to retain in the <filename>pg_wal</filename>
> > > directory at checkpoint time. If
> > > <varname>max_slot_wal_keep_size</varname> is zero (the
> > > default), replication slots retain unlimited size of WAL files.
> > + If restart_lsn of a replication slot gets behind more than that
> > + bytes from the current LSN, the standby using the slot may not
> > + be able to reconnect due to removal of required WAL records.
> ...
> > > Also, I don't think 0 is a good value for the default behavior. 0 would
> > > mean that a slot is not allowed to retain any more WAL than already
> > > exists anyway. Maybe we don't want to support that directly, but it's a
> > > valid configuration. So maybe use -1 for infinity.
> >
> > In realtion to the reply just sent to Sawada-san, remain of a
> > slot can be at most 16MB in the 0 case with the default segment
> > size. So you're right in this sense. Will fix in the coming
> > version. Thanks.
>
> I did the following thinkgs in the new version.
>
> - Changed the disable (or infinite) and default value of
> max_slot_wal_keep_size to -1 from 0.
> (patch 1, 2. guc.c, xlog.c: GetOldestKeepSegment())
>
> - Fixed documentation for max_slot_wal_keep_size tomention what
> happnes when WAL exceeds the size, and additional rewrites.
> (patch 4, catalogs.sgml, config.sgml)
>
> - Folded parameter list of GetOldestKeepSegment().
> (patch 1, 2. xlog.c)
>
> - Provided the plural form of errdetail of checkpoint-time
> warning. (patch 1, xlog.c: KeepLogSeg())
>
> - Some cosmetic change and small refactor.
> (patch 1, 2, 3)
>

Sorry for the late response. The patch still can be applied to the
curent HEAD so I reviewed the latest patch.

The value of 'remain' and 'wal_status' might not be correct. Although
'wal_stats' shows 'lost' but we can get changes from the slot. I've
tested it with the following steps.

=# alter system set max_slot_wal_keep_size to '64MB'; -- while
wal_keep_segments is 0
=# select pg_reload_conf();
=# select slot_name, wal_status, remain, pg_size_pretty(remain) as
remain_pretty from pg_replication_slots ;
slot_name | wal_status | remain | remain_pretty
-----------+------------+----------+---------------
1 | streaming | 83885648 | 80 MB
(1 row)

** consume 80MB WAL, and do CHECKPOINT **

=# select slot_name, wal_status, remain, pg_size_pretty(remain) as
remain_pretty from pg_replication_slots ;
slot_name | wal_status | remain | remain_pretty
-----------+------------+--------+---------------
1 | lost | 0 | 0 bytes
(1 row)
=# select count(*) from pg_logical_slot_get_changes('1', NULL, NULL);
count
-------
15
(1 row)

-----
I got the following result with setting of wal_keep_segments >
max_slot_keep_size. The 'wal_status' shows 'streaming' although the
'remain' is 0.

=# select slot_name, wal_status, remain from pg_replication_slots limit 1;
slot_name | wal_status | remain
-----------+------------+--------
1 | streaming | 0
(1 row)

+ XLByteToSeg(targetLSN, restartSeg, wal_segment_size);
+ if (max_slot_wal_keep_size_mb >= 0 && currSeg <=
restartSeg + limitSegs)
+ {

You use limitSegs here but shouldn't we use keepSeg instead? Actually
I've commented this point for v6 patch before[1], and this had been
fixed in the v7 patch. However you're using limitSegs again from v8
patch again. I might be missing something though.

Changed the status to 'Waiting on Author'.

[1] /message-id/CAD21AoD0rChq7wQE%3D_o95quopcQGjcVG9omwdH07nT5cm81hzg%40mail.gmail.com
[2] /message-id/20180904.195250.144186960.horiguchi.kyotaro%40lab.ntt.co.jp

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATIONNTT Open Source Software Center

Hello.

At Mon, 22 Oct 2018 19:35:04 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBdfoLSgujPZ_TpnH5zdQz0jg-Y8OXtZ=TCO787Sey-=w(at)mail(dot)gmail(dot)com>
> On Thu, Sep 13, 2018 at 6:30 PM Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Sorry for the late response. The patch still can be applied to the

It's alright. Thanks.

> curent HEAD so I reviewed the latest patch.
> The value of 'remain' and 'wal_status' might not be correct. Although
> 'wal_stats' shows 'lost' but we can get changes from the slot. I've
> tested it with the following steps.
>
> =# alter system set max_slot_wal_keep_size to '64MB'; -- while
> wal_keep_segments is 0
> =# select pg_reload_conf();
> =# select slot_name, wal_status, remain, pg_size_pretty(remain) as
> remain_pretty from pg_replication_slots ;
> slot_name | wal_status | remain | remain_pretty
> -----------+------------+----------+---------------
> 1 | streaming | 83885648 | 80 MB
> (1 row)
>
> ** consume 80MB WAL, and do CHECKPOINT **
>
> =# select slot_name, wal_status, remain, pg_size_pretty(remain) as
> remain_pretty from pg_replication_slots ;
> slot_name | wal_status | remain | remain_pretty
> -----------+------------+--------+---------------
> 1 | lost | 0 | 0 bytes
> (1 row)
> =# select count(*) from pg_logical_slot_get_changes('1', NULL, NULL);
> count
> -------
> 15
> (1 row)

Mmm. The function looks into the segment already open before
losing the segment in the file system (precisely, its direcotory
entry has been deleted). So just 1 lost segment doesn't
matter. Please try losing more one segment.

=# select * from pg_logical_slot_get_changes('s1', NULL, NULL);
ERROR: unexpected pageaddr 0/29000000 in log segment 000000010000000000000023, offset 0

Or, instead just restarting will let the opened segment forgotten.

...
> 1 | lost | 0 | 0 bytes
(just restart)
> =# select * from pg_logical_slot_get_changes('s1', NULL, NULL);
> ERROR: requested WAL segment pg_wal/000000010000000000000029 has already been removed

I'm not sure this is counted to be a bug...

> -----
> I got the following result with setting of wal_keep_segments >
> max_slot_keep_size. The 'wal_status' shows 'streaming' although the
> 'remain' is 0.
>
> =# select slot_name, wal_status, remain from pg_replication_slots limit 1;
> slot_name | wal_status | remain
> -----------+------------+--------
> 1 | streaming | 0
> (1 row)
>
> + XLByteToSeg(targetLSN, restartSeg, wal_segment_size);
> + if (max_slot_wal_keep_size_mb >= 0 && currSeg <=
> restartSeg + limitSegs)
> + {
>
> You use limitSegs here but shouldn't we use keepSeg instead? Actually
> I've commented this point for v6 patch before[1], and this had been
> fixed in the v7 patch. However you're using limitSegs again from v8
> patch again. I might be missing something though.

No. keepSegs is the number of segments *actually* kept around. So
reverting it to keptSegs just resurrects the bug you pointed
upthread. What needed here is at most how many segments will be
kept. So raising limitSegs by wal_keep_segments fixes that.
Sorry for the sequence of silly bugs. TAP test for the case
added.

> Changed the status to 'Waiting on Author'.
>
> [1] /message-id/CAD21AoD0rChq7wQE%3D_o95quopcQGjcVG9omwdH07nT5cm81hzg%40mail.gmail.com
> [2] /message-id/20180904.195250.144186960.horiguchi.kyotaro%40lab.ntt.co.jp

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Thu, 25 Oct 2018 21:55:18 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20181025(dot)215518(dot)189844649(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > =# alter system set max_slot_wal_keep_size to '64MB'; -- while
> > wal_keep_segments is 0
> > =# select pg_reload_conf();
> > =# select slot_name, wal_status, remain, pg_size_pretty(remain) as
> > remain_pretty from pg_replication_slots ;
> > slot_name | wal_status | remain | remain_pretty
> > -----------+------------+----------+---------------
> > 1 | streaming | 83885648 | 80 MB
> > (1 row)
> >
> > ** consume 80MB WAL, and do CHECKPOINT **
> >
> > =# select slot_name, wal_status, remain, pg_size_pretty(remain) as
> > remain_pretty from pg_replication_slots ;
> > slot_name | wal_status | remain | remain_pretty
> > -----------+------------+--------+---------------
> > 1 | lost | 0 | 0 bytes
> > (1 row)
> > =# select count(*) from pg_logical_slot_get_changes('1', NULL, NULL);
> > count
> > -------
> > 15
> > (1 row)
>
> Mmm. The function looks into the segment already open before
> losing the segment in the file system (precisely, its direcotory
> entry has been deleted). So just 1 lost segment doesn't
> matter. Please try losing more one segment.

I considered this a bit more and the attached patch let
XLogReadRecord() check for segment removal every time it is
called and emits the following error in the case.

> =# select * from pg_logical_slot_get_changes('s1', NULL, NULL);
> ERROR: WAL record at 0/870001B0 no longer available
> DETAIL: The segment for the record has been removed.

The reason for doing that in the fucntion is it can happen also
for physical replication when walsender is active but far
behind. The removed(renamed)-but-still-open segment may be
recycled and can be overwritten while reading, and it will be
caught by page/record validation. It is substantially lost in
that sense. I don't think the strictness is useful for anything..

Thoughts?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Oct 25, 2018 at 9:56 PM Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> Hello.
>
> At Mon, 22 Oct 2018 19:35:04 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBdfoLSgujPZ_TpnH5zdQz0jg-Y8OXtZ=TCO787Sey-=w(at)mail(dot)gmail(dot)com>
> > On Thu, Sep 13, 2018 at 6:30 PM Kyotaro HORIGUCHI
> > <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > Sorry for the late response. The patch still can be applied to the
>
> It's alright. Thanks.
>
> > curent HEAD so I reviewed the latest patch.
> > The value of 'remain' and 'wal_status' might not be correct. Although
> > 'wal_stats' shows 'lost' but we can get changes from the slot. I've
> > tested it with the following steps.
> >
> > =# alter system set max_slot_wal_keep_size to '64MB'; -- while
> > wal_keep_segments is 0
> > =# select pg_reload_conf();
> > =# select slot_name, wal_status, remain, pg_size_pretty(remain) as
> > remain_pretty from pg_replication_slots ;
> > slot_name | wal_status | remain | remain_pretty
> > -----------+------------+----------+---------------
> > 1 | streaming | 83885648 | 80 MB
> > (1 row)
> >
> > ** consume 80MB WAL, and do CHECKPOINT **
> >
> > =# select slot_name, wal_status, remain, pg_size_pretty(remain) as
> > remain_pretty from pg_replication_slots ;
> > slot_name | wal_status | remain | remain_pretty
> > -----------+------------+--------+---------------
> > 1 | lost | 0 | 0 bytes
> > (1 row)
> > =# select count(*) from pg_logical_slot_get_changes('1', NULL, NULL);
> > count
> > -------
> > 15
> > (1 row)
>
> Mmm. The function looks into the segment already open before
> losing the segment in the file system (precisely, its direcotory
> entry has been deleted). So just 1 lost segment doesn't
> matter. Please try losing more one segment.
>
> =# select * from pg_logical_slot_get_changes('s1', NULL, NULL);
> ERROR: unexpected pageaddr 0/29000000 in log segment 000000010000000000000023, offset 0
>
> Or, instead just restarting will let the opened segment forgotten.
>
> ...
> > 1 | lost | 0 | 0 bytes
> (just restart)
> > =# select * from pg_logical_slot_get_changes('s1', NULL, NULL);
> > ERROR: requested WAL segment pg_wal/000000010000000000000029 has already been removed
>
> I'm not sure this is counted to be a bug...
>
>
> > -----
> > I got the following result with setting of wal_keep_segments >
> > max_slot_keep_size. The 'wal_status' shows 'streaming' although the
> > 'remain' is 0.
> >
> > =# select slot_name, wal_status, remain from pg_replication_slots limit 1;
> > slot_name | wal_status | remain
> > -----------+------------+--------
> > 1 | streaming | 0
> > (1 row)
> >
> > + XLByteToSeg(targetLSN, restartSeg, wal_segment_size);
> > + if (max_slot_wal_keep_size_mb >= 0 && currSeg <=
> > restartSeg + limitSegs)
> > + {
> >
> > You use limitSegs here but shouldn't we use keepSeg instead? Actually
> > I've commented this point for v6 patch before[1], and this had been
> > fixed in the v7 patch. However you're using limitSegs again from v8
> > patch again. I might be missing something though.
>
> No. keepSegs is the number of segments *actually* kept around. So
> reverting it to keptSegs just resurrects the bug you pointed
> upthread. What needed here is at most how many segments will be
> kept. So raising limitSegs by wal_keep_segments fixes that.
> Sorry for the sequence of silly bugs. TAP test for the case
> added.
>

Thank you for updating the patch. The 0001 - 0004 patches work fine
and looks good to me except for the following comment for the code.

+ /*
+ * Calculate keep segments by slots first. The second term of the
+ * condition is just a sanity check.
+ */
+ if (minSlotLSN != InvalidXLogRecPtr && minSlotSeg <= currSeg)
+ keepSegs = currSeg - minSlotSeg;

I think that we can use assertion of the second term of the condition
instead of just checking. If the function get minSlotSeg > currSeg the
return value will be incorrect. That means that the function requires
the condition is always true. Thought?

Since this comment can be deferred to committers I've marked this
patch as "Ready for Committer". For 0005 patch the issue I reported is
a relatively rare issue and is not critical, we can discuss it after
this patch gets committed.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Fri, Oct 26, 2018 at 11:26:36AM +0900, Kyotaro HORIGUCHI wrote:
> The reason for doing that in the fucntion is it can happen also
> for physical replication when walsender is active but far
> behind. The removed(renamed)-but-still-open segment may be
> recycled and can be overwritten while reading, and it will be
> caught by page/record validation. It is substantially lost in
> that sense. I don't think the strictness is useful for anything..

I was just coming by to look at bit at the patch series, and bumped
into that:

> + /*
> + * checkpoint can remove the segment currently looking for. make sure the
> + * current segment is still exists. We check this only once per record.
> + */
> + XLByteToSeg(targetPagePtr, targetSegNo, state->wal_segment_size);
> + if (targetSegNo <= XLogGetLastRemovedSegno())
> + ereport(ERROR,
> + (errcode(ERRCODE_NO_DATA),
> + errmsg("WAL record at %X/%X no longer available",
> + (uint32)(RecPtr >> 32), (uint32) RecPtr),
> + errdetail("The segment for the record has been removed.")));
> +

ereport should not be called within xlogreader.c as a base rule:
* This file is compiled as both front-end and backend code, so it
* may not use ereport, server-defined static variables, etc.
--
Michael

On Mon, Nov 19, 2018 at 01:39:58PM +0900, Michael Paquier wrote:
> I was just coming by to look at bit at the patch series, and bumped
> into that:

So I have been looking at the last patch series 0001-0004 posted on this
thread, and coming from here:
https://postgr.es/m/20181025.215518.189844649.horiguchi.kyotaro@lab.ntt.co.jp

/* check that the slot is gone */
SELECT * FROM pg_replication_slots
It could be an idea to switch to the expanded mode here, not that it
matters much still..

+IsLsnStillAvaiable(XLogRecPtr targetLSN, uint64 *restBytes)
You mean Available here, not Avaiable. This function is only used when
scanning for slot information with pg_replication_slots, so wouldn't it
be better to just return the status string in this case?

Not sure I see the point of the "remain" field, which can be found with
a simple calculation using the current insertion LSN, the segment size
and the amount of WAL that the slot is retaining. It may be interesting
to document a query to do that though.

GetOldestXLogFileSegNo() has race conditions if WAL recycling runs in
parallel, no? How is it safe to scan pg_wal on a process querying
pg_replication_slots while another process may manipulate its contents
(aka the checkpointer or just the startup process with an
end-of-recovery checkpoint.). This routine relies on unsafe
assumptions as this is not concurrent-safe. You can avoid problems by
making sure instead that lastRemovedSegNo is initialized correctly at
startup, which would be normally one segment older than what's in
pg_wal, which feels a bit hacky to rely on to track the oldest segment.

It seems to me that GetOldestXLogFileSegNo() should also check for
segments matching the current timeline, no?

+ if (prev_lost_segs != lost_segs)
+ ereport(WARNING,
+ (errmsg ("some replication slots have lost
required WAL segments"),
+ errdetail_plural(
+ "The mostly affected slot has lost %ld
segment.",
+ "The mostly affected slot has lost %ld
segments.",
+ lost_segs, lost_segs)));
This can become very noisy with the time, and it would be actually
useful to know which replication slot is impacted by that.

+ slot doesn't have valid restart_lsn, this field
Missing a determinant here, and restart_lsn should have a <literal>
markup.

+ many WAL segments that they fill up the space allotted
s/allotted/allocated/.

+ available. The last two states are seen only when
+ <xref linkend="guc-max-slot-wal-keep-size"/> is non-negative. If the
+ slot doesn't have valid restart_lsn, this field
+ is <literal>unknown</literal>.
I am a bit confused by this statement. The last two states are "lost"
and "keeping", but shouldn't "keeping" be the state showing up by
default as it means that all WAL segments are kept around.

+# Advance WAL by ten segments (= 160MB) on master
+advance_wal($node_master, 10);
+$node_master->safe_psql('postgres', "CHECKPOINT;");
This makes the tests very costly, which is something we should avoid as
much as possible. One trick which could be used here, on top of
reducing the number of segment switches, is to use initdb
--wal-segsize=1.
--
Michael

Thank you for piking this and sorry being late.

At Mon, 19 Nov 2018 13:39:58 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20181119043958(dot)GE4400(at)paquier(dot)xyz>
> ereport should not be called within xlogreader.c as a base rule:

Ouch! I forgot that. Fixed to use report_invalid_record slightly
changing the message. The code is not required (or cannot be
used) on frontend so #ifndef FRONTENDed the code.

At Tue, 20 Nov 2018 14:07:44 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20181120050744(dot)GJ4400(at)paquier(dot)xyz>
> On Mon, Nov 19, 2018 at 01:39:58PM +0900, Michael Paquier wrote:
> > I was just coming by to look at bit at the patch series, and bumped
> > into that:
>
> So I have been looking at the last patch series 0001-0004 posted on this
> thread, and coming from here:
> https://postgr.es/m/20181025.215518.189844649.horiguchi.kyotaro@lab.ntt.co.jp
>
> /* check that the slot is gone */
> SELECT * FROM pg_replication_slots
> It could be an idea to switch to the expanded mode here, not that it
> matters much still..

No problem doing that. Done.

TAP test complains that it still uses recovery.conf. Fixed. On
the way doing that I added parameter primary_slot_name to
init_from_backup in PostgresNode.pm

> +IsLsnStillAvaiable(XLogRecPtr targetLSN, uint64 *restBytes)
> You mean Available here, not Avaiable. This function is only used when
> scanning for slot information with pg_replication_slots, so wouldn't it
> be better to just return the status string in this case?

Mmm. Sure. Auto-completion hid it from my eyes. Fixed the name.
The fix sounds reasonable. The function was created as returning
boolean and the name doen't fit the current function. I renamed
the name to GetLsnAvailability() that returns a string.

> Not sure I see the point of the "remain" field, which can be found with
> a simple calculation using the current insertion LSN, the segment size
> and the amount of WAL that the slot is retaining. It may be interesting
> to document a query to do that though.

It's not that simple. wal_segment_size, max_slot_wal_keep_size,
wal_keep_segments, max_slot_wal_keep_size and the current LSN are
invoved in the calculation which including several conditional
branches, maybe as you see upthread. We could show "the largest
current LSN until WAL is lost" but the "current LSN" is not shown
there. So it is showing the "remain".

> GetOldestXLogFileSegNo() has race conditions if WAL recycling runs in
> parallel, no? How is it safe to scan pg_wal on a process querying
> pg_replication_slots while another process may manipulate its contents
> (aka the checkpointer or just the startup process with an
> end-of-recovery checkpoint.). This routine relies on unsafe
> assumptions as this is not concurrent-safe. You can avoid problems by
> making sure instead that lastRemovedSegNo is initialized correctly at
> startup, which would be normally one segment older than what's in
> pg_wal, which feels a bit hacky to rely on to track the oldest segment.

Concurrent recycling makes the function's result vary between the
segment numbers before and after it. It is unstable but doesn't
matter so much. The reason for the timing is to avoid extra
startup time by a scan over pg_wal that is unncecessary in most
cases.

Anyway the attached patch initializes lastRemovedSegNo in
StartupXLOG().

> It seems to me that GetOldestXLogFileSegNo() should also check for
> segments matching the current timeline, no?

RemoveOldXlogFiles() ignores timeline and the function is made to
behave the same way (in different manner). I added a comment for
the behavior in the function.

> + if (prev_lost_segs != lost_segs)
> + ereport(WARNING,
> + (errmsg ("some replication slots have lost
> required WAL segments"),
> + errdetail_plural(
> + "The mostly affected slot has lost %ld
> segment.",
> + "The mostly affected slot has lost %ld
> segments.",
> + lost_segs, lost_segs)));
> This can become very noisy with the time, and it would be actually
> useful to know which replication slot is impacted by that.

One message per one segment doen't seem so noisy. The reason for
not showing slot identifier individually is just to avoid
complexity comes from involving slot details. DBAs will see the
details in pg_stat_replication.

Anyway I did that in the attached patch. ReplicationSlotsBehind
returns the list of the slot names that behind specified
LSN. With this patch the messages looks as the follows:

WARNING: some replication slots have lost required WAL segments
DETAIL: Slot s1 lost 8 segment(s).
WARNING: some replication slots have lost required WAL segments
DETAIL: Slots s1, s2, s3 lost at most 9 segment(s).

> + slot doesn't have valid restart_lsn, this field
> Missing a determinant here, and restart_lsn should have a <literal>
> markup.

structfield? Reworded as below:

| non-negative. If <structfield>restart_lsn</structfield> is NULL, this
| field is <literal>unknown</literal>.

I changed "the slot" with "this slot" in the two added fields
(wal_status, remain).

> + many WAL segments that they fill up the space allotted
> s/allotted/allocated/.

Fixed.

> + available. The last two states are seen only when
> + <xref linkend="guc-max-slot-wal-keep-size"/> is non-negative. If the
> + slot doesn't have valid restart_lsn, this field
> + is <literal>unknown</literal>.
> I am a bit confused by this statement. The last two states are "lost"
> and "keeping", but shouldn't "keeping" be the state showing up by
> default as it means that all WAL segments are kept around.

It's "streaming". I didn't came up with nice words to
distinguish the two states. I'm not sure "keep around" exactly
means but "keeping" here means rather "just not removed yet". The
states could be reworded as the follows:

streaming: kept/keeping/(secure, in the first version)
keeping : mortal/about to be removed
lost/unkown : (lost/unknown)

Do you have any better wording?

> +# Advance WAL by ten segments (= 160MB) on master
> +advance_wal($node_master, 10);
> +$node_master->safe_psql('postgres', "CHECKPOINT;");
> This makes the tests very costly, which is something we should avoid as
> much as possible. One trick which could be used here, on top of
> reducing the number of segment switches, is to use initdb
> --wal-segsize=1.

That sounds nice. Done. In the new version the number of segments
can be reduced and a new test item for the initial unkonwn state
as the first item.

Please find the attached new version.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Thu, 20 Dec 2018 16:24:38 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20181220(dot)162438(dot)121484007(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> Thank you for piking this and sorry being late.
>
> At Mon, 19 Nov 2018 13:39:58 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20181119043958(dot)GE4400(at)paquier(dot)xyz>
> > ereport should not be called within xlogreader.c as a base rule:
>
> Ouch! I forgot that. Fixed to use report_invalid_record slightly
> changing the message. The code is not required (or cannot be
> used) on frontend so #ifndef FRONTENDed the code.
>
> At Tue, 20 Nov 2018 14:07:44 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20181120050744(dot)GJ4400(at)paquier(dot)xyz>
> > On Mon, Nov 19, 2018 at 01:39:58PM +0900, Michael Paquier wrote:
> > > I was just coming by to look at bit at the patch series, and bumped
> > > into that:
> >
> > So I have been looking at the last patch series 0001-0004 posted on this
> > thread, and coming from here:
> > https://postgr.es/m/20181025.215518.189844649.horiguchi.kyotaro@lab.ntt.co.jp
> >
> > /* check that the slot is gone */
> > SELECT * FROM pg_replication_slots
> > It could be an idea to switch to the expanded mode here, not that it
> > matters much still..
>
> No problem doing that. Done.
>
> TAP test complains that it still uses recovery.conf. Fixed. On
> the way doing that I added parameter primary_slot_name to
> init_from_backup in PostgresNode.pm
>
> > +IsLsnStillAvaiable(XLogRecPtr targetLSN, uint64 *restBytes)
> > You mean Available here, not Avaiable. This function is only used when
> > scanning for slot information with pg_replication_slots, so wouldn't it
> > be better to just return the status string in this case?
>
> Mmm. Sure. Auto-completion hid it from my eyes. Fixed the name.
> The fix sounds reasonable. The function was created as returning
> boolean and the name doen't fit the current function. I renamed
> the name to GetLsnAvailability() that returns a string.
>
> > Not sure I see the point of the "remain" field, which can be found with
> > a simple calculation using the current insertion LSN, the segment size
> > and the amount of WAL that the slot is retaining. It may be interesting
> > to document a query to do that though.
>
> It's not that simple. wal_segment_size, max_slot_wal_keep_size,
> wal_keep_segments, max_slot_wal_keep_size and the current LSN are
> invoved in the calculation which including several conditional
> branches, maybe as you see upthread. We could show "the largest
> current LSN until WAL is lost" but the "current LSN" is not shown
> there. So it is showing the "remain".
>
> > GetOldestXLogFileSegNo() has race conditions if WAL recycling runs in
> > parallel, no? How is it safe to scan pg_wal on a process querying
> > pg_replication_slots while another process may manipulate its contents
> > (aka the checkpointer or just the startup process with an
> > end-of-recovery checkpoint.). This routine relies on unsafe
> > assumptions as this is not concurrent-safe. You can avoid problems by
> > making sure instead that lastRemovedSegNo is initialized correctly at
> > startup, which would be normally one segment older than what's in
> > pg_wal, which feels a bit hacky to rely on to track the oldest segment.
>
> Concurrent recycling makes the function's result vary between the
> segment numbers before and after it. It is unstable but doesn't
> matter so much. The reason for the timing is to avoid extra
> startup time by a scan over pg_wal that is unncecessary in most
> cases.
>
> Anyway the attached patch initializes lastRemovedSegNo in
> StartupXLOG().
>
> > It seems to me that GetOldestXLogFileSegNo() should also check for
> > segments matching the current timeline, no?
>
> RemoveOldXlogFiles() ignores timeline and the function is made to
> behave the same way (in different manner). I added a comment for
> the behavior in the function.
>
> > + if (prev_lost_segs != lost_segs)
> > + ereport(WARNING,
> > + (errmsg ("some replication slots have lost
> > required WAL segments"),
> > + errdetail_plural(
> > + "The mostly affected slot has lost %ld
> > segment.",
> > + "The mostly affected slot has lost %ld
> > segments.",
> > + lost_segs, lost_segs)));
> > This can become very noisy with the time, and it would be actually
> > useful to know which replication slot is impacted by that.
>
> One message per one segment doen't seem so noisy. The reason for
> not showing slot identifier individually is just to avoid
> complexity comes from involving slot details. DBAs will see the
> details in pg_stat_replication.
>
> Anyway I did that in the attached patch. ReplicationSlotsBehind
> returns the list of the slot names that behind specified
> LSN. With this patch the messages looks as the follows:
>
> WARNING: some replication slots have lost required WAL segments
> DETAIL: Slot s1 lost 8 segment(s).
> WARNING: some replication slots have lost required WAL segments
> DETAIL: Slots s1, s2, s3 lost at most 9 segment(s).
>
> > + slot doesn't have valid restart_lsn, this field
> > Missing a determinant here, and restart_lsn should have a <literal>
> > markup.
>
> structfield? Reworded as below:
>
> | non-negative. If <structfield>restart_lsn</structfield> is NULL, this
> | field is <literal>unknown</literal>.
>
> I changed "the slot" with "this slot" in the two added fields
> (wal_status, remain).
>
> > + many WAL segments that they fill up the space allotted
> > s/allotted/allocated/.
>
> Fixed.
>
> > + available. The last two states are seen only when
> > + <xref linkend="guc-max-slot-wal-keep-size"/> is non-negative. If the
> > + slot doesn't have valid restart_lsn, this field
> > + is <literal>unknown</literal>.
> > I am a bit confused by this statement. The last two states are "lost"
> > and "keeping", but shouldn't "keeping" be the state showing up by
> > default as it means that all WAL segments are kept around.
>
> It's "streaming". I didn't came up with nice words to
> distinguish the two states. I'm not sure "keep around" exactly
> means but "keeping" here means rather "just not removed yet". The
> states could be reworded as the follows:
>
> streaming: kept/keeping/(secure, in the first version)
> keeping : mortal/about to be removed
> lost/unkown : (lost/unknown)
>
> Do you have any better wording?
>
> > +# Advance WAL by ten segments (= 160MB) on master
> > +advance_wal($node_master, 10);
> > +$node_master->safe_psql('postgres', "CHECKPOINT;");
> > This makes the tests very costly, which is something we should avoid as
> > much as possible. One trick which could be used here, on top of
> > reducing the number of segment switches, is to use initdb
> > --wal-segsize=1.
>
> That sounds nice. Done. In the new version the number of segments
> can be reduced and a new test item for the initial unkonwn state
> as the first item.
>
> Please find the attached new version.

Rebased. No conflict found since the last version.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

On 2019-01-30 10:42:04 +0900, Kyotaro HORIGUCHI wrote:
> From 270aff9b08ced425b4c4e23b53193285eb2359a6 Mon Sep 17 00:00:00 2001
> From: Kyotaro Horiguchi <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> Date: Thu, 21 Dec 2017 21:20:20 +0900
> Subject: [PATCH 1/6] Add WAL relief vent for replication slots
>
> Adds a capability to limit the number of segments kept by replication
> slots by a GUC variable.

Maybe I'm missing something, but how does this prevent issues with
active slots that are currently accessing the WAL this patch now
suddenly allows to be removed? Especially for logical slots that seems
not unproblematic?

Besides that, this patch needs substantial spelling / language / comment
polishing. Horiguchi-san, it'd probably be good if you could make a
careful pass, and then maybe a native speaker could go over it?

Greetings,

Andres Freund

At Fri, 15 Feb 2019 19:13:23 -0800, Andres Freund <andres(at)anarazel(dot)de> wrote in <20190216031323(dot)t7tfrae4l6zqtseo(at)alap3(dot)anarazel(dot)de>
> Hi,
>
> On 2019-01-30 10:42:04 +0900, Kyotaro HORIGUCHI wrote:
> > From 270aff9b08ced425b4c4e23b53193285eb2359a6 Mon Sep 17 00:00:00 2001
> > From: Kyotaro Horiguchi <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > Date: Thu, 21 Dec 2017 21:20:20 +0900
> > Subject: [PATCH 1/6] Add WAL relief vent for replication slots
> >
> > Adds a capability to limit the number of segments kept by replication
> > slots by a GUC variable.
>
> Maybe I'm missing something, but how does this prevent issues with
> active slots that are currently accessing the WAL this patch now
> suddenly allows to be removed? Especially for logical slots that seems
> not unproblematic?

No matter whether logical or physical, when reading an
overwritten page of a recycled/renamed segment file, page
validation at reading-in finds that it is of a different segment
than expected. 0006 in [1] introduces more active checking on
that.

[1] /message-id/20181220.162438.121484007.horiguchi.kyotaro%40lab.ntt.co.jp

> Besides that, this patch needs substantial spelling / language / comment
> polishing. Horiguchi-san, it'd probably be good if you could make a
> careful pass, and then maybe a native speaker could go over it?

Thank you for your kind suggestion. As I did for other patches,
I'll review it by myself and come up with a new version soon.

# I often don't understand what I wrote:(

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Fri, 22 Feb 2019 10:12:51 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20190222(dot)101251(dot)03333542(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
horiguchi.kyotaro> At Fri, 15 Feb 2019 19:13:23 -0800, Andres Freund <andres(at)anarazel(dot)de> wrote in <20190216031323(dot)t7tfrae4l6zqtseo(at)alap3(dot)anarazel(dot)de>
> > Maybe I'm missing something, but how does this prevent issues with
> > active slots that are currently accessing the WAL this patch now
> > suddenly allows to be removed? Especially for logical slots that seems
> > not unproblematic?
>
> No matter whether logical or physical, when reading an
> overwritten page of a recycled/renamed segment file, page
> validation at reading-in finds that it is of a different segment
> than expected. 0006 in [1] introduces more active checking on
> that.
>
> [1] /message-id/20181220.162438.121484007.horiguchi.kyotaro%40lab.ntt.co.jp
>
> > Besides that, this patch needs substantial spelling / language / comment
> > polishing. Horiguchi-san, it'd probably be good if you could make a
> > careful pass, and then maybe a native speaker could go over it?
>
> Thank you for your kind suggestion. As I did for other patches,
> I'll review it by myself and come up with a new version soon.

I checked spelling comments and commit messages, then perhaps
corrected and improved them. I hope they looks nice..

0006 is separate from 0001, since I still doubt the necessity.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi all,

Being interested by this feature, I did a patch review.

This features adds the GUC "max_slot_wal_keep_size". This is the maximum amount
of WAL that can be kept in "pg_wal" by active slots.

If the amount of WAL is superior to this limit, the slot is deactivated and
its status (new filed in pg_replication_slot) is set as "lost".

Patching
========

The patch v13-0003 does not apply on HEAD anymore.

The patch v13-0005 applies using "git am --ignore-space-change"

Other patches applies correctly.

Please, find attached the v14 set of patches rebased on master.

Documentation
=============

The documentation explains the GUC and related columns in "pg_replication_slot".

It reflects correctly the current behavior of the patch.

Usability
=========

The patch implement what it described. It is easy to enable and disable. The
GUC name is describing correctly its purpose.

This feature is useful in some HA scenario where slot are required (eg. no
possible archiving), but where primary availability is more important than
standbys.

In "pg_replication_slots" view, the new "wal_status" field is misleading.
Consider this sentence and the related behavior from documentation
(catalogs.sgml):

<literal>keeping</literal> means that some of them are to be removed by the
next checkpoint.

"keeping" appears when the current checkpoint will delete some WAL further than
"current_lsn - max_slot_wal_keep_size", but still required by at least one slot.
As some WAL required by some slots will be deleted quite soon, probably before
anyone can react, "keeping" status is misleading here. We are already in the
red zone.

I would expect this "wal_status" to be:

- streaming: slot lag between 0 and "max_wal_size"
- keeping: slot lag between "max_wal_size" and "max_slot_wal_keep_size". the
slot actually protect some WALs from being deleted
- lost: slot lag superior of max_slot_wal_keep_size. The slot couldn't protect
some WAL from deletion

Documentation follows with:

The last two states are seen only when max_slot_wal_keep_size is
non-negative

This is true with the current behavior. However, if "keeping" is set as soon as
te slot lag is superior than "max_wal_size", this status could be useful even
with "max_slot_wal_keep_size = -1". As soon as a slot is stacking WALs that
should have been removed by previous checkpoint, it "keeps" them.

Feature tests
=============

I have played with various traffic shaping setup between nodes, to observe how
columns "active", "wal_status" and "remain" behaves in regard to each others
using:

while true; do
sleep 0.3;
psql -p 5432 -AXtc "
select now(), active, restart_lsn, wal_status, pg_size_pretty(remain)
from pg_replication_slots
where slot_name='slot_limit_st'"
done

The primary is created using:

initdb -U postgres -D slot_limit_pr --wal-segsize=1

cat<<EOF >>slot_limit_pr/postgresql.conf
port=5432
max_wal_size = 3MB
min_wal_size = 2MB
max_slot_wal_keep_size = 4MB
logging_collector = on
synchronous_commit = off
EOF

WAL activity is generated using a simple pgbench workload. Then, during
this activity, packets on loopback are delayed using:

tc qdisc add dev lo root handle 1:0 netem delay 140msec

Here is how the wal_status behave. I removed the timestamps, but the
record order is the original one:

t|1/7B116898|streaming|1872 kB
t|1/7B1A0000|lost|0 bytes
t|1/7B320000|keeping|0 bytes
t|1/7B780000|lost|0 bytes
t|1/7BB00000|keeping|0 bytes
t|1/7BE00000|keeping|0 bytes
t|1/7C100000|lost|0 bytes
t|1/7C400000|keeping|0 bytes
t|1/7C700000|lost|0 bytes
t|1/7CA40000|keeping|0 bytes
t|1/7CDE0000|lost|0 bytes
t|1/7D100000|keeping|0 bytes
t|1/7D400000|keeping|0 bytes
t|1/7D7C0000|keeping|0 bytes
t|1/7DB40000|keeping|0 bytes
t|1/7DE60000|lost|0 bytes
t|1/7E180000|keeping|0 bytes
t|1/7E500000|keeping|0 bytes
t|1/7E860000|lost|0 bytes
t|1/7EB80000|keeping|0 bytes
[...x15]
t|1/80800000|keeping|0 bytes
t|1/80900000|streaming|940 kB
t|1/80A00000|streaming|1964 kB

When increasing the network delay to 145ms, the slot has been lost for real.
Note that it has been shown as lost but active twice (during approx 0.6s) before
being deactivated.

t|1/85700000|streaming|2048 kB
t|1/85800000|keeping|0 bytes
t|1/85940000|lost|0 bytes
t|1/85AC0000|lost|0 bytes
f|1/85C40000|lost|0 bytes

Finally, at least once the following messages appeared in primary logs
**before** the "wal_status" changed from "keeping" to "streaming":

WARNING: some replication slots have lost required WAL segments

So the slot lost one WAL, but the standby has been able to catch-up anyway.

My humble opinion about these results:

* after many different tests, the status "keeping" appears only when "remain"
equals 0. In current implementation, "keeping" really adds no value...
* "remain" should be NULL if "max_slot_wal_keep_size=-1 or if the slot isn't
active
* the "lost" status should be a definitive status
* it seems related, but maybe the "wal_status" should be set as "lost"
only when the slot has been deactivate ?
* logs should warn about a failing slot as soon as it is effectively
deactivated, not before.

Thanks for reviewing!

At Thu, 27 Jun 2019 16:22:56 +0200, Jehan-Guillaume de Rorthais <jgdr(at)dalibo(dot)com> wrote in <20190627162256(dot)4f4872b8(at)firost>
> Hi all,
>
> Being interested by this feature, I did a patch review.
>
> This features adds the GUC "max_slot_wal_keep_size". This is the maximum amount
> of WAL that can be kept in "pg_wal" by active slots.
>
> If the amount of WAL is superior to this limit, the slot is deactivated and
> its status (new filed in pg_replication_slot) is set as "lost".

This patch doesn't deactivate walsender. A walsender stops by
itself when it finds that it cannot continue ongoing replication.

> Patching
> ========
>
> The patch v13-0003 does not apply on HEAD anymore.
>
> The patch v13-0005 applies using "git am --ignore-space-change"
>
> Other patches applies correctly.
>
> Please, find attached the v14 set of patches rebased on master.

Sorry for missing this for log time. It is hit by 67b9b3ca32
again so I repost a rebased version.

> Documentation
> =============
>
> The documentation explains the GUC and related columns in "pg_replication_slot".
>
> It reflects correctly the current behavior of the patch.
>
>
> Usability
> =========
>
> The patch implement what it described. It is easy to enable and disable. The
> GUC name is describing correctly its purpose.
>
> This feature is useful in some HA scenario where slot are required (eg. no
> possible archiving), but where primary availability is more important than
> standbys.

Yes. Thanks for the clear explanation on the purpose.

> In "pg_replication_slots" view, the new "wal_status" field is misleading.
> Consider this sentence and the related behavior from documentation
> (catalogs.sgml):
>
> <literal>keeping</literal> means that some of them are to be removed by the
> next checkpoint.
>
> "keeping" appears when the current checkpoint will delete some WAL further than
> "current_lsn - max_slot_wal_keep_size", but still required by at least one slot.
> As some WAL required by some slots will be deleted quite soon, probably before
> anyone can react, "keeping" status is misleading here. We are already in the
> red zone.

It may be "losing", which would be less misleading.

> I would expect this "wal_status" to be:
>
> - streaming: slot lag between 0 and "max_wal_size"
> - keeping: slot lag between "max_wal_size" and "max_slot_wal_keep_size". the
> slot actually protect some WALs from being deleted
> - lost: slot lag superior of max_slot_wal_keep_size. The slot couldn't protect
> some WAL from deletion

I agree that comparing to max_wal_size is meaningful. The revised
version behaves as that.

> Documentation follows with:
>
> The last two states are seen only when max_slot_wal_keep_size is
> non-negative
>
> This is true with the current behavior. However, if "keeping" is set as soon as
> te slot lag is superior than "max_wal_size", this status could be useful even
> with "max_slot_wal_keep_size = -1". As soon as a slot is stacking WALs that
> should have been removed by previous checkpoint, it "keeps" them.

I revised the documentation that way. Both
view-pg-replication-slots.html and
runtime-config-replication.html are reworded.

> Feature tests
> =============
>
> I have played with various traffic shaping setup between nodes, to observe how
> columns "active", "wal_status" and "remain" behaves in regard to each others
> using:
>
> while true; do
>
<removed testing details>
>
> Finally, at least once the following messages appeared in primary logs
> **before** the "wal_status" changed from "keeping" to "streaming":
>
> WARNING: some replication slots have lost required WAL segments
>
> So the slot lost one WAL, but the standby has been able to catch-up anyway.

Thanks for the intensive test run. It is quite helpful.

> My humble opinion about these results:
>
> * after many different tests, the status "keeping" appears only when "remain"
> equals 0. In current implementation, "keeping" really adds no value...

Hmm. I agree that given that the "lost" (or "losing" in the
patch) state is a not definite state. That is, the slot may
recover from the state.

> * "remain" should be NULL if "max_slot_wal_keep_size=-1 or if the slot isn't
> active

The revised version shows the following statuses.

streaming / NULL max_slot_wal_keep_size is -1
unkown / NULL mswks >= 0 and restart_lsn is invalid
<status> / <bytes> elsewise

> * the "lost" status should be a definitive status
> * it seems related, but maybe the "wal_status" should be set as "lost"
> only when the slot has been deactivate ?

Agreed. While replication is active, if required segments seems
to be lost once, delayed walreceiver ack can advance restart_lsn
to "safe" zone later. So, in the revised version, if the segment
for restart_lsn has been removed, GetLsnAvailablity() returns
"losing" if walsender is active and "lost" if not.

> * logs should warn about a failing slot as soon as it is effectively
> deactivated, not before.

Agreed. Slots on which walsender is running are exlucded from the
output of ReplicationSlotsEnumerateBehnds. As theresult the "some
replcation slots lost.." is emitted after related walsender
stops.

I attach the revised patch. I'll repost the polished version
sooner.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Tue, 30 Jul 2019 21:30:45 +0900 (Tokyo Standard Time), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in <20190730(dot)213045(dot)221405075(dot)horikyota(dot)ntt(at)gmail(dot)com>
> I attach the revised patch. I'll repost the polished version
> sooner.

(Mainly TAP test and documentation, code comments will be revised)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center