public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v1 1/1] add note about re-archiving in docs
9+ messages / 6 participants
[nested] [flat]
* [PATCH v1 1/1] add note about re-archiving in docs
@ 2022-07-07 17:43 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Nathan Bossart @ 2022-07-07 17:43 UTC (permalink / raw)
---
doc/src/sgml/backup.sgml | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 73a774d3d7..dd1c360455 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -685,10 +685,24 @@ test ! -f /mnt/server/archivedir/00000001000000A900000065 && cp pg_wal/0
</para>
<para>
- It is advisable to test your proposed archive library to ensure that it
- indeed does not overwrite an existing file, <emphasis>and that it returns
- <literal>false</literal> in this case</emphasis>.
- The example command above for Unix ensures this by including a separate
+ In rare cases, <productname>PostgreSQL</productname> may attempt to
+ re-archive a WAL file that was previously archived. For example, if the
+ system crashes before the server makes a durable record of archival success,
+ the server will attempt to archive the file again after restarting (provided
+ archiving is still enabled). It is advisable to test your proposed archive
+ library to ensure that it indeed does not overwrite an existing file. When
+ a pre-existing file is encountered, the archive library may return
+ <literal>true</literal> if the WAL file has identical contents to the
+ pre-existing archive. Alternatively, the archive library can return
+ <literal>false</literal> anytime a pre-existing file is encountered, but
+ this will require manual action by an administrator to resolve. If a
+ pre-existing file contains different contents than the WAL file being
+ archived, the archive library <emphasis>must</emphasis> return false.
+ </para>
+
+ <para>
+ The example command above for Unix avoids overwriting a pre-existing archive
+ by including a separate
<command>test</command> step. On some Unix platforms, <command>cp</command> has
switches such as <option>-i</option> that can be used to do the same thing
less verbosely, but you should not rely on these without verifying that
--
2.25.1
--SLDf9lqlvOQaIe6s--
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: 001_rep_changes.pl fails due to publisher stuck on shutdown
@ 2024-06-11 08:57 Amit Kapila <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Amit Kapila @ 2024-06-11 08:57 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; pgsql-hackers
On Tue, Jun 11, 2024 at 12:34 PM Kyotaro Horiguchi
<[email protected]> wrote:
>
> At Tue, 11 Jun 2024 11:32:12 +0530, Amit Kapila <[email protected]> wrote in
> > Sorry, it is not clear to me why we failed to flush the last
> > continuation record in logical walsender? I see that we try to flush
> > the WAL after receiving got_STOPPING in WalSndWaitForWal(), why is
> > that not sufficient?
>
> It seems that, it uses XLogBackgroundFlush(), which does not guarantee
> flushing WAL until the end.
>
What would it take to ensure the same? I am trying to explore this
path because currently logical WALSender sends any outstanding logs up
to the shutdown checkpoint record (i.e., the latest record) and waits
for them to be replicated to the standby before exit. Please take a
look at the comments where we call WalSndDone(). The fix you are
proposing will break that guarantee.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: 001_rep_changes.pl fails due to publisher stuck on shutdown
@ 2024-06-12 01:13 Kyotaro Horiguchi <[email protected]>
parent: Amit Kapila <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Kyotaro Horiguchi @ 2024-06-12 01:13 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers
At Tue, 11 Jun 2024 14:27:28 +0530, Amit Kapila <[email protected]> wrote in
> On Tue, Jun 11, 2024 at 12:34 PM Kyotaro Horiguchi
> <[email protected]> wrote:
> >
> > At Tue, 11 Jun 2024 11:32:12 +0530, Amit Kapila <[email protected]> wrote in
> > > Sorry, it is not clear to me why we failed to flush the last
> > > continuation record in logical walsender? I see that we try to flush
> > > the WAL after receiving got_STOPPING in WalSndWaitForWal(), why is
> > > that not sufficient?
> >
> > It seems that, it uses XLogBackgroundFlush(), which does not guarantee
> > flushing WAL until the end.
> >
>
> What would it take to ensure the same? I am trying to explore this
> path because currently logical WALSender sends any outstanding logs up
> to the shutdown checkpoint record (i.e., the latest record) and waits
> for them to be replicated to the standby before exit. Please take a
> look at the comments where we call WalSndDone(). The fix you are
> proposing will break that guarantee.
Shutdown checkpoint is performed after the walsender completed
termination since 086221cf6b, aiming to prevent walsenders from
generating competing WAL (by, for example, CREATE_REPLICATION_SLOT)
records with the shutdown checkpoint. Thus, it seems that the
walsender cannot see the shutdown record, and a certain amount of
bytes before it, as the walsender appears to have relied on the
checkpoint flushing its record, rather than on XLogBackgroundFlush().
If we approve of the walsender being terminated before the shutdown
checkpoint, we need to "fix" the comment, then provide a function to
ensure the synchronization of WAL records.
I'll consider this direction for a while.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: 001_rep_changes.pl fails due to publisher stuck on shutdown
@ 2024-06-13 03:59 Amit Kapila <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Amit Kapila @ 2024-06-13 03:59 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; pgsql-hackers
On Wed, Jun 12, 2024 at 6:43 AM Kyotaro Horiguchi
<[email protected]> wrote:
>
> At Tue, 11 Jun 2024 14:27:28 +0530, Amit Kapila <[email protected]> wrote in
> > On Tue, Jun 11, 2024 at 12:34 PM Kyotaro Horiguchi
> > <[email protected]> wrote:
> > >
> > > At Tue, 11 Jun 2024 11:32:12 +0530, Amit Kapila <[email protected]> wrote in
> > > > Sorry, it is not clear to me why we failed to flush the last
> > > > continuation record in logical walsender? I see that we try to flush
> > > > the WAL after receiving got_STOPPING in WalSndWaitForWal(), why is
> > > > that not sufficient?
> > >
> > > It seems that, it uses XLogBackgroundFlush(), which does not guarantee
> > > flushing WAL until the end.
> > >
> >
> > What would it take to ensure the same? I am trying to explore this
> > path because currently logical WALSender sends any outstanding logs up
> > to the shutdown checkpoint record (i.e., the latest record) and waits
> > for them to be replicated to the standby before exit. Please take a
> > look at the comments where we call WalSndDone(). The fix you are
> > proposing will break that guarantee.
>
> Shutdown checkpoint is performed after the walsender completed
> termination since 086221cf6b,
>
Yeah, but the commit you quoted later reverted by commit 703f148e98
and committed again as c6c3334364.
> aiming to prevent walsenders from
> generating competing WAL (by, for example, CREATE_REPLICATION_SLOT)
> records with the shutdown checkpoint. Thus, it seems that the
> walsender cannot see the shutdown record,
>
This is true of logical walsender. The physical walsender do send
shutdown checkpoint record before getting terminated.
> and a certain amount of
> bytes before it, as the walsender appears to have relied on the
> checkpoint flushing its record, rather than on XLogBackgroundFlush().
>
> If we approve of the walsender being terminated before the shutdown
> checkpoint, we need to "fix" the comment, then provide a function to
> ensure the synchronization of WAL records.
>
Which comment do you want to fix?
> I'll consider this direction for a while.
>
Okay, thanks.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: 001_rep_changes.pl fails due to publisher stuck on shutdown
@ 2024-06-18 08:07 Kyotaro Horiguchi <[email protected]>
parent: Amit Kapila <[email protected]>
0 siblings, 2 replies; 9+ messages in thread
From: Kyotaro Horiguchi @ 2024-06-18 08:07 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers
At Thu, 13 Jun 2024 09:29:03 +0530, Amit Kapila <[email protected]> wrote in
> Yeah, but the commit you quoted later reverted by commit 703f148e98
> and committed again as c6c3334364.
Yeah, right..
> > aiming to prevent walsenders from
> > generating competing WAL (by, for example, CREATE_REPLICATION_SLOT)
> > records with the shutdown checkpoint. Thus, it seems that the
> > walsender cannot see the shutdown record,
> >
>
> This is true of logical walsender. The physical walsender do send
> shutdown checkpoint record before getting terminated.
Yes, I know. They differ in their blocking mechanisms.
> > and a certain amount of
> > bytes before it, as the walsender appears to have relied on the
> > checkpoint flushing its record, rather than on XLogBackgroundFlush().
> >
> > If we approve of the walsender being terminated before the shutdown
> > checkpoint, we need to "fix" the comment, then provide a function to
> > ensure the synchronization of WAL records.
> >
>
> Which comment do you want to fix?
Yeah. The part you seem to think I was trying to fix is actually
fine. Instead, I have revised the comment on the modified section to
make its intention clearer.
> > I'll consider this direction for a while.
> >
>
> Okay, thanks.
The attached patch is it. It's only for the master.
I decided not to create a new function because the simple code has
only one caller. I haven't seen the test script fail with this fix.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
[text/x-patch] 0001-Ensure-WAL-is-written-out-when-terminating-a-logical.patch (1.7K, ../../[email protected]/2-0001-Ensure-WAL-is-written-out-when-terminating-a-logical.patch)
download | inline diff:
From 663bdeaf8d4d2f5a192dd3ecb6d2817d9b1311f1 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email protected]>
Date: Tue, 18 Jun 2024 16:36:43 +0900
Subject: [PATCH] Ensure WAL is written out when terminating a logical
walsender
In commit c6c3334364, XLogBackgroundFlush() was assumed to flush all
written WAL to the end, but this function may not flush the last
incomplete page in a single call. In such cases, if the last written
page ends with a continuation record, the latter part will not be
flushed, causing shutdown to wait indefinitely for that part. This
commit ensures that the written records are fully flushed to the end,
guaranteeing expected behavior.
---
src/backend/replication/walsender.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index c623b07cf0..5aa0f58238 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1858,12 +1858,13 @@ WalSndWaitForWal(XLogRecPtr loc)
ProcessRepliesIfAny();
/*
- * If we're shutting down, trigger pending WAL to be written out,
- * otherwise we'd possibly end up waiting for WAL that never gets
- * written, because walwriter has shut down already.
+ * If we're shutting down, all WAL-writing processes are gone, leaving
+ * only checkpointer to perform the shutdown checkpoint. Ensure that
+ * any pending WAL is written out here; otherwise, we'd possibly end up
+ * waiting for WAL that never gets written.
*/
- if (got_STOPPING)
- XLogBackgroundFlush();
+ if (got_STOPPING && !RecoveryInProgress())
+ XLogFlush(GetInsertRecPtr());
/*
* To avoid the scenario where standbys need to catch up to a newer
--
2.43.0
^ permalink raw reply [nested|flat] 9+ messages in thread
* RE: 001_rep_changes.pl fails due to publisher stuck on shutdown
@ 2024-06-19 05:14 Hayato Kuroda (Fujitsu) <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 2 replies; 9+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2024-06-19 05:14 UTC (permalink / raw)
To: 'Kyotaro Horiguchi' <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers; [email protected] <[email protected]>
Dear Horiguchi-san,
Thanks for sharing the patch! I agree this approach (ensure WAL records are flushed)
Is more proper than others.
I have an unclear point. According to the comment atop GetInsertRecPtr(), it just
returns the approximated value - the position of the last full WAL page [1].
If there is a continuation WAL record which across a page, will it return the
halfway point of the WAL record (end of the first WAL page)? If so, the proposed
fix seems not sufficient. We have to point out the exact the end of the record.
[1]:
/*
* GetInsertRecPtr -- Returns the current insert position.
*
* NOTE: The value *actually* returned is the position of the last full
* xlog page. It lags behind the real insert position by at most 1 page.
* For that, we don't need to scan through WAL insertion locks, and an
* approximation is enough for the current usage of this function.
*/
XLogRecPtr
GetInsertRecPtr(void)
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: 001_rep_changes.pl fails due to publisher stuck on shutdown
@ 2024-06-19 07:58 Peter Smith <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
1 sibling, 0 replies; 9+ messages in thread
From: Peter Smith @ 2024-06-19 07:58 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; pgsql-hackers
FYI - I applied this latest patch and re-ran the original failing test
script 10 times (e.g. 10 x 100 test iterations; it took 4+ hours).
There were zero failures observed in my environment.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: 001_rep_changes.pl fails due to publisher stuck on shutdown
@ 2024-06-21 01:48 Michael Paquier <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
1 sibling, 0 replies; 9+ messages in thread
From: Michael Paquier @ 2024-06-21 01:48 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: 'Kyotaro Horiguchi' <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers; [email protected] <[email protected]>
On Wed, Jun 19, 2024 at 05:14:50AM +0000, Hayato Kuroda (Fujitsu) wrote:
> I have an unclear point. According to the comment atop GetInsertRecPtr(), it just
> returns the approximated value - the position of the last full WAL page [1].
> If there is a continuation WAL record which across a page, will it return the
> halfway point of the WAL record (end of the first WAL page)? If so, the proposed
> fix seems not sufficient. We have to point out the exact the end of the record.
Yeah, that a thing of the patch I am confused with. How are we sure
that this is the correct LSN to rely on? If that it the case, the
patch does not offer an explanation about why it is better.
WalSndWaitForWal() is called only in the context of page callback for a
logical WAL sender. Shouldn't we make the flush conditional on what's
stored in XLogReaderState.missingContrecPtr? Aka, if we know that
we're in the middle of the decoding of a continuation record, we
should wait until we've dealt with it, no?
In short, I would imagine that WalSndWaitForWal() should know more
about XLogReaderState is doing. But perhaps I'm missing something.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: 001_rep_changes.pl fails due to publisher stuck on shutdown
@ 2024-06-21 06:18 Amit Kapila <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
1 sibling, 0 replies; 9+ messages in thread
From: Amit Kapila @ 2024-06-21 06:18 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers
On Wed, Jun 19, 2024 at 10:44 AM Hayato Kuroda (Fujitsu)
<[email protected]> wrote:
>
> Dear Horiguchi-san,
>
> Thanks for sharing the patch! I agree this approach (ensure WAL records are flushed)
> Is more proper than others.
>
> I have an unclear point. According to the comment atop GetInsertRecPtr(), it just
> returns the approximated value - the position of the last full WAL page [1].
> If there is a continuation WAL record which across a page, will it return the
> halfway point of the WAL record (end of the first WAL page)? If so, the proposed
> fix seems not sufficient. We have to point out the exact the end of the record.
>
You have a point but if this theory is correct why we are not able to
reproduce the issue after this patch? Also, how to get the WAL
location up to which we need to flush? Is XLogCtlData->logInsertResult
the one we are looking for?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2024-06-21 06:18 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-07-07 17:43 [PATCH v1 1/1] add note about re-archiving in docs Nathan Bossart <[email protected]>
2024-06-11 08:57 Re: 001_rep_changes.pl fails due to publisher stuck on shutdown Amit Kapila <[email protected]>
2024-06-12 01:13 ` Re: 001_rep_changes.pl fails due to publisher stuck on shutdown Kyotaro Horiguchi <[email protected]>
2024-06-13 03:59 ` Re: 001_rep_changes.pl fails due to publisher stuck on shutdown Amit Kapila <[email protected]>
2024-06-18 08:07 ` Re: 001_rep_changes.pl fails due to publisher stuck on shutdown Kyotaro Horiguchi <[email protected]>
2024-06-19 05:14 ` RE: 001_rep_changes.pl fails due to publisher stuck on shutdown Hayato Kuroda (Fujitsu) <[email protected]>
2024-06-21 01:48 ` Re: 001_rep_changes.pl fails due to publisher stuck on shutdown Michael Paquier <[email protected]>
2024-06-21 06:18 ` Re: 001_rep_changes.pl fails due to publisher stuck on shutdown Amit Kapila <[email protected]>
2024-06-19 07:58 ` Re: 001_rep_changes.pl fails due to publisher stuck on shutdown Peter Smith <[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