public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Fix timeline-tracking failure while sending a historic timeline
3+ messages / 3 participants
[nested] [flat]
* [PATCH] Fix timeline-tracking failure while sending a historic timeline
@ 2021-01-05 04:34 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Kyotaro Horiguchi @ 2021-01-05 04:34 UTC (permalink / raw)
Walsender should track timeline switches while sending a historic
timeline. Regain that behavior, which was broken in PG13, by a thinko
of 709d003fbd. Backpatch to PG13.
---
src/backend/replication/walsender.c | 2 +-
src/test/perl/PostgresNode.pm | 36 ++++++++++++++++++++
src/test/recovery/t/001_stream_rep.pl | 41 ++++++++++++++++++++++-
src/test/recovery/t/019_replslot_limit.pl | 37 ++++----------------
4 files changed, 83 insertions(+), 33 deletions(-)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 7f87eb7f19..04f6c3ebb4 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2478,7 +2478,7 @@ WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo,
XLogSegNo endSegNo;
XLByteToSeg(sendTimeLineValidUpto, endSegNo, state->segcxt.ws_segsize);
- if (state->seg.ws_segno == endSegNo)
+ if (nextSegNo == endSegNo)
*tli_p = sendTimeLineNextTLI;
}
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 980f1f1533..a08c71b549 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -2138,6 +2138,42 @@ sub pg_recvlogical_upto
=pod
+=item $node->current_log_position()
+
+Return the current position of server log.
+
+=cut
+
+sub current_log_position
+{
+ my $self = shift;
+
+ return (stat $self->logfile)[7];
+}
+
+=pod
+
+=item $node->find_in_log($pattern, $startpos)
+
+Returns whether the $pattern occurs after $startpos in the server log.
+
+=cut
+
+sub find_in_log
+{
+ my ($self, $pattern, $startpos) = @_;
+
+ $startpos = 0 unless defined $startpos;
+ my $log = TestLib::slurp_file($self->logfile);
+ return 0 if (length($log) <= $startpos);
+
+ $log = substr($log, $startpos);
+
+ return $log =~ m/$pattern/;
+}
+
+=pod
+
=back
=cut
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 778f11b28b..8d2b24fe55 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -2,8 +2,9 @@
use strict;
use warnings;
use PostgresNode;
+use Time::HiRes qw(usleep);
use TestLib;
-use Test::More tests => 36;
+use Test::More tests => 37;
# Initialize master node
my $node_master = get_new_node('master');
@@ -409,3 +410,41 @@ ok( ($phys_restart_lsn_pre cmp $phys_restart_lsn_post) == 0,
my $master_data = $node_master->data_dir;
ok(!-f "$master_data/pg_wal/$segment_removed",
"WAL segment $segment_removed recycled after physical slot advancing");
+
+#
+# Check if timeline-increment works while reading a historic timeline.
+my $node_primary_2 = get_new_node('primary_2');
+# archiving is needed to create .paritial segment
+$node_primary_2->init(allows_streaming => 1, has_archiving => 1);
+$node_primary_2->start;
+$node_primary_2->backup($backup_name);
+my $node_standby_3 = get_new_node('standby_3');
+$node_standby_3->init_from_backup($node_primary_2, $backup_name,
+ has_streaming => 1);
+$node_primary_2->stop;
+$node_primary_2->set_standby_mode; # increment primary timeline
+$node_primary_2->start;
+$node_primary_2->promote;
+my $logstart = $node_standby_3->current_log_position();
+$node_standby_3->start;
+
+my $success = 0;
+for (my $i = 0 ; $i < 1000; $i++)
+{
+ if ($node_standby_3->find_in_log(
+ "requested WAL segment [0-9A-F]+ has already been removed",
+ $logstart))
+ {
+ last;
+ }
+ elsif ($node_standby_3->find_in_log(
+ "End of WAL reached on timeline",
+ $logstart))
+ {
+ $success = 1;
+ last;
+ }
+ usleep(100_000);
+}
+
+ok($success, 'Timeline increment while reading a historic timeline');
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index a7231dcd47..8b3c5de057 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -165,19 +165,17 @@ $node_master->wait_for_catchup($node_standby, 'replay', $start_lsn);
$node_standby->stop;
-ok( !find_in_log(
- $node_standby,
- "requested WAL segment [0-9A-F]+ has already been removed"),
+ok( !$node_standby->find_in_log(
+ "requested WAL segment [0-9A-F]+ has already been removed"),
'check that required WAL segments are still available');
# Advance WAL again, the slot loses the oldest segment.
-my $logstart = get_log_size($node_master);
+my $logstart = $node_master->current_log_position();
advance_wal($node_master, 7);
$node_master->safe_psql('postgres', "CHECKPOINT;");
# WARNING should be issued
-ok( find_in_log(
- $node_master,
+ok( $node_master->find_in_log(
"invalidating slot \"rep1\" because its restart_lsn [0-9A-F/]+ exceeds max_slot_wal_keep_size",
$logstart),
'check that the warning is logged');
@@ -190,14 +188,13 @@ is($result, "rep1|f|t|lost|",
'check that the slot became inactive and the state "lost" persists');
# The standby no longer can connect to the master
-$logstart = get_log_size($node_standby);
+$logstart = $node_standby->current_log_position();
$node_standby->start;
my $failed = 0;
for (my $i = 0; $i < 10000; $i++)
{
- if (find_in_log(
- $node_standby,
+ if ($node_standby->find_in_log(
"requested WAL segment [0-9A-F]+ has already been removed",
$logstart))
{
@@ -264,25 +261,3 @@ sub advance_wal
}
return;
}
-
-# return the size of logfile of $node in bytes
-sub get_log_size
-{
- my ($node) = @_;
-
- return (stat $node->logfile)[7];
-}
-
-# find $pat in logfile of $node after $off-th byte
-sub find_in_log
-{
- my ($node, $pat, $off) = @_;
-
- $off = 0 unless defined $off;
- my $log = TestLib::slurp_file($node->logfile);
- return 0 if (length($log) <= $off);
-
- $log = substr($log, $off);
-
- return $log =~ m/$pat/;
-}
--
2.27.0
----Next_Part(Thu_Jan__7_16_32_36_2021_122)----
^ permalink raw reply [nested|flat] 3+ messages in thread
* Incorrect errno used in OpenWalSummaryFile()
@ 2026-02-02 03:54 Michael Paquier <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Michael Paquier @ 2026-02-02 03:54 UTC (permalink / raw)
To: Postgres hackers <[email protected]>; +Cc: Robert Haas <[email protected]>
Hi all,
While looking at walsummary.c for a different thread, I have bumped
into this code:
OpenWalSummaryFile(WalSummaryFile *ws, bool missing_ok)
[...]
file = PathNameOpenFile(path, O_RDONLY);
if (file < 0 && (errno != EEXIST || !missing_ok))
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m", path)));
And it seems to me that this EEXIST should be an ENOENT? The top of
the function also documents that we want to handle an error when a
summary file does not exist. The comment makes sense to me, not the
code.
It's also worth noting that this function has two callers, both use
missing_ok = false, meaning that the errno check does not really
matter today. If someone plays with this code on HEAD or the
back-branches and decides to introduce a missing_ok=true call, it
could matter, so I'd rather not change this function signature.
This issue has been mentioned here as well, I've just bumped into it
independently a few hours ago:
https://www.postgresql.org/message-id/[email protected]
Regards,
--
Michael
diff --git a/src/backend/backup/walsummary.c b/src/backend/backup/walsummary.c
index 4ee510092f99..4cd1824fbc6b 100644
--- a/src/backend/backup/walsummary.c
+++ b/src/backend/backup/walsummary.c
@@ -214,7 +214,7 @@ OpenWalSummaryFile(WalSummaryFile *ws, bool missing_ok)
LSN_FORMAT_ARGS(ws->end_lsn));
file = PathNameOpenFile(path, O_RDONLY);
- if (file < 0 && (errno != EEXIST || !missing_ok))
+ if (file < 0 && (errno != ENOENT || !missing_ok))
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m", path)));
Attachments:
[text/plain] wal-summary-errno.patch (563B, ../../[email protected]/2-wal-summary-errno.patch)
download | inline diff:
diff --git a/src/backend/backup/walsummary.c b/src/backend/backup/walsummary.c
index 4ee510092f99..4cd1824fbc6b 100644
--- a/src/backend/backup/walsummary.c
+++ b/src/backend/backup/walsummary.c
@@ -214,7 +214,7 @@ OpenWalSummaryFile(WalSummaryFile *ws, bool missing_ok)
LSN_FORMAT_ARGS(ws->end_lsn));
file = PathNameOpenFile(path, O_RDONLY);
- if (file < 0 && (errno != EEXIST || !missing_ok))
+ if (file < 0 && (errno != ENOENT || !missing_ok))
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m", path)));
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Incorrect errno used in OpenWalSummaryFile()
@ 2026-02-02 04:07 Chao Li <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Chao Li @ 2026-02-02 04:07 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]>; Robert Haas <[email protected]>
> On Feb 2, 2026, at 11:54, Michael Paquier <[email protected]> wrote:
>
> Hi all,
>
> While looking at walsummary.c for a different thread, I have bumped
> into this code:
> OpenWalSummaryFile(WalSummaryFile *ws, bool missing_ok)
> [...]
> file = PathNameOpenFile(path, O_RDONLY);
> if (file < 0 && (errno != EEXIST || !missing_ok))
> ereport(ERROR,
> (errcode_for_file_access(),
> errmsg("could not open file \"%s\": %m", path)));
>
> And it seems to me that this EEXIST should be an ENOENT? The top of
> the function also documents that we want to handle an error when a
> summary file does not exist. The comment makes sense to me, not the
> code.
>
> It's also worth noting that this function has two callers, both use
> missing_ok = false, meaning that the errno check does not really
> matter today. If someone plays with this code on HEAD or the
> back-branches and decides to introduce a missing_ok=true call, it
> could matter, so I'd rather not change this function signature.
>
> This issue has been mentioned here as well, I've just bumped into it
> independently a few hours ago:
> https://www.postgresql.org/message-id/[email protected]
>
> Regards,
> --
> Michael
> <wal-summary-errno.patch>
I think this is a correct fix.
```
[EEXIST] O_CREAT and O_EXCL are specified and the file exists.
[ENOENT] O_CREAT is not set and the named file does not exist.
```
Here, the open flag is only O_RDONLY, so we should only expect ENOENT.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-02-02 04:07 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-05 04:34 [PATCH] Fix timeline-tracking failure while sending a historic timeline Kyotaro Horiguchi <[email protected]>
2026-02-02 03:54 Incorrect errno used in OpenWalSummaryFile() Michael Paquier <[email protected]>
2026-02-02 04:07 ` Re: Incorrect errno used in OpenWalSummaryFile() Chao Li <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox