Horton

Thread

Commits

  1. Fix newly introduced 010_keep_recycled_wals.pl

  2. Avoid deleting critical WAL segments during pg_rewind

  1. pg_rewind WAL segments deletion pitfall

    Polina Bungina <bungina@gmail.com> — 2022-08-23T15:46:30Z

    Hello,

It seems for me that there is currently a pitfall in the pg_rewind
implementation.

Imagine the following situation:


There is a cluster consisting of a primary with the following
configuration: wal_level=‘replica’, archive_mode=‘on’ and a replica.

   1. The primary that is not fast enough in archiving WAL segments (e.g.
   network issues, high CPU/Disk load...)
   2. The primary fails
   3. The replica is promoted
   4. We are not lucky enough, the new and the old primary’s timelines
   diverged, we need to run pg_rewind
   5. We are even less lucky: the old primary still has some WAL segments
   with .ready signal files that were generated before the point of divergence
   and were not archived. (e.g. 000000020004D20200000095.done,
   000000020004D20200000096.ready, 000000020004D20200000097.ready,
   000000020004D20200000098.ready)
   6. The promoted primary runs for some time and recycles the old WAL
   segments.
   7. We revive the old primary and try to rewind it
   8. When pg_rewind finished successfully, we see that the WAL segments
   with .ready files are removed, because they were already absent on the
   promoted replica. We end up in a situation where we completely lose some
   WAL segments, even though we had a clear sign that they were not
archived and
   more importantly, pg_rewind read these segments while collecting
   information about the data blocks.
   9. The old primary fails to start because of the missing WAL segments
   (more strictly, the records between the last common checkpoint and the
   point of divergence) with the following log record: "ERROR:  requested WAL
   segment 000000020004D20200000096 has already been removed"


In this situation, after pg_rewind:
archived:

000000020004D20200000095

000000020004D20200000099.partial

000000030004D20200000099


the following segments are lost:

000000020004D20200000096

000000020004D20200000097

000000020004D20200000098


Thus, my thoughts are: why can’t pg_rewind be a little bit wiser in terms
of creating filemap for WALs? Can it preserve the WAL segments that contain
those potentially lost records (> the last common checkpoint and  < the
point of divergence) on the target? (see the patch attached)


If I am missing something however, please correct me or explain why it is
not possible to implement this straightforward solution.


Thank you,

Polina Bungina

  2. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-08-25T07:49:17Z

    In the first place, this is not a bug. (At least doesn't seem.)
If you mean to propose behavioral changes, -hackers is the place.

At Tue, 23 Aug 2022 17:46:30 +0200, Полина Бунгина <bungina@gmail.com> wrote in 
>    4. We are not lucky enough, the new and the old primary’s timelines
>    diverged, we need to run pg_rewind
>    5. We are even less lucky: the old primary still has some WAL segments
>    with .ready signal files that were generated before the point of divergence
>    and were not archived.

That dones't harm pg_rewind at all.

>    6. The promoted primary runs for some time and recycles the old WAL
>    segments.
>    7. We revive the old primary and try to rewind it
>    8. When pg_rewind finished successfully, we see that the WAL segments
>    with .ready files are removed, because they were already absent on the
>    promoted replica. We end up in a situation where we completely lose some
>    WAL segments, even though we had a clear sign that they were not
>    archived and
>    more importantly, pg_rewind read these segments while collecting
>    information about the data blocks.

In terms of syncing the old primary to the new primary, no data has
been lost. The "lost" segments are anyway unusable for the new primary
since they no longer compatible with it.  How do you intended to use
the WAL files for the incompatible cluster?

>    9. The old primary fails to start because of the missing WAL segments
>    (more strictly, the records between the last common checkpoint and the
>    point of divergence) with the following log record: "ERROR:  requested WAL
>    segment 000000020004D20200000096 has already been removed"

That means that the tail end of the rewound old primary has been lost
on the new primary's pg_wal.  In that case, you need to somehow
copy-in the archived WAL files on the new primary. You can just do
that or you can set up restore_command properly.
> Thus, my thoughts are: why can’t pg_rewind be a little bit wiser in terms
> of creating filemap for WALs? Can it preserve the WAL segments that contain
> those potentially lost records (> the last common checkpoint and  < the
> point of divergence) on the target? (see the patch attached)

Since they are not really needed once rewind completes.

> If I am missing something however, please correct me or explain why it is
> not possible to implement this straightforward solution.

Maybe you're mistaking the operation.  If I understand the situation
correctly, I think the following steps replays your "issue" and then
resolve that.


# killall -9 postgres
# rm -r oldprim newprim oldarch newarch oldprim.log newprim.log
mkdir newarch oldarch
initdb -k -D oldprim
echo "archive_mode = 'always'">> oldprim/postgresql.conf
echo "archive_command = 'cp %p `pwd`/oldarch/%f'">> oldprim/postgresql.conf
pg_ctl -D oldprim -o '-p 5432' -l oldprim.log start
psql -p 5432 -c 'create table t(a int)'
pg_basebackup -D newprim -p 5432
echo "primary_conninfo='host=/tmp port=5432'">> oldprim/postgresql.conf
echo "archive_command = 'cp %p `pwd`/newarch/%f'">> newprim/postgresql.conf
touch newprim/standby.signal
pg_ctl -D newprim -o '-p 5433' -l newprim.log start
pg_ctl -D newprim promote
for i in $(seq 1 4); do psql -p 5432 -c 'insert into t values(0); select pg_switch_wal();'; done
psql -p 5432 -c 'checkpoint'
pg_ctl -D oldprim stop
echo "restore_command = 'cp `pwd`/oldarch/%f %p'">> oldprim/postgresql.conf
# pg_rewind -D oldprim --source-server='port=5433' # fails
pg_rewind -D oldprim --source-server='port=5433' -c
for i in $(seq 1 4); do psql -p 5433 -c 'insert into t values(0); select pg_switch_wal();'; done
psql -p 5433 -c 'checkpoint'
echo "primary_conninfo='host=/tmp port=5433'">> oldprim/postgresql.conf
touch oldprim/standby.signal

postgres -D oldprim

>  FATAL:  could not receive data from WAL stream: ERROR:  requested WAL segment 000000020000000000000003 has already been removed

[ctrl-C]
======

Now that the old primary requires older WAL files *on the new
primary*.  Here, define restore command to do that.

=====
echo "restore_command='cp `pwd`/newarch/%f %p'">> oldprim/postgresql.conf
postgres -D oldprim
=====

Now the old primary run as the standby of the new primary.

> LOG:  restored log file "000000020000000000000006" from archive
> LOG:  consistent recovery state reached at 0/30020B0
> LOG:  database system is ready to accept read-only connections

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  3. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2022-08-25T08:34:40Z

    Hello Kyotaro,


On Thu, 25 Aug 2022 at 09:49, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

> In the first place, this is not a bug. (At least doesn't seem.)
> If you mean to propose behavioral changes, -hackers is the place.
>

Well, maybe... We can always change it.


> >    8. When pg_rewind finished successfully, we see that the WAL segments
> >    with .ready files are removed, because they were already absent on the
> >    promoted replica. We end up in a situation where we completely lose
> some
> >    WAL segments, even though we had a clear sign that they were not
> >    archived and
> >    more importantly, pg_rewind read these segments while collecting
> >    information about the data blocks.
>
> In terms of syncing the old primary to the new primary, no data has
> been lost. The "lost" segments are anyway unusable for the new primary
> since they no longer compatible with it.  How do you intended to use
> the WAL files for the incompatible cluster?
>

These files are required for the old primary to start as a replica.


>
> >    9. The old primary fails to start because of the missing WAL segments
> >    (more strictly, the records between the last common checkpoint and the
> >    point of divergence) with the following log record: "ERROR:
> requested WAL
> >    segment 000000020004D20200000096 has already been removed"
>
> That means that the tail end of the rewound old primary has been lost
> on the new primary's pg_wal.


Correct. The old primary was down for about 20m and we have
checkpoint_timeout = 5m, so the new primary already recycled them.


>   In that case, you need to somehow
> copy-in the archived WAL files on the new primary. You can just do
> that or you can set up restore_command properly.
>

These files never made it to the archive because the server crashed. The
only place where they existed was pg_wal in the old primary.



> > Thus, my thoughts are: why can’t pg_rewind be a little bit wiser in terms
> > of creating filemap for WALs? Can it preserve the WAL segments that
> contain
> > those potentially lost records (> the last common checkpoint and  < the
> > point of divergence) on the target? (see the patch attached)
>
> Since they are not really needed once rewind completes.
>

The pg_rewind creates the backup_label file with START WAL LOCATION and
CHECKPOINT LOCATION that point to the last common checkpoint.
Removed files are between the last common checkpoint and diverged WAL
location, and therefore are required for Postgres to do successful recovery.
Since these files never made it to the archive and are also absent on the
new primary the old primary can't start as a replica.
And I will emphasize one more time, that these files were removed by
pg_rewind despite the known fact that they are required to perform a
recovery.



>
> > If I am missing something however, please correct me or explain why it is
> > not possible to implement this straightforward solution.
>
> Maybe you're mistaking the operation.


We are not (Patroni author is here).


> If I understand the situation
> correctly, I think the following steps replays your "issue" and then
> resolve that.
>
>
> # killall -9 postgres
> # rm -r oldprim newprim oldarch newarch oldprim.log newprim.log
> mkdir newarch oldarch
> initdb -k -D oldprim
> echo "archive_mode = 'always'">> oldprim/postgresql.conf
>

With archive_mode = always you can't reproduce it.
It is very rarely people set it to always in production due to the overhead.




> echo "archive_command = 'cp %p `pwd`/oldarch/%f'">> oldprim/postgresql.conf
> pg_ctl -D oldprim -o '-p 5432' -l oldprim.log start
> psql -p 5432 -c 'create table t(a int)'
> pg_basebackup -D newprim -p 5432
> echo "primary_conninfo='host=/tmp port=5432'">> oldprim/postgresql.conf
> echo "archive_command = 'cp %p `pwd`/newarch/%f'">> newprim/postgresql.conf
> touch newprim/standby.signal
> pg_ctl -D newprim -o '-p 5433' -l newprim.log start
> pg_ctl -D newprim promote
> for i in $(seq 1 4); do psql -p 5432 -c 'insert into t values(0); select
> pg_switch_wal();'; done
> psql -p 5432 -c 'checkpoint'
> pg_ctl -D oldprim stop
>

The archive_mode has to be set to on and the archive_command should be
failing when you do pg_ctl -D oldprim stop


> echo "restore_command = 'cp `pwd`/oldarch/%f %p'">> oldprim/postgresql.conf
> # pg_rewind -D oldprim --source-server='port=5433' # fails
> pg_rewind -D oldprim --source-server='port=5433' -c
> for i in $(seq 1 4); do psql -p 5433 -c 'insert into t values(0); select
> pg_switch_wal();'; done
> psql -p 5433 -c 'checkpoint'
> echo "primary_conninfo='host=/tmp port=5433'">> oldprim/postgresql.conf
> touch oldprim/standby.signal
>
> postgres -D oldprim
>
> >  FATAL:  could not receive data from WAL stream: ERROR:  requested WAL
> segment 000000020000000000000003 has already been removed
>
>
Regards,
--
Alexander Kukushkin

  4. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-08-26T08:04:32Z

    (Moved to -hackers)

At Thu, 25 Aug 2022 10:34:40 +0200, Alexander Kukushkin <cyberdemn@gmail.com> wrote in 
> > # killall -9 postgres
> > # rm -r oldprim newprim oldarch newarch oldprim.log newprim.log
> > mkdir newarch oldarch
> > initdb -k -D oldprim
> > echo "archive_mode = 'always'">> oldprim/postgresql.conf
> >
> 
> With archive_mode = always you can't reproduce it.
> It is very rarely people set it to always in production due to the overhead.
...
> The archive_mode has to be set to on and the archive_command should be
> failing when you do pg_ctl -D oldprim stop

Ah, I see.

What I don't still understand is why pg_rewind doesn't work for the
old primary in that case. When archive_mode=on, the old primary has
the complete set of WAL files counting both pg_wal and its archive. So
as the same to the privious repro, pg_rewind -c ought to work (but it
uses its own archive this time).  In that sense the proposed solution
is still not needed in this case.

A bit harder situation comes after the server successfully rewound; if
the new primary goes so far that the old primary cannot connect. Even
in that case, you can copy-in the requried WAL files or configure
restore_command of the old pimary so that it finds required WAL files
there.

As the result the system in total doesn't lose a WAL file.

So.. I might still be missing something..


###############################
# killall -9 postgres
# rm -r oldprim newprim oldarch newarch oldprim.log newprim.log
mkdir newarch oldarch
initdb -k -D oldprim
echo "archive_mode = 'on'">> oldprim/postgresql.conf
echo "archive_command = 'cp %p `pwd`/oldarch/%f'">> oldprim/postgresql.conf
pg_ctl -D oldprim -o '-p 5432' -l oldprim.log start
psql -p 5432 -c 'create table t(a int)'
pg_basebackup -D newprim -p 5432
echo "primary_conninfo='host=/tmp port=5432'">> newprim/postgresql.conf
echo "archive_command = 'cp %p `pwd`/newarch/%f'">> newprim/postgresql.conf
touch newprim/standby.signal
pg_ctl -D newprim -o '-p 5433' -l newprim.log start

# the last common checkpoint
psql -p 5432 -c 'checkpoint'

# record approx. diverging WAL segment
start_wal=`psql -p 5433 -Atc 'select pg_walfile_name(pg_last_wal_replay_lsn() - (select setting from pg_settings where name = 'wal_segment_size')::int);
`
psql -p 5432 -c 'insert into t values(0); select pg_switch_wal();'
pg_ctl -D newprim promote
psql -p 5433 -c 'checkpoint'

# old rprimary loses diverging WAL segment
for i in $(seq 1 4); do psql -p 5432 -c 'insert into t values(0); select pg_switch_wal();'; done

# old primary cannot archive any more
echo "archive_command = 'false'">> oldprim/postgresql.conf
pg_ctl -D oldprim reload
pg_ctl -D oldprim stop

# rewind the old primary, using its own archive
# pg_rewind -D oldprim --source-server='port=5433' # should fail
echo "restore_command = 'cp `pwd`/oldarch/%f %p'">> oldprim/postgresql.conf
pg_rewind -D oldprim --source-server='port=5433' -c

# advance WAL on the old primary; new primary loses the launching WAL seg
for i in $(seq 1 4); do psql -p 5433 -c 'insert into t values(0); select pg_switch_wal();'; done
psql -p 5433 -c 'checkpoint'
echo "primary_conninfo='host=/tmp port=5433'">> oldprim/postgresql.conf
touch oldprim/standby.signal

postgres -D oldprim  # fails with "WAL file has been removed"

# The alternative of copying-in
# echo "restore_command = 'cp `pwd`/newarch/%f %p'">> oldprim/postgresql.conf

# copy-in WAL files from new primary's archive to old primary
(cd newarch;
for f in `ls`; do
  if [[ "$f" > "$start_wal" ]]; then echo copy $f; cp $f ../oldprim/pg_wal; fi
done)

postgres -D oldprim

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  5. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2022-08-26T08:57:25Z

    Hello Kyotaro,


On Fri, 26 Aug 2022 at 10:04, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

> > With archive_mode = always you can't reproduce it.
> > It is very rarely people set it to always in production due to the
> overhead.
> ...
> > The archive_mode has to be set to on and the archive_command should be
> > failing when you do pg_ctl -D oldprim stop
>
> Ah, I see.
>
> What I don't still understand is why pg_rewind doesn't work for the
> old primary in that case. When archive_mode=on, the old primary has
> the complete set of WAL files counting both pg_wal and its archive. So
> as the same to the privious repro, pg_rewind -c ought to work (but it
> uses its own archive this time).  In that sense the proposed solution
> is still not needed in this case.
>

The pg_rewind finishes successfully. But as a result it removes some files
from pg_wal that are required to perform recovery because they are missing
on the new primary.



>
> A bit harder situation comes after the server successfully rewound; if
> the new primary goes so far that the old primary cannot connect. Even
> in that case, you can copy-in the requried WAL files or configure
> restore_command of the old pimary so that it finds required WAL files
> there.
>

Yes, we can do the backup of pg_wal before running pg_rewind, but it feels
very ugly, because we will also have to clean this "backup" after a
successful recovery.
It would be much better if pg_rewind didn't remove WAL files between the
last common checkpoint and diverged LSN in the first place.

Regards,
--
Alexander Kukushkin

  6. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-08-30T05:50:26Z

    Hello, Alex.

At Fri, 26 Aug 2022 10:57:25 +0200, Alexander Kukushkin <cyberdemn@gmail.com> wrote in 
> On Fri, 26 Aug 2022 at 10:04, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> wrote:
> > What I don't still understand is why pg_rewind doesn't work for the
> > old primary in that case. When archive_mode=on, the old primary has
> > the complete set of WAL files counting both pg_wal and its archive. So
> > as the same to the privious repro, pg_rewind -c ought to work (but it
> > uses its own archive this time).  In that sense the proposed solution
> > is still not needed in this case.
> >
> 
> The pg_rewind finishes successfully. But as a result it removes some files
> from pg_wal that are required to perform recovery because they are missing
> on the new primary.

IFAIS pg_rewind doesn't. -c option contrarily restores the all
segments after the last (common) checkpoint and all of them are left
alone after pg_rewind finishes.  postgres itself removes the WAL files
after recovery. After-promotion cleanup and checkpoint revmoes the
files on the previous timeline.

Before pg_rewind runs in the repro below, the old primary has the
following segments.

TLI1:  2 8 9 A B C D

Just after pg_rewind finishes, the old primary has the following
segments.

TLI1:  2 3   5 6 7
TLI2:      4        (and 00000002.history)

pg_rewind copied 1-2 to 1-3 and 2-4 and history file from the new
primary, 1-4 to 1-7 from archive. After rewind finished, 1-4,1-8 to
1-D have been removed since the new primary didn't have them.

Recovery starts from 1-3 and promotes at 0/4_000000. postgres removes
1-5 to 1-7 by post-promotion cleanup and removes 1-2 to 1-4 by a
restartpoint. All of the segments are useless after the old primary
promotes.

When the old primary starts, it uses 1-3 and 2-4 for recovery and
fails to fetch 2-5 from the new primary.  But it is not an issue of
pg_rewind at all.

> > A bit harder situation comes after the server successfully rewound; if
> > the new primary goes so far that the old primary cannot connect. Even
> > in that case, you can copy-in the requried WAL files or configure
> > restore_command of the old pimary so that it finds required WAL files
> > there.
> >
> 
> Yes, we can do the backup of pg_wal before running pg_rewind, but it feels

So, if I understand you correctly, the issue you are complaining is
not about the WAL segments on the old timeline but about those on the
new timeline, which don't have a business with what pg_rewind does. As
the same with the case of pg_basebackup, the missing segments need to
be somehow copied from the new primary since the old primary never had
the chance to have them before.

> very ugly, because we will also have to clean this "backup" after a
> successful recovery.

What do you mean by the "backup" here? Concretely what WAL segments do
you feel need to remove, for example, in the repro case?  Or, could
you show your issue by something like the repro below?

> It would be much better if pg_rewind didn't remove WAL files between the
> last common checkpoint and diverged LSN in the first place.

Thus I don't follow this..

regards.


(Fixed a bug and slightly modified)
====
# killall -9 postgres
# rm -r oldprim newprim oldarch newarch oldprim.log newprim.log
mkdir newarch oldarch
initdb -k -D oldprim
echo "archive_mode = 'on'">> oldprim/postgresql.conf
echo "archive_command = 'echo "archive %f" >&2; cp %p `pwd`/oldarch/%f'">> oldprim/postgresql.conf
pg_ctl -D oldprim -o '-p 5432' -l oldprim.log start
psql -p 5432 -c 'create table t(a int)'
pg_basebackup -D newprim -p 5432
echo "primary_conninfo='host=/tmp port=5432'">> newprim/postgresql.conf
echo "archive_command = 'echo "archive %f" >&2; cp %p `pwd`/newarch/%f'">> newprim/postgresql.conf
touch newprim/standby.signal
pg_ctl -D newprim -o '-p 5433' -l newprim.log start

# the last common checkpoint
psql -p 5432 -c 'checkpoint'

# record approx. diverging WAL segment
start_wal=`psql -p 5433 -Atc "select pg_walfile_name(pg_last_wal_replay_lsn() - (select setting from pg_settings where name = 'wal_segment_size')::int);"`
psql -p 5432 -c 'insert into t values(0); select pg_switch_wal();'
pg_ctl -D newprim promote

# old rprimary loses diverging WAL segment
for i in $(seq 1 4); do psql -p 5432 -c 'insert into t values(0); select pg_switch_wal();'; done
psql -p 5432 -c 'checkpoint;'
psql -p 5433 -c 'checkpoint;'

# old primary cannot archive any more
echo "archive_command = 'false'">> oldprim/postgresql.conf
pg_ctl -D oldprim reload
pg_ctl -D oldprim stop

# rewind the old primary, using its own archive
# pg_rewind -D oldprim --source-server='port=5433' # should fail
echo "restore_command = 'echo "restore %f" >&2; cp `pwd`/oldarch/%f %p'">> oldprim/postgresql.conf
pg_rewind -D oldprim --source-server='port=5433' -c

# advance WAL on the old primary; new primary loses the launching WAL seg
for i in $(seq 1 4); do psql -p 5433 -c 'insert into t values(0); select pg_switch_wal();'; done
psql -p 5433 -c 'checkpoint'
echo "primary_conninfo='host=/tmp port=5433'">> oldprim/postgresql.conf
touch oldprim/standby.signal

postgres -D oldprim  # fails with "WAL file has been removed"

# The alternative of copying-in
# echo "restore_command = 'echo "restore %f" >&2; cp `pwd`/newarch/%f %p'">> oldprim/postgresql.conf

# copy-in WAL files from new primary's archive to old primary
(cd newarch;
for f in `ls`; do
  if [[ "$f" > "$start_wal" ]]; then echo copy $f; cp $f ../oldprim/pg_wal; fi
done)

postgres -D oldprim
====

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  7. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2022-08-30T06:49:27Z

    Hello Kyotaro,

On Tue, 30 Aug 2022 at 07:50, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:



> So, if I understand you correctly, the issue you are complaining is
> not about the WAL segments on the old timeline but about those on the
> new timeline, which don't have a business with what pg_rewind does. As
> the same with the case of pg_basebackup, the missing segments need to
> be somehow copied from the new primary since the old primary never had
> the chance to have them before.
>

No, we are complaining exactly about WAL segments from the old timeline
that are removed by pg_rewind.
Those segments haven't been archived by the old primary and the new primary
already recycled them.




>
> Thus I don't follow this..
>

I did a slight modification of your script that reproduces a problem.


====
mkdir newarch oldarch
initdb -k -D oldprim
echo "archive_mode = 'on'">> oldprim/postgresql.conf
echo "archive_command = 'echo "archive %f" >&2; cp %p `pwd`/oldarch/%f'">>
oldprim/postgresql.conf
pg_ctl -D oldprim -o '-p 5432' -l oldprim.log start
psql -p 5432 -c 'create table t(a int)'
pg_basebackup -D newprim -p 5432
echo "primary_conninfo='host=/tmp port=5432'">> newprim/postgresql.conf
echo "archive_command = 'echo "archive %f" >&2; cp %p `pwd`/newarch/%f'">>
newprim/postgresql.conf
touch newprim/standby.signal
pg_ctl -D newprim -o '-p 5433' -l newprim.log start

# the last common checkpoint
psql -p 5432 -c 'checkpoint'

# old primary cannot archive any more
echo "archive_command = 'false'">> oldprim/postgresql.conf
pg_ctl -D oldprim reload
# advance WAL on the old primary; four WAL segments will never make it to
the archive
for i in $(seq 1 4); do psql -p 5432 -c 'insert into t values(0); select
pg_switch_wal();'; done

# record approx. diverging WAL segment
start_wal=`psql -p 5432 -Atc "select
pg_walfile_name(pg_last_wal_replay_lsn() - (select setting from pg_settings
where name = 'wal_segment_size')::int);"`
pg_ctl -D newprim promote

# old rprimary loses diverging WAL segment
for i in $(seq 1 4); do psql -p 5432 -c 'insert into t values(0); select
pg_switch_wal();'; done
psql -p 5432 -c 'checkpoint;'
psql -p 5433 -c 'checkpoint;'

pg_ctl -D oldprim stop

# rewind the old primary, using its own archive
# pg_rewind -D oldprim --source-server='port=5433' # should fail
echo "restore_command = 'echo "restore %f" >&2; cp `pwd`/oldarch/%f %p'">>
oldprim/postgresql.conf
pg_rewind -D oldprim --source-server='port=5433' -c

# advance WAL on the old primary; new primary loses the launching WAL seg
for i in $(seq 1 4); do psql -p 5433 -c 'insert into t values(0); select
pg_switch_wal();'; done
psql -p 5433 -c 'checkpoint'
echo "primary_conninfo='host=/tmp port=5433'">> oldprim/postgresql.conf
touch oldprim/standby.signal

postgres -D oldprim  # fails with "WAL file has been removed"

# The alternative of copying-in
# echo "restore_command = 'echo "restore %f" >&2; cp `pwd`/newarch/%f
%p'">> oldprim/postgresql.conf

# copy-in WAL files from new primary's archive to old primary
(cd newarch;
for f in `ls`; do
  if [[ "$f" > "$start_wal" ]]; then echo copy $f; cp $f ../oldprim/pg_wal;
fi
done)

postgres -D oldprim  # also fails with "requested WAL segment XXX has
already been removed"
===

Regards,
--
Alexander Kukushkin

  8. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2022-08-30T06:56:10Z

    >
>
> I did a slight modification of your script that reproduces a problem.
>
>
> ====
>

It seems that formatting damaged the script, so I better attach it as a
file.

Regards,
--
Alexander Kukushkin

  9. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-08-30T07:39:45Z

    At Tue, 30 Aug 2022 14:50:26 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> IFAIS pg_rewind doesn't. -c option contrarily restores the all
> segments after the last (common) checkpoint and all of them are left
> alone after pg_rewind finishes.  postgres itself removes the WAL files
> after recovery. After-promotion cleanup and checkpoint revmoes the
> files on the previous timeline.
> 
> Before pg_rewind runs in the repro below, the old primary has the
> following segments.
> 
> TLI1:  2 8 9 A B C D
> 
> Just after pg_rewind finishes, the old primary has the following
> segments.
> 
> TLI1:  2 3   5 6 7
> TLI2:      4        (and 00000002.history)
> 
> pg_rewind copied 1-2 to 1-3 and 2-4 and history file from the new
1> primary, 1-4 to 1-7 from archive. After rewind finished, 1-4,1-8 to
> 1-D have been removed since the new primary didn't have them.
> 
> Recovery starts from 1-3 and promotes at 0/4_000000. postgres removes
> 1-5 to 1-7 by post-promotion cleanup and removes 1-2 to 1-4 by a
> restartpoint. All of the segments are useless after the old primary
> promotes.
> 
> When the old primary starts, it uses 1-3 and 2-4 for recovery and
> fails to fetch 2-5 from the new primary.  But it is not an issue of
> pg_rewind at all.

Ah. I think I understand what you are mentioning. If the new primary
didn't have the segment 1-3 to 1-6, pg_rewind removes it.  The new
primary doesn't have it in pg_wal nor in archive. The old primary has
it in its archive.  So get out from the situation, we need to the
following *two* things before the old primary can start:

1. copy 1-3 to 1-6 from the archive of the *old* primary
2. copy 2-7 and later from the archive of the *new* primary

Since pg_rewind have copied in to the old primary's pg_wal, removing them just have users to perform the task duplicatedly, as you stated.

Okay, I completely understand the problem and convinced that it is
worth changing the behavior.

However, the proposed patch looks too complex to me.  It can be done
by just comparing xlog file name and the last checkpoint location and
TLI in decide_file_actions().

regards.

=====
# killall -9 postgres
# rm -r oldprim newprim oldarch newarch oldprim.log newprim.log
mkdir newarch oldarch
initdb -k -D oldprim
echo "archive_mode = 'on'">> oldprim/postgresql.conf
echo "archive_command = 'echo "archive %f" >&2; cp %p `pwd`/oldarch/%f'">> oldprim/postgresql.conf
pg_ctl -D oldprim -o '-p 5432' -l oldprim.log start
psql -p 5432 -c 'create table t(a int)'
pg_basebackup -D newprim -p 5432
echo "primary_conninfo='host=/tmp port=5432'">> newprim/postgresql.conf
echo "archive_command = 'echo "archive %f" >&2; cp %p `pwd`/newarch/%f'">> newprim/postgresql.conf
touch newprim/standby.signal
pg_ctl -D newprim -o '-p 5433' -l newprim.log start

# the last common checkpoint
psql -p 5432 -c 'checkpoint'

# record approx. diverging WAL segment
start_wal=`psql -p 5433 -Atc "select pg_walfile_name(pg_last_wal_replay_lsn() - (select setting from pg_settings where name = 'wal_segment_size')::int);"`
for i in $(seq 1 5); do psql -p 5432 -c 'insert into t values(0); select pg_switch_wal();'; done
psql -p 5432 -c 'checkpoint'
pg_ctl -D newprim promote

# old rprimary loses diverging WAL segment
for i in $(seq 1 4); do psql -p 5432 -c 'insert into t values(0); select pg_switch_wal();'; done
psql -p 5432 -c 'checkpoint;'
psql -p 5433 -c 'checkpoint;'

# old primary cannot archive any more
echo "archive_command = 'false'">> oldprim/postgresql.conf
pg_ctl -D oldprim reload
pg_ctl -D oldprim stop

# rewind the old primary, using its own archive
# pg_rewind -D oldprim --source-server='port=5433' # should fail
echo "restore_command = 'echo "restore %f" >&2; cp `pwd`/oldarch/%f %p'">> oldprim/postgresql.conf
pg_rewind -D oldprim --source-server='port=5433' -c

# advance WAL on the old primary; new primary loses the launching WAL seg
for i in $(seq 1 4); do psql -p 5433 -c 'insert into t values(0); select pg_switch_wal();'; done
psql -p 5433 -c 'checkpoint'
echo "primary_conninfo='host=/tmp port=5433'">> oldprim/postgresql.conf
touch oldprim/standby.signal

#### copy the missing file of the old timeline
## cp oldarch/00000001000000000000000[3456] oldprim/pg_wal
## cp newarch/00000002000000000000000* oldprim/pg_wal

postgres -D oldprim  # fails with "WAL file has been removed"


# The alternative of copying-in
# echo "restore_command = 'echo "restore %f" >&2; cp `pwd`/newarch/%f %p'">> oldprim/postgresql.conf

# copy-in WAL files from new primary's archive to old primary
(cd newarch;
for f in `ls`; do
  if [[ "$f" > "$start_wal" ]]; then echo copy $f; cp $f ../oldprim/pg_wal; fi
done)

postgres -D oldprim
=====

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  10. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-08-30T07:51:31Z

    At Tue, 30 Aug 2022 08:49:27 +0200, Alexander Kukushkin <cyberdemn@gmail.com> wrote in 
> No, we are complaining exactly about WAL segments from the old timeline
> that are removed by pg_rewind.
> Those segments haven't been archived by the old primary and the new primary
> already recycled them.

Yeah, sorry for my thick skull but I finally got your point.

And as I said in a mail I sent just before, the patch looks too
complex.  How about just comparing WAL file name aginst the last
common checkpoint's tli and lsn? We can tell filemap.c about the last
checkpoint and decide_file_action can compare the file name with it.

It is sufficient to preserve WAL files if tli matches and the segment
number of the WAL file is equal to or later than the checkpoint
location.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  11. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2022-08-30T08:03:07Z

    Hello Kyotaro,


On Tue, 30 Aug 2022 at 09:51, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

>
>
> And as I said in a mail I sent just before, the patch looks too
> complex.  How about just comparing WAL file name aginst the last
> common checkpoint's tli and lsn? We can tell filemap.c about the last
> checkpoint and decide_file_action can compare the file name with it.
>
> It is sufficient to preserve WAL files if tli matches and the segment
> number of the WAL file is equal to or later than the checkpoint
> location.
>

What if the last common checkpoint was on a previous timeline?
I.e., standby was promoted to primary, the timeline changed from 1 to 2,
and after that the node crashed _before_ the CHECKPOINT after promote has
finished.
The next node will advance the timeline from 2 to 3.
In this case, the last common checkpoint will be on timeline 1, and the
check becomes more complex because we will have to consider both timelines,
1 and 2.

Also, we need to take into account the divergency LSN. Files after it are
not required.

Regards,
--
Alexander Kukushkin

  12. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-08-30T08:27:30Z

    At Tue, 30 Aug 2022 10:03:07 +0200, Alexander Kukushkin <cyberdemn@gmail.com> wrote in 
> On Tue, 30 Aug 2022 at 09:51, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> wrote:
> > And as I said in a mail I sent just before, the patch looks too
> > complex.  How about just comparing WAL file name aginst the last
> > common checkpoint's tli and lsn? We can tell filemap.c about the last
> > checkpoint and decide_file_action can compare the file name with it.
> >
> > It is sufficient to preserve WAL files if tli matches and the segment
> > number of the WAL file is equal to or later than the checkpoint
> > location.
> >
> 
> What if the last common checkpoint was on a previous timeline?
> I.e., standby was promoted to primary, the timeline changed from 1 to 2,
> and after that the node crashed _before_ the CHECKPOINT after promote has
> finished.
> The next node will advance the timeline from 2 to 3.
> In this case, the last common checkpoint will be on timeline 1, and the
> check becomes more complex because we will have to consider both timelines,
> 1 and 2.

Hmm. Doesn't it work to ignoring tli then? All segments that their
segment number is equal to or larger than the checkpoint locaiton are
preserved regardless of TLI?

> Also, we need to take into account the divergency LSN. Files after it are
> not required.

They are removed at the later checkpoints. But also we can remove
segments that are out of the range between the last common checkpoint
and divergence point ignoring TLI.  the divergence point is also
compared?

> if (file_segno >= last_common_checkpoint_seg &&
>     file_segno <= divergence_seg)
>     <PRESERVE IT>;


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  13. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2022-08-30T09:01:58Z

    On Tue, 30 Aug 2022 at 10:27, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

>
> Hmm. Doesn't it work to ignoring tli then? All segments that their
> segment number is equal to or larger than the checkpoint locaiton are
> preserved regardless of TLI?
>

If we ignore TLI there is a chance that we may retain some unnecessary (or
just wrong) files.


>
> > Also, we need to take into account the divergency LSN. Files after it are
> > not required.
>
> They are removed at the later checkpoints. But also we can remove
> segments that are out of the range between the last common checkpoint
> and divergence point ignoring TLI.


Everything that is newer last_common_checkpoint_seg could be removed (but
it already happens automatically, because these files are missing on the
new primary).
WAL files that are older than last_common_checkpoint_seg could be either
removed or at least not copied from the new primary.



> the divergence point is also
> compared?
>
> > if (file_segno >= last_common_checkpoint_seg &&
> >     file_segno <= divergence_seg)
> >     <PRESERVE IT>;
>

The current implementation relies on tracking WAL files being open while
searching for the last common checkpoint. It automatically starts from the
divergence_seg, automatically finishes at last_common_checkpoint_seg, and
last but not least, automatically handles timeline changes. I don't think
that manually written code that decides what to do from the WAL file name
(and also takes into account TLI) could be much simpler than the current
approach.


Actually, since we start doing some additional "manipulations" with files
in pg_wal, we probably should do a symmetric action with files inside
pg_wal/archive_status

Regards,
--
Alexander Kukushkin

  14. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-08-31T05:30:31Z

    At Tue, 30 Aug 2022 11:01:58 +0200, Alexander Kukushkin <cyberdemn@gmail.com> wrote in 
> On Tue, 30 Aug 2022 at 10:27, Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> wrote:
> 
> >
> > Hmm. Doesn't it work to ignoring tli then? All segments that their
> > segment number is equal to or larger than the checkpoint locaiton are
> > preserved regardless of TLI?
> >
> 
> If we ignore TLI there is a chance that we may retain some unnecessary (or
> just wrong) files.

Right. I mean I don't think thats a problem and we can rely on
postgres itself for later cleanup. Theoretically some out-of-range tli
or segno files are left alone but they surely will be gone soon after
the server starts.

> > > Also, we need to take into account the divergency LSN. Files after it are
> > > not required.
> >
> > They are removed at the later checkpoints. But also we can remove
> > segments that are out of the range between the last common checkpoint
> > and divergence point ignoring TLI.
> 
> 
> Everything that is newer last_common_checkpoint_seg could be removed (but
> it already happens automatically, because these files are missing on the
> new primary).
> WAL files that are older than last_common_checkpoint_seg could be either
> removed or at least not copied from the new primary.
..
> The current implementation relies on tracking WAL files being open while
> searching for the last common checkpoint. It automatically starts from the
> divergence_seg, automatically finishes at last_common_checkpoint_seg, and
> last but not least, automatically handles timeline changes. I don't think
> that manually written code that decides what to do from the WAL file name
> (and also takes into account TLI) could be much simpler than the current
> approach.

Yeah, I know.  My expectation is taking the simplest way for the same
effect. My concern was the additional hash. On second thought, I
concluded that we should that on the existing filehash.

We can just add a FILE_ACTION_NONE entry to the file hash from
SimpleXLogPageRead.  Since this happens before decide_file_action()
call, decide_file_action() should ignore the entries with
FILE_ACTION_NONE. Also we need to call filehash_init() earlier.

> Actually, since we start doing some additional "manipulations" with files
> in pg_wal, we probably should do a symmetric action with files inside
> pg_wal/archive_status

In that sense, pg_rewind rather should place missing
archive_status/*.done for segments including restored ones seen while
finding checkpoint.  This is analogous of the behavior with
pg_basebackup and pg_receivewal. Also we should add FILE_ACTION_NONE
entries for .done files for segments read while finding checkpoint.

What do you think about that?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  15. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-08-31T05:36:38Z

    At Wed, 31 Aug 2022 14:30:31 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> What do you think about that?

By the way don't you add an CF entry for this?

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  16. Re: pg_rewind WAL segments deletion pitfall

    Polina Bungina <bungina@gmail.com> — 2022-09-01T11:33:09Z

    Hello Kayotaro,


Here is the new version of the patch that includes the changes you
suggested. It is smaller now but I doubt if it is as easy to understand as
it used to be.


The need of manipulations with the target’s pg_wal/archive_status directory
is a question to discuss…

At first glance it seems to be useless for .ready files: checkpointer
process will anyway recreate them if archiving is enabled on the rewound
old primary and we will finally have them in the archive. As for the .done
files, it seems reasonable to follow the pg_basebackup logic and keep .done
files together with the corresponding segments (those between the last
common checkpoint and the point of divergence) to protect them from being
archived once again.

But on the other hand it seems to be not that straightforward: imaging we
have WAL segment X on the target along with X.done file and we decide to
preserve them both (or we download it from archive and force .done file
creation), while archive_mode was set to ‘always’ and the source (promoted
replica) also still has WAL segment X and X.ready file. After pg_rewind we
will end up with both X.ready and X.done, which seems to be not a good
situation (but most likely not critical either).


Regards,

Polina Bungina


On Wed, Aug 31, 2022 at 7:30 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

> At Tue, 30 Aug 2022 11:01:58 +0200, Alexander Kukushkin <
> cyberdemn@gmail.com> wrote in
> > On Tue, 30 Aug 2022 at 10:27, Kyotaro Horiguchi <horikyota.ntt@gmail.com
> >
> > wrote:
> >
> > >
> > > Hmm. Doesn't it work to ignoring tli then? All segments that their
> > > segment number is equal to or larger than the checkpoint locaiton are
> > > preserved regardless of TLI?
> > >
> >
> > If we ignore TLI there is a chance that we may retain some unnecessary
> (or
> > just wrong) files.
>
> Right. I mean I don't think thats a problem and we can rely on
> postgres itself for later cleanup. Theoretically some out-of-range tli
> or segno files are left alone but they surely will be gone soon after
> the server starts.
>
> > > > Also, we need to take into account the divergency LSN. Files after
> it are
> > > > not required.
> > >
> > > They are removed at the later checkpoints. But also we can remove
> > > segments that are out of the range between the last common checkpoint
> > > and divergence point ignoring TLI.
> >
> >
> > Everything that is newer last_common_checkpoint_seg could be removed (but
> > it already happens automatically, because these files are missing on the
> > new primary).
> > WAL files that are older than last_common_checkpoint_seg could be either
> > removed or at least not copied from the new primary.
> ..
> > The current implementation relies on tracking WAL files being open while
> > searching for the last common checkpoint. It automatically starts from
> the
> > divergence_seg, automatically finishes at last_common_checkpoint_seg, and
> > last but not least, automatically handles timeline changes. I don't think
> > that manually written code that decides what to do from the WAL file name
> > (and also takes into account TLI) could be much simpler than the current
> > approach.
>
> Yeah, I know.  My expectation is taking the simplest way for the same
> effect. My concern was the additional hash. On second thought, I
> concluded that we should that on the existing filehash.
>
> We can just add a FILE_ACTION_NONE entry to the file hash from
> SimpleXLogPageRead.  Since this happens before decide_file_action()
> call, decide_file_action() should ignore the entries with
> FILE_ACTION_NONE. Also we need to call filehash_init() earlier.
>
> > Actually, since we start doing some additional "manipulations" with files
> > in pg_wal, we probably should do a symmetric action with files inside
> > pg_wal/archive_status
>
> In that sense, pg_rewind rather should place missing
> archive_status/*.done for segments including restored ones seen while
> finding checkpoint.  This is analogous of the behavior with
> pg_basebackup and pg_receivewal. Also we should add FILE_ACTION_NONE
> entries for .done files for segments read while finding checkpoint.
>
> What do you think about that?
>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>

  17. Re: pg_rewind WAL segments deletion pitfall

    Polina Bungina <bungina@gmail.com> — 2022-09-01T11:58:04Z

    Terribly sorry for misspelling your name and for the topposting!

Regards,
Polina Bungina

  18. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2022-09-26T07:08:25Z

    Hello Kyotaro,

any further thoughts on it?

Regards,
--
Alexander Kukushkin

  19. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-09-27T07:50:54Z

    At Thu, 1 Sep 2022 13:33:09 +0200, Polina Bungina <bungina@gmail.com> wrote in 
> Here is the new version of the patch that includes the changes you
> suggested. It is smaller now but I doubt if it is as easy to understand as
> it used to be.

pg_rewind works in two steps. First it constructs file map which
decides the action for each file, then second, it performs file
operations according to the file map. So, if we are going to do
something on some files, that action should be record that in the file
map, I think.

Regarding the the patch, pg_rewind starts reading segments from the
divergence point back to the nearest checkpoint, then moves foward
during rewinding. So, the fact that SimpleXLogPageRead have read a
segment suggests that the segment is required during the next startup.
So I don't think we need to move around the keepWalSeg flag.  All
files that are wanted while rewinding should be preserved
unconditionally.

It's annoying that the file path for file map and open(2) have
different top directory. But sharing the same path string between the
two seems rather ugly..

I feel uncomfortable to directly touch the internal of file_entry_t
outside filemap.c. I'd like to hide the internals in filemap.c, but
pg_rewind already does that..

+		/*
+		 * Some entries (WAL segments) already have an action assigned
+		 * (see SimpleXLogPageRead()).
+		 */
+		if (entry->action == FILE_ACTION_NONE)
+			continue;
 		entry->action = decide_file_action(entry);

It might be more reasonable to call decide_file_action() when action
is UNDECIDED.

> The need of manipulations with the target’s pg_wal/archive_status directory
> is a question to discuss…
>
> At first glance it seems to be useless for .ready files: checkpointer
> process will anyway recreate them if archiving is enabled on the rewound
> old primary and we will finally have them in the archive. As for the .done
> files, it seems reasonable to follow the pg_basebackup logic and keep .done
> files together with the corresponding segments (those between the last
> common checkpoint and the point of divergence) to protect them from being
> archived once again.
> 
> But on the other hand it seems to be not that straightforward: imaging we
> have WAL segment X on the target along with X.done file and we decide to
> preserve them both (or we download it from archive and force .done file
> creation), while archive_mode was set to ‘always’ and the source (promoted
> replica) also still has WAL segment X and X.ready file. After pg_rewind we
> will end up with both X.ready and X.done, which seems to be not a good
> situation (but most likely not critical either).

Thanks for the thought. Yes, it's not so straight-forward. And, as you
mentioned, the worst result comes from not doing that is that some
already-archived segments are archived at next run, which is generally
harmless. So I think we're ok to ignore that in this patdh then create
other patch if we still want to do that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  20. Re: pg_rewind WAL segments deletion pitfall

    Polina Bungina <bungina@gmail.com> — 2022-09-28T08:09:05Z

    On Tue, Sep 27, 2022 at 9:50 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

> Regarding the the patch, pg_rewind starts reading segments from the
> divergence point back to the nearest checkpoint, then moves foward
> during rewinding. So, the fact that SimpleXLogPageRead have read a
> segment suggests that the segment is required during the next startup.
> So I don't think we need to move around the keepWalSeg flag.  All
> files that are wanted while rewinding should be preserved
> unconditionally.
>

I am probably not getting this right but as far as I see SimpleXLogPageRead
is called at most 3 times during pg_rewind run:
1. From readOneRecord to determine the end-of-WAL on the target by reading
the last shutdown checkpoint record/minRecoveryPoint on it
2. From findLastCheckpoint to find last common checkpoint (here it
indeed reads all the segments that are required during the startup, hence
the keepWalSeg flag set to true)
3. From extractPageMap to extract all the pages modified after the fork
(here we also read all the segments that should be kept but also the ones
further, until the target's end record. Doesn't seem we should
unconditionally preserve them all).
Am I missing something?



> +               /*
> +                * Some entries (WAL segments) already have an action
> assigned
> +                * (see SimpleXLogPageRead()).
> +                */
> +               if (entry->action == FILE_ACTION_NONE)
> +                       continue;
>                 entry->action = decide_file_action(entry);

It might be more reasonable to call decide_file_action() when action
> is UNDECIDED.
>

Agree, will change this part.

Regards,
Polina Bungina

  21. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-09-28T09:17:39Z

    At Wed, 28 Sep 2022 10:09:05 +0200, Polina Bungina <bungina@gmail.com> wrote in 
> On Tue, Sep 27, 2022 at 9:50 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> wrote:
> 
> > Regarding the the patch, pg_rewind starts reading segments from the
> > divergence point back to the nearest checkpoint, then moves foward
> > during rewinding. So, the fact that SimpleXLogPageRead have read a
> > segment suggests that the segment is required during the next startup.
> > So I don't think we need to move around the keepWalSeg flag.  All
> > files that are wanted while rewinding should be preserved
> > unconditionally.
> >
> 
> I am probably not getting this right but as far as I see SimpleXLogPageRead
> is called at most 3 times during pg_rewind run:
> 1. From readOneRecord to determine the end-of-WAL on the target by reading
> the last shutdown checkpoint record/minRecoveryPoint on it
> 2. From findLastCheckpoint to find last common checkpoint (here it
> indeed reads all the segments that are required during the startup, hence
> the keepWalSeg flag set to true)
> 3. From extractPageMap to extract all the pages modified after the fork
> (here we also read all the segments that should be kept but also the ones
> further, until the target's end record. Doesn't seem we should
> unconditionally preserve them all).
> Am I missing something?

No. You're right. I have to admit that I was confused at the time X(,
sorry for that.  Those extra files are I believe harmless but of
course it's preferable to avoid them. So the keepWalSeg is useful.

So the latest version become very similar to v1 in that the both have
keepWalSeg flag. The difference is the need of the file name hash.  I
still think that it's better if we don't need the additional file
hash.  If we move out the bare code in v2 added to
SimpleXLogPageRead(), then name it "preserve_file(char *filepath)",
the code become more easy to read.

+		if (private->keepWalSeg)
+		{
+			/* the caller told us to preserve this file for future use */
+			snprintf(xlogfpath, MAXPGPATH, XLOGDIR "/%s", xlogfname);
+           preserve_file(xlogfpath);
+		}

Instead, I think we should add a comment here:

@@ -192,6 +195,7 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
 
 > 	private.tliIndex = tliIndex;
 > 	private.restoreCommand = restoreCommand;
++	/*
++   * WAL files read during searching for the last checkpoint are required
++   * by the next startup recovery of the target cluster.
++  */
 >+	private.keepWalSeg = true;

What do you think about the above?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  22. Re: pg_rewind WAL segments deletion pitfall

    Polina Bungina <bungina@gmail.com> — 2022-09-29T08:18:43Z

    I agree with your suggestions, so here is the updated version of patch.
Hope I haven't missed anything.

Regards,
Polina Bungina

  23. Re: pg_rewind WAL segments deletion pitfall

    torikoshia <torikoshia@oss.nttdata.com> — 2023-06-28T13:28:13Z

    On 2022-09-29 17:18, Polina Bungina wrote:
> I agree with your suggestions, so here is the updated version of
> patch. Hope I haven't missed anything.
> 
> Regards,
> Polina Bungina

Thanks for working on this!
It seems like we are also facing the same issue.

I tested the v3 patch under our condition, old primary has succeeded to 
become new standby.


BTW when I used pg_rewind-removes-wal-segments-reproduce.sh attached in 
[1], old primary also failed to become standby:

   FATAL:  could not receive data from WAL stream: ERROR:  requested WAL 
segment 000000020000000000000007 has already been removed

However, I think this is not a problem:  just adding restore_command 
like below fixed the situation.

   echo "restore_command = '/bin/cp `pwd`/newarch/%f %p'" >> 
oldprim/postgresql.conf

Attached modified reproduction script for reference.

[1]https://www.postgresql.org/message-id/CAFh8B%3DnNiFZOAPsv49gffxHBPzwmZ%3D6Msd4miMis87K%3Dd9rcRA%40mail.gmail.com


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

  24. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-06-29T01:25:33Z

    At Wed, 28 Jun 2023 22:28:13 +0900, torikoshia <torikoshia@oss.nttdata.com> wrote in 
> 
> On 2022-09-29 17:18, Polina Bungina wrote:
> > I agree with your suggestions, so here is the updated version of
> > patch. Hope I haven't missed anything.
> > Regards,
> > Polina Bungina
> 
> Thanks for working on this!
> It seems like we are also facing the same issue.

Thanks for looking this.

> I tested the v3 patch under our condition, old primary has succeeded
> to become new standby.
> 
> 
> BTW when I used pg_rewind-removes-wal-segments-reproduce.sh attached
> in [1], old primary also failed to become standby:
> 
>   FATAL: could not receive data from WAL stream: ERROR: requested WAL
>   segment 000000020000000000000007 has already been removed
> 
> However, I think this is not a problem: just adding restore_command
> like below fixed the situation.
> 
>   echo "restore_command = '/bin/cp `pwd`/newarch/%f %p'" >>
>   oldprim/postgresql.conf

I thought on the same line at first, but that's not the point
here. The problem we want ot address is that pg_rewind ultimately
removes certain crucial WAL files required for the new primary to
start, despite them being present previously. In other words, that
restore_command works, but it only undoes what pg_rewind wrongly did,
resulting in unnecessary consupmtion of I/O and/or network bandwidth
that essentially serves no purpose.

pg_rewind already has a feature that determines how each file should
be handled, but it is currently making wrong dicisions for WAL
files. The goal here is to rectify this behavior and ensure that
pg_rewind makes the right decisions.

> Attached modified reproduction script for reference.
> 
> [1]https://www.postgresql.org/message-id/CAFh8B%3DnNiFZOAPsv49gffxHBPzwmZ%3D6Msd4miMis87K%3Dd9rcRA%40mail.gmail.com

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  25. Re: pg_rewind WAL segments deletion pitfall

    torikoshia <torikoshia@oss.nttdata.com> — 2023-06-29T09:42:37Z

    On 2023-06-29 10:25, Kyotaro Horiguchi wrote:
Thanks for the comment!

> At Wed, 28 Jun 2023 22:28:13 +0900, torikoshia
> <torikoshia@oss.nttdata.com> wrote in
>> 
>> On 2022-09-29 17:18, Polina Bungina wrote:
>> > I agree with your suggestions, so here is the updated version of
>> > patch. Hope I haven't missed anything.
>> > Regards,
>> > Polina Bungina
>> 
>> Thanks for working on this!
>> It seems like we are also facing the same issue.
> 
> Thanks for looking this.
> 
>> I tested the v3 patch under our condition, old primary has succeeded
>> to become new standby.
>> 
>> 
>> BTW when I used pg_rewind-removes-wal-segments-reproduce.sh attached
>> in [1], old primary also failed to become standby:
>> 
>>   FATAL: could not receive data from WAL stream: ERROR: requested WAL
>>   segment 000000020000000000000007 has already been removed
>> 
>> However, I think this is not a problem: just adding restore_command
>> like below fixed the situation.
>> 
>>   echo "restore_command = '/bin/cp `pwd`/newarch/%f %p'" >>
>>   oldprim/postgresql.conf
> 
> I thought on the same line at first, but that's not the point
> here.

Yes. I don't think adding restore_command solves the problem and
modification to prevent deleting necessary WAL like proposed
patch is necessary.

I added restore_command since
pg_rewind-removes-wal-segments-reproduce.sh failed to catch up
even after applying v3 patch and prevent pg_rewind from delete
WALs(*), because some necessary WALs were archived.

It's not a problem we are discussing here, but I wanted to get
the script to work to the point where old primary could
successfully catch up to new primary.

(*)Specifically, running the script without apply the patch,
recovery failed because 000000010000000000000003 which has
already been removed. This file was deleted by pg_rewind as
we know.
OTHO without the restore_command, recovery failed because
000000020000000000000007 has already been removed even after
applying the patch.

> The problem we want ot address is that pg_rewind ultimately
> removes certain crucial WAL files required for the new primary to
> start, despite them being present previously.

I thought it's not "new primary", but "old primary".

> In other words, that
> restore_command works, but it only undoes what pg_rewind wrongly did,
> resulting in unnecessary consupmtion of I/O and/or network bandwidth
> that essentially serves no purpose.

As far as I tested using the script and the situation we are facing,
after promoting newprim necessary WAL(000000010000000000000003..) were
not available and just adding restore_command did not solve the problem.

> pg_rewind already has a feature that determines how each file should
> be handled, but it is currently making wrong dicisions for WAL
> files. The goal here is to rectify this behavior and ensure that
> pg_rewind makes the right decisions.

+1


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

  26. Re: pg_rewind WAL segments deletion pitfall

    torikoshia <torikoshia@oss.nttdata.com> — 2023-08-18T06:40:57Z

    On 2022-09-29 17:18, Polina Bungina wrote:
> I agree with your suggestions, so here is the updated version of
> patch. Hope I haven't missed anything.

Thanks for the patch, I've marked this as ready-for-committer.

BTW, this issue can be considered a bug, right?
I think it would be appropriate to provide backpatch.

On 2023-06-29 18:42, torikoshia wrote:
> On 2023-06-29 10:25, Kyotaro Horiguchi wrote:
> Thanks for the comment!
> 
>> At Wed, 28 Jun 2023 22:28:13 +0900, torikoshia
>> <torikoshia@oss.nttdata.com> wrote in
>>> 
>>> On 2022-09-29 17:18, Polina Bungina wrote:
>>> > I agree with your suggestions, so here is the updated version of
>>> > patch. Hope I haven't missed anything.
>>> > Regards,
>>> > Polina Bungina
>>> 
>>> Thanks for working on this!
>>> It seems like we are also facing the same issue.
>> 
>> Thanks for looking this.
>> 
>>> I tested the v3 patch under our condition, old primary has succeeded
>>> to become new standby.
>>> 
>>> 
>>> BTW when I used pg_rewind-removes-wal-segments-reproduce.sh attached
>>> in [1], old primary also failed to become standby:
>>> 
>>>   FATAL: could not receive data from WAL stream: ERROR: requested WAL
>>>   segment 000000020000000000000007 has already been removed
>>> 
>>> However, I think this is not a problem: just adding restore_command
>>> like below fixed the situation.
>>> 
>>>   echo "restore_command = '/bin/cp `pwd`/newarch/%f %p'" >>
>>>   oldprim/postgresql.conf
>> 
>> I thought on the same line at first, but that's not the point
>> here.
> 
> Yes. I don't think adding restore_command solves the problem and
> modification to prevent deleting necessary WAL like proposed
> patch is necessary.
> 
> I added restore_command since
> pg_rewind-removes-wal-segments-reproduce.sh failed to catch up
> even after applying v3 patch and prevent pg_rewind from delete
> WALs(*), because some necessary WALs were archived.
> 
> It's not a problem we are discussing here, but I wanted to get
> the script to work to the point where old primary could
> successfully catch up to new primary.
> 
> (*)Specifically, running the script without apply the patch,
> recovery failed because 000000010000000000000003 which has
> already been removed. This file was deleted by pg_rewind as
> we know.
> OTHO without the restore_command, recovery failed because
> 000000020000000000000007 has already been removed even after
> applying the patch.
> 
>> The problem we want ot address is that pg_rewind ultimately
>> removes certain crucial WAL files required for the new primary to
>> start, despite them being present previously.
> 
> I thought it's not "new primary", but "old primary".
> 
>> In other words, that
>> restore_command works, but it only undoes what pg_rewind wrongly did,
>> resulting in unnecessary consupmtion of I/O and/or network bandwidth
>> that essentially serves no purpose.
> 
> As far as I tested using the script and the situation we are facing,
> after promoting newprim necessary WAL(000000010000000000000003..) were
> not available and just adding restore_command did not solve the 
> problem.
> 
>> pg_rewind already has a feature that determines how each file should
>> be handled, but it is currently making wrong dicisions for WAL
>> files. The goal here is to rectify this behavior and ensure that
>> pg_rewind makes the right decisions.
> 
> +1

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

  27. Re: pg_rewind WAL segments deletion pitfall

    Michael Paquier <michael@paquier.xyz> — 2023-08-22T05:32:06Z

    On Fri, Aug 18, 2023 at 03:40:57PM +0900, torikoshia wrote:
> Thanks for the patch, I've marked this as ready-for-committer.
> 
> BTW, this issue can be considered a bug, right?
> I think it would be appropriate to provide backpatch.

Hmm, I agree that there is a good argument in back-patching as we have
the WAL files between the redo LSN and the divergence LSN, but
pg_rewind is not smart enough to keep them around.  If the archives of
the primary were not able to catch up, the old primary is as good as
kaput, and restore_command won't help here.

I don't like much this patch.  While it takes correctly advantage of
the backward record read logic from SimpleXLogPageRead() able to
handle correctly timeline jumps, it creates a hidden dependency in the
code between the hash table from filemap.c and the page callback.
Wouldn't it be simpler to build a list of the segment names using the
information from WALOpenSegment and build this list in
findLastCheckpoint()?  Also, I am wondering if we should be smarter
with any potential conflict handling between the source and the
target, rather than just enforcing a FILE_ACTION_NONE for all these
files.  In short, could it be better to copy the WAL file from the
source if it exists there?

+       /*
+        * Some entries (WAL segments) already have an action assigned
+        * (see SimpleXLogPageRead()).
+        */
+       if (entry->action == FILE_ACTION_UNDECIDED)
+           entry->action = decide_file_action(entry);

This change makes me a bit uneasy, per se my previous comment with the
additional code dependencies.

I think that this scenario deserves a test case.  If one wants to
emulate a delay in WAL archiving, it is possible to set
archive_command to a command that we know will fail, for instance.
--
Michael

  28. Re: pg_rewind WAL segments deletion pitfall

    torikoshia <torikoshia@oss.nttdata.com> — 2023-08-23T09:04:18Z

    On 2023-08-22 14:32, Michael Paquier wrote:
Thanks for your review!

> On Fri, Aug 18, 2023 at 03:40:57PM +0900, torikoshia wrote:
>> Thanks for the patch, I've marked this as ready-for-committer.
>> 
>> BTW, this issue can be considered a bug, right?
>> I think it would be appropriate to provide backpatch.
> 
> Hmm, I agree that there is a good argument in back-patching as we have
> the WAL files between the redo LSN and the divergence LSN, but
> pg_rewind is not smart enough to keep them around.  If the archives of
> the primary were not able to catch up, the old primary is as good as
> kaput, and restore_command won't help here.

True.
I also imagine that in the typical failover scenario where the target 
cluster was shut down soon after the divergence and pg_rewind was 
executed without much time, we can avoid this kind of 'requested WAL 
segment has already removed' error by preventing pg_rewind from deleting 
necessary WALs.


> I don't like much this patch.  While it takes correctly advantage of
> the backward record read logic from SimpleXLogPageRead() able to
> handle correctly timeline jumps, it creates a hidden dependency in the
> code between the hash table from filemap.c and the page callback.
> Wouldn't it be simpler to build a list of the segment names using the
> information from WALOpenSegment and build this list in
> findLastCheckpoint()?  Also, I am wondering if we should be smarter
> with any potential conflict handling between the source and the
> target, rather than just enforcing a FILE_ACTION_NONE for all these
> files.  In short, could it be better to copy the WAL file from the
> source if it exists there?
> 
> +       /*
> +        * Some entries (WAL segments) already have an action assigned
> +        * (see SimpleXLogPageRead()).
> +        */
> +       if (entry->action == FILE_ACTION_UNDECIDED)
> +           entry->action = decide_file_action(entry);
> 
> This change makes me a bit uneasy, per se my previous comment with the
> additional code dependencies.
> 
> I think that this scenario deserves a test case.  If one wants to
> emulate a delay in WAL archiving, it is possible to set
> archive_command to a command that we know will fail, for instance.
> --
> Michael

Bungina, are you going to respond to these comments?

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

  29. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2023-08-23T11:44:52Z

    Hi,



On Tue, 22 Aug 2023 at 07:32, Michael Paquier <michael@paquier.xyz> wrote:

>
>
> I don't like much this patch.  While it takes correctly advantage of
> the backward record read logic from SimpleXLogPageRead() able to
> handle correctly timeline jumps, it creates a hidden dependency in the
> code between the hash table from filemap.c and the page callback.
> Wouldn't it be simpler to build a list of the segment names using the
> information from WALOpenSegment and build this list in
> findLastCheckpoint()?


I think the first version of the patch more or less did that. Not
necessarily a list, but a hash table of WAL file names that we want to
keep. But Kyotaro Horiguchi didn't like it and suggested creating entries
in the filemap.c hash table instead.
But, I agree, doing it directly from the findLastCheckpoint() makes the
code easier to understand.



>   Also, I am wondering if we should be smarter
> with any potential conflict handling between the source and the
> target, rather than just enforcing a FILE_ACTION_NONE for all these
> files.  In short, could it be better to copy the WAL file from the
> source if it exists there?
>

Before the switchpoint these files are supposed to be the same on the old
primary, new primary, and also in the archive. Also, if there is a
restore_command postgres will fetch the same file from the archive even if
it already exists in pg_wal, which effectively discards all pg_rewind
efforts on copying WAL files.


>
> +       /*
> +        * Some entries (WAL segments) already have an action assigned
> +        * (see SimpleXLogPageRead()).
> +        */
> +       if (entry->action == FILE_ACTION_UNDECIDED)
> +           entry->action = decide_file_action(entry);
>
> This change makes me a bit uneasy, per se my previous comment with the
> additional code dependencies.
>

We can revert to the original approach (see
v1-0001-pg_rewind-wal-deletion.patch from the very first email) if you like.


> I think that this scenario deserves a test case.  If one wants to
> emulate a delay in WAL archiving, it is possible to set
> archive_command to a command that we know will fail, for instance.
>

Yes, I totally agree, it is on our radar, but meanwhile please see the new
version, just to check if I correctly understood your idea.

Regards,
--
Alexander Kukushkin

  30. Re: pg_rewind WAL segments deletion pitfall

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2023-08-24T00:45:38Z

    At Wed, 23 Aug 2023 13:44:52 +0200, Alexander Kukushkin <cyberdemn@gmail.com> wrote in 
> On Tue, 22 Aug 2023 at 07:32, Michael Paquier <michael@paquier.xyz> wrote:
> > I don't like much this patch.  While it takes correctly advantage of
> > the backward record read logic from SimpleXLogPageRead() able to
> > handle correctly timeline jumps, it creates a hidden dependency in the
> > code between the hash table from filemap.c and the page callback.
> > Wouldn't it be simpler to build a list of the segment names using the
> > information from WALOpenSegment and build this list in
> > findLastCheckpoint()?
> 
> I think the first version of the patch more or less did that. Not
> necessarily a list, but a hash table of WAL file names that we want to
> keep. But Kyotaro Horiguchi didn't like it and suggested creating entries
> in the filemap.c hash table instead.
> But, I agree, doing it directly from the findLastCheckpoint() makes the
> code easier to understand.
...
> > +       /*
> > +        * Some entries (WAL segments) already have an action assigned
> > +        * (see SimpleXLogPageRead()).
> > +        */
> > +       if (entry->action == FILE_ACTION_UNDECIDED)
> > +           entry->action = decide_file_action(entry);
> >
> > This change makes me a bit uneasy, per se my previous comment with the
> > additional code dependencies.
> >
> 
> We can revert to the original approach (see
> v1-0001-pg_rewind-wal-deletion.patch from the very first email) if you like.

On the other hand, that approach brings in another source that
suggests the way that file should be handled. I still think that
entry->action should be the only source. However, it seems I'm in the
minority here. So I'm not tied to that approach.

> > I think that this scenario deserves a test case.  If one wants to
> > emulate a delay in WAL archiving, it is possible to set
> > archive_command to a command that we know will fail, for instance.
> >
> 
> Yes, I totally agree, it is on our radar, but meanwhile please see the new
> version, just to check if I correctly understood your idea.

Agreed.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  31. Re: pg_rewind WAL segments deletion pitfall

    torikoshia <torikoshia@oss.nttdata.com> — 2023-08-29T13:15:51Z

    On 2023-08-24 09:45, Kyotaro Horiguchi wrote:
> At Wed, 23 Aug 2023 13:44:52 +0200, Alexander Kukushkin
> <cyberdemn@gmail.com> wrote in
>> On Tue, 22 Aug 2023 at 07:32, Michael Paquier <michael@paquier.xyz> 
>> wrote:
>> > I don't like much this patch.  While it takes correctly advantage of
>> > the backward record read logic from SimpleXLogPageRead() able to
>> > handle correctly timeline jumps, it creates a hidden dependency in the
>> > code between the hash table from filemap.c and the page callback.
>> > Wouldn't it be simpler to build a list of the segment names using the
>> > information from WALOpenSegment and build this list in
>> > findLastCheckpoint()?
>> 
>> I think the first version of the patch more or less did that. Not
>> necessarily a list, but a hash table of WAL file names that we want to
>> keep. But Kyotaro Horiguchi didn't like it and suggested creating 
>> entries
>> in the filemap.c hash table instead.
>> But, I agree, doing it directly from the findLastCheckpoint() makes 
>> the
>> code easier to understand.
> ...
>> > +       /*
>> > +        * Some entries (WAL segments) already have an action assigned
>> > +        * (see SimpleXLogPageRead()).
>> > +        */
>> > +       if (entry->action == FILE_ACTION_UNDECIDED)
>> > +           entry->action = decide_file_action(entry);
>> >
>> > This change makes me a bit uneasy, per se my previous comment with the
>> > additional code dependencies.
>> >
>> 
>> We can revert to the original approach (see
>> v1-0001-pg_rewind-wal-deletion.patch from the very first email) if you 
>> like.
> 
> On the other hand, that approach brings in another source that
> suggests the way that file should be handled. I still think that
> entry->action should be the only source.

+1.
Imaging a case when we come to need decide how to treat files based on 
yet another factor, I feel that a single source of truth is better than 
creating a list or hash for each factor.

> However, it seems I'm in the
> minority here. So I'm not tied to that approach.
> 
>> > I think that this scenario deserves a test case.  If one wants to
>> > emulate a delay in WAL archiving, it is possible to set
>> > archive_command to a command that we know will fail, for instance.
>> >
>> 
>> Yes, I totally agree, it is on our radar, but meanwhile please see the 
>> new
>> version, just to check if I correctly understood your idea.

Thanks for the patch.
I tested v4 patch using the script attached below thread and it has 
successfully finished.

https://www.postgresql.org/message-id/2e75ae22dce9a227c3d47fa6d0ed094a%40oss.nttdata.com


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

  32. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2023-09-12T13:29:46Z

    Hi,

Please find attached v5.
What changed:
1. Now we collect which files should be kept  in a separate hash table.
2. Decision whether to keep the file is made only when the file is actually
missing on the source. That is, remaining WAL files will be copied over as
it currently is, although it could be extremely inefficient and unnecessary.
3. Added TAP test that actually at least one file isn't removed.

Regards,
--
Alexander Kukushkin

  33. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2023-09-13T07:21:35Z

    Hi,

Please find attached v6.
Changes compared to v5:
1. use "perl -e 'exit(1)'" instead of "false" as archive_command, so it
also works on Windows
2. fixed the test name

Regards,
--
Alexander Kukushkin

  34. Re: pg_rewind WAL segments deletion pitfall

    torikoshia <torikoshia@oss.nttdata.com> — 2023-10-18T06:50:39Z

    Thanks for the patch.

I tested the v6 patch using the test script attached on [1], old primary 
has succeeded to become new standby.

I have very minor questions on the regression tests mainly regarding the 
consistency with other tests for pg_rewind:


> +setup_cluster;
> +create_standby;

Would it be better to add parentheses?
Also should we add "RewindTest::" for these function?


> +primary_psql("create table t(a int)");
> +primary_psql("insert into t values(0)");
> +primary_psql("select pg_switch_wal()");
..

Should 'select', 'create', etc be capitalized?


> my $false = "$^X -e 'exit(1)'";
I feel it's hard to understand what does this mean.
Isn't it better to add comments and describe this is for windows 
environments?


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

  35. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2023-10-30T15:26:39Z

    Hi,

On Wed, 18 Oct 2023 at 08:50, torikoshia <torikoshia@oss.nttdata.com> wrote:

>
> I have very minor questions on the regression tests mainly regarding the
> consistency with other tests for pg_rewind:
>

Please find attached a new version of the patch. It addresses all your
comments.

Regards,
--
Alexander Kukushkin

  36. Re: pg_rewind WAL segments deletion pitfall

    torikoshia <torikoshia@oss.nttdata.com> — 2023-11-02T03:24:50Z

    On 2023-10-31 00:26, Alexander Kukushkin wrote:
> Hi,
> 
> On Wed, 18 Oct 2023 at 08:50, torikoshia <torikoshia@oss.nttdata.com>
> wrote:
> 
>> I have very minor questions on the regression tests mainly regarding
>> the
>> consistency with other tests for pg_rewind:
> 
> Please find attached a new version of the patch. It addresses all your
> comments.

Thanks for updating the patch!

> +extern void preserve_file(char *filepath);

Is this necessary?
This function was defined in older version patch, but no longer seems to 
exist.

+# We use "perl -e 'exit(1)'" as a alternative to "false", because the 
last one
'a' should be 'an'?


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

  37. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2023-11-06T14:58:56Z

    Hi Torikoshia,


On Thu, 2 Nov 2023 at 04:24, torikoshia <torikoshia@oss.nttdata.com> wrote:

>
>
> > +extern void preserve_file(char *filepath);
>
> Is this necessary?
> This function was defined in older version patch, but no longer seems to
> exist.
>
> +# We use "perl -e 'exit(1)'" as a alternative to "false", because the
> last one
> 'a' should be 'an'?
>
>
Thanks for the feedback

Please find the new version attached.

Regards,
--
Alexander Kukushkin

On Thu, 2 Nov 2023 at 04:24, torikoshia <torikoshia@oss.nttdata.com> wrote:

> On 2023-10-31 00:26, Alexander Kukushkin wrote:
> > Hi,
> >
> > On Wed, 18 Oct 2023 at 08:50, torikoshia <torikoshia@oss.nttdata.com>
> > wrote:
> >
> >> I have very minor questions on the regression tests mainly regarding
> >> the
> >> consistency with other tests for pg_rewind:
> >
> > Please find attached a new version of the patch. It addresses all your
> > comments.
>
> Thanks for updating the patch!
>
> > +extern void preserve_file(char *filepath);
>
> Is this necessary?
> This function was defined in older version patch, but no longer seems to
> exist.
>
> +# We use "perl -e 'exit(1)'" as a alternative to "false", because the
> last one
> 'a' should be 'an'?
>
>
> --
> Regards,
>
> --
> Atsushi Torikoshi
> NTT DATA Group Corporation
>


-- 
Regards,
--
Alexander Kukushkin

  38. Re: pg_rewind WAL segments deletion pitfall

    torikoshia <torikoshia@oss.nttdata.com> — 2023-11-09T06:30:56Z

    On 2023-11-06 23:58, Alexander Kukushkin wrote:
> Hi Torikoshia,
> 
> On Thu, 2 Nov 2023 at 04:24, torikoshia <torikoshia@oss.nttdata.com>
> wrote:
> 
>>> +extern void preserve_file(char *filepath);
>> 
>> Is this necessary?
>> This function was defined in older version patch, but no longer
>> seems to
>> exist.
>> 
>> +# We use "perl -e 'exit(1)'" as a alternative to "false", because
>> the
>> last one
>> 'a' should be 'an'?
> 
> Thanks for the feedback
> 
> Please find the new version attached.
Thanks for the update!

I've set the CF entry to "Ready for Committer".

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation

  39. Re: pg_rewind WAL segments deletion pitfall

    Peter Smith <smithpb2250@gmail.com> — 2024-01-21T23:38:08Z

    2024-01 Commitfest.

Hi, This patch has a CF status of "Ready for Committer", but it is
currently failing some CFbot tests [1]. Please have a look and post an
updated version..

======
[1] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/3874

Kind Regards,
Peter Smith.

  40. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2024-01-23T08:23:29Z

    Hi Peter,

On Mon, 22 Jan 2024 at 00:38, Peter Smith <smithpb2250@gmail.com> wrote:

> 2024-01 Commitfest.
>
> Hi, This patch has a CF status of "Ready for Committer", but it is
> currently failing some CFbot tests [1]. Please have a look and post an
> updated version..
>
> ======
> [1]
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/3874
>
>
>From what I can see all failures are not related to this patch:
1. Windows build failed with
[10:52:49.679] 126/281 postgresql:recovery / recovery/019_replslot_limit
ERROR 185.84s (exit status 255 or signal 127 SIGinvalid)
2. FreeBSD build failed with
[09:11:57.656] 190/285 postgresql:psql / psql/010_tab_completion ERROR
0.46s exit status 2
[09:11:57.656] 220/285 postgresql:authentication /
authentication/001_password ERROR 0.57s exit status 2

In fact, I don't even see this patch being applied for these builds and the
introduced TAP test being executed.

Regards,
--
Alexander Kukushkin

  41. Re: pg_rewind WAL segments deletion pitfall

    Sutou Kouhei <kou@clear-code.com> — 2024-07-12T07:24:06Z

    Hi,

I'm reviewing patches in Commitfest 2024-07 from top to bottom:
https://commitfest.postgresql.org/48/

This is the 1st patch:
https://commitfest.postgresql.org/48/3874/

The latest patch can't be applied on master:
https://www.postgresql.org/message-id/CAFh8B=nNJtm9ke4_1mhpwGz2PV9yoyF6hMnYh5XACt0AA4VG-A@mail.gmail.com

I've rebased on master. See the attached patch.
Here are changes for it:

* Resolve conflict
* Update copyright year to 2024 from 2023
* Add an added test to meson.build
* Run pgindent

Here are my review comments:

@@ -217,6 +221,26 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
+			char		xlogfname[MAXFNAMELEN];
+
+			tli = xlogreader->seg.ws_tli;
+			segno = xlogreader->seg.ws_segno;
+
+			snprintf(xlogfname, MAXPGPATH, XLOGDIR "/");
+			XLogFileName(xlogfname + strlen(xlogfname),
+						 xlogreader->seg.ws_tli,
+						 xlogreader->seg.ws_segno, WalSegSz);
+
+			/*
+			 * Make sure pg_rewind doesn't remove this file, because it is
+			 * required for postgres to start after rewind.
+			 */
+			insert_keepwalhash_entry(xlogfname);

MAXFNAMELEN is 64 and MAXPGPATH is 1024. strlen(XLOGDIR "/")
is 7 because XLOGDIR is "pg_wal". So xlogfname has enough
size but snprintf(xlogfname, MAXPGPATH) is wrong usage.
(And XLogFileName() uses snprintf(xlogfname, MAXFNAMELEN)
internally.)

How about using one more buffer?

----
char		xlogpath[MAXPGPATH];
char		xlogfname[MAXFNAMELEN];

tli = xlogreader->seg.ws_tli;
segno = xlogreader->seg.ws_segno;

XLogFileName(xlogfname,
			 xlogreader->seg.ws_tli,
			 xlogreader->seg.ws_segno, WalSegSz);
snprintf(xlogpath, MAXPGPATH, "%s/%s", XLOGDIR, xlogfname);

/*
 * Make sure pg_rewind doesn't remove this file, because it is
 * required for postgres to start after rewind.
 */
insert_keepwalhash_entry(xlogpath);
----


Thanks,
-- 
kou

In <CAFh8B=mDDZEsK0jDMfvP3MmxkWaeTCxW4yN42OH33JY6sQWS5Q@mail.gmail.com>
  "Re: pg_rewind WAL segments deletion pitfall" on Tue, 23 Jan 2024 09:23:29 +0100,
  Alexander Kukushkin <cyberdemn@gmail.com> wrote:

> Hi Peter,
> 
> On Mon, 22 Jan 2024 at 00:38, Peter Smith <smithpb2250@gmail.com> wrote:
> 
>> 2024-01 Commitfest.
>>
>> Hi, This patch has a CF status of "Ready for Committer", but it is
>> currently failing some CFbot tests [1]. Please have a look and post an
>> updated version..
>>
>> ======
>> [1]
>> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/3874
>>
>>
> From what I can see all failures are not related to this patch:
> 1. Windows build failed with
> [10:52:49.679] 126/281 postgresql:recovery / recovery/019_replslot_limit
> ERROR 185.84s (exit status 255 or signal 127 SIGinvalid)
> 2. FreeBSD build failed with
> [09:11:57.656] 190/285 postgresql:psql / psql/010_tab_completion ERROR
> 0.46s exit status 2
> [09:11:57.656] 220/285 postgresql:authentication /
> authentication/001_password ERROR 0.57s exit status 2
> 
> In fact, I don't even see this patch being applied for these builds and the
> introduced TAP test being executed.
> 
> Regards,
> --
> Alexander Kukushkin

  42. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2024-07-12T09:00:24Z

    Hi Sutou,

Thank you for picking it up!

On Fri, 12 Jul 2024 at 09:24, Sutou Kouhei <kou@clear-code.com> wrote:

Here are my review comments:
>
> @@ -217,6 +221,26 @@ findLastCheckpoint(const char *datadir, XLogRecPtr
> forkptr, int tliIndex,
> +                       char            xlogfname[MAXFNAMELEN];
> +
> +                       tli = xlogreader->seg.ws_tli;
> +                       segno = xlogreader->seg.ws_segno;
> +
> +                       snprintf(xlogfname, MAXPGPATH, XLOGDIR "/");
> +                       XLogFileName(xlogfname + strlen(xlogfname),
> +                                                xlogreader->seg.ws_tli,
> +                                                xlogreader->seg.ws_segno,
> WalSegSz);
> +
> +                       /*
> +                        * Make sure pg_rewind doesn't remove this file,
> because it is
> +                        * required for postgres to start after rewind.
> +                        */
> +                       insert_keepwalhash_entry(xlogfname);
>
> MAXFNAMELEN is 64 and MAXPGPATH is 1024. strlen(XLOGDIR "/")
> is 7 because XLOGDIR is "pg_wal". So xlogfname has enough
> size but snprintf(xlogfname, MAXPGPATH) is wrong usage.
> (And XLogFileName() uses snprintf(xlogfname, MAXFNAMELEN)
> internally.)
>

Nice catch!

I don't think we need another buffer here, just need to use MAXFNAMELEN,
because strlen("pg_wal/$wal_filename") + 1 = 32 perfectly fits into 64
bytes.

The new version is attached.

Regards,
--
Alexander Kukushkin

  43. Re: pg_rewind WAL segments deletion pitfall

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-12T19:18:30Z

    Hello

After reading the whole thread a couple of times to make sure I
understood the problem correctly, I think the approach in the v10 patch
is a reasonable one.  I agree that it's better for maintainability to
keep a separate hash table.  I made some cosmetic adjustments -- didn't
find any fault in the code.  I also verified that the test is hitting
the problematic case.

So here's v11, which I'll try to get out tomorrow.

I also assessed back-patching this.  There's some conflicts, but nothing
serious, back to 14.  Unfortunately, 13 lacks requisite infrastructure,
so I think it'll have to stay as it is.  (12 is dead.)

Can you please verify that my explanation in the commit message is
sound?

Thanks

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Cada quien es cada cual y baja las escaleras como quiere" (JMSerrat)

  44. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2024-11-14T14:38:20Z

    Hi Alvaro,

The commit message looks good to me, except maybe using a "master" word,
which I would suggest to replace with "primary".

And a small nitpick:
+/*
+ * Initialize a hash table to store WAL file names that must be kept.
+ */
+void
+keepwal_init(void)
+{
+   /*
+    * This hash table is empty in the vast majority of cases, so set an
+    * initial size of 0.
+    */
+   keepwal = keepwal_create(0, NULL);
+}

I don't think that the hash table will be empty. It needs to hold all WAL
filenames starting from the last checkpoint and up to divergent point.
On loaded clusters it could be hundreds and thousands of files.

On Tue, 12 Nov 2024 at 20:18, Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> Hello
>
> After reading the whole thread a couple of times to make sure I
> understood the problem correctly, I think the approach in the v10 patch
> is a reasonable one.  I agree that it's better for maintainability to
> keep a separate hash table.  I made some cosmetic adjustments -- didn't
> find any fault in the code.  I also verified that the test is hitting
> the problematic case.
>
> So here's v11, which I'll try to get out tomorrow.
>
> I also assessed back-patching this.  There's some conflicts, but nothing
> serious, back to 14.  Unfortunately, 13 lacks requisite infrastructure,
> so I think it'll have to stay as it is.  (12 is dead.)
>
> Can you please verify that my explanation in the commit message is
> sound?
>
> Thanks
>
> --
> Álvaro Herrera               48°01'N 7°57'E  —
> https://www.EnterpriseDB.com/
> "Cada quien es cada cual y baja las escaleras como quiere" (JMSerrat)
>


Regards,
--
Alexander Kukushkin

  45. Re: pg_rewind WAL segments deletion pitfall

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-15T12:22:21Z

    Hello Alexander,

On 2024-Nov-14, Alexander Kukushkin wrote:

> The commit message looks good to me, except maybe using a "master" word,
> which I would suggest to replace with "primary".

Oh wow, thanks for noticing that.  I had already rewritten the commit
message to some extent, but "master" had remained.  Now I pushed the
patch to branches 14+, having replaced it as you suggested.

(This reminds me that I used to have a notification set in the
2ndQuadrant Mattermost instance so that I could LART anybody who used
the words 'master' or 'slave' in the chats there).

> +   /*
> +    * This hash table is empty in the vast majority of cases, so set an
> +    * initial size of 0.
> +    */
> +   keepwal = keepwal_create(0, NULL);
> +}
> 
> I don't think that the hash table will be empty. It needs to hold all WAL
> filenames starting from the last checkpoint and up to divergent point.
> On loaded clusters it could be hundreds and thousands of files.

Oh, okay.  The initial size is just there to avoid having to grow the
hash table, but using the same constant that we use for the filemap hash
table seems good enough ... it shouldn't make much of a difference in
practice.

Regards

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.

  46. Re: pg_rewind WAL segments deletion pitfall

    Michael Paquier <michael@paquier.xyz> — 2024-11-20T00:41:58Z

    On Fri, Nov 15, 2024 at 01:22:21PM +0100, Alvaro Herrera wrote:
> Oh wow, thanks for noticing that.  I had already rewritten the commit
> message to some extent, but "master" had remained.  Now I pushed the
> patch to branches 14+, having replaced it as you suggested.

Thanks for taking care of this thread, Alvaro.
--
Michael

  47. Re: pg_rewind WAL segments deletion pitfall

    Antonin Houska <ah@cybertec.at> — 2024-11-21T05:43:45Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> Oh wow, thanks for noticing that.  I had already rewritten the commit
> message to some extent, but "master" had remained.  Now I pushed the
> patch to branches 14+, having replaced it as you suggested.

When doing some unrelated work I noticed that in the new test
010_keep_recycled_wals.pl the server fails to reload the configuration
file. The line it complains about is

archive_command = '/usr/bin/perl -e 'exit(1)''

The test still succeeds for some reason.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  48. Re: pg_rewind WAL segments deletion pitfall

    Alexander Kukushkin <cyberdemn@gmail.com> — 2024-11-21T10:42:13Z

    On Thu, 21 Nov 2024 at 06:43, Antonin Houska <ah@cybertec.at> wrote:

> When doing some unrelated work I noticed that in the new test
> 010_keep_recycled_wals.pl the server fails to reload the configuration
> file. The line it complains about is
>
> archive_command = '/usr/bin/perl -e 'exit(1)''
>
> The test still succeeds for some reason.
>

Oh, nice catch.

The attached patch should address it.

-- 
Regards,
--
Alexander Kukushkin

  49. Re: pg_rewind WAL segments deletion pitfall

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-21T16:51:39Z

    On 2024-Nov-21, Alexander Kukushkin wrote:

> Oh, nice catch.

Yeah, sharp eyes there.

> The attached patch should address it.

Thanks, pushed.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)