public inbox for [email protected]
help / color / mirror / Atom feedFrom: Heikki Linnakangas <[email protected]>
To: Álvaro Herrera <[email protected]>
Cc: Maxim Orlov <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: Rework SLRU I/O errors handle
Date: Fri, 6 Mar 2026 18:49:22 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
On 06/03/2026 18:38, Heikki Linnakangas wrote:
> On 06/03/2026 18:22, Heikki Linnakangas wrote:
>> On 06/03/2026 17:33, Álvaro Herrera wrote:
>>> The comment added in 0005 is bogus too. It mentions
>>> InvalidTransactionId,
>>> but the problem is not that the value is 0 but rather that we get no
>>> pointer. Also, in all other callbacks the pointer is asserted to not be
>>> NULL, so why don't we do the same here, and avoid an error message
>>> that's not going to help anyone? I see however that in the patch we're
>>> passing a NULL to SlruReportIOError(), which means if you get an IO
>>> error with any SLRU other than xact, you're going to get either a crash
>>> on the assertion, or (on non-debug builds) a crash on dereferencing the
>>> NULL pointer.
>>
>> Hmm, I thought we could just never pass a NULL pointer, but if a Write
>> fails, slru.c has no context where to pull that opaque_data. Perhaps
>> we should just not call the callback in that case.
>>
>> I'm starting to feel that what SlruReportIOError() currently uses as
>> the errdetail, could well be the main error message, and the callback
>> could provide the errdetail. I.e. swap the errmsg and errdetail we
>> have now. That way, we can just leave out the errdetail for Write
>> failures. The current errmsg we have for Write failures is pretty bad:
>> "ERROR: could not access status of transaction 0". What's currently
>> the errdetail, e.g. "Could not read from file \"%s\" at offset %d:
>> %m", is a lot more informative.
>
> Attached version that does that.
That version was a little rushed, with wrong message styles etc.
leftovers. Sorry about that, here's yet another, more cleaned-up version.
- Heikki
Attachments:
[text/x-patch] v4-0001-Add-a-callback-for-I-O-error-messages-in-SLRUs.patch (31.8K, 2-v4-0001-Add-a-callback-for-I-O-error-messages-in-SLRUs.patch)
download | inline diff:
From 8ce55ee5f5957c810dad9d0e076dbff3354ffd45 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
Date: Fri, 6 Mar 2026 18:45:23 +0200
Subject: [PATCH v4 1/1] Add a callback for I/O error messages in SLRUs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Historically, all SLRUs were addressed by transaction IDs, but that
hasn't been true for long time. However, the error message on I/O
error still always talked about accessing a transaction ID.
This commit adds a callback that allow subsystems to construct their
own error messages, which can then correctly refer to a transaction
ID, a multixid or whatever else is used to address the SLRU.
Author: Maxim Orlov <[email protected]>
Reviewed-by: Álvaro Herrera <[email protected]>
Discussion: https://www.postgresql.org/message-id/CACG=ezZZfurhYV+66ceubxQAyWqv9vaUi0yoO4-t48OE5xc0DQ@mail.gmail.com
---
src/backend/access/transam/clog.c | 17 ++++-
src/backend/access/transam/commit_ts.c | 13 +++-
src/backend/access/transam/multixact.c | 42 +++++++---
src/backend/access/transam/slru.c | 76 ++++++++++---------
src/backend/access/transam/subtrans.c | 14 +++-
src/backend/commands/async.c | 20 +++--
src/backend/storage/lmgr/predicate.c | 14 +++-
src/include/access/slru.h | 15 +++-
.../modules/test_slru/expected/test_slru.out | 4 +
src/test/modules/test_slru/sql/test_slru.sql | 3 +
src/test/modules/test_slru/test_slru--1.0.sql | 2 +-
src/test/modules/test_slru/test_slru.c | 15 +++-
12 files changed, 168 insertions(+), 67 deletions(-)
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index b5c38bbb162..9ed7f2f7477 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -111,6 +111,7 @@ static SlruCtlData XactCtlData;
static bool CLOGPagePrecedes(int64 page1, int64 page2);
+static int clog_errdetail_for_io_error(const void *opaque_data);
static void WriteTruncateXlogRec(int64 pageno, TransactionId oldestXact,
Oid oldestXactDb);
static void TransactionIdSetPageStatus(TransactionId xid, int nsubxids,
@@ -381,8 +382,7 @@ TransactionIdSetPageStatusInternal(TransactionId xid, int nsubxids,
* write-busy, since we don't care if the update reaches disk sooner than
* we think.
*/
- slotno = SimpleLruReadPage(XactCtl, pageno, !XLogRecPtrIsValid(lsn),
- xid);
+ slotno = SimpleLruReadPage(XactCtl, pageno, !XLogRecPtrIsValid(lsn), &xid);
/*
* Set the main transaction id, if any.
@@ -743,7 +743,7 @@ TransactionIdGetStatus(TransactionId xid, XLogRecPtr *lsn)
/* lock is acquired by SimpleLruReadPage_ReadOnly */
- slotno = SimpleLruReadPage_ReadOnly(XactCtl, pageno, xid);
+ slotno = SimpleLruReadPage_ReadOnly(XactCtl, pageno, &xid);
byteptr = XactCtl->shared->page_buffer[slotno] + byteno;
status = (*byteptr >> bshift) & CLOG_XACT_BITMASK;
@@ -807,6 +807,7 @@ CLOGShmemInit(void)
Assert(transaction_buffers != 0);
XactCtl->PagePrecedes = CLOGPagePrecedes;
+ XactCtl->errdetail_for_io_error = clog_errdetail_for_io_error;
SimpleLruInit(XactCtl, "transaction", CLOGShmemBuffers(), CLOG_LSNS_PER_PAGE,
"pg_xact", LWTRANCHE_XACT_BUFFER,
LWTRANCHE_XACT_SLRU, SYNC_HANDLER_CLOG, false);
@@ -882,7 +883,7 @@ TrimCLOG(void)
int slotno;
char *byteptr;
- slotno = SimpleLruReadPage(XactCtl, pageno, false, xid);
+ slotno = SimpleLruReadPage(XactCtl, pageno, false, &xid);
byteptr = XactCtl->shared->page_buffer[slotno] + byteno;
/* Zero so-far-unused positions in the current byte */
@@ -1033,6 +1034,14 @@ CLOGPagePrecedes(int64 page1, int64 page2)
TransactionIdPrecedes(xid1, xid2 + CLOG_XACTS_PER_PAGE - 1));
}
+static int
+clog_errdetail_for_io_error(const void *opaque_data)
+{
+ TransactionId xid = *(const TransactionId *) opaque_data;
+
+ return errdetail("Could not access commit status of transaction %u.", xid);
+}
+
/*
* Write a TRUNCATE xlog record
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 6fa2178f1dd..36219dd13cc 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -115,6 +115,7 @@ static void TransactionIdSetCommitTs(TransactionId xid, TimestampTz ts,
ReplOriginId nodeid, int slotno);
static void error_commit_ts_disabled(void);
static bool CommitTsPagePrecedes(int64 page1, int64 page2);
+static int commit_ts_errdetail_for_io_error(const void *opaque_data);
static void ActivateCommitTs(void);
static void DeactivateCommitTs(void);
static void WriteTruncateXlogRec(int64 pageno, TransactionId oldestXid);
@@ -227,7 +228,7 @@ SetXidCommitTsInPage(TransactionId xid, int nsubxids,
LWLockAcquire(lock, LW_EXCLUSIVE);
- slotno = SimpleLruReadPage(CommitTsCtl, pageno, true, xid);
+ slotno = SimpleLruReadPage(CommitTsCtl, pageno, true, &xid);
TransactionIdSetCommitTs(xid, ts, nodeid, slotno);
for (i = 0; i < nsubxids; i++)
@@ -332,7 +333,7 @@ TransactionIdGetCommitTsData(TransactionId xid, TimestampTz *ts,
}
/* lock is acquired by SimpleLruReadPage_ReadOnly */
- slotno = SimpleLruReadPage_ReadOnly(CommitTsCtl, pageno, xid);
+ slotno = SimpleLruReadPage_ReadOnly(CommitTsCtl, pageno, &xid);
memcpy(&entry,
CommitTsCtl->shared->page_buffer[slotno] +
SizeOfCommitTimestampEntry * entryno,
@@ -551,6 +552,7 @@ CommitTsShmemInit(void)
Assert(commit_timestamp_buffers != 0);
CommitTsCtl->PagePrecedes = CommitTsPagePrecedes;
+ CommitTsCtl->errdetail_for_io_error = commit_ts_errdetail_for_io_error;
SimpleLruInit(CommitTsCtl, "commit_timestamp", CommitTsShmemBuffers(), 0,
"pg_commit_ts", LWTRANCHE_COMMITTS_BUFFER,
LWTRANCHE_COMMITTS_SLRU,
@@ -959,6 +961,13 @@ CommitTsPagePrecedes(int64 page1, int64 page2)
TransactionIdPrecedes(xid1, xid2 + COMMIT_TS_XACTS_PER_PAGE - 1));
}
+static int
+commit_ts_errdetail_for_io_error(const void *opaque_data)
+{
+ TransactionId xid = *(const TransactionId *) opaque_data;
+
+ return errdetail("Could not access commit timestamp of transaction %u.", xid);
+}
/*
* Write a TRUNCATE xlog record
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 8a5c9818ed6..2ae252fd05a 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -315,6 +315,8 @@ static void mXactCachePut(MultiXactId multi, int nmembers,
/* management of SLRU infrastructure */
static bool MultiXactOffsetPagePrecedes(int64 page1, int64 page2);
static bool MultiXactMemberPagePrecedes(int64 page1, int64 page2);
+static int MultiXactOffsetIoErrorDetail(const void *opaque_data);
+static int MultiXactMemberIoErrorDetail(const void *opaque_data);
static void ExtendMultiXactOffset(MultiXactId multi);
static void ExtendMultiXactMember(MultiXactOffset offset, int nmembers);
static void SetOldestOffset(void);
@@ -836,7 +838,7 @@ RecordNewMultiXact(MultiXactId multi, MultiXactOffset offset,
* enough that a MultiXactId is really involved. Perhaps someday we'll
* take the trouble to generalize the slru.c error reporting code.
*/
- slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
+ slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, &multi);
offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
offptr += entryno;
@@ -865,7 +867,7 @@ RecordNewMultiXact(MultiXactId multi, MultiXactOffset offset,
lock = SimpleLruGetBankLock(MultiXactOffsetCtl, next_pageno);
LWLockAcquire(lock, LW_EXCLUSIVE);
- slotno = SimpleLruReadPage(MultiXactOffsetCtl, next_pageno, true, next);
+ slotno = SimpleLruReadPage(MultiXactOffsetCtl, next_pageno, true, &next);
next_offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
next_offptr += next_entryno;
}
@@ -919,7 +921,8 @@ RecordNewMultiXact(MultiXactId multi, MultiXactOffset offset,
LWLockAcquire(lock, LW_EXCLUSIVE);
prevlock = lock;
}
- slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
+ slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true,
+ &offset);
prev_pageno = pageno;
}
@@ -1244,7 +1247,7 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
LWLockAcquire(lock, LW_EXCLUSIVE);
/* read this multi's offset */
- slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi);
+ slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, &multi);
offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
offptr += entryno;
offset = *offptr;
@@ -1282,7 +1285,7 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
LWLockAcquire(newlock, LW_EXCLUSIVE);
lock = newlock;
}
- slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact);
+ slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, &tmpMXact);
}
offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
@@ -1347,7 +1350,8 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
lock = newlock;
}
- slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi);
+ slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true,
+ &offset);
prev_pageno = pageno;
}
@@ -1779,6 +1783,9 @@ MultiXactShmemInit(void)
MultiXactOffsetCtl->PagePrecedes = MultiXactOffsetPagePrecedes;
MultiXactMemberCtl->PagePrecedes = MultiXactMemberPagePrecedes;
+ MultiXactOffsetCtl->errdetail_for_io_error = MultiXactOffsetIoErrorDetail;
+ MultiXactMemberCtl->errdetail_for_io_error = MultiXactMemberIoErrorDetail;
+
SimpleLruInit(MultiXactOffsetCtl,
"multixact_offset", multixact_offset_buffers, 0,
"pg_multixact/offsets", LWTRANCHE_MULTIXACTOFFSET_BUFFER,
@@ -1928,7 +1935,7 @@ TrimMultiXact(void)
if (entryno == 0 || nextMXact == FirstMultiXactId)
slotno = SimpleLruZeroPage(MultiXactOffsetCtl, pageno);
else
- slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, nextMXact);
+ slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, &nextMXact);
offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
offptr += entryno;
@@ -1963,7 +1970,7 @@ TrimMultiXact(void)
LWLockAcquire(lock, LW_EXCLUSIVE);
memberoff = MXOffsetToMemberOffset(offset);
- slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, offset);
+ slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, &offset);
xidptr = (TransactionId *)
(MultiXactMemberCtl->shared->page_buffer[slotno] + memberoff);
@@ -2497,7 +2504,7 @@ find_multixact_start(MultiXactId multi, MultiXactOffset *result)
return false;
/* lock is acquired by SimpleLruReadPage_ReadOnly */
- slotno = SimpleLruReadPage_ReadOnly(MultiXactOffsetCtl, pageno, multi);
+ slotno = SimpleLruReadPage_ReadOnly(MultiXactOffsetCtl, pageno, &multi);
offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
offptr += entryno;
offset = *offptr;
@@ -2808,6 +2815,23 @@ MultiXactMemberPagePrecedes(int64 page1, int64 page2)
return page1 < page2;
}
+static int
+MultiXactOffsetIoErrorDetail(const void *opaque_data)
+{
+ MultiXactId multixid = *(const MultiXactId *) opaque_data;
+
+ return errdetail("Could not access offset of multixact %u.", multixid);
+}
+
+static int
+MultiXactMemberIoErrorDetail(const void *opaque_data)
+{
+ MultiXactOffset mxoff = *(const MultiXactOffset *) opaque_data;
+
+ return errdetail("Could not access multixact member at offset %" PRIu64 ".",
+ mxoff);
+}
+
/*
* Decide which of two MultiXactIds is earlier.
*
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 549c7e3e64b..5d127cb915e 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -181,7 +181,8 @@ static void SlruInternalWritePage(SlruCtl ctl, int slotno, SlruWriteAll fdata);
static bool SlruPhysicalReadPage(SlruCtl ctl, int64 pageno, int slotno);
static bool SlruPhysicalWritePage(SlruCtl ctl, int64 pageno, int slotno,
SlruWriteAll fdata);
-static void SlruReportIOError(SlruCtl ctl, int64 pageno, TransactionId xid);
+static void SlruReportIOError(SlruCtl ctl, int64 pageno,
+ const void *opaque_data);
static int SlruSelectLRUPage(SlruCtl ctl, int64 pageno);
static bool SlruScanDirCbDeleteCutoff(SlruCtl ctl, char *filename,
@@ -515,8 +516,9 @@ SimpleLruWaitIO(SlruCtl ctl, int slotno)
* that modification of the page is safe. If write_ok is false then we
* will not return the page until it is not undergoing active I/O.
*
- * The passed-in xid is used only for error reporting, and may be
- * InvalidTransactionId if no specific xid is associated with the action.
+ * On error, the passed-in 'opaque_data' is passed to the
+ * 'errdetail_for_io_error' callback, to provide details on the operation that
+ * failed. It is only used for error reporting.
*
* Return value is the shared-buffer slot number now holding the page.
* The buffer's LRU access info is updated.
@@ -525,7 +527,7 @@ SimpleLruWaitIO(SlruCtl ctl, int slotno)
*/
int
SimpleLruReadPage(SlruCtl ctl, int64 pageno, bool write_ok,
- TransactionId xid)
+ const void *opaque_data)
{
SlruShared shared = ctl->shared;
LWLock *banklock = SimpleLruGetBankLock(ctl, pageno);
@@ -601,7 +603,7 @@ SimpleLruReadPage(SlruCtl ctl, int64 pageno, bool write_ok,
/* Now it's okay to ereport if we failed */
if (!ok)
- SlruReportIOError(ctl, pageno, xid);
+ SlruReportIOError(ctl, pageno, opaque_data);
SlruRecentlyUsed(shared, slotno);
@@ -617,8 +619,9 @@ SimpleLruReadPage(SlruCtl ctl, int64 pageno, bool write_ok,
* The page number must correspond to an already-initialized page.
* The caller must intend only read-only access to the page.
*
- * The passed-in xid is used only for error reporting, and may be
- * InvalidTransactionId if no specific xid is associated with the action.
+ * On error, the passed-in 'opaque_data' is passed to the
+ * 'errdetail_for_io_error' callback, to provide details on the operation that
+ * failed. It is only used for error reporting.
*
* Return value is the shared-buffer slot number now holding the page.
* The buffer's LRU access info is updated.
@@ -627,7 +630,7 @@ SimpleLruReadPage(SlruCtl ctl, int64 pageno, bool write_ok,
* It is unspecified whether the lock will be shared or exclusive.
*/
int
-SimpleLruReadPage_ReadOnly(SlruCtl ctl, int64 pageno, TransactionId xid)
+SimpleLruReadPage_ReadOnly(SlruCtl ctl, int64 pageno, const void *opaque_data)
{
SlruShared shared = ctl->shared;
LWLock *banklock = SimpleLruGetBankLock(ctl, pageno);
@@ -659,7 +662,7 @@ SimpleLruReadPage_ReadOnly(SlruCtl ctl, int64 pageno, TransactionId xid)
LWLockRelease(banklock);
LWLockAcquire(banklock, LW_EXCLUSIVE);
- return SimpleLruReadPage(ctl, pageno, true, xid);
+ return SimpleLruReadPage(ctl, pageno, true, opaque_data);
}
/*
@@ -739,7 +742,7 @@ SlruInternalWritePage(SlruCtl ctl, int slotno, SlruWriteAll fdata)
/* Now it's okay to ereport if we failed */
if (!ok)
- SlruReportIOError(ctl, pageno, InvalidTransactionId);
+ SlruReportIOError(ctl, pageno, NULL);
/* If part of a checkpoint, count this as a SLRU buffer written. */
if (fdata)
@@ -793,14 +796,14 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int64 pageno)
/* report error normally */
slru_errcause = SLRU_OPEN_FAILED;
slru_errno = errno;
- SlruReportIOError(ctl, pageno, 0);
+ SlruReportIOError(ctl, pageno, NULL);
}
if ((endpos = lseek(fd, 0, SEEK_END)) < 0)
{
slru_errcause = SLRU_SEEK_FAILED;
slru_errno = errno;
- SlruReportIOError(ctl, pageno, 0);
+ SlruReportIOError(ctl, pageno, NULL);
}
result = endpos >= (off_t) (offset + BLCKSZ);
@@ -1070,7 +1073,7 @@ SlruPhysicalWritePage(SlruCtl ctl, int64 pageno, int slotno, SlruWriteAll fdata)
* SlruPhysicalWritePage. Call this after cleaning up shared-memory state.
*/
static void
-SlruReportIOError(SlruCtl ctl, int64 pageno, TransactionId xid)
+SlruReportIOError(SlruCtl ctl, int64 pageno, const void *opaque_data)
{
int64 segno = pageno / SLRU_PAGES_PER_SEGMENT;
int rpageno = pageno % SLRU_PAGES_PER_SEGMENT;
@@ -1084,54 +1087,55 @@ SlruReportIOError(SlruCtl ctl, int64 pageno, TransactionId xid)
case SLRU_OPEN_FAILED:
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not access status of transaction %u", xid),
- errdetail("Could not open file \"%s\": %m.", path)));
+ errmsg("could not open file \"%s\": %m", path),
+ opaque_data ? ctl->errdetail_for_io_error(opaque_data) : 0));
break;
case SLRU_SEEK_FAILED:
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not access status of transaction %u", xid),
- errdetail("Could not seek in file \"%s\" to offset %d: %m.",
- path, offset)));
+ errmsg("could not seek in file \"%s\" to offset %d: %m",
+ path, offset),
+ opaque_data ? ctl->errdetail_for_io_error(opaque_data) : 0));
break;
case SLRU_READ_FAILED:
if (errno)
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not access status of transaction %u", xid),
- errdetail("Could not read from file \"%s\" at offset %d: %m.",
- path, offset)));
+ errmsg("could not read from file \"%s\" at offset %d: %m",
+ path, offset),
+ opaque_data ? ctl->errdetail_for_io_error(opaque_data) : 0));
else
ereport(ERROR,
- (errmsg("could not access status of transaction %u", xid),
- errdetail("Could not read from file \"%s\" at offset %d: read too few bytes.", path, offset)));
+ (errmsg("could not read from file \"%s\" at offset %d: read too few bytes",
+ path, offset),
+ opaque_data ? ctl->errdetail_for_io_error(opaque_data) : 0));
break;
case SLRU_WRITE_FAILED:
if (errno)
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not access status of transaction %u", xid),
- errdetail("Could not write to file \"%s\" at offset %d: %m.",
- path, offset)));
+ errmsg("Could not write to file \"%s\" at offset %d: %m",
+ path, offset),
+ opaque_data ? ctl->errdetail_for_io_error(opaque_data) : 0));
else
ereport(ERROR,
- (errmsg("could not access status of transaction %u", xid),
- errdetail("Could not write to file \"%s\" at offset %d: wrote too few bytes.",
- path, offset)));
+ (errmsg("Could not write to file \"%s\" at offset %d: wrote too few bytes.",
+ path, offset),
+ opaque_data ? ctl->errdetail_for_io_error(opaque_data) : 0));
break;
case SLRU_FSYNC_FAILED:
ereport(data_sync_elevel(ERROR),
(errcode_for_file_access(),
- errmsg("could not access status of transaction %u", xid),
- errdetail("Could not fsync file \"%s\": %m.",
- path)));
+ errmsg("could not fsync file \"%s\": %m",
+ path),
+ opaque_data ? ctl->errdetail_for_io_error(opaque_data) : 0));
break;
case SLRU_CLOSE_FAILED:
ereport(ERROR,
(errcode_for_file_access(),
- errmsg("could not access status of transaction %u", xid),
- errdetail("Could not close file \"%s\": %m.",
- path)));
+ errmsg("could not close file \"%s\": %m",
+ path),
+ opaque_data ? ctl->errdetail_for_io_error(opaque_data) : 0));
break;
default:
/* can't get here, we trust */
@@ -1411,7 +1415,7 @@ SimpleLruWriteAll(SlruCtl ctl, bool allow_redirtied)
}
}
if (!ok)
- SlruReportIOError(ctl, pageno, InvalidTransactionId);
+ SlruReportIOError(ctl, pageno, NULL);
/* Ensure that directory entries for new files are on disk. */
if (ctl->sync_handler != SYNC_HANDLER_NONE)
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index c0987f43f11..c6ce71fc703 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -75,6 +75,7 @@ static SlruCtlData SubTransCtlData;
static bool SubTransPagePrecedes(int64 page1, int64 page2);
+static int subtrans_errdetail_for_io_error(const void *opaque_data);
/*
@@ -95,7 +96,7 @@ SubTransSetParent(TransactionId xid, TransactionId parent)
lock = SimpleLruGetBankLock(SubTransCtl, pageno);
LWLockAcquire(lock, LW_EXCLUSIVE);
- slotno = SimpleLruReadPage(SubTransCtl, pageno, true, xid);
+ slotno = SimpleLruReadPage(SubTransCtl, pageno, true, &xid);
ptr = (TransactionId *) SubTransCtl->shared->page_buffer[slotno];
ptr += entryno;
@@ -135,7 +136,7 @@ SubTransGetParent(TransactionId xid)
/* lock is acquired by SimpleLruReadPage_ReadOnly */
- slotno = SimpleLruReadPage_ReadOnly(SubTransCtl, pageno, xid);
+ slotno = SimpleLruReadPage_ReadOnly(SubTransCtl, pageno, &xid);
ptr = (TransactionId *) SubTransCtl->shared->page_buffer[slotno];
ptr += entryno;
@@ -240,6 +241,7 @@ SUBTRANSShmemInit(void)
Assert(subtransaction_buffers != 0);
SubTransCtl->PagePrecedes = SubTransPagePrecedes;
+ SubTransCtl->errdetail_for_io_error = subtrans_errdetail_for_io_error;
SimpleLruInit(SubTransCtl, "subtransaction", SUBTRANSShmemBuffers(), 0,
"pg_subtrans", LWTRANCHE_SUBTRANS_BUFFER,
LWTRANCHE_SUBTRANS_SLRU, SYNC_HANDLER_NONE, false);
@@ -419,3 +421,11 @@ SubTransPagePrecedes(int64 page1, int64 page2)
return (TransactionIdPrecedes(xid1, xid2) &&
TransactionIdPrecedes(xid1, xid2 + SUBTRANS_XACTS_PER_PAGE - 1));
}
+
+static int
+subtrans_errdetail_for_io_error(const void *opaque_data)
+{
+ TransactionId xid = *(const TransactionId *) opaque_data;
+
+ return errdetail("Could not access subtransaction status of transaction %u.", xid);
+}
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 0b6d119dad0..5c9a56c3d40 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -570,6 +570,7 @@ bool Trace_notify = false;
int max_notify_queue_pages = 1048576;
/* local function prototypes */
+static int asyncQueueErrdetailForIoError(const void *opaque_data);
static inline int64 asyncQueuePageDiff(int64 p, int64 q);
static inline bool asyncQueuePagePrecedes(int64 p, int64 q);
static inline void GlobalChannelKeyInit(GlobalChannelKey *key, Oid dboid,
@@ -610,6 +611,15 @@ static uint32 notification_hash(const void *key, Size keysize);
static int notification_match(const void *key1, const void *key2, Size keysize);
static void ClearPendingActionsAndNotifies(void);
+static int
+asyncQueueErrdetailForIoError(const void *opaque_data)
+{
+ const QueuePosition *position = opaque_data;
+
+ return errdetail("Could not access async queue at page %" PRId64 ", offset %d.",
+ position->page, position->offset);
+}
+
/*
* Compute the difference between two queue page numbers.
* Previously this function accounted for a wraparound.
@@ -830,6 +840,7 @@ AsyncShmemInit(void)
* names are used in order to avoid wraparound.
*/
NotifyCtl->PagePrecedes = asyncQueuePagePrecedes;
+ NotifyCtl->errdetail_for_io_error = asyncQueueErrdetailForIoError;
SimpleLruInit(NotifyCtl, "notify", notify_buffers, 0,
"pg_notify", LWTRANCHE_NOTIFY_BUFFER, LWTRANCHE_NOTIFY_SLRU,
SYNC_HANDLER_NONE, true);
@@ -2068,8 +2079,7 @@ asyncQueueAddEntries(ListCell *nextNotify)
if (QUEUE_POS_IS_ZERO(queue_head))
slotno = SimpleLruZeroPage(NotifyCtl, pageno);
else
- slotno = SimpleLruReadPage(NotifyCtl, pageno, true,
- InvalidTransactionId);
+ slotno = SimpleLruReadPage(NotifyCtl, pageno, true, &queue_head);
/* Note we mark the page dirty before writing in it */
NotifyCtl->shared->page_dirty[slotno] = true;
@@ -2739,8 +2749,7 @@ asyncQueueProcessPageEntries(QueuePosition *current,
alignas(AsyncQueueEntry) char local_buf[QUEUE_PAGESIZE];
char *local_buf_end = local_buf;
- slotno = SimpleLruReadPage_ReadOnly(NotifyCtl, curpage,
- InvalidTransactionId);
+ slotno = SimpleLruReadPage_ReadOnly(NotifyCtl, curpage, current);
page_buffer = NotifyCtl->shared->page_buffer[slotno];
do
@@ -2998,8 +3007,7 @@ AsyncNotifyFreezeXids(TransactionId newFrozenXid)
lock = SimpleLruGetBankLock(NotifyCtl, pageno);
LWLockAcquire(lock, LW_EXCLUSIVE);
- slotno = SimpleLruReadPage(NotifyCtl, pageno, true,
- InvalidTransactionId);
+ slotno = SimpleLruReadPage(NotifyCtl, pageno, true, &pos);
page_buffer = NotifyCtl->shared->page_buffer[slotno];
curpage = pageno;
}
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index fe75ead3501..e72eb80a03f 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -442,6 +442,7 @@ static void ReleaseRWConflict(RWConflict conflict);
static void FlagSxactUnsafe(SERIALIZABLEXACT *sxact);
static bool SerialPagePrecedesLogically(int64 page1, int64 page2);
+static int serial_errdetail_for_io_error(const void *opaque_data);
static void SerialInit(void);
static void SerialAdd(TransactionId xid, SerCommitSeqNo minConflictCommitSeqNo);
static SerCommitSeqNo SerialGetMinConflictCommitSeqNo(TransactionId xid);
@@ -742,6 +743,14 @@ SerialPagePrecedesLogically(int64 page1, int64 page2)
TransactionIdPrecedes(xid1, xid2 + SERIAL_ENTRIESPERPAGE - 1));
}
+static int
+serial_errdetail_for_io_error(const void *opaque_data)
+{
+ TransactionId xid = *(const TransactionId *) opaque_data;
+
+ return errdetail("Could not access serializable CSN of transaction %u.", xid);
+}
+
#ifdef USE_ASSERT_CHECKING
static void
SerialPagePrecedesLogicallyUnitTests(void)
@@ -811,6 +820,7 @@ SerialInit(void)
* Set up SLRU management of the pg_serial data.
*/
SerialSlruCtl->PagePrecedes = SerialPagePrecedesLogically;
+ SerialSlruCtl->errdetail_for_io_error = serial_errdetail_for_io_error;
SimpleLruInit(SerialSlruCtl, "serializable",
serializable_buffers, 0, "pg_serial",
LWTRANCHE_SERIAL_BUFFER, LWTRANCHE_SERIAL_SLRU,
@@ -930,7 +940,7 @@ SerialAdd(TransactionId xid, SerCommitSeqNo minConflictCommitSeqNo)
else
{
LWLockAcquire(lock, LW_EXCLUSIVE);
- slotno = SimpleLruReadPage(SerialSlruCtl, targetPage, true, xid);
+ slotno = SimpleLruReadPage(SerialSlruCtl, targetPage, true, &xid);
}
SerialValue(slotno, xid) = minConflictCommitSeqNo;
@@ -974,7 +984,7 @@ SerialGetMinConflictCommitSeqNo(TransactionId xid)
* but will return with that lock held, which must then be released.
*/
slotno = SimpleLruReadPage_ReadOnly(SerialSlruCtl,
- SerialPage(xid), xid);
+ SerialPage(xid), &xid);
val = SerialValue(slotno, xid);
LWLockRelease(SimpleLruGetBankLock(SerialSlruCtl, SerialPage(xid)));
return val;
diff --git a/src/include/access/slru.h b/src/include/access/slru.h
index 4cb8f478fce..dcb59ef8761 100644
--- a/src/include/access/slru.h
+++ b/src/include/access/slru.h
@@ -13,6 +13,7 @@
#ifndef SLRU_H
#define SLRU_H
+#include "access/transam.h"
#include "access/xlogdefs.h"
#include "storage/lwlock.h"
#include "storage/sync.h"
@@ -141,6 +142,16 @@ typedef struct SlruCtlData
*/
bool (*PagePrecedes) (int64, int64);
+ /*
+ * Callback to provide more details on an I/O error. This is called as
+ * part of ereport(), and the callback function is expected to call
+ * errdetail() to provide more details.
+ *
+ * The opaque_data argument here is the argument passed to the
+ * SimpleLruReadPage() function.
+ */
+ int (*errdetail_for_io_error) (const void *opaque_data);
+
/*
* Dir is set during SimpleLruInit and does not change thereafter. Since
* it's always the same, it doesn't need to be in shared memory.
@@ -174,9 +185,9 @@ extern void SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
extern int SimpleLruZeroPage(SlruCtl ctl, int64 pageno);
extern void SimpleLruZeroAndWritePage(SlruCtl ctl, int64 pageno);
extern int SimpleLruReadPage(SlruCtl ctl, int64 pageno, bool write_ok,
- TransactionId xid);
+ const void *opaque_data);
extern int SimpleLruReadPage_ReadOnly(SlruCtl ctl, int64 pageno,
- TransactionId xid);
+ const void *opaque_data);
extern void SimpleLruWritePage(SlruCtl ctl, int slotno);
extern void SimpleLruWriteAll(SlruCtl ctl, bool allow_redirtied);
#ifdef USE_ASSERT_CHECKING
diff --git a/src/test/modules/test_slru/expected/test_slru.out b/src/test/modules/test_slru/expected/test_slru.out
index 185c56e5d62..7ae1b0d4a98 100644
--- a/src/test/modules/test_slru/expected/test_slru.out
+++ b/src/test/modules/test_slru/expected/test_slru.out
@@ -23,6 +23,10 @@ SELECT test_slru_page_exists(12345);
t
(1 row)
+-- Test read failure
+SELECT test_slru_page_read(54321, false, '123'::xid);
+ERROR: could not open file "pg_test_slru/0000000000006A1": No such file or directory
+DETAIL: Could not access test_slru entry 123.
-- 48 extra pages
SELECT count(test_slru_page_write(a, 'Test SLRU'))
FROM generate_series(12346, 12393, 1) as a;
diff --git a/src/test/modules/test_slru/sql/test_slru.sql b/src/test/modules/test_slru/sql/test_slru.sql
index b1b376581ab..c6454c5bf82 100644
--- a/src/test/modules/test_slru/sql/test_slru.sql
+++ b/src/test/modules/test_slru/sql/test_slru.sql
@@ -5,6 +5,9 @@ SELECT test_slru_page_write(12345, 'Test SLRU');
SELECT test_slru_page_read(12345);
SELECT test_slru_page_exists(12345);
+-- Test read failure
+SELECT test_slru_page_read(54321, false, '123'::xid);
+
-- 48 extra pages
SELECT count(test_slru_page_write(a, 'Test SLRU'))
FROM generate_series(12346, 12393, 1) as a;
diff --git a/src/test/modules/test_slru/test_slru--1.0.sql b/src/test/modules/test_slru/test_slru--1.0.sql
index abecb5e2183..2148e7204ec 100644
--- a/src/test/modules/test_slru/test_slru--1.0.sql
+++ b/src/test/modules/test_slru/test_slru--1.0.sql
@@ -7,7 +7,7 @@ CREATE OR REPLACE FUNCTION test_slru_page_writeall() RETURNS VOID
AS 'MODULE_PATHNAME', 'test_slru_page_writeall' LANGUAGE C;
CREATE OR REPLACE FUNCTION test_slru_page_sync(bigint) RETURNS VOID
AS 'MODULE_PATHNAME', 'test_slru_page_sync' LANGUAGE C;
-CREATE OR REPLACE FUNCTION test_slru_page_read(bigint, bool DEFAULT true) RETURNS text
+CREATE OR REPLACE FUNCTION test_slru_page_read(bigint, bool DEFAULT true, xid DEFAULT '0') RETURNS text
AS 'MODULE_PATHNAME', 'test_slru_page_read' LANGUAGE C;
CREATE OR REPLACE FUNCTION test_slru_page_readonly(bigint) RETURNS text
AS 'MODULE_PATHNAME', 'test_slru_page_readonly' LANGUAGE C;
diff --git a/src/test/modules/test_slru/test_slru.c b/src/test/modules/test_slru/test_slru.c
index 4dc74e19620..e4bd2af0bf5 100644
--- a/src/test/modules/test_slru/test_slru.c
+++ b/src/test/modules/test_slru/test_slru.c
@@ -93,14 +93,14 @@ test_slru_page_read(PG_FUNCTION_ARGS)
{
int64 pageno = PG_GETARG_INT64(0);
bool write_ok = PG_GETARG_BOOL(1);
+ TransactionId xid = PG_GETARG_TRANSACTIONID(2);
char *data = NULL;
int slotno;
LWLock *lock = SimpleLruGetBankLock(TestSlruCtl, pageno);
/* find page in buffers, reading it if necessary */
LWLockAcquire(lock, LW_EXCLUSIVE);
- slotno = SimpleLruReadPage(TestSlruCtl, pageno,
- write_ok, InvalidTransactionId);
+ slotno = SimpleLruReadPage(TestSlruCtl, pageno, write_ok, &xid);
data = (char *) TestSlruCtl->shared->page_buffer[slotno];
LWLockRelease(lock);
@@ -118,7 +118,7 @@ test_slru_page_readonly(PG_FUNCTION_ARGS)
/* find page in buffers, reading it if necessary */
slotno = SimpleLruReadPage_ReadOnly(TestSlruCtl,
pageno,
- InvalidTransactionId);
+ NULL);
Assert(LWLockHeldByMe(lock));
data = (char *) TestSlruCtl->shared->page_buffer[slotno];
LWLockRelease(lock);
@@ -210,6 +210,14 @@ test_slru_page_precedes_logically(int64 page1, int64 page2)
return page1 < page2;
}
+static int
+test_slru_errdetail_for_io_error(const void *opaque_data)
+{
+ TransactionId xid = *(const TransactionId *) opaque_data;
+
+ return errdetail("Could not access test_slru entry %u.", xid);
+}
+
static void
test_slru_shmem_startup(void)
{
@@ -245,6 +253,7 @@ test_slru_shmem_startup(void)
}
TestSlruCtl->PagePrecedes = test_slru_page_precedes_logically;
+ TestSlruCtl->errdetail_for_io_error = test_slru_errdetail_for_io_error;
SimpleLruInit(TestSlruCtl, "TestSLRU",
NUM_TEST_BUFFERS, 0, slru_dir_name,
test_buffer_tranche_id, test_tranche_id, SYNC_HANDLER_NONE,
--
2.47.3
view thread (6+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Rework SLRU I/O errors handle
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox