Lazy allocation of pages required for verifying FPI consistency
Hi,
Postgres verifies consistency of FPI from WAL record with the replayed
page during recovery in verifyBackupPageConsistency() when either
wal_consistency_checking for the resource manager is enabled or a WAL
record with XLR_CHECK_CONSISTENCY flag is inserted. While doing so, it
uses two intermediate pages primary_image_masked (FPI from WAL record)
and replay_image_masked (replayed page) which are dynamically
allocated (palloc'd) before the recovery starts, however, they're not
used unless verifyBackupPageConsistency() is called. And these
variables are palloc'd here for getting MAXALIGNed memory as opposed
to static char arrays. Since verifyBackupPageConsistency() gets called
conditionally only when the WAL record has the XLR_CHECK_CONSISTENCY
flag set, it's a waste of memory for these two page variables.
I propose to statically allocate these two pages using PGAlignedBlock
structure lazily in verifyBackupPageConsistency() to not waste dynamic
memory worth 2*BLCKSZ bytes. I'm attaching a small patch herewith.
Thoughts?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Allocate-pages-required-for-checking-backup-consi.patchapplication/x-patch; name=v1-0001-Allocate-pages-required-for-checking-backup-consi.patchDownload
From c5dbad7441b49f90e52b4d2312480f18358b1bae Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 9 Jan 2023 07:11:00 +0000
Subject: [PATCH v1] Allocate pages required for checking backup consistency
lazily
---
src/backend/access/transam/xlogrecovery.c | 30 +++++++----------------
1 file changed, 9 insertions(+), 21 deletions(-)
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index bc3c3eb3e7..c7713dd985 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -293,11 +293,6 @@ static bool backupEndRequired = false;
*/
bool reachedConsistency = false;
-/* Buffers dedicated to consistency checks of size BLCKSZ */
-static char *replay_image_masked = NULL;
-static char *primary_image_masked = NULL;
-
-
/*
* Shared-memory state for WAL recovery.
*/
@@ -580,16 +575,6 @@ InitWalRecovery(ControlFileData *ControlFile, bool *wasShutdown_ptr,
/* Create a WAL prefetcher. */
xlogprefetcher = XLogPrefetcherAllocate(xlogreader);
- /*
- * Allocate two page buffers dedicated to WAL consistency checks. We do
- * it this way, rather than just making static arrays, for two reasons:
- * (1) no need to waste the storage in most instantiations of the backend;
- * (2) a static char array isn't guaranteed to have any particular
- * alignment, whereas palloc() will provide MAXALIGN'd storage.
- */
- replay_image_masked = (char *) palloc(BLCKSZ);
- primary_image_masked = (char *) palloc(BLCKSZ);
-
if (read_backup_label(&CheckPointLoc, &CheckPointTLI, &backupEndRequired,
&backupFromStandby))
{
@@ -2343,6 +2328,8 @@ verifyBackupPageConsistency(XLogReaderState *record)
ForkNumber forknum;
BlockNumber blkno;
int block_id;
+ PGAlignedBlock primary_image_masked;
+ PGAlignedBlock replay_image_masked;
/* Records with no backup blocks have no need for consistency checks. */
if (!XLogRecHasAnyBlockRefs(record))
@@ -2394,7 +2381,7 @@ verifyBackupPageConsistency(XLogReaderState *record)
* Take a copy of the local page where WAL has been applied to have a
* comparison base before masking it...
*/
- memcpy(replay_image_masked, page, BLCKSZ);
+ memcpy(replay_image_masked.data, page, BLCKSZ);
/* No need for this page anymore now that a copy is in. */
UnlockReleaseBuffer(buf);
@@ -2404,7 +2391,7 @@ verifyBackupPageConsistency(XLogReaderState *record)
* expect contents to match. This can happen if recovery is
* restarted.
*/
- if (PageGetLSN(replay_image_masked) > record->EndRecPtr)
+ if (PageGetLSN(replay_image_masked.data) > record->EndRecPtr)
continue;
/*
@@ -2413,7 +2400,7 @@ verifyBackupPageConsistency(XLogReaderState *record)
* page here, a local buffer is fine to hold its contents and a mask
* can be directly applied on it.
*/
- if (!RestoreBlockImage(record, block_id, primary_image_masked))
+ if (!RestoreBlockImage(record, block_id, primary_image_masked.data))
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
errmsg_internal("%s", record->errormsg_buf)));
@@ -2424,12 +2411,13 @@ verifyBackupPageConsistency(XLogReaderState *record)
*/
if (rmgr.rm_mask != NULL)
{
- rmgr.rm_mask(replay_image_masked, blkno);
- rmgr.rm_mask(primary_image_masked, blkno);
+ rmgr.rm_mask(replay_image_masked.data, blkno);
+ rmgr.rm_mask(primary_image_masked.data, blkno);
}
/* Time to compare the primary and replay images. */
- if (memcmp(replay_image_masked, primary_image_masked, BLCKSZ) != 0)
+ if (memcmp(replay_image_masked.data, primary_image_masked.data,
+ BLCKSZ) != 0)
{
elog(FATAL,
"inconsistent page found, rel %u/%u/%u, forknum %u, blkno %u",
--
2.34.1
At Mon, 9 Jan 2023 20:00:00 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
I propose to statically allocate these two pages using PGAlignedBlock
structure lazily in verifyBackupPageConsistency() to not waste dynamic
memory worth 2*BLCKSZ bytes. I'm attaching a small patch herewith.Thoughts?
IMHO, it's a bit scaring to me to push down the execution stack by
that large size. I tend to choose the (current) possible memory
wasting only on startup process than digging stack deeply.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thu, Jan 12, 2023 at 4:29 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Mon, 9 Jan 2023 20:00:00 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
I propose to statically allocate these two pages using PGAlignedBlock
structure lazily in verifyBackupPageConsistency() to not waste dynamic
memory worth 2*BLCKSZ bytes. I'm attaching a small patch herewith.Thoughts?
IMHO, it's a bit scaring to me to push down the execution stack by
that large size. I tend to choose the (current) possible memory
wasting only on startup process than digging stack deeply.
+1
On Thu, Jan 12, 2023 at 1:59 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Mon, 9 Jan 2023 20:00:00 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
I propose to statically allocate these two pages using PGAlignedBlock
structure lazily in verifyBackupPageConsistency() to not waste dynamic
memory worth 2*BLCKSZ bytes. I'm attaching a small patch herewith.Thoughts?
IMHO, it's a bit scaring to me to push down the execution stack by
that large size. I tend to choose the (current) possible memory
wasting only on startup process than digging stack deeply.
On the contrary, PGAlignedBlock is being used elsewhere in the code;
some of them are hot paths. verifyBackupPageConsistency() is not
something that gets called always i.e. WAL consistency checks are done
conditionally - when either one enables wal_consistency_checking for
the rmgr or the WAL record is flagged with
XLR_CHECK_CONSISTENCY (core doesn't do, it's an external module, if
any, do that).
I really don't see much of a problem in allocating them statically and
pushing closer to where they're being used. If this really concerns,
at the least, the dynamic allocation needs to be pushed to
verifyBackupPageConsistency() IMO with if (first_time) { allocate two
blocks with palloc} and use them. This at least saves some memory on
the heap for most of the servers out there.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Thu, Jan 12, 2023 at 04:37:38PM +0800, Julien Rouhaud wrote:
On Thu, Jan 12, 2023 at 4:29 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:IMHO, it's a bit scaring to me to push down the execution stack by
that large size. I tend to choose the (current) possible memory
wasting only on startup process than digging stack deeply.+1
Indeed. I agree to leave that be.
--
Michael
At Thu, 12 Jan 2023 15:02:25 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
On the contrary, PGAlignedBlock is being used elsewhere in the code;
I noticed it and had the same feeling, and thought that they don't
justify to do the same at other places.
some of them are hot paths. verifyBackupPageConsistency() is not
something that gets called always i.e. WAL consistency checks are done
conditionally - when either one enables wal_consistency_checking for
the rmgr or the WAL record is flagged with
XLR_CHECK_CONSISTENCY (core doesn't do, it's an external module, if
any, do that).
Right. So we could allocate them at the first use as below, but...
I really don't see much of a problem in allocating them statically and
pushing closer to where they're being used. If this really concerns,
at the least, the dynamic allocation needs to be pushed to
verifyBackupPageConsistency() IMO with if (first_time) { allocate two
blocks with palloc} and use them. This at least saves some memory on
the heap for most of the servers out there.
Yeah, we could do that. But as I mentioned before, that happens only
on startup thus it can be said that that's not worth bothering. On
the other hand I don't think it's great to waste 16kB * max_backends
memory especially when it is clearly recognized and easily avoidable.
I guess the reason for the code is more or less that.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Mon, Jan 16, 2023 at 10:52:43AM +0900, Kyotaro Horiguchi wrote:
Yeah, we could do that. But as I mentioned before, that happens only
on startup thus it can be said that that's not worth bothering. On
the other hand I don't think it's great to waste 16kB * max_backends
memory especially when it is clearly recognized and easily avoidable.
Memory's cheap, but basically nobody would use these except
developers..
I guess the reason for the code is more or less that.
The original discussion spreads across these threads:
/messages/by-id/CAB7nPqR0jzhF=U4AXLm+cmaE4J-HkUzbaRXtg+6ieERTqr=pcg@mail.gmail.com
/messages/by-id/CAGz5QC+_CNcDJkkmDyPg2zJ_R8AtEg1KyYADbU6B673RaTySAg@mail.gmail.com
There was a specific point about using static buffers from me, though
these would not have been aligned as of the lack of PGAlignedBlock
back in 2017 which is why palloc() was used. That should be around
here:
/messages/by-id/CAB7nPqR=OcojLCP=1Ho6Zo312CKzUZU8d4aJO+VvpUYV-waU_Q@mail.gmail.com
--
Michael