On 26.10.2018 10:33, Kyotaro HORIGUCHI wrote:
Hello.
At Tue, 23 Oct 2018 10:25:27 +0500, Andrey Lepikhov <a.lepik...@postgrespro.ru> wrote
in <2553f2b0-0e39-eb0e-d382-6c0ed08ca...@postgrespro.ru>
On 23.10.2018 0:53, Heikki Linnakangas wrote:
I'd expect the decompression to read from the on-disk buffer, and
unpack to readRecordBuf, I still don't see a need to copy the packed
record to readRecordBuf. If there is a need for that, though, the
patch that implements the packing or compression can add the memcpy()
where it needs it.
I agree with it. Eventually, placement of the WAL-record can be
defined by comparison the record, readBuf and readRecordBuf pointers.
In attachment new version of the patch.
This looks quite clear and what it does is reasonable to me.
Applies cleanly on top of current master and no regression seen.
Just one comment. This patch leaves the following code.
> /* Record does not cross a page boundary */
> if (!ValidXLogRecord(state, record, RecPtr))
> goto err;
>
> state->EndRecPtr = RecPtr + MAXALIGN(total_len);
!>
> state->ReadRecPtr = RecPtr;
> }
The empty line (marked by '!') looks a bit too much standing out
after this patch. Could you remove the line? Then could you
transpose the two assignments if you don't mind?
Thanks, see attachment.
--
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company
>From ec1df6c2b41fdfe2c79e3f0944653057e394c535 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepik...@postgrespro.ru>
Date: Tue, 23 Oct 2018 10:17:55 +0500
Subject: [PATCH] WAL record buffer pointer fix
---
src/backend/access/transam/xlogreader.c | 30 ++++++++++++-------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 0768ca7822..82a16e78f3 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -353,19 +353,6 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
gotheader = false;
}
- /*
- * Enlarge readRecordBuf as needed.
- */
- if (total_len > state->readRecordBufSize &&
- !allocate_recordbuf(state, total_len))
- {
- /* We treat this as a "bogus data" condition */
- report_invalid_record(state, "record length %u at %X/%X too long",
- total_len,
- (uint32) (RecPtr >> 32), (uint32) RecPtr);
- goto err;
- }
-
len = XLOG_BLCKSZ - RecPtr % XLOG_BLCKSZ;
if (total_len > len)
{
@@ -375,6 +362,19 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
char *buffer;
uint32 gotlen;
+ /*
+ * Enlarge readRecordBuf as needed.
+ */
+ if (total_len > state->readRecordBufSize &&
+ !allocate_recordbuf(state, total_len))
+ {
+ /* We treat this as a "bogus data" condition */
+ report_invalid_record(state, "record length %u at %X/%X too long",
+ total_len,
+ (uint32) (RecPtr >> 32), (uint32) RecPtr);
+ goto err;
+ }
+
/* Copy the first fragment of the record from the first page. */
memcpy(state->readRecordBuf,
state->readBuf + RecPtr % XLOG_BLCKSZ, len);
@@ -476,10 +476,8 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
if (!ValidXLogRecord(state, record, RecPtr))
goto err;
- state->EndRecPtr = RecPtr + MAXALIGN(total_len);
-
state->ReadRecPtr = RecPtr;
- memcpy(state->readRecordBuf, record, total_len);
+ state->EndRecPtr = RecPtr + MAXALIGN(total_len);
}
/*
--
2.17.1
