pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17903: There is a bug in the KeepLogSeg()
13+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2023-04-27 b72623671d c98b06e2f8 1370030369 be40dd63e0 Prevent underflow in KeepLogSeg().
* BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-19 10:26 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 13+ messages in thread
From: PG Bug reporting form @ 2023-04-19 10:26 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: xu.xw2008@163.com
The following bug has been logged on the website:
Bug reference: 17903
Logged by: xu xingwang
Email address: xu.xw2008@163.com
PostgreSQL version: 13.3
Operating system: openEuler
Description:
Hi,
I found that KeepLogSeg() has a piece of code that is not correctly.
segno may be larger than currSegNo, since the slot_keep_segs variable is of
type "uint64", in this case the code "if (currSegNo - segno >
slot_keep_segs)" is incorrect.
"if (currSegNo - segno < keep_segs)" is also the same.
Checkpoint calls the KeepLogSeg function, and there are many operations
between recptr and XLogGetReplicationSlotMinimumLSN, including updating the
pg_control file, so segno may be larger than currSegNo.
KeepLogSeg(XLogRecPtr recptr, XLogSegNo *logSegNo)
{
XLogSegNo currSegNo;
XLogSegNo segno;
XLogRecPtr keep;
XLByteToSeg(recptr, currSegNo, wal_segment_size);
segno = currSegNo;
/*
* Calculate how many segments are kept by slots first, adjusting for
* max_slot_wal_keep_size.
*/
keep = XLogGetReplicationSlotMinimumLSN();
if (keep != InvalidXLogRecPtr)
{
XLByteToSeg(keep, segno, wal_segment_size);
/* Cap by max_slot_wal_keep_size ... */
if (max_slot_wal_keep_size_mb >= 0)
{
uint64 slot_keep_segs;
slot_keep_segs =
ConvertToXSegs(max_slot_wal_keep_size_mb, wal_segment_size);
if (currSegNo - segno > slot_keep_segs)
segno = currSegNo - slot_keep_segs;
}
}
/* but, keep at least wal_keep_size if that's set */
if (wal_keep_size_mb > 0)
{
uint64 keep_segs;
keep_segs = ConvertToXSegs(wal_keep_size_mb, wal_segment_size);
if (currSegNo - segno < keep_segs)
{
/* avoid underflow, don't go below 1 */
if (currSegNo <= keep_segs)
segno = 1;
else
segno = currSegNo - keep_segs;
}
}
regards.
--
xu xingwang
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-20 03:04 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 13+ messages in thread
From: Kyotaro Horiguchi @ 2023-04-20 03:04 UTC (permalink / raw)
To: xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
At Wed, 19 Apr 2023 10:26:13 +0000, PG Bug reporting form <noreply@postgresql.org> wrote in
> I found that KeepLogSeg() has a piece of code that is not correctly.
>
> segno may be larger than currSegNo, since the slot_keep_segs variable is of
> type "uint64", in this case the code "if (currSegNo - segno >
> slot_keep_segs)" is incorrect.
>
> "if (currSegNo - segno < keep_segs)" is also the same.
>
> Checkpoint calls the KeepLogSeg function, and there are many operations
> between recptr and XLogGetReplicationSlotMinimumLSN, including updating the
> pg_control file, so segno may be larger than currSegNo.
Correct. Thanks for the report.
If checkpointer somehow takes a long time between inserting a
checkpoint record and removing WAL files, while replication advances a
certain distnace, it can actually happen. Although that behavior
doesn't directly affect max_slot_wal_keep_size, it does disrupt the
effect of wal_keep_size.
The thinko was that we incorrectly assumed the slot minimum LSN can't
be larger than the checkpoint record LSN. We don't need to consider
max_slot_wal_keep_size if the slot minimum LSN is already larger than
currSegNo.
The attached fix works. However, I can't come up with a reasonable
testing script.
This dates back to 13, where max_slot_wal_keep_size was introduced.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Fix-incorrect-calculation-regarding-max_slot_wal_kee.patch (text/x-patch, 0B) download
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-20 07:40 Junwang Zhao <zhjwpku@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Junwang Zhao @ 2023-04-20 07:40 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
If `segno` can be larger than `currSegNo`, your patch seems to miss
the following branch,
are you missing this for some particular reason?
```
/* but, keep at least wal_keep_size if that's set */
if (wal_keep_size_mb > 0)
{
uint64 keep_segs;
keep_segs = ConvertToXSegs(wal_keep_size_mb, wal_segment_size);
if (currSegNo - segno < keep_segs) <<<< see here
{
/* avoid underflow, don't go below 1 */
if (currSegNo <= keep_segs)
segno = 1;
else
segno = currSegNo - keep_segs;
}
}
```
On Thu, Apr 20, 2023 at 11:04 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Wed, 19 Apr 2023 10:26:13 +0000, PG Bug reporting form <noreply@postgresql.org> wrote in
> > I found that KeepLogSeg() has a piece of code that is not correctly.
> >
> > segno may be larger than currSegNo, since the slot_keep_segs variable is of
> > type "uint64", in this case the code "if (currSegNo - segno >
> > slot_keep_segs)" is incorrect.
> >
> > "if (currSegNo - segno < keep_segs)" is also the same.
> >
> > Checkpoint calls the KeepLogSeg function, and there are many operations
> > between recptr and XLogGetReplicationSlotMinimumLSN, including updating the
> > pg_control file, so segno may be larger than currSegNo.
>
> Correct. Thanks for the report.
>
> If checkpointer somehow takes a long time between inserting a
> checkpoint record and removing WAL files, while replication advances a
> certain distnace, it can actually happen. Although that behavior
> doesn't directly affect max_slot_wal_keep_size, it does disrupt the
> effect of wal_keep_size.
>
> The thinko was that we incorrectly assumed the slot minimum LSN can't
> be larger than the checkpoint record LSN. We don't need to consider
> max_slot_wal_keep_size if the slot minimum LSN is already larger than
> currSegNo.
>
> The attached fix works. However, I can't come up with a reasonable
> testing script.
>
> This dates back to 13, where max_slot_wal_keep_size was introduced.
>
> regards.
>
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
--
Regards
Junwang Zhao
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-20 08:58 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Junwang Zhao <zhjwpku@gmail.com>
0 siblings, 2 replies; 13+ messages in thread
From: Kyotaro Horiguchi @ 2023-04-20 08:58 UTC (permalink / raw)
To: zhjwpku@gmail.com; +Cc: xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
At Thu, 20 Apr 2023 15:40:14 +0800, Junwang Zhao <zhjwpku@gmail.com> wrote in
> If `segno` can be larger than `currSegNo`, your patch seems to miss
> the following branch,
> are you missing this for some particular reason?
Oops! Sorry for the mistake and thanks for pointing it out.
I should have kept segno within the reasonable range.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
v2-0001-Fix-incorrect-calculation-regarding-max_slot_wal_.patch (text/x-patch, 0B) download
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-20 11:17 Junwang Zhao <zhjwpku@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
1 sibling, 0 replies; 13+ messages in thread
From: Junwang Zhao @ 2023-04-20 11:17 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
This patch looks reasonable.
+1
On Thu, Apr 20, 2023 at 4:58 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Thu, 20 Apr 2023 15:40:14 +0800, Junwang Zhao <zhjwpku@gmail.com> wrote in
> > If `segno` can be larger than `currSegNo`, your patch seems to miss
> > the following branch,
> > are you missing this for some particular reason?
>
> Oops! Sorry for the mistake and thanks for pointing it out.
>
> I should have kept segno within the reasonable range.
>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
--
Regards
Junwang Zhao
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-20 22:02 Nathan Bossart <nathandbossart@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
1 sibling, 1 reply; 13+ messages in thread
From: Nathan Bossart @ 2023-04-20 22:02 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: zhjwpku@gmail.com; xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
On Thu, Apr 20, 2023 at 05:58:14PM +0900, Kyotaro Horiguchi wrote:
> + /*
> + * Slot minimum LSN could be greater than recptr. In such cases, no
> + * segments are protected by slots but we still need to keep segno in a
> + * reasonable range for subsequent calculations to avoid underflow.
> + */
> + if (segno > currSegNo)
> + segno = currSegNo;
> +
I wonder if it'd be better to instead change
if (currSegNo - segno > slot_keep_segs)
to
if (currSegNo > segno + slot_keep_segs)
and
if (currSegNo - segno < keep_segs)
to
if (currSegNo < segNo + keep_segs)
If segno > currSegNo, the first conditional would be false as expected, and
the second would be true as expected. We could also use
pg_sub_u64_overflow() to detect underflow, but that might be excessive in
this case.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-21 01:42 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Kyotaro Horiguchi @ 2023-04-21 01:42 UTC (permalink / raw)
To: nathandbossart@gmail.com; +Cc: zhjwpku@gmail.com; xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
At Thu, 20 Apr 2023 15:02:42 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
> I wonder if it'd be better to instead change
>
> if (currSegNo - segno > slot_keep_segs)
> to
> if (currSegNo > segno + slot_keep_segs)
>
> and
>
> if (currSegNo - segno < keep_segs)
> to
> if (currSegNo < segNo + keep_segs)
>
> If segno > currSegNo, the first conditional would be false as expected, and
> the second would be true as expected. We could also use
> pg_sub_u64_overflow() to detect underflow, but that might be excessive in
> this case.
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-21 21:43 Nathan Bossart <nathandbossart@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Bossart @ 2023-04-21 21:43 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: zhjwpku@gmail.com; xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
On Fri, Apr 21, 2023 at 10:42:31AM +0900, Kyotaro Horiguchi wrote:
> From what I understand, the XLogSegNo calculations are designed
> without considering the actual value range. Basically, it assumes that
> (XLogSegNo + <any positive int>) can overflow. If we take the actual
> value range into account, we can make that change.
>
> The choice lies on whether we assume the actual value range or not.
Yeah, after staring at this some more, I think your proposed fix is the way
to go. Alternatively, we could adjust the conditional for the
max_slot_wal_keep_size block to
if (keep != InvalidXLogRecPtr && keep < recptr)
but AFAICT that yields the exact same behavior.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-24 19:14 Nathan Bossart <nathandbossart@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Nathan Bossart @ 2023-04-24 19:14 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: zhjwpku@gmail.com; xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
Here is a new version of the patch. It is fundamentally the same as v2,
but I've adjusted the comment and commit message a bit. Barring
objections, I am planning to commit this (and back-patch to v13) in the
near future.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v3-0001-Prevent-underflow-in-KeepLogSeg.patch (text/x-diff, 0B) download
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-25 04:14 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Kyotaro Horiguchi @ 2023-04-25 04:14 UTC (permalink / raw)
To: nathandbossart@gmail.com; +Cc: zhjwpku@gmail.com; xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
At Mon, 24 Apr 2023 12:14:52 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
> Here is a new version of the patch. It is fundamentally the same as v2,
> but I've adjusted the comment and commit message a bit. Barring
> objections, I am planning to commit this (and back-patch to v13) in the
> near future.
> When determining the oldest segment that must be kept for
> replication slots, KeepLogSeg() might calculate a segment number
> ahead of the one for the LSN given to the function. This causes
Maybe the KeepLogSeg() is a mistake of
XLogGetReplicationSlotMinimumLSN()?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-27 21:47 Nathan Bossart <nathandbossart@gmail.com>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 2 replies; 13+ messages in thread
From: Nathan Bossart @ 2023-04-27 21:47 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: zhjwpku@gmail.com; xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
On Tue, Apr 25, 2023 at 01:14:52PM +0900, Kyotaro Horiguchi wrote:
> At Mon, 24 Apr 2023 12:14:52 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
>> When determining the oldest segment that must be kept for
>> replication slots, KeepLogSeg() might calculate a segment number
>> ahead of the one for the LSN given to the function. This causes
>
> Maybe the KeepLogSeg() is a mistake of
> XLogGetReplicationSlotMinimumLSN()?
I adjusted the commit message to call out that function explicitly. Also,
I decided to go with the "keep < recptr" approach since there's no reason
to do anything in that block otherwise.
Thanks!
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-27 23:08 Michael Paquier <michael@paquier.xyz>
parent: Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 0 replies; 13+ messages in thread
From: Michael Paquier @ 2023-04-27 23:08 UTC (permalink / raw)
To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; zhjwpku@gmail.com; xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
On Thu, Apr 27, 2023 at 02:47:46PM -0700, Nathan Bossart wrote:
> I adjusted the commit message to call out that function explicitly. Also,
> I decided to go with the "keep < recptr" approach since there's no reason
> to do anything in that block otherwise.
b726236 exists in the tree, but it seems like the message of
pgsql-committers is held on moderation?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: BUG #17903: There is a bug in the KeepLogSeg()
@ 2023-04-28 06:21 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
1 sibling, 0 replies; 13+ messages in thread
From: Kyotaro Horiguchi @ 2023-04-28 06:21 UTC (permalink / raw)
To: nathandbossart@gmail.com; +Cc: zhjwpku@gmail.com; xu.xw2008@163.com; pgsql-bugs@lists.postgresql.org
At Thu, 27 Apr 2023 14:47:46 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
> On Tue, Apr 25, 2023 at 01:14:52PM +0900, Kyotaro Horiguchi wrote:
> > At Mon, 24 Apr 2023 12:14:52 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
> >> When determining the oldest segment that must be kept for
> >> replication slots, KeepLogSeg() might calculate a segment number
> >> ahead of the one for the LSN given to the function. This causes
> >
> > Maybe the KeepLogSeg() is a mistake of
> > XLogGetReplicationSlotMinimumLSN()?
>
> I adjusted the commit message to call out that function explicitly. Also,
> I decided to go with the "keep < recptr" approach since there's no reason
> to do anything in that block otherwise.
That works, too. Thanks!
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2023-04-28 06:21 UTC | newest]
Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-04-19 10:26 BUG #17903: There is a bug in the KeepLogSeg() PG Bug reporting form <noreply@postgresql.org>
2023-04-20 03:04 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2023-04-20 07:40 ` Junwang Zhao <zhjwpku@gmail.com>
2023-04-20 08:58 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2023-04-20 11:17 ` Junwang Zhao <zhjwpku@gmail.com>
2023-04-20 22:02 ` Nathan Bossart <nathandbossart@gmail.com>
2023-04-21 01:42 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2023-04-21 21:43 ` Nathan Bossart <nathandbossart@gmail.com>
2023-04-24 19:14 ` Nathan Bossart <nathandbossart@gmail.com>
2023-04-25 04:14 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2023-04-27 21:47 ` Nathan Bossart <nathandbossart@gmail.com>
2023-04-27 23:08 ` Michael Paquier <michael@paquier.xyz>
2023-04-28 06:21 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox