On Fri, Nov 11, 2022 at 5:52 PM Maxim Orlov <orlo...@gmail.com> wrote:
>
> Hi!
>
> I've watched over the patch and consider it useful. Applies without
> conflicts. The functionality of the patch itself is
> meets declared functionality.
Thanks for reviewing.
> But, in my view, some improvements may be proposed. We should be more, let's
> say, cautious (or a paranoid if you wish),
> in pg_walfile_offset_lsn while dealing with user input values. What I mean by
> that is:
> - to init input vars of sscanf, i.e. tli, log andseg;
> - check the return value of sscanf call;
> - check filename max length.
IsXLogFileName() will take care of this. Also, I've added a new inline
function XLogIdFromFileName() that parses file name and returns log
and seg along with XLogSegNo and timeline id. This new function avoids
an extra sscanf() call as well.
> Another question arises for me: is this function can be called during
> recovery? If not, we should ereport about this, should we?
It's just a utility function and doesn't depend on any of the server's
current state (unlike pg_walfile_name()), hence it doesn't matter if
this function is called during recovery.
> And one last note here: pg_walfile_offset_lsn is accepting NULL values and
> return NULL in this case. From a theoretical
> point of view, this is perfectly fine. Actually, I think this is exactly how
> it supposed to be, but I'm not sure if there are no other opinions here.
These functions are marked as 'STRICT', meaning a null is returned,
without even calling the function, if any of the input parameters is
null. I think we can keep the behaviour the same as its friends.
On Fri, Nov 11, 2022 at 11:05 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote:
>
Thanks for reviewing.
> Hmm, in 0002, why not return the timeline from the LSN too? It seems a
> waste not to have it.
Yeah, that actually makes sense. We might be tempted to return segno
too, but it's not something that we emit to the user elsewhere,
whereas we emit timeline id.
> + ereport(ERROR,
> + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> + errmsg("\"offset\" must not be negative or
> greater than or "
> + "equal to WAL segment
> size")));
>
> I don't think the word offset should be in quotes; and please don't cut
> the line. So I propose
>
> errmsg("offset must not be negative or greater than or equal to the WAL
> segment size")));
Changed.
While on this, I noticed that the pg_walfile_name_offset() isn't
covered in tests. I took an opportunity and added a simple test case
along with pg_walfile_offset_lsn().
I'm attaching the v3 patch set for further review.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 1b83f37b789ecf1f6ad1fa3dd71673c369584b6d Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 15 Nov 2022 06:04:57 +0000
Subject: [PATCH v3] Add LSN along with offset to error messages reported for
WAL file read/write/validate header failures
Author: Bharath Rupireddy
---
src/backend/access/transam/xlog.c | 8 ++++++--
src/backend/access/transam/xlogreader.c | 16 +++++++++++-----
src/backend/access/transam/xlogrecovery.c | 13 ++++++++-----
src/backend/access/transam/xlogutils.c | 10 ++++++----
src/include/access/xlogreader.h | 1 +
5 files changed, 32 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index a31fbbff78..42ee51f380 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2214,6 +2214,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
{
char xlogfname[MAXFNAMELEN];
int save_errno;
+ XLogRecPtr lsn;
if (errno == EINTR)
continue;
@@ -2222,11 +2223,14 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
XLogFileName(xlogfname, tli, openLogSegNo,
wal_segment_size);
errno = save_errno;
+ XLogSegNoOffsetToRecPtr(openLogSegNo, startoffset,
+ wal_segment_size, lsn);
ereport(PANIC,
(errcode_for_file_access(),
errmsg("could not write to log file %s "
- "at offset %u, length %zu: %m",
- xlogfname, startoffset, nleft)));
+ "at offset %u, LSN %X/%X, length %zu: %m",
+ xlogfname, startoffset,
+ LSN_FORMAT_ARGS(lsn), nleft)));
}
nleft -= written;
from += written;
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 93f667b254..a321c55d8f 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -1226,9 +1226,10 @@ XLogReaderValidatePageHeader(XLogReaderState *state, XLogRecPtr recptr,
XLogFileName(fname, state->seg.ws_tli, segno, state->segcxt.ws_segsize);
report_invalid_record(state,
- "invalid magic number %04X in WAL segment %s, offset %u",
+ "invalid magic number %04X in WAL segment %s, LSN %X/%X, offset %u",
hdr->xlp_magic,
fname,
+ LSN_FORMAT_ARGS(recptr),
offset);
return false;
}
@@ -1240,9 +1241,10 @@ XLogReaderValidatePageHeader(XLogReaderState *state, XLogRecPtr recptr,
XLogFileName(fname, state->seg.ws_tli, segno, state->segcxt.ws_segsize);
report_invalid_record(state,
- "invalid info bits %04X in WAL segment %s, offset %u",
+ "invalid info bits %04X in WAL segment %s, LSN %X/%X, offset %u",
hdr->xlp_info,
fname,
+ LSN_FORMAT_ARGS(recptr),
offset);
return false;
}
@@ -1281,9 +1283,10 @@ XLogReaderValidatePageHeader(XLogReaderState *state, XLogRecPtr recptr,
/* hmm, first page of file doesn't have a long header? */
report_invalid_record(state,
- "invalid info bits %04X in WAL segment %s, offset %u",
+ "invalid info bits %04X in WAL segment %s, LSN %X/%X, offset %u",
hdr->xlp_info,
fname,
+ LSN_FORMAT_ARGS(recptr),
offset);
return false;
}
@@ -1300,9 +1303,10 @@ XLogReaderValidatePageHeader(XLogReaderState *state, XLogRecPtr recptr,
XLogFileName(fname, state->seg.ws_tli, segno, state->segcxt.ws_segsize);
report_invalid_record(state,
- "unexpected pageaddr %X/%X in WAL segment %s, offset %u",
+ "unexpected pageaddr %X/%X in WAL segment %s, LSN %X/%X, offset %u",
LSN_FORMAT_ARGS(hdr->xlp_pageaddr),
fname,
+ LSN_FORMAT_ARGS(recptr),
offset);
return false;
}
@@ -1325,10 +1329,11 @@ XLogReaderValidatePageHeader(XLogReaderState *state, XLogRecPtr recptr,
XLogFileName(fname, state->seg.ws_tli, segno, state->segcxt.ws_segsize);
report_invalid_record(state,
- "out-of-sequence timeline ID %u (after %u) in WAL segment %s, offset %u",
+ "out-of-sequence timeline ID %u (after %u) in WAL segment %s, LSN %X/%X, offset %u",
hdr->xlp_tli,
state->latestPageTLI,
fname,
+ LSN_FORMAT_ARGS(recptr),
offset);
return false;
}
@@ -1553,6 +1558,7 @@ WALRead(XLogReaderState *state,
errinfo->wre_req = segbytes;
errinfo->wre_read = readbytes;
errinfo->wre_off = startoff;
+ errinfo->wre_lsn = recptr;
errinfo->wre_seg = state->seg;
return false;
}
diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index cb07694aea..c6ffb29c05 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -3079,9 +3079,10 @@ ReadRecord(XLogPrefetcher *xlogprefetcher, int emode,
XLogFileName(fname, xlogreader->seg.ws_tli, segno,
wal_segment_size);
ereport(emode_for_corrupt_record(emode, xlogreader->EndRecPtr),
- (errmsg("unexpected timeline ID %u in WAL segment %s, offset %u",
+ (errmsg("unexpected timeline ID %u in WAL segment %s, LSN %X/%X, offset %u",
xlogreader->latestPageTLI,
fname,
+ LSN_FORMAT_ARGS(xlogreader->latestPagePtr),
offset)));
record = NULL;
}
@@ -3284,14 +3285,16 @@ retry:
errno = save_errno;
ereport(emode_for_corrupt_record(emode, targetPagePtr + reqLen),
(errcode_for_file_access(),
- errmsg("could not read from WAL segment %s, offset %u: %m",
- fname, readOff)));
+ errmsg("could not read from WAL segment %s, LSN %X/%X, offset %u: %m",
+ fname, LSN_FORMAT_ARGS(targetPagePtr),
+ readOff)));
}
else
ereport(emode_for_corrupt_record(emode, targetPagePtr + reqLen),
(errcode(ERRCODE_DATA_CORRUPTED),
- errmsg("could not read from WAL segment %s, offset %u: read %d of %zu",
- fname, readOff, r, (Size) XLOG_BLCKSZ)));
+ errmsg("could not read from WAL segment %s, LSN %X/%X, offset %u: read %d of %zu",
+ fname, LSN_FORMAT_ARGS(targetPagePtr),
+ readOff, r, (Size) XLOG_BLCKSZ)));
goto next_record_is_invalid;
}
pgstat_report_wait_end();
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 563cba258d..9d171f2ad9 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -1051,15 +1051,17 @@ WALReadRaiseError(WALReadError *errinfo)
errno = errinfo->wre_errno;
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not read from WAL segment %s, offset %d: %m",
- fname, errinfo->wre_off)));
+ errmsg("could not read from WAL segment %s, LSN %X/%X, offset %d: %m",
+ fname, LSN_FORMAT_ARGS(errinfo->wre_lsn),
+ errinfo->wre_off)));
}
else if (errinfo->wre_read == 0)
{
ereport(ERROR,
(errcode(ERRCODE_DATA_CORRUPTED),
- errmsg("could not read from WAL segment %s, offset %d: read %d of %d",
- fname, errinfo->wre_off, errinfo->wre_read,
+ errmsg("could not read from WAL segment %s, LSN %X/%X, offset %d: read %d of %d",
+ fname, LSN_FORMAT_ARGS(errinfo->wre_lsn),
+ errinfo->wre_off, errinfo->wre_read,
errinfo->wre_req)));
}
}
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index e87f91316a..1e47169b5a 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -386,6 +386,7 @@ typedef struct WALReadError
int wre_off; /* Offset we tried to read from. */
int wre_req; /* Bytes requested to be read. */
int wre_read; /* Bytes read by the last read(). */
+ XLogRecPtr wre_lsn; /* WAL LSN being read. */
WALOpenSegment wre_seg; /* Segment we tried to read from. */
} WALReadError;
--
2.34.1
From 4923356f6143ecdb68c4a9ac41c05dcbba1253a3 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 15 Nov 2022 09:25:14 +0000
Subject: [PATCH v3] Introduce pg_walfile_offset_lsn()
Author: Nathan Bossart, Bharath Rupireddy
---
doc/src/sgml/func.sgml | 16 +++++
src/backend/access/transam/xlogfuncs.c | 62 ++++++++++++++++++++
src/include/access/xlog_internal.h | 8 +++
src/include/catalog/pg_proc.dat | 6 ++
src/test/regress/expected/misc_functions.out | 26 ++++++++
src/test/regress/sql/misc_functions.sql | 12 ++++
6 files changed, 130 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 6e0425cb3d..26970345fd 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25900,6 +25900,22 @@ LOG: Grand total: 1651920 bytes in 201 blocks; 622360 free (88 chunks); 1029560
</para></entry>
</row>
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>pg_walfile_offset_lsn</primary>
+ </indexterm>
+ <function>pg_walfile_offset_lsn</function> ( <parameter>file_name</parameter> <type>text</type>, <parameter>file_offset</parameter> <type>integer</type> )
+ <returnvalue>record</returnvalue>
+ ( <parameter>lsn</parameter> <type>pg_lsn</type>,
+ <parameter>timeline_id</parameter> <type>integer</type> )
+ </para>
+ <para>
+ Computes write-ahead log location and timline ID from a given WAL file
+ name and byte offset within that file.
+ </para></entry>
+ </row>
+
<row>
<entry role="func_table_entry"><para role="func_signature">
<indexterm>
diff --git a/src/backend/access/transam/xlogfuncs.c b/src/backend/access/transam/xlogfuncs.c
index 487d5d9cac..ed2690b49a 100644
--- a/src/backend/access/transam/xlogfuncs.c
+++ b/src/backend/access/transam/xlogfuncs.c
@@ -339,6 +339,68 @@ pg_last_wal_replay_lsn(PG_FUNCTION_ARGS)
PG_RETURN_LSN(recptr);
}
+/*
+ * Compute an LSN and timline ID given a WAL file name and decimal byte offset.
+ */
+Datum
+pg_walfile_offset_lsn(PG_FUNCTION_ARGS)
+{
+ char *fname = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ int offset = PG_GETARG_INT32(1);
+ TimeLineID tli;
+ XLogSegNo segno;
+ XLogRecPtr lsn;
+ uint32 log;
+ uint32 seg;
+ Datum values[2] = {0};
+ bool isnull[2] = {0};
+ TupleDesc resultTupleDesc;
+ HeapTuple resultHeapTuple;
+ Datum result;
+
+ if (!IsXLogFileName(fname))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid WAL file name \"%s\"", fname)));
+
+ XLogIdFromFileName(fname, &tli, &segno, &log, &seg, wal_segment_size);
+
+ if (seg >= XLogSegmentsPerXLogId(wal_segment_size) ||
+ (log == 0 && seg == 0) ||
+ segno == 0 ||
+ tli == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid WAL file name \"%s\"", fname)));
+
+ if (offset < 0 || offset >= wal_segment_size)
+ ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("offset must not be negative or greater than or equal to WAL segment size")));
+
+ /*
+ * Construct a tuple descriptor for the result row. This must match this
+ * function's pg_proc entry!
+ */
+ resultTupleDesc = CreateTemplateTupleDesc(2);
+ TupleDescInitEntry(resultTupleDesc, (AttrNumber) 1, "lsn",
+ PG_LSNOID, -1, 0);
+ TupleDescInitEntry(resultTupleDesc, (AttrNumber) 2, "timeline_id",
+ INT4OID, -1, 0);
+
+ resultTupleDesc = BlessTupleDesc(resultTupleDesc);
+
+ XLogSegNoOffsetToRecPtr(segno, offset, wal_segment_size, lsn);
+
+ values[0] = LSNGetDatum(lsn);
+ values[1] = UInt32GetDatum(tli);
+
+ resultHeapTuple = heap_form_tuple(resultTupleDesc, values, isnull);
+ result = HeapTupleGetDatum(resultHeapTuple);
+
+ PG_RETURN_DATUM(result);
+}
+
/*
* Compute an xlog file name and decimal byte offset given a WAL location,
* such as is returned by pg_backup_stop() or pg_switch_wal().
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index 44291b337b..0ffe4142be 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -206,6 +206,14 @@ XLogFromFileName(const char *fname, TimeLineID *tli, XLogSegNo *logSegNo, int wa
*logSegNo = (uint64) log * XLogSegmentsPerXLogId(wal_segsz_bytes) + seg;
}
+static inline void
+XLogIdFromFileName(const char *fname, TimeLineID *tli, XLogSegNo *logSegNo,
+ uint32 *log, uint32 *seg, int wal_segsz_bytes)
+{
+ sscanf(fname, "%08X%08X%08X", tli, log, seg);
+ *logSegNo = (uint64) (*log) * XLogSegmentsPerXLogId(wal_segsz_bytes) + *seg;
+}
+
static inline void
XLogFilePath(char *path, TimeLineID tli, XLogSegNo logSegNo, int wal_segsz_bytes)
{
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 9dbe9ec801..f1a732c172 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6336,6 +6336,12 @@
proargtypes => 'pg_lsn', proallargtypes => '{pg_lsn,text,int4}',
proargmodes => '{i,o,o}', proargnames => '{lsn,file_name,file_offset}',
prosrc => 'pg_walfile_name_offset' },
+{ oid => '8205',
+ descr => 'wal location and timeline ID given a wal filename and byte offset',
+ proname => 'pg_walfile_offset_lsn', prorettype => 'record',
+ proargtypes => 'text int4', proallargtypes => '{text,int4,pg_lsn,int4}',
+ proargmodes => '{i,i,o,o}', proargnames => '{file_name,file_offset,lsn,timeline_id}',
+ prosrc => 'pg_walfile_offset_lsn' },
{ oid => '2851', descr => 'wal filename, given a wal location',
proname => 'pg_walfile_name', prorettype => 'text', proargtypes => 'pg_lsn',
prosrc => 'pg_walfile_name' },
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index 88bb696ded..1b2ab4f25b 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -619,3 +619,29 @@ SELECT count(*) > 0 AS ok FROM pg_control_system();
t
(1 row)
+-- pg_walfile_name_offset
+SELECT * FROM pg_walfile_name_offset('1/F');
+ file_name | file_offset
+--------------------------+-------------
+ 000000010000000100000000 | 15
+(1 row)
+
+-- pg_walfile_offset_lsn
+SELECT * FROM pg_walfile_offset_lsn('invalid', 15);
+ERROR: invalid WAL file name "invalid"
+SELECT * FROM pg_walfile_offset_lsn('0000000100000000FFFFFFFF', 15);
+ERROR: invalid WAL file name "0000000100000000FFFFFFFF"
+SELECT * FROM pg_walfile_offset_lsn('000000010000000000000000', 15);
+ERROR: invalid WAL file name "000000010000000000000000"
+SELECT * FROM pg_walfile_offset_lsn('000000000000000100000000', 15);
+ERROR: invalid WAL file name "000000000000000100000000"
+SELECT * FROM pg_walfile_offset_lsn('000000010000000100000000', -1);
+ERROR: offset must not be negative or greater than or equal to WAL segment size
+SELECT * FROM pg_walfile_offset_lsn('000000010000000100000000', 2000000000);
+ERROR: offset must not be negative or greater than or equal to WAL segment size
+SELECT * FROM pg_walfile_offset_lsn('000000010000000100000000', 15);
+ lsn | timeline_id
+-----+-------------
+ 1/F | 1
+(1 row)
+
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index b07e9e8dbb..9cb59cecf7 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -229,3 +229,15 @@ SELECT count(*) > 0 AS ok FROM pg_control_checkpoint();
SELECT count(*) > 0 AS ok FROM pg_control_init();
SELECT count(*) > 0 AS ok FROM pg_control_recovery();
SELECT count(*) > 0 AS ok FROM pg_control_system();
+
+-- pg_walfile_name_offset
+SELECT * FROM pg_walfile_name_offset('1/F');
+
+-- pg_walfile_offset_lsn
+SELECT * FROM pg_walfile_offset_lsn('invalid', 15);
+SELECT * FROM pg_walfile_offset_lsn('0000000100000000FFFFFFFF', 15);
+SELECT * FROM pg_walfile_offset_lsn('000000010000000000000000', 15);
+SELECT * FROM pg_walfile_offset_lsn('000000000000000100000000', 15);
+SELECT * FROM pg_walfile_offset_lsn('000000010000000100000000', -1);
+SELECT * FROM pg_walfile_offset_lsn('000000010000000100000000', 2000000000);
+SELECT * FROM pg_walfile_offset_lsn('000000010000000100000000', 15);
--
2.34.1
