pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
45+ messages / 10 participants
[nested] [flat]
* BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-03 17:25 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 45+ messages in thread
From: PG Bug reporting form @ 2023-10-03 17:25 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: rootcause000@gmail.com
The following bug has been logged on the website:
Bug reference: 18146
Logged by: Root Cause
Email address: rootcause000@gmail.com
PostgreSQL version: Unsupported/Unknown
Operating system: Windows
Description:
Version: PostgreSQL 10.21, compiled by Visual C++ build 1800, 64-bit
Platform: Windows
In our application code, we've implemented logic to clean up entries in
several PostgreSQL tables using a loop. Although some of these tables have
foreign key references, we've included them to ensure a thorough deletion
process. Here's a simplified code snippet:
String tables[] = {"TableA", "TableB", "TableC", "TableD", "TableE",
"TableF", "TableG", "TableH"};
for (String tableName : tables) {
try {
stmt = conn.prepareStatement("DELETE FROM " + tableName);
stmt.execute();
} catch (Exception ex) {
// Log the exception
}
}
Notably, it leaves entries in different tables each time, and we've found
auto-vacuum failure traces in the pg_log for these tables at the same time.
Here's an example of the error messages from pg_log:
2023-09-28 12:03:35.955 IST,,,11592,,65151e2d.2d48,1,,2023-09-28 12:03:17
IST,6/19,122786262,ERROR,42501,"could not truncate file
""base/16509/6935026"" to 0 blocks: Permission denied",,,,,"automatic vacuum
of table ""DB.public.TableB""",,,,"" 2023-09-28 12:03:37.205
IST,,,11592,,65151e2d.2d48,2,,2023-09-28 12:03:17
IST,6/45,122786326,ERROR,42501,"could not truncate file
""base/16509/6935443"" to 0 blocks: Permission denied",,,,,"automatic vacuum
of table ""DB.public.TableB""",,,,""
We've already attempted to exclude the entire pg folder from antivirus
scans, but the problem persists. Any insights or solutions to this issue
would be greatly appreciated. Thank you!
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-04 07:17 Laurenz Albe <laurenz.albe@cybertec.at>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 45+ messages in thread
From: Laurenz Albe @ 2023-10-04 07:17 UTC (permalink / raw)
To: rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, 2023-10-03 at 17:25 +0000, PG Bug reporting form wrote:
> Version: PostgreSQL 10.21, compiled by Visual C++ build 1800, 64-bit
>
> Platform: Windows
>
> In our application code, we've implemented logic to clean up entries in
> several PostgreSQL tables using a loop. Although some of these tables have
> foreign key references, we've included them to ensure a thorough deletion
> process. Here's a simplified code snippet:
>
> String tables[] = {"TableA", "TableB", "TableC", "TableD", "TableE",
> "TableF", "TableG", "TableH"};
>
> for (String tableName : tables) {
> try {
> stmt = conn.prepareStatement("DELETE FROM " + tableName);
> stmt.execute();
> } catch (Exception ex) {
> // Log the exception
> }
> }
> Notably, it leaves entries in different tables each time, and we've found
> auto-vacuum failure traces in the pg_log for these tables at the same time.
> Here's an example of the error messages from pg_log:
>
> 2023-09-28 12:03:35.955 IST,,,11592,,65151e2d.2d48,1,,2023-09-28 12:03:17
> IST,6/19,122786262,ERROR,42501,"could not truncate file
> ""base/16509/6935026"" to 0 blocks: Permission denied",,,,,"automatic vacuum
> of table ""DB.public.TableB""",,,,"" 2023-09-28 12:03:37.205
> IST,,,11592,,65151e2d.2d48,2,,2023-09-28 12:03:17
> IST,6/45,122786326,ERROR,42501,"could not truncate file
> ""base/16509/6935443"" to 0 blocks: Permission denied",,,,,"automatic vacuum
> of table ""DB.public.TableB""",,,,""
>
> We've already attempted to exclude the entire pg folder from antivirus
> scans, but the problem persists. Any insights or solutions to this issue
> would be greatly appreciated. Thank you!
PostgreSQL v10 is out of support.
Data corruption like this is not necessarily caused by a PostgreSQL bug.
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-04 07:24 Michael Paquier <michael@paquier.xyz>
parent: Laurenz Albe <laurenz.albe@cybertec.at>
0 siblings, 1 reply; 45+ messages in thread
From: Michael Paquier @ 2023-10-04 07:24 UTC (permalink / raw)
To: Laurenz Albe <laurenz.albe@cybertec.at>; +Cc: rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Oct 04, 2023 at 09:17:11AM +0200, Laurenz Albe wrote:
> Data corruption like this is not necessarily caused by a PostgreSQL bug.
Err, well... A failure on the end-of-vacuum truncation should not
lead to corruption afterwards as well, and this ought to be safe even
if this step failed. This is a very tricky problem that nobody has
really looked into yet.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ZR0TPQvVQqIzuPsG@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-04 14:24 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 45+ messages in thread
From: Tom Lane @ 2023-10-04 14:24 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Oct 04, 2023 at 09:17:11AM +0200, Laurenz Albe wrote:
>> Data corruption like this is not necessarily caused by a PostgreSQL bug.
> Err, well... A failure on the end-of-vacuum truncation should not
> lead to corruption afterwards as well, and this ought to be safe even
> if this step failed. This is a very tricky problem that nobody has
> really looked into yet.
ISTM we did identify the problem: while all the tuples in the
pages-to-be-truncated should be dead and thus invisible, it may
be that some of those pages are dirty and haven't been written
out of shared buffers yet, and the page versions on disk contain
tuples that look live. If VACUUM discards those dirty buffers
and then fails to truncate, voila you have tuples rising from
the dead.
I'm too lazy to check the commit log right now, but I think
we did implement a fix for that (ie, flush dirty pages even
if we anticipate them going away due to truncation). But as
Laurenz says, v10 is out of support and possibly didn't get
that fix. Even if it did, you'd need to be running one of
the last minor releases, because this wasn't very long ago.
In the end though, the *real* problem here is running on a
platform that randomly disallows writes to disk. There's only
so much that Postgres can possibly do about unreliability of the
underlying platform. I would never run a production database on
Windows, because it's just too prone to that sort of BS.
regards, tom lane
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-04 21:12 Thomas Munro <thomas.munro@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 3 replies; 45+ messages in thread
From: Thomas Munro @ 2023-10-04 21:12 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Oct 5, 2023 at 3:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > On Wed, Oct 04, 2023 at 09:17:11AM +0200, Laurenz Albe wrote:
> >> Data corruption like this is not necessarily caused by a PostgreSQL bug.
>
> > Err, well... A failure on the end-of-vacuum truncation should not
> > lead to corruption afterwards as well, and this ought to be safe even
> > if this step failed. This is a very tricky problem that nobody has
> > really looked into yet.
>
> ISTM we did identify the problem: while all the tuples in the
> pages-to-be-truncated should be dead and thus invisible, it may
> be that some of those pages are dirty and haven't been written
> out of shared buffers yet, and the page versions on disk contain
> tuples that look live. If VACUUM discards those dirty buffers
> and then fails to truncate, voila you have tuples rising from
> the dead.
>
> I'm too lazy to check the commit log right now, but I think
> we did implement a fix for that (ie, flush dirty pages even
> if we anticipate them going away due to truncation). But as
> Laurenz says, v10 is out of support and possibly didn't get
> that fix. Even if it did, you'd need to be running one of
> the last minor releases, because this wasn't very long ago.
This thread seems to be saying otherwise:
https://www.postgresql.org/message-id/flat/2348.1544474335%40sss.pgh.pa.us
> In the end though, the *real* problem here is running on a
> platform that randomly disallows writes to disk. There's only
> so much that Postgres can possibly do about unreliability of the
> underlying platform. I would never run a production database on
> Windows, because it's just too prone to that sort of BS.
It's surprising that ftruncate() AKA chsize() is able to fail like
this (I am not a Windows user but AFAIR that sharing stuff obstructs
stuff like open, unlink, rename, so it surprises me to see it come up
with ftruncate, since we must already have made it past the open
stage). Hmm, the documentation is scant, but I know from my attempts
to use large files that chsize() is probably some kind of wrapper
around SetEndOfFile() or similar, and that is documented as failing if
someone has the file mapped. I don't know why someone would have the
file mapped, though.
But as for what we should do about it, PANIC (as suggested by several
people) seems better than corruption, if we're not going to write some
kind of resilience? How else are we supposed to deal with "this
shouldn't happen, and if it does we're hosed?"
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-04 22:38 Thomas Munro <thomas.munro@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
2 siblings, 0 replies; 45+ messages in thread
From: Thomas Munro @ 2023-10-04 22:38 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Oct 5, 2023 at 10:12 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> It's surprising that ftruncate() AKA chsize() is able to fail like
> this (I am not a Windows user but AFAIR that sharing stuff obstructs
> stuff like open, unlink, rename, so it surprises me to see it come up
> with ftruncate, since we must already have made it past the open
> stage). Hmm, the documentation is scant, but I know from my attempts
> to use large files that chsize() is probably some kind of wrapper
> around SetEndOfFile() or similar, and that is documented as failing if
> someone has the file mapped. I don't know why someone would have the
> file mapped, though.
Some more thoughts: I guess it would probably also fail like that if
someone explicitly locked a range with LockFile(), but I think we can
rule that out as read and/or write calls would also fail. As for the
mapping theory, apparently the underlying NT error for that is
ERROR_USER_MAPPED_FILE, and searching for that brings up various
unexplained errors vaguely blamed on anti-virus tools etc. But if all
of that sort of thing really is turned off for the data directory, I
wonder if there could be a backup tool in use that thinks it can go
faster by mapping files while copying?
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-04 22:44 Michael Paquier <michael@paquier.xyz>
parent: Thomas Munro <thomas.munro@gmail.com>
2 siblings, 1 reply; 45+ messages in thread
From: Michael Paquier @ 2023-10-04 22:44 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Oct 05, 2023 at 10:12:27AM +1300, Thomas Munro wrote:
> On Thu, Oct 5, 2023 at 3:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm too lazy to check the commit log right now, but I think
>> we did implement a fix for that (ie, flush dirty pages even
>> if we anticipate them going away due to truncation). But as
>> Laurenz says, v10 is out of support and possibly didn't get
>> that fix. Even if it did, you'd need to be running one of
>> the last minor releases, because this wasn't very long ago.
>
> This thread seems to be saying otherwise:
>
> https://www.postgresql.org/message-id/flat/2348.1544474335%40sss.pgh.pa.us
Yeah, that's not been solved. I've put my mind on this problem for a
few hours last May, just before PGCon, and there was an argument that
marking all the pages as dirty is kind of a waste of resources as it
would create WAL for data that's going to be gone a little bit later
as an effect of the truncate, leading to an extra burst of I/O
especially for large truncatoins. FWIW, I think that I'd be
personally OK with using this method. At least that's safe, simple,
backpatchable and it does not require any new magic. I know that
there are voices that argued against this method, but here we are N
years later, so perhaps we should just do that on HEAD at least.
>> In the end though, the *real* problem here is running on a
>> platform that randomly disallows writes to disk. There's only
>> so much that Postgres can possibly do about unreliability of the
>> underlying platform. I would never run a production database on
>> Windows, because it's just too prone to that sort of BS.
>
> It's surprising that ftruncate() AKA chsize() is able to fail like
> this (I am not a Windows user but AFAIR that sharing stuff obstructs
> stuff like open, unlink, rename, so it surprises me to see it come up
> with ftruncate, since we must already have made it past the open
> stage). Hmm, the documentation is scant, but I know from my attempts
> to use large files that chsize() is probably some kind of wrapper
> around SetEndOfFile() or similar, and that is documented as failing if
> someone has the file mapped. I don't know why someone would have the
> file mapped, though.
(shrug)
> But as for what we should do about it, PANIC (as suggested by several
> people) seems better than corruption, if we're not going to write some
> kind of resilience? How else are we supposed to deal with "this
> shouldn't happen, and if it does we're hosed?"
A PANIC may be OK for this specific syscall and would be better, but
the problematic area is larger than that as we'd still finish with a
corruption as long as there's an ERROR or a FATAL between the moment
the buffers (potentially dirty, with live-still-dead-in-memory tuples
on disk) are discarded and the moment the truncation fails. Another
method discussed is the use of a critical section (I recall that there
were some pallocs in this area, actually, but got nothing on my notes
about that...).
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ZR3qvrYULJWaUnBK@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-04 23:02 Tom Lane <tgl@sss.pgh.pa.us>
parent: Thomas Munro <thomas.munro@gmail.com>
2 siblings, 1 reply; 45+ messages in thread
From: Tom Lane @ 2023-10-04 23:02 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Oct 5, 2023 at 3:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm too lazy to check the commit log right now, but I think
>> we did implement a fix for that (ie, flush dirty pages even
>> if we anticipate them going away due to truncation).
> This thread seems to be saying otherwise:
> https://www.postgresql.org/message-id/flat/2348.1544474335%40sss.pgh.pa.us
Hmph. OK, I was remembering the discussion not the (lack of)
end result.
> But as for what we should do about it, PANIC (as suggested by several
> people) seems better than corruption, if we're not going to write some
> kind of resilience?
Maybe that's an acceptable answer now ... it's not great, but nobody
is in love with any of the other options either. And it would definitely
get DBAs' attention about this misbehavior of their file systems.
regards, tom lane
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-05 19:55 Thomas Munro <thomas.munro@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2023-10-05 19:55 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Oct 5, 2023 at 11:44 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Thu, Oct 05, 2023 at 10:12:27AM +1300, Thomas Munro wrote:
> > But as for what we should do about it, PANIC (as suggested by several
> > people) seems better than corruption, if we're not going to write some
> > kind of resilience? How else are we supposed to deal with "this
> > shouldn't happen, and if it does we're hosed?"
>
> A PANIC may be OK for this specific syscall and would be better, but
> the problematic area is larger than that as we'd still finish with a
> corruption as long as there's an ERROR or a FATAL between the moment
> the buffers (potentially dirty, with live-still-dead-in-memory tuples
> on disk) are discarded and the moment the truncation fails. Another
> method discussed is the use of a critical section (I recall that there
> were some pallocs in this area, actually, but got nothing on my notes
> about that...).
Yeah. I guess the obvious place for a critical section to start would
be in RelationTruncate() near DELAY_CHKPT_COMPLETE where a similar
concern about recovery is discussed. There is a comment explaining
that it's a bad idea to use a critical section, but evidently it
overestimated the harmlessness of that choice. It has a point that if
DO can't truncate, maybe REDO will fail too and you might be stuck in
an eternal samsara of failed recovery, but... that's because your
system isn't doing things we fundamentally need it to do to make
progress, and an administrator needs to find out why. And although
eternity sounds bad, as far as I can tell from the rare reports we've
had of this failure, it seems to be transient, right?
Perhaps we could consider adding ftruncate() to the set of horrible
Windows wrapper functions that hide a few sleep-retry loops before
they give up so there's a good chance of avoiding a crash, but it'd be
better if someone more knowledgeable/hands-on with Windows could get
to the bottom of what is causing this and document how to avoid it...
I think we understand why we need that in the other cases (this OS's
peculiar dirent management), and this case seems a bit different.
As for Unix, if I guessed right about mmap being involved, the kernel
always lets the truncation proceed but kills the mapping process on
access to non-backed region of memory. And for EINTR, handling was
recently added for this (0d369ac6500), so I guess there may be ways in
older branches on some systems (eg the old 'interruptible' NFS, but
probably not on normal file systems...). The only other things I can
think of are EIO, and then various left field errors caused by the
file system going read only or the file being 'sealed', etc, and
promotion to PANIC seems appropriate for those cases.
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-05 20:18 Thomas Munro <thomas.munro@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2023-10-05 20:18 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 6, 2023 at 8:55 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> ... DELAY_CHKPT_COMPLETE ...
About that... If the lights go out after the truncation and the
delayed logging of the checkpoint, how do we know the truncation has
actually reached the disk? mdtruncate() enqueues fsync() calls, but
if we were already in phase 2 (see proc.h) of a checkpoint at that
moment, they might be processed by the *next* checkpoint, not the one
whose phase 3 we've carefully delayed there, no?
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-06 04:46 Thomas Munro <thomas.munro@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2023-10-06 04:46 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 6, 2023 at 9:18 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Fri, Oct 6, 2023 at 8:55 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> > ... DELAY_CHKPT_COMPLETE ...
>
> About that... If the lights go out after the truncation and the
> delayed logging of the checkpoint, how do we know the truncation has
> actually reached the disk? mdtruncate() enqueues fsync() calls, but
> if we were already in phase 2 (see proc.h) of a checkpoint at that
> moment, they might be processed by the *next* checkpoint, not the one
> whose phase 3 we've carefully delayed there, no?
I didn't look into this for very long so I might be missing something
here, but I think there could be at least one bad sequence. If you
insert a couple of sleeps to jinx the scheduling, and hack
RelationTruncate() to request a checkpoint at a carefully chosen wrong
moment (see attached), then:
postgres=# create table t (i int);
CREATE TABLE
postgres=# insert into t select 1 from generate_series(1, 100);
INSERT 0 100
postgres=# checkpoint; -- checkpoint #1 just puts some data on disk
CHECKPOINT
postgres=# delete from t;
DELETE 100
postgres=# vacuum freeze t; -- truncates, starts unlucky checkpoint #2
VACUUM
If you trace the checkpointer's system calls you will see that
base/5/16384 (or whatever t's relfilenode is for you) is *not* fsync'd
by checkpoint #2. The following checkpoint #3 might eventually do it,
but if the kernel loses power after checkpoint #2 completes and there
is no checkpoint #3, the kernel might forget the truncation, and yet
replay starts too late to redo it. I think that bad sequence looks
like this:
P1: log truncate
P2: choose redo LSN
P1: DropRelationBuffers()
P2: CheckPointBuffers()
P2: ProcessSyncRequests()
P1: ftruncate()
P1: RegisterSyncRequest()
P2: log checkpoint
*** system loses power ***
I realise it is a different problem than the one reported, but it's
close. My initial thought is that perhaps we shouldn't allow a redo
LSN to be chosen until the sync request is registered, which is also
fairly close to the critical section boundaries being discussed for
ftruncate() error case. But that's not a phase the checkpoint delay
machinery currently knows how to delay. And there may well be better
ways...
Attachments:
[text/x-patch] hack.diff (1.7K, ../../CA+hUKG+-2rjGZC2kwqr2NMLBcEBp4uf59QT1advbWYF_uc+0Aw@mail.gmail.com/2-hack.diff)
download | inline diff:
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index fcbde10529..cc51a0ea44 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7051,6 +7051,7 @@ CheckPointGuts(XLogRecPtr checkPointRedo, int flags)
CheckPointSUBTRANS();
CheckPointMultiXact();
CheckPointPredicate();
+ sleep(3);
CheckPointBuffers(flags);
/* Perform all queued up fsyncs */
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 93f07e49b7..3785ecd699 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -28,6 +28,7 @@
#include "catalog/storage.h"
#include "catalog/storage_xlog.h"
#include "miscadmin.h"
+#include "postmaster/bgwriter.h" /* XXX for RequestCheckpoint() (!!!) */
#include "storage/freespace.h"
#include "storage/smgr.h"
#include "utils/hsearch.h"
@@ -389,6 +390,9 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
XLogFlush(lsn);
}
+ /* XXX unlucky timing, a checkpoint happens to start now */
+ RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_FORCE);
+
/*
* This will first remove any buffers from the buffer pool that should no
* longer exist after truncation is complete, and then truncate the
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 5d0f3d515c..30ef9263f4 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -663,6 +663,8 @@ smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nb
*/
DropRelationBuffers(reln, forknum, nforks, nblocks);
+ sleep(5);
+
/*
* Send a shared-inval message to force other backends to close any smgr
* references they may have for this rel. This is useful because they
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-11 14:44 Robert Haas <robertmhaas@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 45+ messages in thread
From: Robert Haas @ 2023-10-11 14:44 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Thomas Munro <thomas.munro@gmail.com>; Michael Paquier <michael@paquier.xyz>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Oct 4, 2023 at 7:03 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > But as for what we should do about it, PANIC (as suggested by several
> > people) seems better than corruption, if we're not going to write some
> > kind of resilience?
>
> Maybe that's an acceptable answer now ... it's not great, but nobody
> is in love with any of the other options either. And it would definitely
> get DBAs' attention about this misbehavior of their file systems.
I and others, including Andres, have been thinking that a PANIC is the
right option for some time.
Quoth I in https://www.postgresql.org/message-id/CA%2BTgmobwc_Rdaw%2B6TupT4_g9z55JjL%3DvhwpphsQe%3DYmBN0OPDg%40...
some 2 years ago...
> As you say, this doesn't fix the problem that truncation might fail.
> But as Andres and Sawada-san said, the solution to that is to get rid
> of the comments saying that it's OK for truncation to fail and make it
> a PANIC. However, I don't think that change needs to be part of this
> patch. Even if we do that, we still need to do this. And even if we do
> this, we still need to do that.
I think the only reasons that I didn't do it at the time where (a)
shortage of round tuits and (b) fear of being yelled at. But the
comment is wrong, and a critical section is right.
I do think that it's nice to be tolerant of bad filesystem behavior
when we can. For instance if we try to write() some data to the OS and
it fails for some transient reason, it's nice if we can try to write()
it again. But there are always going to be cases where that sort of
tolerance is not practical. Having PostgreSQL continue to operate when
the filesystem isn't operating is a luxury, and we can't afford it in
every situation. shared_buffers provides a layer of insulation between
the logical act of modifying a buffer and the need to have a system
call succeed -- dirtying the buffer is in effect making a note that
the write() needs to be done later, instead of actually doing it in
the moment. And since the code that actually writes it is
checkpoint-aware and write-outs can be retried, we can avoid
panicking. But for operations such as creating, removing, or
truncating relations, there is no similar, general layer of insulation
-- we have no mechanism that allows us to logically do those things
now and have them actually happen at the FS level later. Which, to me,
seems to mean that we have little choice but to panic if they fail.
Otherwise, the primary diverges from any standbys that it has.
I also think that's OK. Unreliable filesystems lead to unreliable
databases, and it's better to find that out before something really
bad happens. Maybe in the future we'll develop more general mechanisms
for some of this stuff and maybe that will allow us to avoid panics in
more cases, and then we can debate the merits of such changes. But
right now, the cost of avoiding a panic here is a corrupted database,
and I have to believe that the overwhelming majority of users would
think that a corrupted database is worse.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-11 16:59 Robert Haas <robertmhaas@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Robert Haas @ 2023-10-11 16:59 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 6, 2023 at 12:50 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> If you trace the checkpointer's system calls you will see that
> base/5/16384 (or whatever t's relfilenode is for you) is *not* fsync'd
> by checkpoint #2. The following checkpoint #3 might eventually do it,
> but if the kernel loses power after checkpoint #2 completes and there
> is no checkpoint #3, the kernel might forget the truncation, and yet
> replay starts too late to redo it. I think that bad sequence looks
> like this:
>
> P1: log truncate
> P2: choose redo LSN
> P1: DropRelationBuffers()
> P2: CheckPointBuffers()
> P2: ProcessSyncRequests()
> P1: ftruncate()
> P1: RegisterSyncRequest()
> P2: log checkpoint
> *** system loses power ***
>
> I realise it is a different problem than the one reported, but it's
> close. My initial thought is that perhaps we shouldn't allow a redo
> LSN to be chosen until the sync request is registered, which is also
> fairly close to the critical section boundaries being discussed for
> ftruncate() error case. But that's not a phase the checkpoint delay
> machinery currently knows how to delay. And there may well be better
> ways...
Hmm. So the problem in this case is that we not only need the WAL to
be inserted and the filesystem operation to be performed in the same
checkpoint cycle, but we also need the sync request to be processed in
that checkpoint cycle. In this example, the WAL insertion happens
before the redo LSN, so the truncate on disk must happen before the
checkpoint completes, which is guaranteed, and the sync request must
also be processed before the checkpoint completes, which is not
guaranteed. We only guarantee that the checkpoint goes into the
checkpointer's queue before the checkpoint completes, not that it gets
processed before the checkpoint completes.
Suppose that RelationTruncate set both DELAY_CHKPT_START and
DELAY_CHKPT_COMPLETE. I think that would prevent this problem. P2
could still choose the redo LSN after P1 logged the truncate, but it
wouldn't then be able to reach CheckPointBuffers() until after P1 had
reached RegisterSyncRequest. Note that setting *only*
DELAY_CHKPT_START isn't good enough, because then we can get this
history:
P1: log truncate
P2: choose redo LSN
P2: CheckPointBuffers()
P1: DropRelationBuffers()
P2: ProcessSyncRequests()
P2: log checkpoint
*** system loses power ***
I think, in general, that DELAY_CHKPT_START is useful for cases where
CheckPointGuts() is going to flush something that might not be locked
at the time we WAL-log a modification to it. For instance,
MarkBufferDirtyHint() wants to log that the buffer will be dirty while
holding only a shared lock on the buffer. RecordTransactonCommit() and
RecordTransactionCommitPrepared() write WAL that necesitate CLOG
updates, but they haven't yet locked the relevant CLOG buffers.
DELAY_CHKPT_END, on the other hand, is useful for cases where some
operation needs to get completed before the redo pointer gets moved.
What I think we missed in 412ad7a55639516f284cd0ef9757d6ae5c7abd43 is
that RelationTruncate actually falls into both categories -- the
ftruncate() needs to be protected by DELAY_CHKPT_END so that it
happens before the redo pointer moves, but the RegisterSyncRequest()
needs to be protected by DELAY_CHKPT_START so that it gets "flushed"
by ProcessSyncRequests.
I'm not quite sure that this idea closes all the holes, though. The
delay-checkpoint mechanism is quite hard to reason about. I spent a
while this morning trying to think up something better but realized at
the end I'd just reinvented the wheel.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-12 03:36 Thomas Munro <thomas.munro@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2023-10-12 03:36 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Oct 12, 2023 at 5:59 AM Robert Haas <robertmhaas@gmail.com> wrote:
> Suppose that RelationTruncate set both DELAY_CHKPT_START and
> DELAY_CHKPT_COMPLETE. I think that would prevent this problem. P2
> could still choose the redo LSN after P1 logged the truncate, but it
> wouldn't then be able to reach CheckPointBuffers() until after P1 had
> reached RegisterSyncRequest. [...]
Thanks for thinking about this. Yeah, the existing _START flag does
indeed seem to be enough. I'd been focusing on trying to control the
redo point selection, but we just need to delay ProcessSyncRequests(),
and we had a hammer for that already. Who wants to write the patch?
It should be trivial, except for the comments.
It's interesting that we'd stun the checkpointer just before we try to
send it a request in a fixed size queue, but that's OK because we'll
perform the fsync ourselves if the queue is full.
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-19 19:50 Robert Haas <robertmhaas@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Robert Haas @ 2023-10-19 19:50 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Oct 11, 2023 at 11:37 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> Thanks for thinking about this. Yeah, the existing _START flag does
> indeed seem to be enough. I'd been focusing on trying to control the
> redo point selection, but we just need to delay ProcessSyncRequests(),
> and we had a hammer for that already. Who wants to write the patch?
> It should be trivial, except for the comments.
>
> It's interesting that we'd stun the checkpointer just before we try to
> send it a request in a fixed size queue, but that's OK because we'll
> perform the fsync ourselves if the queue is full.
I don't understand what you mean about stunning the checkpointer, but
here are some patches.
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] 0002-Panic-if-filesystem-truncation-fails.patch (4.7K, ../../CA+Tgmob3tGBXxceozwWH2Ka7Kwbv8vkOfU8nveCx3JDaqNj7yA@mail.gmail.com/2-0002-Panic-if-filesystem-truncation-fails.patch)
download | inline diff:
From c5283146ca0e3cd2355a19d4d29fe455a6ab9fa2 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 19 Oct 2023 15:43:15 -0400
Subject: [PATCH 2/2] Panic if filesystem truncation fails.
It was originally thought that failure to PANIC in the event of a
failed truncation was harmless, but we now know otherwise. Without the
PANIC, you can end up with a corrupted database and broken standbys.
Add a critical section to force a PANIC in this case, and add a
lengthy comment explaining why it's necessary.
Reported by (XXX, multiple people, make an actual list). Patch by me.
---
src/backend/catalog/storage.c | 46 +++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 13 deletions(-)
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index bef4269368..08914d61e3 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -350,11 +350,11 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
* this code executes.
*
* Second, the call to smgrtruncate() below will in turn call
- * RegisterSyncRequest(). We need the sync request created by that call
- * to be processed before the checkpoint completes. CheckPointGuts()
- * will call ProcessSyncRequests(), but if we register our sync request
- * after that happens, then the WAL record for the truncation could end
- * up preceding the checkpoint record, while the actual sync doesn't happen
+ * RegisterSyncRequest(). We need the sync request created by that call to
+ * be processed before the checkpoint completes. CheckPointGuts() will
+ * call ProcessSyncRequests(), but if we register our sync request after
+ * that happens, then the WAL record for the truncation could end up
+ * preceding the checkpoint record, while the actual sync doesn't happen
* until the next checkpoint. To prevent that, we need to set
* DELAY_CHKPT_START here. That way, if the XLOG_SMGR_TRUNCATE precedes
* the redo pointer of a concurrent checkpoint, we're guaranteed that the
@@ -365,14 +365,33 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
MyProc->delayChkptFlags |= DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE;
/*
- * We WAL-log the truncation before actually truncating, which means
- * trouble if the truncation fails. If we then crash, the WAL replay
- * likely isn't going to succeed in the truncation either, and cause a
- * PANIC. It's tempting to put a critical section here, but that cure
- * would be worse than the disease. It would turn a usually harmless
- * failure to truncate, that might spell trouble at WAL replay, into a
- * certain PANIC.
+ * We WAL-log the truncation before actually truncating. That means that
+ * if truncation subsequently fails, we have to PANIC. If we don't, some
+ * really bad things can happen. First, consider that we discard dirty
+ * buffers from memory before actually truncating on disk. That means that
+ * in effect we revert to an older on disk state where, perhaps, some
+ * tuples that have been deleted since the last checkpoint are still
+ * visible. That's equivalent to allowing a committed transaction to
+ * become partially uncommitted, which is clearly unacceptable. Second,
+ * consider that truncation may succeed on some or all standbys even
+ * though it failed on the primary. That means that the primary and
+ * standby are now in persistently different states.
+ *
+ * In general, this is much as if we wrote a WAL record for a change to
+ * the contents of some page and then (for some reason) found ourselves
+ * unable to perform the corresponding page modification in memory. That
+ * situation also causes a PANIC. However, this case is considerably more
+ * uncomfortable, because an in-memory modification to page contents
+ * shouldn't really ever fail unless we've got a bug in the code
+ * somewhere. In contrast, trying to truncate a file on disk certainly can
+ * fail for a variety of reasons that are outside our control. If it does,
+ * then recovery will probably also fail, which probably won't be much fun
+ * for the user. They'll have to fix whatever is making us unable to
+ * truncate files on disk before they can get the database up and running
+ * again. But forcing them to fix permissions (or whatever the problem is)
+ * beats corrupting the database.
*/
+ START_CRIT_SECTION();
if (RelationNeedsWAL(rel))
{
/*
@@ -409,7 +428,8 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
*/
smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks);
- /* We've done all the critical work, so checkpoints are OK now. */
+ /* We've done all the critical work. */
+ END_CRIT_SECTION();
MyProc->delayChkptFlags &= ~(DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE);
/*
--
2.37.1 (Apple Git-137.1)
[application/octet-stream] 0001-RelationTruncate-must-also-set-DELAY_CHKPT_START.patch (3.7K, ../../CA+Tgmob3tGBXxceozwWH2Ka7Kwbv8vkOfU8nveCx3JDaqNj7yA@mail.gmail.com/3-0001-RelationTruncate-must-also-set-DELAY_CHKPT_START.patch)
download | inline diff:
From 0ffe042c70e0b665b6ae673b39713799618b9f02 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 19 Oct 2023 15:12:52 -0400
Subject: [PATCH 1/2] RelationTruncate must also set DELAY_CHKPT_START
Previously, it set only DELAY_CHKPT_COMPLETE. That was important,
because it meant that if the XLOG_SMGR_TRUNCATE record preceded a
XLOG_CHECKPOINT_ONLINE record in the WAL, then the truncation would
also happen on disk before the XLOG_CHECKPOINT_ONLINE record was
written.
However, it didn't guarantee that the sync request for the
truncation got processed before the XLOG_CHECKPOINT_ONLINE record
was written. By setting XLOG_CHKPT_START, we guarantee that if
an XLOG_SMGR_TRUNCATE record is written to WAL before the redo
pointer of some concurrent checkpoint, the sync request queued by
that operation must be processed by that checkpoint (rather than
being left for the following one).
Report by Thomas Munro. Patch by me.
---
src/backend/catalog/storage.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 93f07e49b7..bef4269368 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -336,20 +336,33 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
RelationPreTruncate(rel);
/*
- * Make sure that a concurrent checkpoint can't complete while truncation
- * is in progress.
+ * The code which follows can interact with concurrent checkpoints in two
+ * separate ways.
*
- * The truncation operation might drop buffers that the checkpoint
+ * First, the truncation operation might drop buffers that the checkpoint
* otherwise would have flushed. If it does, then it's essential that the
* files actually get truncated on disk before the checkpoint record is
* written. Otherwise, if reply begins from that checkpoint, the
* to-be-truncated blocks might still exist on disk but have older
* contents than expected, which can cause replay to fail. It's OK for the
* blocks to not exist on disk at all, but not for them to have the wrong
- * contents.
+ * contents. For this reason, we need to set DELAY_CHKPT_COMPLETE while
+ * this code executes.
+ *
+ * Second, the call to smgrtruncate() below will in turn call
+ * RegisterSyncRequest(). We need the sync request created by that call
+ * to be processed before the checkpoint completes. CheckPointGuts()
+ * will call ProcessSyncRequests(), but if we register our sync request
+ * after that happens, then the WAL record for the truncation could end
+ * up preceding the checkpoint record, while the actual sync doesn't happen
+ * until the next checkpoint. To prevent that, we need to set
+ * DELAY_CHKPT_START here. That way, if the XLOG_SMGR_TRUNCATE precedes
+ * the redo pointer of a concurrent checkpoint, we're guaranteed that the
+ * corresponding sync request will be processed before the checkpoint
+ * completes.
*/
- Assert((MyProc->delayChkptFlags & DELAY_CHKPT_COMPLETE) == 0);
- MyProc->delayChkptFlags |= DELAY_CHKPT_COMPLETE;
+ Assert((MyProc->delayChkptFlags & (DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE)) == 0);
+ MyProc->delayChkptFlags |= DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE;
/*
* We WAL-log the truncation before actually truncating, which means
@@ -397,7 +410,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks);
/* We've done all the critical work, so checkpoints are OK now. */
- MyProc->delayChkptFlags &= ~DELAY_CHKPT_COMPLETE;
+ MyProc->delayChkptFlags &= ~(DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE);
/*
* Update upper-level FSM pages to account for the truncation. This is
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-20 03:16 Thomas Munro <thomas.munro@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2023-10-20 03:16 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 20, 2023 at 8:50 AM Robert Haas <robertmhaas@gmail.com> wrote:
> I don't understand what you mean about stunning the checkpointer, but
I just meant that, with your change, we ask the checkpointer to wait
for our signal if it reaches that code path and then while it's
snoozing we call register_dirty_segment(). I wanted to report that
I'd checked that it copes with the request queue being full, by
falling back to calling fsync() itself, which is good news because if
it instead waited for the checkpointer to drain the request queue
instead (an available option), we'd have a potential deadlock.
> here are some patches.
0001: LGTM, except in the commit message "By setting
XLOG_CHKPT_START," s/XLOG/DELAY/.
0002: LGTM
Now I wonder if we will get occasional PANIC reports from Windows
users. I might prepare one of those retry wrappers...
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-20 03:38 Thomas Munro <thomas.munro@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2023-10-20 03:38 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 20, 2023 at 4:16 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Fri, Oct 20, 2023 at 8:50 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > here are some patches.
>
> 0001: LGTM, except in the commit message "By setting
I take that back: there is a palloc() under RelationTruncate(). DNLGTM.
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-20 05:17 Thomas Munro <thomas.munro@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2023-10-20 05:17 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 20, 2023 at 4:38 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Fri, Oct 20, 2023 at 4:16 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > On Fri, Oct 20, 2023 at 8:50 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > > here are some patches.
> >
> > 0001: LGTM, except in the commit message "By setting
>
> I take that back: there is a palloc() under RelationTruncate(). DNLGTM.
Ooops, I should have quoted to the 0002 patch there.
Hmm. We could teach md.c to do all its path manipulation in
MAXPGPATH-sized output buffers but that still wouldn't be enough
because it might decide to allocate more space for the array of
segments, and I'm not even sure where in PostgreSQL we guarantee that
everything that could appear here would fit in that. But that gives
me an idea. It feels like a bit of a dirty hack, which could perhaps
be made less dirty with some kind of function that includes the word
'ensure' in its name, but I think we can make md.c promise not to
allocate anything before the next CFI with something like this:
* truncate files on disk before they can get the database up
and running
* again. But forcing them to fix permissions (or whatever the
problem is)
* beats corrupting the database.
+ *
+ * First, make sure that smgr doesn't need to allocate any memory to
+ * truncate, since that wouldn't be allowed in a critical
section. We don't
+ * need to call XLogEnsureRecord() for such a small insertion.
*/
+
+ smgrnblocks(RelationGetSmgr(rel), MAIN_FORKNUM);
+ if (fsm)
+ smgrnblocks(RelationGetSmgr(rel), FSM_FORKNUM);
+
START_CRIT_SECTION();
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-23 15:06 Robert Haas <robertmhaas@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Robert Haas @ 2023-10-23 15:06 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Oct 20, 2023 at 1:17 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> > I take that back: there is a palloc() under RelationTruncate(). DNLGTM.
>
> Ooops, I should have quoted to the 0002 patch there.
>
> Hmm. We could teach md.c to do all its path manipulation in
> MAXPGPATH-sized output buffers but that still wouldn't be enough
> because it might decide to allocate more space for the array of
> segments, and I'm not even sure where in PostgreSQL we guarantee that
> everything that could appear here would fit in that. But that gives
> me an idea. It feels like a bit of a dirty hack, which could perhaps
> be made less dirty with some kind of function that includes the word
> 'ensure' in its name, but I think we can make md.c promise not to
> allocate anything before the next CFI with something like this:
Ugh. AFAICS the problems are confined to smgrtruncate() and within
that to the callback to smgr_truncate. As far as the rest of
smgrtruncate() is concerned, it seems like DropRelationBuffers() and
CacheInvalidateSmgr() are safe enough in a critical section, and the
other code directly inside RelationTruncate() looks OK, too.
But within mdtruncate(), we've got more than one problem, I think.
mdnblocks() is a problem because of the reason that you mention, but
register_dirty_segment() doesn't look totally safe either, because it
can call RegisterSyncRequest() which, in a standalone backend, can
call RememberSyncRequest().
In general, it seems like it would be a lot nicer if we were doing a
lot less stuff inside the critical section here. So I think you're
right that we need some refactoring. Maybe smgr_prepare_truncate() and
smgr_execute_truncate() or something like that. I wonder if we could
actually register the dirty segment in the "prepare" phase - is it bad
if we register a dirty segment before actually dirtying it? And maybe
even CacheInvalidateSmgr() could be done at that stage? It seems
pretty clear that dropping the dirty buffers and actually truncating
the relation on disk need to happen after we've entered the critical
section, because if we fail after doing the former, we've thrown away
dirty data in anticipation of performing an operation that didn't
happen, and if we fail when attempting the latter, primaries and
standbys diverge and the originally-reported bug on this thread
happens. But we'd like to move as much other stuff as we can out of
that critical section.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2023-10-23 21:54 Thomas Munro <thomas.munro@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2023-10-23 21:54 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, Oct 24, 2023 at 4:06 AM Robert Haas <robertmhaas@gmail.com> wrote:
> But within mdtruncate(), we've got more than one problem, I think.
> mdnblocks() is a problem because of the reason that you mention, but
> register_dirty_segment() doesn't look totally safe either, because it
> can call RegisterSyncRequest() which, in a standalone backend, can
> call RememberSyncRequest().
But that is already enkludgified thusly:
/*
* XXX: The checkpointer needs to add entries to the pending ops table
* when absorbing fsync requests. That is done within a critical
* section, which isn't usually allowed, but we make an exception. It
* means that there's a theoretical possibility that you run out of
* memory while absorbing fsync requests, which leads to a PANIC.
* Fortunately the hash table is small so that's unlikely to happen in
* practice.
*/
pendingOpsCxt = AllocSetContextCreate(TopMemoryContext,
"Pending ops context",
ALLOCSET_DEFAULT_SIZES);
MemoryContextAllowInCriticalSection(pendingOpsCxt, true);
> In general, it seems like it would be a lot nicer if we were doing a
> lot less stuff inside the critical section here. So I think you're
> right that we need some refactoring. Maybe smgr_prepare_truncate() and
> smgr_execute_truncate() or something like that. I wonder if we could
> actually register the dirty segment in the "prepare" phase - is it bad
> if we register a dirty segment before actually dirtying it? And maybe
> even CacheInvalidateSmgr() could be done at that stage? It seems
> pretty clear that dropping the dirty buffers and actually truncating
> the relation on disk need to happen after we've entered the critical
> section, because if we fail after doing the former, we've thrown away
> dirty data in anticipation of performing an operation that didn't
> happen, and if we fail when attempting the latter, primaries and
> standbys diverge and the originally-reported bug on this thread
> happens. But we'd like to move as much other stuff as we can out of
> that critical section.
Hmm, yeah it seems like that direction would be a nice improvement, as
long as we are sure that the fsync request can't be processed too
soon.
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-04-23 07:48 Thomas Munro <thomas.munro@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 2 replies; 45+ messages in thread
From: Thomas Munro @ 2024-04-23 07:48 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
Related bug #18426 sent me back here.
Here is a new attempt to see what it might take to put
RelationTruncate() into a critical section. Problems encountered:
even if you've called mdnblocks() beforehand, dressed up as
smgrpreparetruncate(), if the highest segment is exactly full then the
later mdnblocks() again probes whether the next segment number exists
on disk, which involves GetRelationPath(), which allocates. So I
finished up having to write a GetRelationPathInPlace() function, and
to decide whether it's OK to use a MAXPGPATH-sized array on the stack
for this. I also had to teach _fdvec_resize() not to reallocate when
downsizing, to avoid the critical section assertion. It seems like
quite a lot to back-patch... but also awful to leave this trickle of
data corruption reports unaddressed. The big re-engineering ideas[1]
would be absolutely unbackpatchable, but I hope we can work on
something like that for 18...
[1] https://www.postgresql.org/message-id/flat/2348.1544474335%40sss.pgh.pa.us
Attachments:
[text/x-patch] v2-0001-RelationTruncate-must-set-DELAY_CHKPT_START.patch (3.9K, ../../CA+hUKG+5nfWcpnZ=Z=UpGvY1tTF=4QU_0U_07EFaKmH7Nr+NLQ@mail.gmail.com/2-v2-0001-RelationTruncate-must-set-DELAY_CHKPT_START.patch)
download | inline diff:
From 9990dccdabf854de04a1aa05fe39b727d3ad0fe7 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 19 Oct 2023 15:12:52 -0400
Subject: [PATCH v2 1/2] RelationTruncate() must set DELAY_CHKPT_START.
Previously, it set only DELAY_CHKPT_COMPLETE. That was important,
because it meant that if the XLOG_SMGR_TRUNCATE record preceded a
XLOG_CHECKPOINT_ONLINE record in the WAL, then the truncation would also
happen on disk before the XLOG_CHECKPOINT_ONLINE record was
written.
However, it didn't guarantee that the sync request for the
truncation got processed before the XLOG_CHECKPOINT_ONLINE record
was written. By setting XLOG_CHKPT_START, we guarantee that if
an XLOG_SMGR_TRUNCATE record is written to WAL before the redo
pointer of some concurrent checkpoint, the sync request queued by
that operation must be processed by that checkpoint (rather than
being left for the following one).
Author: Robert Haas <robertmhaas@gmail.com>
Reported-by: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKG%2B-2rjGZC2kwqr2NMLBcEBp4uf59QT1advbWYF_uc%2B0Aw%40mail.gmail.com
---
src/backend/catalog/storage.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index f56b3cc0f23..bb8c4d15612 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -337,20 +337,33 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
RelationPreTruncate(rel);
/*
- * Make sure that a concurrent checkpoint can't complete while truncation
- * is in progress.
+ * The code which follows can interact with concurrent checkpoints in two
+ * separate ways.
*
- * The truncation operation might drop buffers that the checkpoint
+ * First, the truncation operation might drop buffers that the checkpoint
* otherwise would have flushed. If it does, then it's essential that the
* files actually get truncated on disk before the checkpoint record is
* written. Otherwise, if reply begins from that checkpoint, the
* to-be-truncated blocks might still exist on disk but have older
* contents than expected, which can cause replay to fail. It's OK for the
* blocks to not exist on disk at all, but not for them to have the wrong
- * contents.
+ * contents. For this reason, we need to set DELAY_CHKPT_COMPLETE while
+ * this code executes.
+ *
+ * Second, the call to smgrtruncate() below will in turn call
+ * RegisterSyncRequest(). We need the sync request created by that call to
+ * be processed before the checkpoint completes. CheckPointGuts() will
+ * call ProcessSyncRequests(), but if we register our sync request after
+ * that happens, then the WAL record for the truncation could end up
+ * preceding the checkpoint record, while the actual sync doesn't happen
+ * until the next checkpoint. To prevent that, we need to set
+ * DELAY_CHKPT_START here. That way, if the XLOG_SMGR_TRUNCATE precedes
+ * the redo pointer of a concurrent checkpoint, we're guaranteed that the
+ * corresponding sync request will be processed before the checkpoint
+ * completes.
*/
- Assert((MyProc->delayChkptFlags & DELAY_CHKPT_COMPLETE) == 0);
- MyProc->delayChkptFlags |= DELAY_CHKPT_COMPLETE;
+ Assert((MyProc->delayChkptFlags & (DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE)) == 0);
+ MyProc->delayChkptFlags |= DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE;
/*
* We WAL-log the truncation before actually truncating, which means
@@ -398,7 +411,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks);
/* We've done all the critical work, so checkpoints are OK now. */
- MyProc->delayChkptFlags &= ~DELAY_CHKPT_COMPLETE;
+ MyProc->delayChkptFlags &= ~(DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE);
/*
* Update upper-level FSM pages to account for the truncation. This is
--
2.44.0
[text/x-patch] v2-0002-RelationTruncate-must-use-a-critical-section.patch (16.1K, ../../CA+hUKG+5nfWcpnZ=Z=UpGvY1tTF=4QU_0U_07EFaKmH7Nr+NLQ@mail.gmail.com/3-v2-0002-RelationTruncate-must-use-a-critical-section.patch)
download | inline diff:
From 790dc839ac0d3c48ebd10c7a50f3b84cf8ed4fd0 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Sun, 21 Apr 2024 09:33:35 +1200
Subject: [PATCH v2 2/2] RelationTruncate() must use a critical section.
RelationTruncate() does three things, while holding an
AccessExclusiveLock and preventing checkpoints:
1. Logs the truncation.
2. Drops buffers, even if they're dirty.
3. Truncates files, possibly more than one.
Step 3 might fail with an operating system error, but we're already
committed to the operation at this point.
Previously the theory expressed in comments was that crash recovery
would surely fail to truncate again, so we should avoid a PANIC loop.
That didn't account for the seriousness of the failure mode: dead tuples
could come back to life, because we threw away needed dirty buffers, and
replicas could PANIC, because they replayed the truncation but then
later they might see references to blocks they consider to be past the
end.
This commit wraps the whole operation in a critical section, so that any
error triggers a PANIC. In order to make that possible, it provides
smgrpreparetruncate() so that smgrtruncate() can promise not to call
palloc(). Otherwise, the rule about not allocating memory in a critical
section would be broken. This in turn requires teaching relpath.c to
build paths in place. In theory this introduces a new failure mode
where a path is detected as being too long for MAXPGPATH, but that was
already likely to break.
This addresses bug #18146, an ancient problem mostly affecting Windows
in practice, but also bug #18426 which revealed a new way to reach a
partially truncated state even on Unix since PostgreSQL 14. Commit
d8725104 made it possible for WaitIO() reached via DropRelationBuffers()
to be interrupted, but that's no longer possible as a side-effect of the
critical section.
This issue has been independently analyzed by a large number of people
and threads over the years, most recently Robert Haas and myself. Ideas
for how to fix with new buffer states have been proposed but that
wouldn't be back-patchable.
XXX Seems a bit much to back-patch!
Discussion: https://postgr.es/m/18146-04e908c662113ad5%40postgresql.org
Discussion: https://postgr.es/m/2348.1544474335@sss.pgh.pa.us
Discussion: https://postgr.es/m/5BBC590AE8DF4ED1A170E4D48F1B53AC%40tunaPC
---
src/backend/catalog/storage.c | 28 +++++++++---
src/backend/storage/smgr/md.c | 75 +++++++++++++++++++++------------
src/backend/storage/smgr/smgr.c | 12 ++++++
src/common/relpath.c | 57 +++++++++++++++++++------
src/include/common/relpath.h | 3 ++
src/include/storage/smgr.h | 1 +
6 files changed, 132 insertions(+), 44 deletions(-)
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index bb8c4d15612..dabaa48a57b 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -308,6 +308,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
forks[nforks] = MAIN_FORKNUM;
blocks[nforks] = nblocks;
nforks++;
+ smgrpreparetruncate(reln, MAIN_FORKNUM);
/* Prepare for truncation of the FSM if it exists */
fsm = smgrexists(RelationGetSmgr(rel), FSM_FORKNUM);
@@ -320,6 +321,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
nforks++;
need_fsm_vacuum = true;
}
+ smgrpreparetruncate(reln, FSM_FORKNUM);
}
/* Prepare for truncation of the visibility map too if it exists */
@@ -332,6 +334,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
forks[nforks] = VISIBILITYMAP_FORKNUM;
nforks++;
}
+ smgrpreparetruncate(reln, VISIBILITYMAP_FORKNUM);
}
RelationPreTruncate(rel);
@@ -368,12 +371,25 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
/*
* We WAL-log the truncation before actually truncating, which means
* trouble if the truncation fails. If we then crash, the WAL replay
- * likely isn't going to succeed in the truncation either, and cause a
- * PANIC. It's tempting to put a critical section here, but that cure
- * would be worse than the disease. It would turn a usually harmless
- * failure to truncate, that might spell trouble at WAL replay, into a
- * certain PANIC.
+ * likely isn't going to succeed in the truncation either, so it's
+ * tempting not to put a critical section here to avoid a double PANIC.
+ * However, if the file system operation failed after
+ * DropRelationBuffers() had already thrown away dirty buffers, (1) old
+ * deleted data might later come back to life, and (2) would-be-truncated
+ * pages might be modified again, and then a replica that had replayed the
+ * truncation would PANIC because it has a different idea of the relation
+ * size.
+ *
+ * The critical section also makes WaitIO() non-interruptable in
+ * DropRelationBuffers(), which would otherwise be another way to reach
+ * the above problems.
+ *
+ * Note that we called smgrpreparetruncate() above, to allow
+ * smgrtruncate() to be called within a critical section without
+ * allocating memory.
*/
+ START_CRIT_SECTION();
+
if (RelationNeedsWAL(rel))
{
/*
@@ -410,6 +426,8 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
*/
smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks);
+ END_CRIT_SECTION();
+
/* We've done all the critical work, so checkpoints are OK now. */
MyProc->delayChkptFlags &= ~(DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE);
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index bf0f3ca76d1..139cb217cc2 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -117,6 +117,12 @@ static MemoryContext MdCxt; /* context for all MdfdVec objects */
/* don't try to open a segment, if not already open */
#define EXTENSION_DONT_OPEN (1 << 5)
+/*
+ * The maximum possible size of the .NNN extension for segments is 1 for the
+ * dot plus log10(2^32) for the digits for RELSEG_SIZE = 1, and it's not worth
+ * working harder to compute the value for more typical RELSEG_SIZE values.
+ */
+#define MAX_EXTENSION_SIZE (1 + 10)
/* local routines */
static void mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum,
@@ -131,7 +137,7 @@ static void register_forget_request(RelFileLocatorBackend rlocator, ForkNumber f
static void _fdvec_resize(SMgrRelation reln,
ForkNumber forknum,
int nseg);
-static char *_mdfd_segpath(SMgrRelation reln, ForkNumber forknum,
+static char *_mdfd_segpath(char *path, SMgrRelation reln, ForkNumber forknum,
BlockNumber segno);
static MdfdVec *_mdfd_openseg(SMgrRelation reln, ForkNumber forknum,
BlockNumber segno, int oflags);
@@ -408,7 +414,8 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo)
*/
if (ret >= 0 || errno != ENOENT)
{
- char *segpath = (char *) palloc(strlen(path) + 12);
+ char *segpath = (char *) palloc(strlen(path) +
+ MAX_EXTENSION_SIZE + 1);
BlockNumber segno;
for (segno = 1;; segno++)
@@ -1492,7 +1499,7 @@ _fdvec_resize(SMgrRelation reln,
reln->md_seg_fds[forknum] =
MemoryContextAlloc(MdCxt, sizeof(MdfdVec) * nseg);
}
- else
+ else if (nseg > reln->md_num_open_segs[forknum])
{
/*
* It doesn't seem worthwhile complicating the code to amortize
@@ -1504,31 +1511,50 @@ _fdvec_resize(SMgrRelation reln,
repalloc(reln->md_seg_fds[forknum],
sizeof(MdfdVec) * nseg);
}
+ else
+ {
+ /*
+ * We don't reallocate a smaller array, because we want
+ * smgrpreparetruncate() to be able to promise that smgrtruncate()
+ * won't call memory allocation functions that would fail in a
+ * critical section. This means that a bit of space in the array is
+ * now wasted, until the next time we add a segment and reallocate.
+ */
+ }
reln->md_num_open_segs[forknum] = nseg;
}
/*
- * Return the filename for the specified segment of the relation. The
- * returned string is palloc'd.
+ * Return the filename for the specified segment of the relation. The output
+ * buffer must be of size MAXPGPATH.
*/
static char *
-_mdfd_segpath(SMgrRelation reln, ForkNumber forknum, BlockNumber segno)
+_mdfd_segpath(char *path, SMgrRelation reln, ForkNumber forknum, BlockNumber segno)
{
- char *path,
- *fullpath;
+ size_t size;
- path = relpath(reln->smgr_rlocator, forknum);
+ size = GetRelationPathInPlace(path,
+ reln->smgr_rlocator.locator.dbOid,
+ reln->smgr_rlocator.locator.spcOid,
+ reln->smgr_rlocator.locator.relNumber,
+ reln->smgr_rlocator.backend,
+ forknum);
+ /*
+ * Make sure the whole path, maximum possible segment suffix and NUL
+ * terminator can fit into MAXPGPATH.
+ */
+ if (size + MAX_EXTENSION_SIZE >= MAXPGPATH)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("path \"%s\" too long", path)));
+
+ /* Append segment number. */
if (segno > 0)
- {
- fullpath = psprintf("%s.%u", path, segno);
- pfree(path);
- }
- else
- fullpath = path;
+ sprintf(path + size, ".%u", segno);
- return fullpath;
+ return path;
}
/*
@@ -1541,15 +1567,13 @@ _mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno,
{
MdfdVec *v;
File fd;
- char *fullpath;
+ char fullpath[MAXPGPATH];
- fullpath = _mdfd_segpath(reln, forknum, segno);
+ _mdfd_segpath(fullpath, reln, forknum, segno);
/* open the file */
fd = PathNameOpenFile(fullpath, _mdfd_open_flags() | oflags);
- pfree(fullpath);
-
if (fd < 0)
return NULL;
@@ -1584,6 +1608,7 @@ static MdfdVec *
_mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
bool skipFsync, int behavior)
{
+ char path[MAXPGPATH];
MdfdVec *v;
BlockNumber targetseg;
BlockNumber nextsegno;
@@ -1686,7 +1711,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\" (target block %u): previous segment is only %u blocks",
- _mdfd_segpath(reln, forknum, nextsegno),
+ _mdfd_segpath(path, reln, forknum, nextsegno),
blkno, nblocks)));
}
@@ -1700,7 +1725,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\" (target block %u): %m",
- _mdfd_segpath(reln, forknum, nextsegno),
+ _mdfd_segpath(path, reln, forknum, nextsegno),
blkno)));
}
}
@@ -1751,11 +1776,7 @@ mdsyncfiletag(const FileTag *ftag, char *path)
}
else
{
- char *p;
-
- p = _mdfd_segpath(reln, ftag->forknum, ftag->segno);
- strlcpy(path, p, MAXPGPATH);
- pfree(p);
+ _mdfd_segpath(path, reln, ftag->forknum, ftag->segno);
file = PathNameOpenFile(path, _mdfd_open_flags());
if (file < 0)
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 100f6454e53..78f562034ec 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -689,6 +689,18 @@ smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum)
return InvalidBlockNumber;
}
+/*
+ * smgrpreparetruncate() -- Prepare to truncate a fork of a relation.
+ *
+ * This promises that a later call to smgrtruncate() will not have to allocate
+ * memory from MemoryContexts that don't allow that inside critical sections.
+ */
+void
+smgrpreparetruncate(SMgrRelation reln, ForkNumber forknum)
+{
+ smgrnblocks(reln, forknum);
+}
+
/*
* smgrtruncate() -- Truncate the given forks of supplied relation to
* each specified numbers of blocks
diff --git a/src/common/relpath.c b/src/common/relpath.c
index f54c36ef7ac..7974a9cb0ee 100644
--- a/src/common/relpath.c
+++ b/src/common/relpath.c
@@ -141,7 +141,32 @@ char *
GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
int procNumber, ForkNumber forkNumber)
{
- char *path;
+ char path[MAXPGPATH];
+ char *result;
+ size_t size;
+
+ size = GetRelationPathInPlace(path, dbOid, spcOid, relNumber, procNumber, forkNumber);
+
+ /* String plus NUL terminator. */
+ result = palloc(size + 1);
+ memcpy(result, path, size + 1);
+
+ return result;
+}
+
+/*
+ * GetRelationPathInPlace - construct path to a relation's file
+ *
+ * Result is written to path, which must have space for MAXPGPATH characters.
+ * The size of the string is returned. If it is >= MAXPGPATH, the path has
+ * been truncated due to lack of space.
+ */
+size_t
+GetRelationPathInPlace(char *path,
+ Oid dbOid, Oid spcOid, RelFileNumber relNumber,
+ int procNumber, ForkNumber forkNumber)
+{
+ size_t size;
if (spcOid == GLOBALTABLESPACE_OID)
{
@@ -149,10 +174,10 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
Assert(dbOid == 0);
Assert(procNumber == INVALID_PROC_NUMBER);
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("global/%u_%s",
+ size = snprintf(path, MAXPGPATH, "global/%u_%s",
relNumber, forkNames[forkNumber]);
else
- path = psprintf("global/%u", relNumber);
+ size = snprintf(path, MAXPGPATH, "global/%u", relNumber);
}
else if (spcOid == DEFAULTTABLESPACE_OID)
{
@@ -160,21 +185,25 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
if (procNumber == INVALID_PROC_NUMBER)
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("base/%u/%u_%s",
+ size = snprintf(path, MAXPGPATH,
+ "base/%u/%u_%s",
dbOid, relNumber,
forkNames[forkNumber]);
else
- path = psprintf("base/%u/%u",
+ size = snprintf(path, MAXPGPATH,
+ "base/%u/%u",
dbOid, relNumber);
}
else
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("base/%u/t%d_%u_%s",
+ size = snprintf(path, MAXPGPATH,
+ "base/%u/t%d_%u_%s",
dbOid, procNumber, relNumber,
forkNames[forkNumber]);
else
- path = psprintf("base/%u/t%d_%u",
+ size = snprintf(path, MAXPGPATH,
+ "base/%u/t%d_%u",
dbOid, procNumber, relNumber);
}
}
@@ -184,27 +213,31 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
if (procNumber == INVALID_PROC_NUMBER)
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("pg_tblspc/%u/%s/%u/%u_%s",
+ size = snprintf(path, MAXPGPATH,
+ "pg_tblspc/%u/%s/%u/%u_%s",
spcOid, TABLESPACE_VERSION_DIRECTORY,
dbOid, relNumber,
forkNames[forkNumber]);
else
- path = psprintf("pg_tblspc/%u/%s/%u/%u",
+ size = snprintf(path, MAXPGPATH,
+ "pg_tblspc/%u/%s/%u/%u",
spcOid, TABLESPACE_VERSION_DIRECTORY,
dbOid, relNumber);
}
else
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("pg_tblspc/%u/%s/%u/t%d_%u_%s",
+ size = snprintf(path, MAXPGPATH,
+ "pg_tblspc/%u/%s/%u/t%d_%u_%s",
spcOid, TABLESPACE_VERSION_DIRECTORY,
dbOid, procNumber, relNumber,
forkNames[forkNumber]);
else
- path = psprintf("pg_tblspc/%u/%s/%u/t%d_%u",
+ size = snprintf(path, MAXPGPATH,
+ "pg_tblspc/%u/%s/%u/t%d_%u",
spcOid, TABLESPACE_VERSION_DIRECTORY,
dbOid, procNumber, relNumber);
}
}
- return path;
+ return size;
}
diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
index 6f006d5a938..dfab46c0037 100644
--- a/src/include/common/relpath.h
+++ b/src/include/common/relpath.h
@@ -75,6 +75,9 @@ extern char *GetDatabasePath(Oid dbOid, Oid spcOid);
extern char *GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
int procNumber, ForkNumber forkNumber);
+extern size_t GetRelationPathInPlace(char *path,
+ Oid dbOid, Oid spcOid, RelFileNumber relNumber,
+ int procNumber, ForkNumber forkNumber);
/*
* Wrapper macros for GetRelationPath. Beware of multiple
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index fc5f883ce14..77974b0f36f 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -103,6 +103,7 @@ extern void smgrwriteback(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, BlockNumber nblocks);
extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum);
extern BlockNumber smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum);
+extern void smgrpreparetruncate(SMgrRelation reln, ForkNumber forknum);
extern void smgrtruncate(SMgrRelation reln, ForkNumber *forknum,
int nforks, BlockNumber *nblocks);
extern void smgrimmedsync(SMgrRelation reln, ForkNumber forknum);
--
2.44.0
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-05-01 05:00 Michael Paquier <michael@paquier.xyz>
parent: Thomas Munro <thomas.munro@gmail.com>
1 sibling, 0 replies; 45+ messages in thread
From: Michael Paquier @ 2024-05-01 05:00 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, Apr 23, 2024 at 07:48:12PM +1200, Thomas Munro wrote:
> Here is a new attempt to see what it might take to put
> RelationTruncate() into a critical section. Problems encountered:
> even if you've called mdnblocks() beforehand, dressed up as
> smgrpreparetruncate(), if the highest segment is exactly full then the
> later mdnblocks() again probes whether the next segment number exists
> on disk, which involves GetRelationPath(), which allocates. So I
> finished up having to write a GetRelationPathInPlace() function, and
> to decide whether it's OK to use a MAXPGPATH-sized array on the stack
> for this. I also had to teach _fdvec_resize() not to reallocate when
> downsizing, to avoid the critical section assertion. It seems like
> quite a lot to back-patch... but also awful to leave this trickle of
> data corruption reports unaddressed. The big re-engineering ideas[1]
> would be absolutely unbackpatchable, but I hope we can work on
> something like that for 18...
For now I'd agree with something like that, even for a backpatch. No
idea what a good "cheap" solution would look like, though. Anything
considered these past years for this issue was utterly expensive.
> However, it didn't guarantee that the sync request for the
> truncation got processed before the XLOG_CHECKPOINT_ONLINE record
> was written. By setting XLOG_CHKPT_START, we guarantee that if
> an XLOG_SMGR_TRUNCATE record is written to WAL before the redo
> pointer of some concurrent checkpoint, the sync request queued by
> that operation must be processed by that checkpoint (rather than
> being left for the following one).
s/XLOG_CHKPT_START/DELAY_CHKPT_START/ in the commit message of 0001.
The point you are making about d8725104 seems like a good argument to
do a backpatch down to v14, as the possible interrupt make this setof
issues much easier to reach compared to the Windows-only-weird-syscall
failures happening in the middle of the truncation that people have
been complaining about for N years now. I am seeing less reports of
these on WIN32 these days with files switched suddenly to read-only,
or is that just my imagination?
The pieces around GetRelationPathInPlace() and MAX_EXTENSION_SIZE
could be a refactoring piece worth their own, split into its own patch
before introducing the core part of the fix with the critical section.
That would be much cleaner, IMO.
It seems to me that an assertion at the end of
GetRelationPathInPlace() about the computed size would be adapted
based on MAXPGPATH. The interface of _mdfd_segpath() is quite
confusing, actually. Why return the same pointer as the input
argument rather than void?
I was re-reading your comment about last October, and the fact that we
may stuck replay, and finishing with semi-corrupted undetectible
corruptions is worse than that, still I'm going to agree that failing
harder will serve much better at this stage:
https://www.postgresql.org/message-id/CA%2BhUKGJy9iCBfkjUyV8ZuRwd5CAGxZV1STywe%2B0S%2B9YKH1zF8w%40ma...
That may impact availability. Still that's cheaper than any other
solution discussed. The one involving the WAL-logging of dirty pages
that would get truncated shortly after comes first into mind..
PS: On HEAD, I'd like to see one or more tests for these failure
scenarios in the long-term and how replay is able to react after
panicking in the critical section. Now, injection points won't work
inside a critical section as the initial function loading needs to do
a palloc() for the library path. We've discussed about having a
PRELOAD macro that could be called outside the critical path, before
running a INJECTION_POINT. Just mentioning.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ZjHMcwiltqYXsKYb@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-05-14 13:00 Alexander Lakhin <exclusion@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
1 sibling, 1 reply; 45+ messages in thread
From: Alexander Lakhin @ 2024-05-14 13:00 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
Hello Thomas,
23.04.2024 10:48, Thomas Munro wrote:
> Related bug #18426 sent me back here.
>
> Here is a new attempt to see what it might take to put
> RelationTruncate() into a critical section.
When running 027_stream_regress on a slow machine with the aggressive
autovacuum settings, having those patches applied, I've stumbled upon:
TRAP: failed Assert("CritSectionCount == 0 || (context)->allowInCritSection"), File: "mcxt.c", Line: 1353, PID: 24468
...
2024-05-14 12:30:03.542 UTC [22964:4] LOG: server process (PID 24468) was terminated by signal 6: Aborted
2024-05-14 12:30:03.542 UTC [22964:5] DETAIL: Failed process was running: autovacuum: VACUUM ANALYZE pg_catalog.pg_class
with the following stack trace:
Core was generated by `postgres: primary: autovacuum worker regression '.
Program terminated with signal SIGABRT, Aborted.
#0 __libc_do_syscall () at ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47
(gdb) bt
#0 __libc_do_syscall () at ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47
#1 0xb63c90ae in __libc_signal_restore_set (set=0xbe99f34c) at ../sysdeps/unix/sysv/linux/internal-signals.h:84
#2 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:48
#3 0xb63bb1f2 in __GI_abort () at abort.c:79
#4 0xb6d3e2d0 in ExceptionalCondition (conditionName=<optimized out>, fileName=<optimized out>,
lineNumber=lineNumber@entry=1353) at assert.c:66
#5 0xb6d61834 in palloc0 (size=3062661664) at mcxt.c:1353
#6 0xb6bcd304 in CompactCheckpointerRequestQueue () at checkpointer.c:1173
#7 ForwardSyncRequest (ftag=ftag@entry=0xbe99f7d0, type=type@entry=SYNC_REQUEST) at checkpointer.c:1113
#8 0xb6c4b3c4 in RegisterSyncRequest (ftag=ftag@entry=0xbe99f7d0, type=type@entry=SYNC_REQUEST,
retryOnError=retryOnError@entry=false) at sync.c:605
#9 0xb6c48e62 in register_dirty_segment (reln=reln@entry=0xb8ec75a8, forknum=forknum@entry=MAIN_FORKNUM, seg=<optimized
out>, seg=<optimized out>) at md.c:1369
#10 0xb6c4a0c6 in mdtruncate (reln=0xb8ec75a8, forknum=MAIN_FORKNUM, nblocks=45) at md.c:1229
#11 0xb6c4abe8 in smgrtruncate (reln=0xb8ec75a8, forknum=forknum@entry=0xbe99f86c, nforks=nforks@entry=2,
nblocks=nblocks@entry=0xbe99f878) at smgr.c:743
#12 0xb6a3e8c6 in RelationTruncate (rel=0xb2e969a0, nblocks=nblocks@entry=45) at ../../../src/include/utils/rel.h:574
#13 0xb69b8042 in lazy_truncate_heap (vacrel=0xb8ee8f38) at vacuumlazy.c:2642
...
Best regards,
Alexander
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-06-26 20:58 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Heikki Linnakangas @ 2024-06-26 20:58 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; Thomas Munro <thomas.munro@gmail.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On 14/05/2024 16:00, Alexander Lakhin wrote:
> 23.04.2024 10:48, Thomas Munro wrote:
>> Here is a new attempt to see what it might take to put
>> RelationTruncate() into a critical section.
>
> When running 027_stream_regress on a slow machine with the aggressive
> autovacuum settings, having those patches applied, I've stumbled upon:
> TRAP: failed Assert("CritSectionCount == 0 || (context)->allowInCritSection"), File: "mcxt.c", Line: 1353, PID: 24468
> ...
> 2024-05-14 12:30:03.542 UTC [22964:4] LOG: server process (PID 24468) was terminated by signal 6: Aborted
> 2024-05-14 12:30:03.542 UTC [22964:5] DETAIL: Failed process was running: autovacuum: VACUUM ANALYZE pg_catalog.pg_class
>
> with the following stack trace:
> Core was generated by `postgres: primary: autovacuum worker regression '.
> Program terminated with signal SIGABRT, Aborted.
> #0 __libc_do_syscall () at ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47
> (gdb) bt
> #0 __libc_do_syscall () at ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47
> #1 0xb63c90ae in __libc_signal_restore_set (set=0xbe99f34c) at ../sysdeps/unix/sysv/linux/internal-signals.h:84
> #2 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:48
> #3 0xb63bb1f2 in __GI_abort () at abort.c:79
> #4 0xb6d3e2d0 in ExceptionalCondition (conditionName=<optimized out>, fileName=<optimized out>,
> lineNumber=lineNumber@entry=1353) at assert.c:66
> #5 0xb6d61834 in palloc0 (size=3062661664) at mcxt.c:1353
> #6 0xb6bcd304 in CompactCheckpointerRequestQueue () at checkpointer.c:1173
> #7 ForwardSyncRequest (ftag=ftag@entry=0xbe99f7d0, type=type@entry=SYNC_REQUEST) at checkpointer.c:1113
> #8 0xb6c4b3c4 in RegisterSyncRequest (ftag=ftag@entry=0xbe99f7d0, type=type@entry=SYNC_REQUEST,
> retryOnError=retryOnError@entry=false) at sync.c:605
> #9 0xb6c48e62 in register_dirty_segment (reln=reln@entry=0xb8ec75a8, forknum=forknum@entry=MAIN_FORKNUM, seg=<optimized
> out>, seg=<optimized out>) at md.c:1369
> #10 0xb6c4a0c6 in mdtruncate (reln=0xb8ec75a8, forknum=MAIN_FORKNUM, nblocks=45) at md.c:1229
> #11 0xb6c4abe8 in smgrtruncate (reln=0xb8ec75a8, forknum=forknum@entry=0xbe99f86c, nforks=nforks@entry=2,
> nblocks=nblocks@entry=0xbe99f878) at smgr.c:743
> #12 0xb6a3e8c6 in RelationTruncate (rel=0xb2e969a0, nblocks=nblocks@entry=45) at ../../../src/include/utils/rel.h:574
> #13 0xb69b8042 in lazy_truncate_heap (vacrel=0xb8ee8f38) at vacuumlazy.c:2642
> ...
This should've also been fixed by commit b1ffe3ff0b.
Thanks Alexander for pointing me to this thread!
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-06-26 20:59 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 45+ messages in thread
From: Heikki Linnakangas @ 2024-06-26 20:59 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; Thomas Munro <thomas.munro@gmail.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On 26/06/2024 23:58, Heikki Linnakangas wrote:
> On 14/05/2024 16:00, Alexander Lakhin wrote:
>> 23.04.2024 10:48, Thomas Munro wrote:
>>> Here is a new attempt to see what it might take to put
>>> RelationTruncate() into a critical section.
>>
>> When running 027_stream_regress on a slow machine with the aggressive
>> autovacuum settings, having those patches applied, I've stumbled upon:
>> TRAP: failed Assert("CritSectionCount == 0 || (context)->allowInCritSection"), File: "mcxt.c", Line: 1353, PID: 24468
>> ...
>> 2024-05-14 12:30:03.542 UTC [22964:4] LOG: server process (PID 24468) was terminated by signal 6: Aborted
>> 2024-05-14 12:30:03.542 UTC [22964:5] DETAIL: Failed process was running: autovacuum: VACUUM ANALYZE pg_catalog.pg_class
>>
>> with the following stack trace:
>> Core was generated by `postgres: primary: autovacuum worker regression '.
>> Program terminated with signal SIGABRT, Aborted.
>> #0 __libc_do_syscall () at ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47
>> (gdb) bt
>> #0 __libc_do_syscall () at ../sysdeps/unix/sysv/linux/arm/libc-do-syscall.S:47
>> #1 0xb63c90ae in __libc_signal_restore_set (set=0xbe99f34c) at ../sysdeps/unix/sysv/linux/internal-signals.h:84
>> #2 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:48
>> #3 0xb63bb1f2 in __GI_abort () at abort.c:79
>> #4 0xb6d3e2d0 in ExceptionalCondition (conditionName=<optimized out>, fileName=<optimized out>,
>> lineNumber=lineNumber@entry=1353) at assert.c:66
>> #5 0xb6d61834 in palloc0 (size=3062661664) at mcxt.c:1353
>> #6 0xb6bcd304 in CompactCheckpointerRequestQueue () at checkpointer.c:1173
>> #7 ForwardSyncRequest (ftag=ftag@entry=0xbe99f7d0, type=type@entry=SYNC_REQUEST) at checkpointer.c:1113
>> #8 0xb6c4b3c4 in RegisterSyncRequest (ftag=ftag@entry=0xbe99f7d0, type=type@entry=SYNC_REQUEST,
>> retryOnError=retryOnError@entry=false) at sync.c:605
>> #9 0xb6c48e62 in register_dirty_segment (reln=reln@entry=0xb8ec75a8, forknum=forknum@entry=MAIN_FORKNUM, seg=<optimized
>> out>, seg=<optimized out>) at md.c:1369
>> #10 0xb6c4a0c6 in mdtruncate (reln=0xb8ec75a8, forknum=MAIN_FORKNUM, nblocks=45) at md.c:1229
>> #11 0xb6c4abe8 in smgrtruncate (reln=0xb8ec75a8, forknum=forknum@entry=0xbe99f86c, nforks=nforks@entry=2,
>> nblocks=nblocks@entry=0xbe99f878) at smgr.c:743
>> #12 0xb6a3e8c6 in RelationTruncate (rel=0xb2e969a0, nblocks=nblocks@entry=45) at ../../../src/include/utils/rel.h:574
>> #13 0xb69b8042 in lazy_truncate_heap (vacrel=0xb8ee8f38) at vacuumlazy.c:2642
>> ...
>
> This should've also been fixed by commit b1ffe3ff0b.
To clarify commit b1ffe3ff0b only fixed this assertion failure, if you
call RelationTruncate in a critical section, like in with this patch.
Not the original issue.
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-09-05 23:05 Thomas Munro <thomas.munro@gmail.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 2 replies; 45+ messages in thread
From: Thomas Munro @ 2024-09-05 23:05 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Jun 27, 2024 at 8:59 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 26/06/2024 23:58, Heikki Linnakangas wrote:
> > On 14/05/2024 16:00, Alexander Lakhin wrote:
> >> 23.04.2024 10:48, Thomas Munro wrote:
> >>> Here is a new attempt to see what it might take to put
> >>> RelationTruncate() into a critical section.
> >>
> >> When running 027_stream_regress on a slow machine with the aggressive
> >> autovacuum settings, having those patches applied, I've stumbled upon:
> >> TRAP: failed Assert("CritSectionCount == 0 || (context)->allowInCritSection"), File: "mcxt.c", Line: 1353, PID: 24468
> > This should've also been fixed by commit b1ffe3ff0b.
>
> To clarify commit b1ffe3ff0b only fixed this assertion failure, if you
> call RelationTruncate in a critical section, like in with this patch.
> Not the original issue.
Thanks Alexander and Heikki.
I have rebased these patches over c7cd2d6e, which introduced the
PG_TBLSPC_DIR macro used in path construction. I added them to the
commitfest so we don't lose track of them, and applied two changes
requested by Michael upthread: a thinko in a commit message, and I
split the GetRelationPathInPlace() function into its own patch.
Andres and Noah are discussing new ways to solve the
can't-call-palloc-in-critical-section problem[1], but if we want any
chance to be able to back-patch *this* fix, then I think we need to
invent GetRelationPathInPlace() anyway, no?
I realised that my earlier speculation about Windows'
ERROR_USER_MAPPED_FILE was bogus, because that'd be translated to
EINVAL, and here we have EACCES ("Permission denied"). I had been
looking for explanations for just ftruncate() on its own to fail,
thinking that the file was already open, but I had forgotten that
FileTruncate() might need to reopen the file in the vfd layer. That
doesn't require any exotic new explanations: it'd fail like that if
programs unknown had opened the file without the FILE_SHARE_XXX flags,
and our pgwin32_open() kludge failed to open the file after 50 sleep
retry loops.
But let's not forget that these patches also fix two bugs that apply
to Unix too.
For the DELAY_CHKPT_START bug, we should back-patch all the way.
For the WaitIO() bug affecting all OSes, that only needs to go back to
14. We could also opt to be cautious and let it run on master for a
while before we do that, though.
For ftruncate() failure, I wouldn't be too bothered if we just let
sleeping dogs lie in 13. It affects only systems that have serious
file system corruption, or Windows systems that have something
snooping on private files with antisocial flags.
[1] https://www.postgresql.org/message-id/flat/h3a7ftrxypgxbw6ukcrrkspjon5dlninedwb5udkrase3rgqvn%403cok...
Attachments:
[text/x-patch] v3-0001-RelationTruncate-must-set-DELAY_CHKPT_START.patch (3.9K, ../../CA+hUKG+g8ydXzSnHQPtNhmwNhn8A-FborSZGSLg62tivaugP0g@mail.gmail.com/2-v3-0001-RelationTruncate-must-set-DELAY_CHKPT_START.patch)
download | inline diff:
From 92ea02358efe6d7a915b63cd6ec6796f2ebb490d Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 19 Oct 2023 15:12:52 -0400
Subject: [PATCH v3 1/3] RelationTruncate() must set DELAY_CHKPT_START.
Previously, it set only DELAY_CHKPT_COMPLETE. That was important,
because it meant that if the XLOG_SMGR_TRUNCATE record preceded a
XLOG_CHECKPOINT_ONLINE record in the WAL, then the truncation would also
happen on disk before the XLOG_CHECKPOINT_ONLINE record was
written.
However, it didn't guarantee that the sync request for the truncation
got processed before the XLOG_CHECKPOINT_ONLINE record was written. By
setting DELAY_CHKPT_START, we guarantee that if an XLOG_SMGR_TRUNCATE
record is written to WAL before the redo pointer of some concurrent
checkpoint, the sync request queued by that operation must be processed
by that checkpoint (rather than being left for the following one).
Author: Robert Haas <robertmhaas@gmail.com>
Reported-by: Thomas Munro <thomas.munro@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKG%2B-2rjGZC2kwqr2NMLBcEBp4uf59QT1advbWYF_uc%2B0Aw%40mail.gmail.com
---
src/backend/catalog/storage.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index f56b3cc0f23..bb8c4d15612 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -337,20 +337,33 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
RelationPreTruncate(rel);
/*
- * Make sure that a concurrent checkpoint can't complete while truncation
- * is in progress.
+ * The code which follows can interact with concurrent checkpoints in two
+ * separate ways.
*
- * The truncation operation might drop buffers that the checkpoint
+ * First, the truncation operation might drop buffers that the checkpoint
* otherwise would have flushed. If it does, then it's essential that the
* files actually get truncated on disk before the checkpoint record is
* written. Otherwise, if reply begins from that checkpoint, the
* to-be-truncated blocks might still exist on disk but have older
* contents than expected, which can cause replay to fail. It's OK for the
* blocks to not exist on disk at all, but not for them to have the wrong
- * contents.
+ * contents. For this reason, we need to set DELAY_CHKPT_COMPLETE while
+ * this code executes.
+ *
+ * Second, the call to smgrtruncate() below will in turn call
+ * RegisterSyncRequest(). We need the sync request created by that call to
+ * be processed before the checkpoint completes. CheckPointGuts() will
+ * call ProcessSyncRequests(), but if we register our sync request after
+ * that happens, then the WAL record for the truncation could end up
+ * preceding the checkpoint record, while the actual sync doesn't happen
+ * until the next checkpoint. To prevent that, we need to set
+ * DELAY_CHKPT_START here. That way, if the XLOG_SMGR_TRUNCATE precedes
+ * the redo pointer of a concurrent checkpoint, we're guaranteed that the
+ * corresponding sync request will be processed before the checkpoint
+ * completes.
*/
- Assert((MyProc->delayChkptFlags & DELAY_CHKPT_COMPLETE) == 0);
- MyProc->delayChkptFlags |= DELAY_CHKPT_COMPLETE;
+ Assert((MyProc->delayChkptFlags & (DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE)) == 0);
+ MyProc->delayChkptFlags |= DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE;
/*
* We WAL-log the truncation before actually truncating, which means
@@ -398,7 +411,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks);
/* We've done all the critical work, so checkpoints are OK now. */
- MyProc->delayChkptFlags &= ~DELAY_CHKPT_COMPLETE;
+ MyProc->delayChkptFlags &= ~(DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE);
/*
* Update upper-level FSM pages to account for the truncation. This is
--
2.46.0
[text/x-patch] v3-0002-Introduce-GetRelationPathInPlace.patch (4.9K, ../../CA+hUKG+g8ydXzSnHQPtNhmwNhn8A-FborSZGSLg62tivaugP0g@mail.gmail.com/3-v3-0002-Introduce-GetRelationPathInPlace.patch)
download | inline diff:
From 1ca15dcd52fd77a70a596a944becb62a366e6cbe Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Fri, 6 Sep 2024 10:32:24 +1200
Subject: [PATCH v3 2/3] Introduce GetRelationPathInPlace().
This is like GetRelationPath(), but writes the result into a
caller-supplied buffer. This variant can be used in a critical section,
because it doesn't allocate memory.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/18146-04e908c662113ad5%40postgresql.org
---
src/common/relpath.c | 57 ++++++++++++++++++++++++++++--------
src/include/common/relpath.h | 3 ++
2 files changed, 48 insertions(+), 12 deletions(-)
diff --git a/src/common/relpath.c b/src/common/relpath.c
index 9f2e00e83e4..f880ea4d0ba 100644
--- a/src/common/relpath.c
+++ b/src/common/relpath.c
@@ -142,7 +142,32 @@ char *
GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
int procNumber, ForkNumber forkNumber)
{
- char *path;
+ char path[MAXPGPATH];
+ char *result;
+ size_t size;
+
+ size = GetRelationPathInPlace(path, dbOid, spcOid, relNumber, procNumber, forkNumber);
+
+ /* String plus NUL terminator. */
+ result = palloc(size + 1);
+ memcpy(result, path, size + 1);
+
+ return result;
+}
+
+/*
+ * GetRelationPathInPlace - construct path to a relation's file
+ *
+ * Result is written to path, which must have space for MAXPGPATH characters.
+ * The size of the string is returned. If it is >= MAXPGPATH, the path has
+ * been truncated due to lack of space.
+ */
+size_t
+GetRelationPathInPlace(char *path,
+ Oid dbOid, Oid spcOid, RelFileNumber relNumber,
+ int procNumber, ForkNumber forkNumber)
+{
+ size_t size;
if (spcOid == GLOBALTABLESPACE_OID)
{
@@ -150,10 +175,10 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
Assert(dbOid == 0);
Assert(procNumber == INVALID_PROC_NUMBER);
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("global/%u_%s",
+ size = snprintf(path, MAXPGPATH, "global/%u_%s",
relNumber, forkNames[forkNumber]);
else
- path = psprintf("global/%u", relNumber);
+ size = snprintf(path, MAXPGPATH, "global/%u", relNumber);
}
else if (spcOid == DEFAULTTABLESPACE_OID)
{
@@ -161,21 +186,25 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
if (procNumber == INVALID_PROC_NUMBER)
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("base/%u/%u_%s",
+ size = snprintf(path, MAXPGPATH,
+ "base/%u/%u_%s",
dbOid, relNumber,
forkNames[forkNumber]);
else
- path = psprintf("base/%u/%u",
+ size = snprintf(path, MAXPGPATH,
+ "base/%u/%u",
dbOid, relNumber);
}
else
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("base/%u/t%d_%u_%s",
+ size = snprintf(path, MAXPGPATH,
+ "base/%u/t%d_%u_%s",
dbOid, procNumber, relNumber,
forkNames[forkNumber]);
else
- path = psprintf("base/%u/t%d_%u",
+ size = snprintf(path, MAXPGPATH,
+ "base/%u/t%d_%u",
dbOid, procNumber, relNumber);
}
}
@@ -185,13 +214,15 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
if (procNumber == INVALID_PROC_NUMBER)
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("%s/%u/%s/%u/%u_%s",
+ size = snprintf(path, MAXPGPATH,
+ "%s/%u/%s/%u/%u_%s",
PG_TBLSPC_DIR, spcOid,
TABLESPACE_VERSION_DIRECTORY,
dbOid, relNumber,
forkNames[forkNumber]);
else
- path = psprintf("%s/%u/%s/%u/%u",
+ size = snprintf(path, MAXPGPATH,
+ "%s/%u/%s/%u/%u",
PG_TBLSPC_DIR, spcOid,
TABLESPACE_VERSION_DIRECTORY,
dbOid, relNumber);
@@ -199,17 +230,19 @@ GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
else
{
if (forkNumber != MAIN_FORKNUM)
- path = psprintf("%s/%u/%s/%u/t%d_%u_%s",
+ size = snprintf(path, MAXPGPATH,
+ "%s/%u/%s/%u/t%d_%u_%s",
PG_TBLSPC_DIR, spcOid,
TABLESPACE_VERSION_DIRECTORY,
dbOid, procNumber, relNumber,
forkNames[forkNumber]);
else
- path = psprintf("%s/%u/%s/%u/t%d_%u",
+ size = snprintf(path, MAXPGPATH,
+ "%s/%u/%s/%u/t%d_%u",
PG_TBLSPC_DIR, spcOid,
TABLESPACE_VERSION_DIRECTORY,
dbOid, procNumber, relNumber);
}
}
- return path;
+ return size;
}
diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
index 2dabbe01ecd..ec21ecab9ee 100644
--- a/src/include/common/relpath.h
+++ b/src/include/common/relpath.h
@@ -84,6 +84,9 @@ extern char *GetDatabasePath(Oid dbOid, Oid spcOid);
extern char *GetRelationPath(Oid dbOid, Oid spcOid, RelFileNumber relNumber,
int procNumber, ForkNumber forkNumber);
+extern size_t GetRelationPathInPlace(char *path,
+ Oid dbOid, Oid spcOid, RelFileNumber relNumber,
+ int procNumber, ForkNumber forkNumber);
/*
* Wrapper macros for GetRelationPath. Beware of multiple
--
2.46.0
[text/x-patch] v3-0003-RelationTruncate-must-use-a-critical-section.patch (11.9K, ../../CA+hUKG+g8ydXzSnHQPtNhmwNhn8A-FborSZGSLg62tivaugP0g@mail.gmail.com/4-v3-0003-RelationTruncate-must-use-a-critical-section.patch)
download | inline diff:
From 19890fda4fb9e207c3b7dea3ae77c322227c5695 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Fri, 6 Sep 2024 10:34:14 +1200
Subject: [PATCH v3 3/3] RelationTruncate() must use a critical section.
RelationTruncate() does three things, while holding an
AccessExclusiveLock and preventing checkpoints:
1. Logs the truncation.
2. Drops buffers, even if they're dirty.
3. Truncates files, possibly more than one.
Step 3 might fail with an operating system error, but we're already
committed to the operation at this point.
Previously the theory expressed in comments was that crash recovery
would surely fail to truncate again, so we should avoid a PANIC loop.
That didn't account for the seriousness of the failure mode: dead tuples
could come back to life, because we threw away needed dirty buffers, and
replicas could PANIC, because they replayed the truncation but then
later they might see references to blocks they consider to be past the
end.
This commit wraps the whole operation in a critical section, so that any
error triggers a PANIC. In order to make that possible, it provides
smgrpreparetruncate() so that smgrtruncate() can promise not to call
palloc() (banned in critical sections). This involves using
GetRelationPathInPlace() instead of GetRelationPath(), and adjust md.c
to defer reallocating its internal array of segments when its size
decreases.
This addresses bug #18146, an ancient problem mostly affecting Windows
in practice, but also bug #18426 which revealed a new way to reach a
partially truncated state even on Unix since PostgreSQL 14. Commit
d8725104 made it possible for WaitIO() reached via DropRelationBuffers()
to be interrupted, but that's no longer possible as a side-effect of the
critical section.
This issue has been independently analyzed by a large number of people
and threads over the years, most recently Robert Haas and myself. Ideas
for how to fix with new buffer states have been proposed but that
wouldn't be back-patchable.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reported-by: rootcause000@gmail.com
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/18146-04e908c662113ad5%40postgresql.org
Discussion: https://postgr.es/m/2348.1544474335@sss.pgh.pa.us
Discussion: https://postgr.es/m/5BBC590AE8DF4ED1A170E4D48F1B53AC%40tunaPC
---
src/backend/catalog/storage.c | 28 +++++++++---
src/backend/storage/smgr/md.c | 75 +++++++++++++++++++++------------
src/backend/storage/smgr/smgr.c | 12 ++++++
src/include/storage/smgr.h | 1 +
4 files changed, 84 insertions(+), 32 deletions(-)
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index bb8c4d15612..dabaa48a57b 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -308,6 +308,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
forks[nforks] = MAIN_FORKNUM;
blocks[nforks] = nblocks;
nforks++;
+ smgrpreparetruncate(reln, MAIN_FORKNUM);
/* Prepare for truncation of the FSM if it exists */
fsm = smgrexists(RelationGetSmgr(rel), FSM_FORKNUM);
@@ -320,6 +321,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
nforks++;
need_fsm_vacuum = true;
}
+ smgrpreparetruncate(reln, FSM_FORKNUM);
}
/* Prepare for truncation of the visibility map too if it exists */
@@ -332,6 +334,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
forks[nforks] = VISIBILITYMAP_FORKNUM;
nforks++;
}
+ smgrpreparetruncate(reln, VISIBILITYMAP_FORKNUM);
}
RelationPreTruncate(rel);
@@ -368,12 +371,25 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
/*
* We WAL-log the truncation before actually truncating, which means
* trouble if the truncation fails. If we then crash, the WAL replay
- * likely isn't going to succeed in the truncation either, and cause a
- * PANIC. It's tempting to put a critical section here, but that cure
- * would be worse than the disease. It would turn a usually harmless
- * failure to truncate, that might spell trouble at WAL replay, into a
- * certain PANIC.
+ * likely isn't going to succeed in the truncation either, so it's
+ * tempting not to put a critical section here to avoid a double PANIC.
+ * However, if the file system operation failed after
+ * DropRelationBuffers() had already thrown away dirty buffers, (1) old
+ * deleted data might later come back to life, and (2) would-be-truncated
+ * pages might be modified again, and then a replica that had replayed the
+ * truncation would PANIC because it has a different idea of the relation
+ * size.
+ *
+ * The critical section also makes WaitIO() non-interruptable in
+ * DropRelationBuffers(), which would otherwise be another way to reach
+ * the above problems.
+ *
+ * Note that we called smgrpreparetruncate() above, to allow
+ * smgrtruncate() to be called within a critical section without
+ * allocating memory.
*/
+ START_CRIT_SECTION();
+
if (RelationNeedsWAL(rel))
{
/*
@@ -410,6 +426,8 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
*/
smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks);
+ END_CRIT_SECTION();
+
/* We've done all the critical work, so checkpoints are OK now. */
MyProc->delayChkptFlags &= ~(DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE);
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 6796756358f..c01b1f74875 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -117,6 +117,12 @@ static MemoryContext MdCxt; /* context for all MdfdVec objects */
/* don't try to open a segment, if not already open */
#define EXTENSION_DONT_OPEN (1 << 5)
+/*
+ * The maximum possible size of the .NNN extension for segments is 1 for the
+ * dot plus log10(2^32) for the digits for RELSEG_SIZE = 1, and it's not worth
+ * working harder to compute the value for more typical RELSEG_SIZE values.
+ */
+#define MAX_EXTENSION_SIZE (1 + 10)
/* local routines */
static void mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum,
@@ -131,7 +137,7 @@ static void register_forget_request(RelFileLocatorBackend rlocator, ForkNumber f
static void _fdvec_resize(SMgrRelation reln,
ForkNumber forknum,
int nseg);
-static char *_mdfd_segpath(SMgrRelation reln, ForkNumber forknum,
+static char *_mdfd_segpath(char *path, SMgrRelation reln, ForkNumber forknum,
BlockNumber segno);
static MdfdVec *_mdfd_openseg(SMgrRelation reln, ForkNumber forknum,
BlockNumber segno, int oflags);
@@ -408,7 +414,8 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo)
*/
if (ret >= 0 || errno != ENOENT)
{
- char *segpath = (char *) palloc(strlen(path) + 12);
+ char *segpath = (char *) palloc(strlen(path) +
+ MAX_EXTENSION_SIZE + 1);
BlockNumber segno;
for (segno = 1;; segno++)
@@ -1492,7 +1499,7 @@ _fdvec_resize(SMgrRelation reln,
reln->md_seg_fds[forknum] =
MemoryContextAlloc(MdCxt, sizeof(MdfdVec) * nseg);
}
- else
+ else if (nseg > reln->md_num_open_segs[forknum])
{
/*
* It doesn't seem worthwhile complicating the code to amortize
@@ -1504,31 +1511,50 @@ _fdvec_resize(SMgrRelation reln,
repalloc(reln->md_seg_fds[forknum],
sizeof(MdfdVec) * nseg);
}
+ else
+ {
+ /*
+ * We don't reallocate a smaller array, because we want
+ * smgrpreparetruncate() to be able to promise that smgrtruncate()
+ * won't call memory allocation functions that would fail in a
+ * critical section. This means that a bit of space in the array is
+ * now wasted, until the next time we add a segment and reallocate.
+ */
+ }
reln->md_num_open_segs[forknum] = nseg;
}
/*
- * Return the filename for the specified segment of the relation. The
- * returned string is palloc'd.
+ * Return the filename for the specified segment of the relation. The output
+ * buffer must be of size MAXPGPATH.
*/
static char *
-_mdfd_segpath(SMgrRelation reln, ForkNumber forknum, BlockNumber segno)
+_mdfd_segpath(char *path, SMgrRelation reln, ForkNumber forknum, BlockNumber segno)
{
- char *path,
- *fullpath;
+ size_t size;
- path = relpath(reln->smgr_rlocator, forknum);
+ size = GetRelationPathInPlace(path,
+ reln->smgr_rlocator.locator.dbOid,
+ reln->smgr_rlocator.locator.spcOid,
+ reln->smgr_rlocator.locator.relNumber,
+ reln->smgr_rlocator.backend,
+ forknum);
+ /*
+ * Make sure the whole path, maximum possible segment suffix and NUL
+ * terminator can fit into MAXPGPATH.
+ */
+ if (size + MAX_EXTENSION_SIZE >= MAXPGPATH)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("path \"%s\" too long", path)));
+
+ /* Append segment number. */
if (segno > 0)
- {
- fullpath = psprintf("%s.%u", path, segno);
- pfree(path);
- }
- else
- fullpath = path;
+ sprintf(path + size, ".%u", segno);
- return fullpath;
+ return path;
}
/*
@@ -1541,15 +1567,13 @@ _mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno,
{
MdfdVec *v;
File fd;
- char *fullpath;
+ char fullpath[MAXPGPATH];
- fullpath = _mdfd_segpath(reln, forknum, segno);
+ _mdfd_segpath(fullpath, reln, forknum, segno);
/* open the file */
fd = PathNameOpenFile(fullpath, _mdfd_open_flags() | oflags);
- pfree(fullpath);
-
if (fd < 0)
return NULL;
@@ -1584,6 +1608,7 @@ static MdfdVec *
_mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
bool skipFsync, int behavior)
{
+ char path[MAXPGPATH];
MdfdVec *v;
BlockNumber targetseg;
BlockNumber nextsegno;
@@ -1686,7 +1711,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\" (target block %u): previous segment is only %u blocks",
- _mdfd_segpath(reln, forknum, nextsegno),
+ _mdfd_segpath(path, reln, forknum, nextsegno),
blkno, nblocks)));
}
@@ -1700,7 +1725,7 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not open file \"%s\" (target block %u): %m",
- _mdfd_segpath(reln, forknum, nextsegno),
+ _mdfd_segpath(path, reln, forknum, nextsegno),
blkno)));
}
}
@@ -1751,11 +1776,7 @@ mdsyncfiletag(const FileTag *ftag, char *path)
}
else
{
- char *p;
-
- p = _mdfd_segpath(reln, ftag->forknum, ftag->segno);
- strlcpy(path, p, MAXPGPATH);
- pfree(p);
+ _mdfd_segpath(path, reln, ftag->forknum, ftag->segno);
file = PathNameOpenFile(path, _mdfd_open_flags());
if (file < 0)
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 7b9fa103eff..323e5114f6a 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -689,6 +689,18 @@ smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum)
return InvalidBlockNumber;
}
+/*
+ * smgrpreparetruncate() -- Prepare to truncate a fork of a relation.
+ *
+ * This promises that a later call to smgrtruncate() will not have to allocate
+ * memory from MemoryContexts that don't allow that inside critical sections.
+ */
+void
+smgrpreparetruncate(SMgrRelation reln, ForkNumber forknum)
+{
+ smgrnblocks(reln, forknum);
+}
+
/*
* smgrtruncate() -- Truncate the given forks of supplied relation to
* each specified numbers of blocks
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index e15b20a566a..5f3fa3962a6 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -103,6 +103,7 @@ extern void smgrwriteback(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, BlockNumber nblocks);
extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum);
extern BlockNumber smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum);
+extern void smgrpreparetruncate(SMgrRelation reln, ForkNumber forknum);
extern void smgrtruncate(SMgrRelation reln, ForkNumber *forknum,
int nforks, BlockNumber *nblocks);
extern void smgrimmedsync(SMgrRelation reln, ForkNumber forknum);
--
2.46.0
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-09-09 07:21 Michael Paquier <michael@paquier.xyz>
parent: Thomas Munro <thomas.munro@gmail.com>
1 sibling, 1 reply; 45+ messages in thread
From: Michael Paquier @ 2024-09-09 07:21 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Sep 06, 2024 at 11:05:26AM +1200, Thomas Munro wrote:
> But let's not forget that these patches also fix two bugs that apply
> to Unix too.
>
> For the DELAY_CHKPT_START bug, we should back-patch all the way.
>
> For the WaitIO() bug affecting all OSes, that only needs to go back to
> 14. We could also opt to be cautious and let it run on master for a
> while before we do that, though.
>
> For ftruncate() failure, I wouldn't be too bothered if we just let
> sleeping dogs lie in 13. It affects only systems that have serious
> file system corruption, or Windows systems that have something
> snooping on private files with antisocial flags.
0001 looks OK seen from here.
+GetRelationPathInPlace(char *path,
+ Oid dbOid, Oid spcOid, RelFileNumber relNumber,
It's very easy to handle such APIs the wrong way. I'd suggest to also
give the size of the output buffer as an argument of the function,
cross-checking in the function that nothing is wrong with the path
generated.
The trick to use _mdfd_segpath() through smgrpreparetruncate() smells
too much of magic to me. I don't have an idea that does not involve
tweaking the interface of smgr.c or some of its structures on top of
my mind, though, which is to save the path before the critical section
and pass it through (like a private memory area that can be used by
md.c).. Or do a MemoryContextAllowInCriticalSection, even if it is
not its original purpose.
Also, now that we have injection points, could this stuff be worth
adding a test? You could force an error state that gets upgraded to a
PANIC while doing the truncation, checking that we don't lose data
after recovery, for example. This is a problem old enough that I'd
rather see some automation in place with a long-term picture in mind,
even if it means a test for 17~ or just HEAD.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../Zt6h7y308oXttagv@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-09-09 08:57 Thomas Munro <thomas.munro@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2024-09-09 08:57 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, Sep 9, 2024 at 7:21 PM Michael Paquier <michael@paquier.xyz> wrote:
> The trick to use _mdfd_segpath() through smgrpreparetruncate() smells
> too much of magic to me. I don't have an idea that does not involve
> tweaking the interface of smgr.c or some of its structures on top of
> my mind, though, which is to save the path before the critical section
> and pass it through (like a memory area that can be used by
> md.c).. Or do a MemoryContextAllowInCriticalSection, even if it is
> not its original purpose.
It's funny that it's so hard to get or store the pathnames when every
single File knows its pathname, and segment 0 could give it to you:
FilePathName(). Thinking about that made me notice that the vfd layer
does raw strdup() and ERRORs on failure, so the thing I proposed might
be adhering to the letter of the law ("no palloc in critical
sections"), but not the spirit (the fd.c file opening stuff will throw
ERRORs that are promoted to PANICs, exactly what that law intended to
prevent). Mleugh.
Another option would be to introduce mdtruncate2(old_nblocks,
new_nblocks), so that it doesn't have to call smgrnblocks() itself,
and you can obtain old_nblocks outside the CS...
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-09-10 00:43 Thomas Munro <thomas.munro@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 2 replies; 45+ messages in thread
From: Thomas Munro @ 2024-09-10 00:43 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Mon, Sep 9, 2024 at 8:57 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Mon, Sep 9, 2024 at 7:21 PM Michael Paquier <michael@paquier.xyz> wrote:
> > The trick to use _mdfd_segpath() through smgrpreparetruncate() smells
> > too much of magic to me. I don't have an idea that does not involve
> > tweaking the interface of smgr.c or some of its structures on top of
> > my mind, though, which is to save the path before the critical section
> > and pass it through (like a memory area that can be used by
> > md.c).. Or do a MemoryContextAllowInCriticalSection, even if it is
> > not its original purpose.
...
> Another option would be to introduce mdtruncate2(old_nblocks,
> new_nblocks), so that it doesn't have to call smgrnblocks() itself,
> and you can obtain old_nblocks outside the CS...
Here is an experiment to try that out. The requirement to call
smgrnblock() beforehand is still slightly magical, but written in
black and white. I guess it could use an assertion cross-check on the
number of opened segments...
It is possible that an extension that messes with smgrsw[] would not
like this in a minor release:
- smgrsw[reln->smgr_which].smgr_truncate(reln,
forknum[i], nblocks[i]);
+ smgrsw[reln->smgr_which].smgr_truncate(reln, forknum[i],
+
old_nblocks[i], nblocks[i]);
I don't actually know of such an extension myself. I suppose we could
add a new member at the end called smgr_truncatefrom, and have
smgrtruncatefrom() call that if it is non-NULL (md's case), and the
existing smgr_truncate function pointer if it doesn't (ie, some
hypothetical external monkey-patching smgr replacement). Hypothetical
forks of PostgreSQL might be more likely to have used this
interception point, but wouldn't have quite the same ABI problem
(they'd adjust their function when rebasing on a minor release, but
they might also prefer if the old function prototype still worked, or
maybe they'd have some version of this bug themselves and want to be
able to fix it...).
Hmm.
Attachments:
[text/x-patch] v4-0001-RelationTruncate-must-use-a-critical-section.patch (11.2K, ../../CA+hUKGK+tzjU-vqLr_zA8REzPUyiKNmpxk32Jw5AFR0DZNaXfg@mail.gmail.com/2-v4-0001-RelationTruncate-must-use-a-critical-section.patch)
download | inline diff:
From 020ccd14b177494a7d7e00ea3ef50f400a9b1032 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 10 Sep 2024 12:04:38 +1200
Subject: [PATCH v4] RelationTruncate() must use a critical section.
RelationTruncate() does three things, while holding an
AccessExclusiveLock and preventing checkpoints:
1. Logs the truncation.
2. Drops buffers, even if they're dirty.
3. Truncates files, possibly more than one.
Step 3 might fail with an operating system error, but we're already
committed to the operation at this point.
Previously the theory expressed in comments was that crash recovery
would surely fail to truncate again, so we should avoid a PANIC loop.
That didn't account for the seriousness of the failure mode: dead tuples
could come back to life, because we threw away needed dirty buffers, and
replicas could PANIC, because they replayed the truncation but then
later they might see references to blocks they consider to be past the
end.
This commit wraps the whole operation in a critical section, so that any
error triggers a PANIC. In order to make that possible, it provides
smgrtruncatefrom(), so that we can move the smgrnblocks() calls out of
the critical section, and adjusts md.c to defer reallocating its
internal array of segments when its size decreases.
This addresses bug #18146, an ancient problem mostly affecting Windows
in practice, but also bug #18426 which revealed a new way to reach a
partially truncated state even on Unix since PostgreSQL 14. Commit
d8725104 made it possible for WaitIO() reached via DropRelationBuffers()
to be interrupted, but that's no longer possible as a side-effect of the
critical section.
This issue has been independently analyzed by a large number of people
and threads over the years, most recently Robert Haas, Michael Paquier
and myself. Ideas for how to fix with new buffer states have been
mooted, but that wouldn't be back-patchable.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reported-by: rootcause000@gmail.com
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/18146-04e908c662113ad5%40postgresql.org
Discussion: https://postgr.es/m/2348.1544474335@sss.pgh.pa.us
Discussion: https://postgr.es/m/5BBC590AE8DF4ED1A170E4D48F1B53AC%40tunaPC
---
src/backend/catalog/storage.c | 31 +++++++++++++++++++++++++------
src/backend/storage/smgr/md.c | 27 +++++++++++++++++++--------
src/backend/storage/smgr/smgr.c | 25 +++++++++++++++++++++++--
src/include/storage/md.h | 2 +-
src/include/storage/smgr.h | 3 +++
5 files changed, 71 insertions(+), 17 deletions(-)
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index f56b3cc0f23..eefe13aba08 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -291,6 +291,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
bool vm;
bool need_fsm_vacuum = false;
ForkNumber forks[MAX_FORKNUM];
+ BlockNumber old_blocks[MAX_FORKNUM];
BlockNumber blocks[MAX_FORKNUM];
int nforks = 0;
SMgrRelation reln;
@@ -306,6 +307,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
/* Prepare for truncation of MAIN fork of the relation */
forks[nforks] = MAIN_FORKNUM;
+ old_blocks[nforks] = smgrnblocks(reln, MAIN_FORKNUM);
blocks[nforks] = nblocks;
nforks++;
@@ -316,6 +318,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
blocks[nforks] = FreeSpaceMapPrepareTruncateRel(rel, nblocks);
if (BlockNumberIsValid(blocks[nforks]))
{
+ old_blocks[nforks] = smgrnblocks(reln, FSM_FORKNUM);
forks[nforks] = FSM_FORKNUM;
nforks++;
need_fsm_vacuum = true;
@@ -329,6 +332,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
blocks[nforks] = visibilitymap_prepare_truncate(rel, nblocks);
if (BlockNumberIsValid(blocks[nforks]))
{
+ old_blocks[nforks] = smgrnblocks(reln, VISIBILITYMAP_FORKNUM);
forks[nforks] = VISIBILITYMAP_FORKNUM;
nforks++;
}
@@ -355,12 +359,25 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
/*
* We WAL-log the truncation before actually truncating, which means
* trouble if the truncation fails. If we then crash, the WAL replay
- * likely isn't going to succeed in the truncation either, and cause a
- * PANIC. It's tempting to put a critical section here, but that cure
- * would be worse than the disease. It would turn a usually harmless
- * failure to truncate, that might spell trouble at WAL replay, into a
- * certain PANIC.
+ * likely isn't going to succeed in the truncation either, so it's
+ * tempting not to put a critical section here to avoid a double PANIC.
+ * However, if the file system operation failed after
+ * DropRelationBuffers() had already thrown away dirty buffers, (1) old
+ * deleted data might later come back to life, and (2) would-be-truncated
+ * pages might be modified again, and then a replica that had replayed the
+ * truncation would PANIC because it has a different idea of the relation
+ * size.
+ *
+ * The critical section also makes WaitIO() non-interruptable in
+ * DropRelationBuffers(), which would otherwise be another way to reach
+ * the above problems.
+ *
+ * Note that we capture the current size before entering the critical
+ * section, so that we can use smgrtruncatefrom(). smgrtruncate() can't
+ * be used in a critical section because it might allocate memory.
*/
+ START_CRIT_SECTION();
+
if (RelationNeedsWAL(rel))
{
/*
@@ -395,7 +412,9 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
* longer exist after truncation is complete, and then truncate the
* corresponding files on disk.
*/
- smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks);
+ smgrtruncatefrom(RelationGetSmgr(rel), forks, nforks, old_blocks, blocks);
+
+ END_CRIT_SECTION();
/* We've done all the critical work, so checkpoints are OK now. */
MyProc->delayChkptFlags &= ~DELAY_CHKPT_COMPLETE;
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 6796756358f..ab7fd58a2c7 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -1141,19 +1141,20 @@ mdnblocks(SMgrRelation reln, ForkNumber forknum)
/*
* mdtruncate() -- Truncate relation to specified number of blocks.
+ *
+ * Guaranteed not to allocate memory, so it can be used in a critical section.
+ * Caller must have called smgrnblocks() to obtain curnblk while holding a
+ * sufficient lock to prevent a change in relation size. This also makes sure
+ * we have opened all active segments, so that truncate loop will get them
+ * all! That step can't be done in a critical section.
*/
void
-mdtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks)
+mdtruncate(SMgrRelation reln, ForkNumber forknum,
+ BlockNumber curnblk, BlockNumber nblocks)
{
- BlockNumber curnblk;
BlockNumber priorblocks;
int curopensegs;
- /*
- * NOTE: mdnblocks makes sure we have opened all active segments, so that
- * truncation loop will get them all!
- */
- curnblk = mdnblocks(reln, forknum);
if (nblocks > curnblk)
{
/* Bogus request ... but no complaint if InRecovery */
@@ -1492,7 +1493,7 @@ _fdvec_resize(SMgrRelation reln,
reln->md_seg_fds[forknum] =
MemoryContextAlloc(MdCxt, sizeof(MdfdVec) * nseg);
}
- else
+ else if (nseg > reln->md_num_open_segs[forknum])
{
/*
* It doesn't seem worthwhile complicating the code to amortize
@@ -1504,6 +1505,16 @@ _fdvec_resize(SMgrRelation reln,
repalloc(reln->md_seg_fds[forknum],
sizeof(MdfdVec) * nseg);
}
+ else
+ {
+ /*
+ * We don't reallocate a smaller array, because we want mdtruncate()
+ * to be able to promise that it won't allocate memory, so that it is
+ * allowed in a critical section. This means that a bit of space in
+ * the array is now wasted, until the next time we add a segment and
+ * reallocate.
+ */
+ }
reln->md_num_open_segs[forknum] = nseg;
}
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 7b9fa103eff..2aef34c4e9a 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -99,7 +99,7 @@ typedef struct f_smgr
BlockNumber blocknum, BlockNumber nblocks);
BlockNumber (*smgr_nblocks) (SMgrRelation reln, ForkNumber forknum);
void (*smgr_truncate) (SMgrRelation reln, ForkNumber forknum,
- BlockNumber nblocks);
+ BlockNumber old_blocks, BlockNumber nblocks);
void (*smgr_immedsync) (SMgrRelation reln, ForkNumber forknum);
void (*smgr_registersync) (SMgrRelation reln, ForkNumber forknum);
} f_smgr;
@@ -701,6 +701,26 @@ smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum)
*/
void
smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nblocks)
+{
+ BlockNumber old_nblocks[MAX_FORKNUM + 1];
+
+ for (int i = 0; i < nforks; ++i)
+ old_nblocks[i] = smgrnblocks(reln, forknum[i]);
+
+ return smgrtruncatefrom(reln, forknum, nforks, old_nblocks, nblocks);
+}
+
+/*
+ * smgrtruncatefrom() -- Like sgmrtruncate(), but with caller-supplied old
+ * sizes, to allow usage in critical sections.
+ *
+ * See smgrtruncate() for requirements, and additionally the caller must call
+ * smgrnblocks() to obtain the size of each fork to be truncated, and not
+ * allow the relation to be invalidated in between.
+ */
+void
+smgrtruncatefrom(SMgrRelation reln, ForkNumber *forknum, int nforks,
+ BlockNumber *old_nblocks, BlockNumber *nblocks)
{
int i;
@@ -728,7 +748,8 @@ smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nb
/* Make the cached size is invalid if we encounter an error. */
reln->smgr_cached_nblocks[forknum[i]] = InvalidBlockNumber;
- smgrsw[reln->smgr_which].smgr_truncate(reln, forknum[i], nblocks[i]);
+ smgrsw[reln->smgr_which].smgr_truncate(reln, forknum[i],
+ old_nblocks[i], nblocks[i]);
/*
* We might as well update the local smgr_cached_nblocks values. The
diff --git a/src/include/storage/md.h b/src/include/storage/md.h
index 620f10abdeb..5dbd0033c6f 100644
--- a/src/include/storage/md.h
+++ b/src/include/storage/md.h
@@ -41,7 +41,7 @@ extern void mdwriteback(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, BlockNumber nblocks);
extern BlockNumber mdnblocks(SMgrRelation reln, ForkNumber forknum);
extern void mdtruncate(SMgrRelation reln, ForkNumber forknum,
- BlockNumber nblocks);
+ BlockNumber old_blocks, BlockNumber nblocks);
extern void mdimmedsync(SMgrRelation reln, ForkNumber forknum);
extern void mdregistersync(SMgrRelation reln, ForkNumber forknum);
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index e15b20a566a..2dc146dc07d 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -105,6 +105,9 @@ extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum);
extern BlockNumber smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum);
extern void smgrtruncate(SMgrRelation reln, ForkNumber *forknum,
int nforks, BlockNumber *nblocks);
+extern void smgrtruncatefrom(SMgrRelation reln, ForkNumber *forknum,
+ int nforks,
+ BlockNumber *old_nblocks, BlockNumber *nblocks);
extern void smgrimmedsync(SMgrRelation reln, ForkNumber forknum);
extern void smgrregistersync(SMgrRelation reln, ForkNumber forknum);
extern void AtEOXact_SMgr(void);
--
2.46.0
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-09-11 03:20 Thomas Munro <thomas.munro@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
1 sibling, 0 replies; 45+ messages in thread
From: Thomas Munro @ 2024-09-11 03:20 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, Sep 10, 2024 at 12:43 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> It is possible that an extension that messes with smgrsw[] would not
> like this in a minor release:
>
> - smgrsw[reln->smgr_which].smgr_truncate(reln,
> forknum[i], nblocks[i]);
> + smgrsw[reln->smgr_which].smgr_truncate(reln, forknum[i],
> +
> old_nblocks[i], nblocks[i]);
Scratch that concern, smgrsw is static in smgr.c for now (h/t to
Andres for pointing that out in an off-list chat when I described
this...). I must have confused myself with proposals I've read to
make it so an extension *could* mess with this stuff, but it's not
done yet.
Also if this goes somewhere I should note in the commit message that
the analysis of the WaitIO issue was from Alexander L in bug #18426.
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-10-29 07:48 Michael Paquier <michael@paquier.xyz>
parent: Thomas Munro <thomas.munro@gmail.com>
1 sibling, 1 reply; 45+ messages in thread
From: Michael Paquier @ 2024-10-29 07:48 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, Sep 10, 2024 at 12:43:16PM +1200, Thomas Munro wrote:
> Here is an experiment to try that out. The requirement to call
> smgrnblock() beforehand is still slightly magical, but written in
> black and white. I guess it could use an assertion cross-check on the
> number of opened segments...
I was looking at what you have here, and the split with
smgrtruncatefrom() to do the allocations in _mdfd_openseg() for
_mdfd_segpath() and _fdvec_resize() before entering in the critical
section for the physical truncation is elegant.
+mdtruncate(SMgrRelation reln, ForkNumber forknum,
+ BlockNumber curnblk, BlockNumber nblocks)
+ * all! That step can't be done in a critical section.
Perhaps this should have an assert based on CritSectionCount==0 to
force the rule.
Don't you think that we'd better have a regression test on HEAD at
least? It should not be complicated. I can create one if you want,
perhaps for later if we want to catch the next minor release train.
> I don't actually know of such an extension myself. I suppose we could
> add a new member at the end called smgr_truncatefrom, and have
> smgrtruncatefrom() call that if it is non-NULL (md's case), and the
> existing smgr_truncate function pointer if it doesn't (ie, some
> hypothetical external monkey-patching smgr replacement). Hypothetical
> forks of PostgreSQL might be more likely to have used this
> interception point, but wouldn't have quite the same ABI problem
> (they'd adjust their function when rebasing on a minor release, but
> they might also prefer if the old function prototype still worked, or
> maybe they'd have some version of this bug themselves and want to be
> able to fix it...).
Making folks aware of the problem sounds kind of sensible seen from
here. In short, changing the signature of smgr_truncate() in a minor
release to fix what's a severe data corruption issue takes priority
IMO.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ZyCTXqPG_uGvLJtZ@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-11-14 15:51 Robert Haas <robertmhaas@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 45+ messages in thread
From: Robert Haas @ 2024-11-14 15:51 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, Oct 29, 2024 at 3:49 AM Michael Paquier <michael@paquier.xyz> wrote:
> Don't you think that we'd better have a regression test on HEAD at
> least? It should not be complicated. I can create one if you want,
> perhaps for later if we want to catch the next minor release train.
I took a look at v4-0001 today and I think it looks fine. While I'm
not opposed to adding a test case, I think it's more important to fix
the bug at this point than to wait longer for a test case to show up.
I do wonder whether the new smgrtruncatefrom() should be used
everywhere instead of just from one of the call sites, but even if
that's desirable long-term, doing this much is still a lot better than
doing nothing. This data corrupting bug has been known and understood
for more than 4 years at this point.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-12-11 13:32 Thomas Munro <thomas.munro@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2024-12-11 13:32 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
(Replies to separate messages from Michael and Robert below.)
On Tue, Oct 29, 2024 at 8:49 PM Michael Paquier <michael@paquier.xyz> wrote:
> I was looking at what you have here, and the split with
> smgrtruncatefrom() to do the allocations in _mdfd_openseg() for
> _mdfd_segpath() and _fdvec_resize() before entering in the critical
> section for the physical truncation is elegant.
Thanks for the sanity check. It took a few goes around to land there.
> Perhaps this should have an assert based on CritSectionCount==0 to
> force the rule.
Added.
> Don't you think that we'd better have a regression test on HEAD at
> least? It should not be complicated. I can create one if you want,
> perhaps for later if we want to catch the next minor release train.
I'm probably lacking imagination here; I can see that it's useful to
use the injection tooling for repros without the fix, but the fix is
mostly a critical section. We know what critical sections do, they
hold off interrupts and explode quite reliably on error, so there is
not much left to test once it's fixed, no?
> Making folks aware of the problem sounds kind of sensible seen from
> here. In short, changing the signature of smgr_truncate() in a minor
> release to fix what's a severe data corruption issue takes priority
> IMO.
Yeah. I'd avoid it if we had another way, but in the absence of epiphanies...
On Fri, Nov 15, 2024 at 4:51 AM Robert Haas <robertmhaas@gmail.com> wrote:
> I took a look at v4-0001 today and I think it looks fine. While I'm
> not opposed to adding a test case, I think it's more important to fix
> the bug at this point than to wait longer for a test case to show up.
Thanks for taking a look! I started out to re-test and hopefully
commit this today (having already committed 75818b3a from upthread
last week), but while reviewing the reviews, this turned out to be a
thing:
> I do wonder whether the new smgrtruncatefrom() should be used
> everywhere instead of just from one of the call sites, but even if
> that's desirable long-term, doing this much is still a lot better than
> doing nothing. This data corrupting bug has been known and understood
> for more than 4 years at this point.
There are only three callers of smgrtruncate() in our tree:
1. This one.
2. Its redo-time counterpart.
3. pg_truncate_visibility_map(), in contrib/pg_visibility.
The recovery environment promotes errors of both types to FATAL (the
"we have no handler" case in errstart()), and the postmaster handles
startup failure a bit like a PANIC. Or do you think PANIC and more
similar code would be better?
As for pg_truncate_visibility_map(), it is a "a cut-down version of
RelationTruncate". It logs in a different order (which probably
doesn't really matter as it doesn't flush anyway) and it didn't get
the memo about checkpoints. Maybe it's not such a big deal to be
sloppy about whether or not the _vm file is truncated or not after
crash recovery, given the semantics, I'm not sure. It surely has a
version of the zombie tuples bug, though: If a _vm bit recently
changed from "visible" to "not visible" and hasn't been written to
disk yet, and it is booted out of the buffer pool by a call to
pg_truncate_visibility_map() that is then cancelled in a call to
WaitIO() for some other buffer so that ftruncate() is not reached,
then an index-only scan might read in the old disk copy and show you a
bunch of index tuples that are supposed to be invisible.
I am curious about the non-flushing though. RelationTruncate() also
doesn't flush if there is no fsm or vm. Perhaps it always flushes in
practice since VACUUM itself creates those, I'm not entirely sure yet.
If the log-first rule is abandoned and the flush is allowed to happen
after the critical section, isn't there an opportunity for a primary
to insert into but not flush the WAL, truncate the file, power-cycle,
run recovery, and then carry on without ever telling a standby to
truncate? Maybe that's OK for _vm (maybe the standby's untruncated
copy of _vm would still be correct, and be maintained correctly by heap
operations and everything would be fine, I'm not sure), by why would we
want to allow this desynchronisation/confusion, just to make an
infrequently used debugging tool go a bit faster? As for
RelationTruncate(), if the non-flushing case is reachable, it would
seem more obviously worse: it could leave the standby with a bunch of
invisible tuples that the primary doesn't have, and eventually they'd
either reference truncated CLOG or become visible again via wraparound
rebirth, no? Am I missing something? Why wouldn't we just make both
places flush unconditionally?
PFA the first sketch of a patch to make pg_truncate_visibility_map()
into... a cut-down version of RelationTruncate(). Still looking into
the flushing, without which the comments in the patch are entirely
bogus.
Attachments:
[application/x-patch] v5-0001-Fix-corruption-on-failed-interrupted-truncation.patch (12.4K, ../../CA+hUKGJSOEZWx9zRFFfOfDj9zbVmMbWKRq+ZzMCXftHcO1UYQQ@mail.gmail.com/2-v5-0001-Fix-corruption-on-failed-interrupted-truncation.patch)
download | inline diff:
From c0e74c507483c97297ed66cdfb9ec2e16f0dde3f Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 10 Sep 2024 12:04:38 +1200
Subject: [PATCH v5 1/2] Fix corruption on failed/interrupted truncation.
RelationTruncate() does three things, while holding an
AccessExclusiveLock and preventing checkpoints:
1. Logs the truncation.
2. Drops buffers, even if they're dirty.
3. Truncates files, possibly more than one.
Step 3 might fail with an operating system error, but we're already
committed to the operation at this point.
Previously the theory expressed in comments was that crash recovery
would surely fail to truncate again, so we should avoid a PANIC loop.
That didn't account for the seriousness of the failure mode: dead tuples
could come back to life, because we threw away needed dirty buffers, and
replicas could PANIC, because they replayed the truncation but then
later they might see references to reanimated blocks they consider to be
past the end.
This commit wraps the whole operation in a critical section, so that any
error triggers a PANIC. In order to make that possible, it provides
smgrtruncatefrom(), so that we can move the smgrnblocks() calls out of
the critical section, and adjusts md.c to defer reallocating its
internal array of segments when its size decreases.
This addresses bug #18146, an ancient problem mostly affecting Windows
in practice. ftruncate() failed only in very rare circumstances on Unix
(but it could!), while on Windows there seem to be a more reports of
transient failures (really chsize() or more likely the reopen in
FileAccess(), for which a root cause has not been determined).
It also addresses bug #18426, which revealed a new way to reach these
corrupted partially truncated states even on Unix since PostgreSQL 14.
Commit d8725104 made it possible for WaitIO(), reachable via
DropRelationBuffers(), to be interrupted. That's no longer possible as
a side-effect of the new critical section.
The equivalent smgrtruncate() call in recovery doesn't need a critical
section, because both types of errors would be promoted to FATAL in the
startup process (it has no error handler), and then treated much like a
PANIC by the postmaster.
Alternatives involving new buffer states for pending truncation, and
other ways of making smgrtruncate() safe in critical sections have been
mooted, but this approach seems the simplest. Unfortunately it involves
back-patching a change to the smgr interface. That's private but it
might be a configuration point in fork projects. We'd prefer to avoid
that but we agreed it was acceptable to fix a data corruption bug.
The ftruncate() failure has been independently analyzed by a large
number of people and threads over the years, and the WaitIO()
cancellation variant was more recently diagnosed by Alexander Lakhin.
XXX shouldn't we flush the WAL unconditionally?
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Diagnosed-by: Alexander Lakhin <exclusion@gmail.com>
Reported-by: rootcause000@gmail.com
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/18146-04e908c662113ad5%40postgresql.org
Discussion: https://postgr.es/m/2348.1544474335@sss.pgh.pa.us
Discussion: https://postgr.es/m/5BBC590AE8DF4ED1A170E4D48F1B53AC%40tunaPC
---
src/backend/catalog/storage.c | 31 +++++++++++++++++++++++++------
src/backend/storage/smgr/md.c | 27 +++++++++++++++++++--------
src/backend/storage/smgr/smgr.c | 31 +++++++++++++++++++++++++++++--
src/include/storage/md.h | 2 +-
src/include/storage/smgr.h | 3 +++
5 files changed, 77 insertions(+), 17 deletions(-)
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index bb8c4d15612..a14f45c4cff 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -291,6 +291,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
bool vm;
bool need_fsm_vacuum = false;
ForkNumber forks[MAX_FORKNUM];
+ BlockNumber old_blocks[MAX_FORKNUM];
BlockNumber blocks[MAX_FORKNUM];
int nforks = 0;
SMgrRelation reln;
@@ -306,6 +307,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
/* Prepare for truncation of MAIN fork of the relation */
forks[nforks] = MAIN_FORKNUM;
+ old_blocks[nforks] = smgrnblocks(reln, MAIN_FORKNUM);
blocks[nforks] = nblocks;
nforks++;
@@ -316,6 +318,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
blocks[nforks] = FreeSpaceMapPrepareTruncateRel(rel, nblocks);
if (BlockNumberIsValid(blocks[nforks]))
{
+ old_blocks[nforks] = smgrnblocks(reln, FSM_FORKNUM);
forks[nforks] = FSM_FORKNUM;
nforks++;
need_fsm_vacuum = true;
@@ -329,6 +332,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
blocks[nforks] = visibilitymap_prepare_truncate(rel, nblocks);
if (BlockNumberIsValid(blocks[nforks]))
{
+ old_blocks[nforks] = smgrnblocks(reln, VISIBILITYMAP_FORKNUM);
forks[nforks] = VISIBILITYMAP_FORKNUM;
nforks++;
}
@@ -368,12 +372,25 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
/*
* We WAL-log the truncation before actually truncating, which means
* trouble if the truncation fails. If we then crash, the WAL replay
- * likely isn't going to succeed in the truncation either, and cause a
- * PANIC. It's tempting to put a critical section here, but that cure
- * would be worse than the disease. It would turn a usually harmless
- * failure to truncate, that might spell trouble at WAL replay, into a
- * certain PANIC.
+ * likely isn't going to succeed in the truncation either, so it's
+ * tempting not to put a critical section here to avoid a double PANIC.
+ * However, if the file system operation failed after
+ * DropRelationBuffers() had already thrown away dirty buffers, (1) old
+ * deleted data might later come back to life, and (2) would-be-truncated
+ * pages might be modified again, and then a replica that had replayed the
+ * truncation would PANIC because it has a different idea of the relation
+ * size.
+ *
+ * The critical section also makes WaitIO() non-interruptible in
+ * DropRelationBuffers(), which would otherwise be another way to reach
+ * the above problems.
+ *
+ * Note that we capture the current size before entering the critical
+ * section, so that we can use smgrtruncatefrom(). smgrtruncate() can't
+ * be used in a critical section because it might allocate memory.
*/
+ START_CRIT_SECTION();
+
if (RelationNeedsWAL(rel))
{
/*
@@ -408,7 +425,9 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
* longer exist after truncation is complete, and then truncate the
* corresponding files on disk.
*/
- smgrtruncate(RelationGetSmgr(rel), forks, nforks, blocks);
+ smgrtruncatefrom(RelationGetSmgr(rel), forks, nforks, old_blocks, blocks);
+
+ END_CRIT_SECTION();
/* We've done all the critical work, so checkpoints are OK now. */
MyProc->delayChkptFlags &= ~(DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE);
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index cc8a80ee961..1e509d42da5 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -1162,19 +1162,20 @@ mdnblocks(SMgrRelation reln, ForkNumber forknum)
/*
* mdtruncate() -- Truncate relation to specified number of blocks.
+ *
+ * Guaranteed not to allocate memory, so it can be used in a critical section.
+ * Caller must have called smgrnblocks() to obtain curnblk while holding a
+ * sufficient lock to prevent a change in relation size. This also makes sure
+ * we have opened all active segments, so that truncate loop will get them
+ * all! That step can't be done in a critical section.
*/
void
-mdtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks)
+mdtruncate(SMgrRelation reln, ForkNumber forknum,
+ BlockNumber curnblk, BlockNumber nblocks)
{
- BlockNumber curnblk;
BlockNumber priorblocks;
int curopensegs;
- /*
- * NOTE: mdnblocks makes sure we have opened all active segments, so that
- * truncation loop will get them all!
- */
- curnblk = mdnblocks(reln, forknum);
if (nblocks > curnblk)
{
/* Bogus request ... but no complaint if InRecovery */
@@ -1513,7 +1514,7 @@ _fdvec_resize(SMgrRelation reln,
reln->md_seg_fds[forknum] =
MemoryContextAlloc(MdCxt, sizeof(MdfdVec) * nseg);
}
- else
+ else if (nseg > reln->md_num_open_segs[forknum])
{
/*
* It doesn't seem worthwhile complicating the code to amortize
@@ -1525,6 +1526,16 @@ _fdvec_resize(SMgrRelation reln,
repalloc(reln->md_seg_fds[forknum],
sizeof(MdfdVec) * nseg);
}
+ else
+ {
+ /*
+ * We don't reallocate a smaller array, because we want mdtruncate()
+ * to be able to promise that it won't allocate memory, so that it is
+ * allowed in a critical section. This means that a bit of space in
+ * the array is now wasted, until the next time we add a segment and
+ * reallocate.
+ */
+ }
reln->md_num_open_segs[forknum] = nseg;
}
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 925728eb6c1..0aba474134f 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -101,7 +101,7 @@ typedef struct f_smgr
BlockNumber blocknum, BlockNumber nblocks);
BlockNumber (*smgr_nblocks) (SMgrRelation reln, ForkNumber forknum);
void (*smgr_truncate) (SMgrRelation reln, ForkNumber forknum,
- BlockNumber nblocks);
+ BlockNumber old_blocks, BlockNumber nblocks);
void (*smgr_immedsync) (SMgrRelation reln, ForkNumber forknum);
void (*smgr_registersync) (SMgrRelation reln, ForkNumber forknum);
} f_smgr;
@@ -723,6 +723,32 @@ smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum)
*/
void
smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nblocks)
+{
+ BlockNumber old_nblocks[MAX_FORKNUM + 1];
+
+ /*
+ * smgrnblocks() might need to allocate, but see smgrtruncatefrom() for a
+ * version that can be used in a critical section.
+ */
+ Assert(CritSectionCount == 0);
+
+ for (int i = 0; i < nforks; ++i)
+ old_nblocks[i] = smgrnblocks(reln, forknum[i]);
+
+ return smgrtruncatefrom(reln, forknum, nforks, old_nblocks, nblocks);
+}
+
+/*
+ * smgrtruncatefrom() -- Like sgmrtruncate(), but with caller-supplied old
+ * sizes, to allow usage in critical sections.
+ *
+ * See smgrtruncate() for requirements, and additionally the caller must call
+ * smgrnblocks() to obtain the size of each fork to be truncated, and not
+ * allow the relation to be invalidated in between.
+ */
+void
+smgrtruncatefrom(SMgrRelation reln, ForkNumber *forknum, int nforks,
+ BlockNumber *old_nblocks, BlockNumber *nblocks)
{
int i;
@@ -750,7 +776,8 @@ smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks, BlockNumber *nb
/* Make the cached size is invalid if we encounter an error. */
reln->smgr_cached_nblocks[forknum[i]] = InvalidBlockNumber;
- smgrsw[reln->smgr_which].smgr_truncate(reln, forknum[i], nblocks[i]);
+ smgrsw[reln->smgr_which].smgr_truncate(reln, forknum[i],
+ old_nblocks[i], nblocks[i]);
/*
* We might as well update the local smgr_cached_nblocks values. The
diff --git a/src/include/storage/md.h b/src/include/storage/md.h
index b72293c79a5..e7671dd6c18 100644
--- a/src/include/storage/md.h
+++ b/src/include/storage/md.h
@@ -43,7 +43,7 @@ extern void mdwriteback(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, BlockNumber nblocks);
extern BlockNumber mdnblocks(SMgrRelation reln, ForkNumber forknum);
extern void mdtruncate(SMgrRelation reln, ForkNumber forknum,
- BlockNumber nblocks);
+ BlockNumber old_blocks, BlockNumber nblocks);
extern void mdimmedsync(SMgrRelation reln, ForkNumber forknum);
extern void mdregistersync(SMgrRelation reln, ForkNumber forknum);
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 5ab992f5bd5..08c8b6dc289 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -107,6 +107,9 @@ extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum);
extern BlockNumber smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum);
extern void smgrtruncate(SMgrRelation reln, ForkNumber *forknum,
int nforks, BlockNumber *nblocks);
+extern void smgrtruncatefrom(SMgrRelation reln, ForkNumber *forknum,
+ int nforks,
+ BlockNumber *old_nblocks, BlockNumber *nblocks);
extern void smgrimmedsync(SMgrRelation reln, ForkNumber forknum);
extern void smgrregistersync(SMgrRelation reln, ForkNumber forknum);
extern void AtEOXact_SMgr(void);
--
2.39.5
[application/x-patch] v5-0002-Fix-pg_truncate_visibility_map-protocol.patch (3.4K, ../../CA+hUKGJSOEZWx9zRFFfOfDj9zbVmMbWKRq+ZzMCXftHcO1UYQQ@mail.gmail.com/3-v5-0002-Fix-pg_truncate_visibility_map-protocol.patch)
download | inline diff:
From 1e2491a448642e6086e85b8f5e04502f8cdf752c Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Wed, 11 Dec 2024 21:22:08 +1300
Subject: [PATCH v5 2/2] Fix pg_truncate_visibility_map() protocol.
pg_truncate_visibility_map() is supposed to be similar to
RelationTruncate(), but it is missing several developments. Reorder
operations to match, and apply changes equivalent to commits 412ad7a5,
75818b3a, TODO:patch-0001 to create atomicity at do and redo time.
XXX The comments about atomicity are untrue without XLogFlush().
---
contrib/pg_visibility/pg_visibility.c | 30 +++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 5d0deaba61e..32999ac9a97 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -19,6 +19,7 @@
#include "funcapi.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
+#include "storage/proc.h"
#include "storage/procarray.h"
#include "storage/read_stream.h"
#include "storage/smgr.h"
@@ -390,6 +391,7 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
Relation rel;
ForkNumber fork;
BlockNumber block;
+ BlockNumber old_block;
rel = relation_open(relid, AccessExclusiveLock);
@@ -399,12 +401,22 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
/* Forcibly reset cached file size */
RelationGetSmgr(rel)->smgr_cached_nblocks[VISIBILITYMAP_FORKNUM] = InvalidBlockNumber;
+ /* Compute new and old size before entering critical section. */
+ fork = VISIBILITYMAP_FORKNUM;
block = visibilitymap_prepare_truncate(rel, 0);
- if (BlockNumberIsValid(block))
- {
- fork = VISIBILITYMAP_FORKNUM;
- smgrtruncate(RelationGetSmgr(rel), &fork, 1, &block);
- }
+ old_block = BlockNumberIsValid(block) ? smgrnblocks(RelationGetSmgr(rel), fork) : 0;
+
+ /*
+ * Buffer dropping, file truncation and WAL logging must be effectively
+ * atomic. Interrupts are suppressed (buffer I/O waits can't be canceled),
+ * and we either succeed in all steps or panic. Also prevent checkpoint
+ * start/complete, so we can't crash and recover from a redo point after
+ * this record if we haven't finished truncating and fsync'ing the file(s).
+ * See RelationTruncate() for discussion.
+ */
+ Assert((MyProc->delayChkptFlags & (DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE)) == 0);
+ MyProc->delayChkptFlags |= DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE;
+ START_CRIT_SECTION();
if (RelationNeedsWAL(rel))
{
@@ -420,6 +432,12 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
XLogInsert(RM_SMGR_ID, XLOG_SMGR_TRUNCATE | XLR_SPECIAL_REL_UPDATE);
}
+ if (BlockNumberIsValid(block))
+ smgrtruncatefrom(RelationGetSmgr(rel), &fork, 1, &old_block, &block);
+
+ END_CRIT_SECTION();
+ MyProc->delayChkptFlags &= ~(DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE);
+
/*
* Release the lock right away, not at commit time.
*
@@ -430,7 +448,7 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
* here and when we sent the messages at our eventual commit. However,
* we're currently only sending a non-transactional smgr invalidation,
* which will have been posted to shared memory immediately from within
- * smgr_truncate. Therefore, there should be no race here.
+ * smgrtruncatefrom. Therefore, there should be no race here.
*
* The reason why it's desirable to release the lock early here is because
* of the possibility that someone will need to use this to blow away many
--
2.39.5
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-12-19 15:44 Robert Haas <robertmhaas@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Robert Haas @ 2024-12-19 15:44 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Dec 11, 2024 at 8:26 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> > I do wonder whether the new smgrtruncatefrom() should be used
> > everywhere instead of just from one of the call sites, but even if
> > that's desirable long-term, doing this much is still a lot better than
> > doing nothing. This data corrupting bug has been known and understood
> > for more than 4 years at this point.
>
> There are only three callers of smgrtruncate() in our tree:
>
> 1. This one.
> 2. Its redo-time counterpart.
> 3. pg_truncate_visibility_map(), in contrib/pg_visibility.
>
> The recovery environment promotes errors of both types to FATAL (the
> "we have no handler" case in errstart()), and the postmaster handles
> startup failure a bit like a PANIC. Or do you think PANIC and more
> similar code would be better?
I think mostly my thought was that if we could remove smgrtruncate()
entirely in favor of smgrtruncatefrom(), or just keep it called
smgrtruncate() but add a mandatory additional argument, that would be
less error-prone than having two versions between which future hackers
must pick.
> I am curious about the non-flushing though. RelationTruncate() also
> doesn't flush if there is no fsm or vm. Perhaps it always flushes in
> practice since VACUUM itself creates those, I'm not entirely sure yet.
I agree that we should make that unconditional. I think your analysis
shows that abandoning the WAL-before-data isn't safe, but even if
there were some doubt about whether your analysis is correct, we
should have a very strong bias against thinking that a rule as
fundamental as WAL-before-data is negotiable.
> PFA the first sketch of a patch to make pg_truncate_visibility_map()
> into... a cut-down version of RelationTruncate(). Still looking into
> the flushing, without which the comments in the patch are entirely
> bogus.
The patch looks to good to me. It seems to me that you could just add
an XLogFlush() call.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-12-20 02:53 Michael Paquier <michael@paquier.xyz>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Michael Paquier @ 2024-12-20 02:53 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Thomas Munro <thomas.munro@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Dec 19, 2024 at 10:44:05AM -0500, Robert Haas wrote:
> I think mostly my thought was that if we could remove smgrtruncate()
> entirely in favor of smgrtruncatefrom(), or just keep it called
> smgrtruncate() but add a mandatory additional argument, that would be
> less error-prone than having two versions between which future hackers
> must pick.
Hmm. Indeed. As a HEAD change, keeping only a smgrtruncate() is
tempting as it creates a parallel with md.c. I am not completely sure
how to make all that leaner with the smgrnblocks() calls that save the
old number of blocks for each fork. But perhaps Thomas has a fancy
idea if it comes down to that, and it could always be done later.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../Z2TcN-ypscC4Jomj@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-12-20 22:18 Thomas Munro <thomas.munro@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2024-12-20 22:18 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Robert Haas <robertmhaas@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Dec 20, 2024 at 3:53 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Thu, Dec 19, 2024 at 10:44:05AM -0500, Robert Haas wrote:
> > I think mostly my thought was that if we could remove smgrtruncate()
> > entirely in favor of smgrtruncatefrom(), or just keep it called
> > smgrtruncate() but add a mandatory additional argument, that would be
> > less error-prone than having two versions between which future hackers
> > must pick.
Yeah.
> Hmm. Indeed. As a HEAD change, keeping only a smgrtruncate() is
> tempting as it creates a parallel with md.c. I am not completely sure
> how to make all that leaner with the smgrnblocks() calls that save the
> old number of blocks for each fork. But perhaps Thomas has a fancy
> idea if it comes down to that, and it could always be done later.
I did that, but also back-patched it like that. I realise now that
that was an ABI mistake, and I plan to change it back to the v5
arrangement (smgrtruncate() unchanged, smgrtruncatefrom() with the new
argument) in the back-branches only, just in case someone is using raw
smgrtruncate() in compiled code in the wild. Will post a patch soon.
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-12-20 23:15 Thomas Munro <thomas.munro@gmail.com>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Thomas Munro @ 2024-12-20 23:15 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Robert Haas <robertmhaas@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, Dec 21, 2024 at 11:18 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Fri, Dec 20, 2024 at 3:53 PM Michael Paquier <michael@paquier.xyz> wrote:
> > Hmm. Indeed. As a HEAD change, keeping only a smgrtruncate() is
> > tempting as it creates a parallel with md.c. I am not completely sure
> > how to make all that leaner with the smgrnblocks() calls that save the
> > old number of blocks for each fork. But perhaps Thomas has a fancy
> > idea if it comes down to that, and it could always be done later.
>
> I did that, but also back-patched it like that. I realise now that
> that was an ABI mistake, and I plan to change it back to the v5
> arrangement (smgrtruncate() unchanged, smgrtruncatefrom() with the new
> argument) in the back-branches only, just in case someone is using raw
> smgrtruncate() in compiled code in the wild. Will post a patch soon.
Here. Better/tidier ideas welcome.
Attachments:
[application/x-patch] 0001-Restore-smgrtruncate-prototype-in-back-branches.patch (4.8K, ../../CA+hUKGJ4jqhJVTnwiMFbmVonSMn+E9MW3YDZ+JEz9ov_h19RNA@mail.gmail.com/2-0001-Restore-smgrtruncate-prototype-in-back-branches.patch)
download | inline diff:
From 54cd0e6f63f5e7e757d9d1e636507fa782ac8a4f Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Sat, 21 Dec 2024 11:40:09 +1300
Subject: [PATCH] Restore smgrtruncate() prototype in back-branches.
It's possible that external code is calling smgrtruncate(). Any
external callers that copied core code might like to consider the recent
changes to RelationTruncate(), but commit 38c579b0 should not have
changed the function prototype in the back-branches, per ABI stability
policy.
Restore smgrtruncate()'s traditional argument list in the back-branches,
but make it a wrapper for a new function smgrtruncate2(). The three
callers in core can use smgrtruncate2() directly. In master (18-to-be),
smgrtruncate2() is effectively renamed to smgrtruncate().
Discussion: https://postgr.es/m/CA%2BhUKG%2BThae6x6%2BjmQiuALQBT2Ae1ChjMh1%3DkMvJ8y_SBJZrvA%40mail.gmail.com
---
contrib/pg_visibility/pg_visibility.c | 2 +-
src/backend/catalog/storage.c | 4 ++--
src/backend/storage/smgr/smgr.c | 24 ++++++++++++++++++++++--
src/include/storage/smgr.h | 4 +++-
4 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index adeb9512a70..ad0f2a109a6 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -421,7 +421,7 @@ pg_truncate_visibility_map(PG_FUNCTION_ARGS)
}
if (BlockNumberIsValid(block))
- smgrtruncate(RelationGetSmgr(rel), &fork, 1, &old_block, &block);
+ smgrtruncate2(RelationGetSmgr(rel), &fork, 1, &old_block, &block);
END_CRIT_SECTION();
MyProc->delayChkptFlags &= ~(DELAY_CHKPT_START | DELAY_CHKPT_COMPLETE);
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index 5b22cf10990..11b3ea40200 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -418,7 +418,7 @@ RelationTruncate(Relation rel, BlockNumber nblocks)
* longer exist after truncation is complete, and then truncate the
* corresponding files on disk.
*/
- smgrtruncate(RelationGetSmgr(rel), forks, nforks, old_blocks, blocks);
+ smgrtruncate2(RelationGetSmgr(rel), forks, nforks, old_blocks, blocks);
END_CRIT_SECTION();
@@ -1059,7 +1059,7 @@ smgr_redo(XLogReaderState *record)
if (nforks > 0)
{
START_CRIT_SECTION();
- smgrtruncate(reln, forks, nforks, old_blocks, blocks);
+ smgrtruncate2(reln, forks, nforks, old_blocks, blocks);
END_CRIT_SECTION();
}
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index b3fb8bc3ea8..89d62d1dee8 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -693,6 +693,26 @@ smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum)
* smgrtruncate() -- Truncate the given forks of supplied relation to
* each specified numbers of blocks
*
+ * Backward-compatible version of smgrtruncate2() for the benefit of external
+ * callers. This version isn't used in PostgreSQL core code, and can't be
+ * used in a critical section.
+ */
+void
+smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks,
+ BlockNumber *nblocks)
+{
+ BlockNumber old_nblocks[MAX_FORKNUM + 1];
+
+ for (int i = 0; i < nforks; ++i)
+ old_nblocks[i] = smgrnblocks(reln, forknum[i]);
+
+ return smgrtruncate2(reln, forknum, nforks, old_nblocks, nblocks);
+}
+
+/*
+ * smgrtruncate2() -- Truncate the given forks of supplied relation to
+ * each specified numbers of blocks
+ *
* The truncation is done immediately, so this can't be rolled back.
*
* The caller must hold AccessExclusiveLock on the relation, to ensure that
@@ -704,8 +724,8 @@ smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum)
* to this relation should be called in between.
*/
void
-smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks,
- BlockNumber *old_nblocks, BlockNumber *nblocks)
+smgrtruncate2(SMgrRelation reln, ForkNumber *forknum, int nforks,
+ BlockNumber *old_nblocks, BlockNumber *nblocks)
{
int i;
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 98fabe9935c..3856d1d4f8b 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -104,8 +104,10 @@ extern void smgrwriteback(SMgrRelation reln, ForkNumber forknum,
extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum);
extern BlockNumber smgrnblocks_cached(SMgrRelation reln, ForkNumber forknum);
extern void smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks,
- BlockNumber *old_nblocks,
BlockNumber *nblocks);
+extern void smgrtruncate2(SMgrRelation reln, ForkNumber *forknum, int nforks,
+ BlockNumber *old_nblocks,
+ BlockNumber *nblocks);
extern void smgrimmedsync(SMgrRelation reln, ForkNumber forknum);
extern void smgrregistersync(SMgrRelation reln, ForkNumber forknum);
extern void AtEOXact_SMgr(void);
--
2.47.1
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2024-12-24 05:50 Michael Paquier <michael@paquier.xyz>
parent: Thomas Munro <thomas.munro@gmail.com>
0 siblings, 1 reply; 45+ messages in thread
From: Michael Paquier @ 2024-12-24 05:50 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, Dec 21, 2024 at 12:15:56PM +1300, Thomas Munro wrote:
> Here. Better/tidier ideas welcome.
Indeed, the state of back-branches is incorrect this way. I don't
know of any out-of-core callers of smgrtruncate(), but well, the world
is a wide place.
extern void smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks,
- BlockNumber *old_nblocks,
BlockNumber *nblocks);
+extern void smgrtruncate2(SMgrRelation reln, ForkNumber *forknum, int nforks,
+ BlockNumber *old_nblocks,
+ BlockNumber *nblocks);
Please don't rely on my naming sense, still I'm OK with what you are
using here.
Some inconsistent notes in the code with smgrtruncate2() in place:
src/backend/access/heap/visibilitymap.c:
* otherwise the caller is responsible for calling smgrtruncate()
src/backend/catalog/storage.c:
* Second, the call to smgrtruncate() below will in turn call
src/backend/access/heap/vacuumlazy.c:
* processed the smgr invalidation that smgrtruncate sent out ... but
That's not really critical, so leaving them as they are is equally OK
for me. Looks good to me, otherwise.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../Z2pLwQGNaKq2_JTk@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2025-01-07 21:57 Thomas Munro <thomas.munro@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 45+ messages in thread
From: Thomas Munro @ 2025-01-07 21:57 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Robert Haas <robertmhaas@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, Dec 24, 2024 at 6:51 PM Michael Paquier <michael@paquier.xyz> wrote:
> That's not really critical, so leaving them as they are is equally OK
> for me. Looks good to me, otherwise.
Thanks. I pushed it just like that. I didn't want to make up a
"nice" name for it and then have people getting attached to it, given
the decision that we should only retain only smgrtruncate() with the
new signature in master.
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2025-01-07 23:50 Michael Paquier <michael@paquier.xyz>
parent: Thomas Munro <thomas.munro@gmail.com>
1 sibling, 1 reply; 45+ messages in thread
From: Michael Paquier @ 2025-01-07 23:50 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Jan 08, 2025 at 10:57:20AM +1300, Thomas Munro wrote:
> Thanks. I pushed it just like that. I didn't want to make up a
> "nice" name for it and then have people getting attached to it, given
> the decision that we should only retain only smgrtruncate() with the
> new signature in master.
Sounds good to me. Thanks for the commit!
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../Z329yX7VREL86mnm@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2025-01-08 03:22 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 45+ messages in thread
From: Michael Paquier @ 2025-01-08 03:22 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Jan 08, 2025 at 08:50:33AM +0900, Michael Paquier wrote:
> Sounds good to me. Thanks for the commit!
Actually not that good, and wrasse is reporting a failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=wrasse&dt=2025-01-07%2022%3A26%3A00
"/export/home/nm/farm/studio64v12_6/REL_15_STABLE/pgsql.build/../pgsql/src/backend/storage/smgr/smgr.c",
line 634: void function cannot return value
It seems to me that you meant to do the following in the back
branches:
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -706,7 +706,7 @@ smgrtruncate(SMgrRelation reln, ForkNumber *forknum, int nforks,
for (int i = 0; i < nforks; ++i)
old_nblocks[i] = smgrnblocks(reln, forknum[i]);
- return smgrtruncate2(reln, forknum, nforks, old_nblocks, nblocks);
+ smgrtruncate2(reln, forknum, nforks, old_nblocks, nblocks);
}
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../Z33vgfVgvOnbFLN9@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2025-01-08 03:48 Thomas Munro <thomas.munro@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 45+ messages in thread
From: Thomas Munro @ 2025-01-08 03:48 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Robert Haas <robertmhaas@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Jan 8, 2025 at 4:22 PM Michael Paquier <michael@paquier.xyz> wrote:
> It seems to me that you meant to do the following in the back
> branches:
> - return smgrtruncate2(reln, forknum, nforks, old_nblocks, nblocks);
> + smgrtruncate2(reln, forknum, nforks, old_nblocks, nblocks);
Right, that was accepted by my compiler and apparently almost all the
others, but is not really allowed in C (it's allowed in C++, and
useful for generic forwarding). Duh. Will fix.
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2025-02-05 16:36 Andres Freund <andres@anarazel.de>
parent: Thomas Munro <thomas.munro@gmail.com>
1 sibling, 1 reply; 45+ messages in thread
From: Andres Freund @ 2025-02-05 16:36 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Robert Haas <robertmhaas@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
Hi,
On 2025-01-08 10:57:20 +1300, Thomas Munro wrote:
> On Tue, Dec 24, 2024 at 6:51 PM Michael Paquier <michael@paquier.xyz> wrote:
> > That's not really critical, so leaving them as they are is equally OK
> > for me. Looks good to me, otherwise.
>
> Thanks. I pushed it just like that.
I assume there's nothing left to do for the CF entry?
https://commitfest.postgresql.org/51/5242/
It doesn't matter that much anymore, with things not getting moved to new CFs,
but probably still worth updating.
Andres
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2025-02-06 02:43 Thomas Munro <thomas.munro@gmail.com>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 45+ messages in thread
From: Thomas Munro @ 2025-02-06 02:43 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Michael Paquier <michael@paquier.xyz>; Robert Haas <robertmhaas@gmail.com>; Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Feb 6, 2025 at 5:36 AM Andres Freund <andres@anarazel.de> wrote:
> On 2025-01-08 10:57:20 +1300, Thomas Munro wrote:
> > On Tue, Dec 24, 2024 at 6:51 PM Michael Paquier <michael@paquier.xyz> wrote:
> > > That's not really critical, so leaving them as they are is equally OK
> > > for me. Looks good to me, otherwise.
> >
> > Thanks. I pushed it just like that.
>
> I assume there's nothing left to do for the CF entry?
>
> https://commitfest.postgresql.org/51/5242/
>
> It doesn't matter that much anymore, with things not getting moved to new CFs,
> but probably still worth updating.
Done.
^ permalink raw reply [nested|flat] 45+ messages in thread
* Re: BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows
@ 2025-04-06 18:16 Noah Misch <noah@leadboat.com>
parent: Thomas Munro <thomas.munro@gmail.com>
1 sibling, 0 replies; 45+ messages in thread
From: Noah Misch @ 2025-04-06 18:16 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@gmail.com>; +Cc: Heikki Linnakangas <hlinnaka@iki.fi>; Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; Michael Paquier <michael@paquier.xyz>; Tom Lane <tgl@sss.pgh.pa.us>; Laurenz Albe <laurenz.albe@cybertec.at>; rootcause000@gmail.com; pgsql-bugs@lists.postgresql.org
On Fri, Sep 06, 2024 at 11:05:26AM +1200, Thomas Munro wrote:
> Subject: [PATCH v3 1/3] RelationTruncate() must set DELAY_CHKPT_START.
>
> Previously, it set only DELAY_CHKPT_COMPLETE. That was important,
> because it meant that if the XLOG_SMGR_TRUNCATE record preceded a
> XLOG_CHECKPOINT_ONLINE record in the WAL, then the truncation would also
> happen on disk before the XLOG_CHECKPOINT_ONLINE record was
> written.
I read this commit (75818b3) with intense interest, mostly to see if it found
a rule that commit 8e7e672 needs to follow and didn't (no). I did want to
find or make an example of interleaved events where DELAY_CHKPT_COMPLETE is
still necessary. Do any of you have such an example? Here were my
unsuccessful attempts:
==== Attempt: from upthread
On Wed, Oct 11, 2023 at 12:59:39PM -0400, Robert Haas wrote:
> Suppose that RelationTruncate set both DELAY_CHKPT_START and
> DELAY_CHKPT_COMPLETE. I think that would prevent this problem. P2
> could still choose the redo LSN after P1 logged the truncate, but it
> wouldn't then be able to reach CheckPointBuffers() until after P1 had
> reached RegisterSyncRequest. Note that setting *only*
> DELAY_CHKPT_START isn't good enough, because then we can get this
> history:
>
> P1: log truncate
> P2: choose redo LSN
> P2: CheckPointBuffers()
> P1: DropRelationBuffers()
> P2: ProcessSyncRequests()
> P2: log checkpoint
> *** system loses power ***
If "choose redo LSN" happens at the point shown there, DELAY_CHKPT_START won't
allow CheckPointBuffers() at the point shown there, so this interleaving
doesn't happen.
==== Attempt: XLOG_SMGR_TRUNCATE+DropRelationBuffers() just after checkpoint waits for DELAY_CHKPT_START
P2: choose redo LSN 1
P2: CheckPointBuffers() enter
P1: DELAY_CHKPT_START enter
P1: log truncate
P1: DropRelationBuffers()
P2: CheckPointBuffers() actual flushes
P2: ProcessSyncRequests()
P2: log checkpoint
P2: choose redo LSN 2 (next checkpoint)
*** system loses power, below were still in future ***
P1: ftruncate()
P1: RegisterSyncRequest()
P1: DELAY_CHKPT_START exit
DropRelationBuffers() made CheckPointBuffers() do less work, and the validity
of the checkpoint is now tied to truncate happening eventually. However, if
XLOG_CHECKPOINT_ONLINE reaches disk, that implies XLOG_SMGR_TRUNCATE reached
disk first. Any recovery starting at or before LSN 1 will redo
XLOG_SMGR_TRUNCATE. Data integrity is fine.
==== Attempt: replay finding "older contents than expected"
A key RelationTruncate() code comment mentions this:
* First, the truncation operation might drop buffers that the checkpoint
* otherwise would have flushed. If it does, then it's essential that the
* files actually get truncated on disk before the checkpoint record is
* written. Otherwise, if replay begins from that checkpoint, the
* to-be-truncated blocks might still exist on disk but have older
* contents than expected, which can cause replay to fail. It's OK for the
* blocks to not exist on disk at all, but not for them to have the wrong
* contents. For this reason, we need to set DELAY_CHKPT_COMPLETE while
* this code executes.
However, I can't see how to make that happen. RelationTruncate() has
AccessExclusiveLock on the relation, so no other WAL records for the truncated
block range are happening while we hold DELAY_CHKPT_START. In the previous
attempt, any WAL records for the truncated block range must be before
XLOG_SMGR_TRUNCATE (such records tolerate older content) or after
XLOG_CHECKPOINT_ONLINE (since RelationTruncate() held AccessExclusiveLock at
least that late).
==== Attempt: all truncate steps just after XLOG_CHECKPOINT_REDO
P1: DELAY_CHKPT_START enter
P2: choose redo LSN
P1: log truncate
P1: DropRelationBuffers()
P1: ftruncate()
P1: RegisterSyncRequest()
P1: DELAY_CHKPT_START exit
P2: CheckPointBuffers()
P2: ProcessSyncRequests()
P2: log checkpoint
*** system loses power ***
Recovery sees the XLOG_CHECKPOINT_ONLINE, which implies it also sees and
replays the XLOG_SMGR_TRUNCATE. Data integrity is fine.
==== Attempt: XLOG_SMGR_TRUNCATE before XLOG_CHECKPOINT_REDO
P1: DELAY_CHKPT_START enter
P1: log truncate
P2: choose redo LSN
P1: DropRelationBuffers()
P1: ftruncate()
P1: RegisterSyncRequest()
P1: DELAY_CHKPT_START exit
P2: CheckPointBuffers()
P2: ProcessSyncRequests()
P2: log checkpoint
*** system loses power ***
XLOG_SMGR_TRUNCATE precedes the redo point, so recovery finds no need to redo
the XLOG_SMGR_TRUNCATE. Data integrity is fine.
^ permalink raw reply [nested|flat] 45+ messages in thread
end of thread, other threads:[~2025-04-06 18:16 UTC | newest]
Thread overview: 45+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-10-03 17:25 BUG #18146: Rows reappearing in Tables after Auto-Vacuum Failure in PostgreSQL on Windows PG Bug reporting form <noreply@postgresql.org>
2023-10-04 07:17 ` Laurenz Albe <laurenz.albe@cybertec.at>
2023-10-04 07:24 ` Michael Paquier <michael@paquier.xyz>
2023-10-04 14:24 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-04 21:12 ` Thomas Munro <thomas.munro@gmail.com>
2023-10-04 22:38 ` Thomas Munro <thomas.munro@gmail.com>
2023-10-04 22:44 ` Michael Paquier <michael@paquier.xyz>
2023-10-05 19:55 ` Thomas Munro <thomas.munro@gmail.com>
2023-10-05 20:18 ` Thomas Munro <thomas.munro@gmail.com>
2023-10-06 04:46 ` Thomas Munro <thomas.munro@gmail.com>
2023-10-11 16:59 ` Robert Haas <robertmhaas@gmail.com>
2023-10-12 03:36 ` Thomas Munro <thomas.munro@gmail.com>
2023-10-19 19:50 ` Robert Haas <robertmhaas@gmail.com>
2023-10-20 03:16 ` Thomas Munro <thomas.munro@gmail.com>
2023-10-20 03:38 ` Thomas Munro <thomas.munro@gmail.com>
2023-10-20 05:17 ` Thomas Munro <thomas.munro@gmail.com>
2023-10-23 15:06 ` Robert Haas <robertmhaas@gmail.com>
2023-10-23 21:54 ` Thomas Munro <thomas.munro@gmail.com>
2024-04-23 07:48 ` Thomas Munro <thomas.munro@gmail.com>
2024-05-01 05:00 ` Michael Paquier <michael@paquier.xyz>
2024-05-14 13:00 ` Alexander Lakhin <exclusion@gmail.com>
2024-06-26 20:58 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-06-26 20:59 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-09-05 23:05 ` Thomas Munro <thomas.munro@gmail.com>
2024-09-09 07:21 ` Michael Paquier <michael@paquier.xyz>
2024-09-09 08:57 ` Thomas Munro <thomas.munro@gmail.com>
2024-09-10 00:43 ` Thomas Munro <thomas.munro@gmail.com>
2024-09-11 03:20 ` Thomas Munro <thomas.munro@gmail.com>
2024-10-29 07:48 ` Michael Paquier <michael@paquier.xyz>
2024-11-14 15:51 ` Robert Haas <robertmhaas@gmail.com>
2024-12-11 13:32 ` Thomas Munro <thomas.munro@gmail.com>
2024-12-19 15:44 ` Robert Haas <robertmhaas@gmail.com>
2024-12-20 02:53 ` Michael Paquier <michael@paquier.xyz>
2024-12-20 22:18 ` Thomas Munro <thomas.munro@gmail.com>
2024-12-20 23:15 ` Thomas Munro <thomas.munro@gmail.com>
2024-12-24 05:50 ` Michael Paquier <michael@paquier.xyz>
2025-01-07 21:57 ` Thomas Munro <thomas.munro@gmail.com>
2025-01-07 23:50 ` Michael Paquier <michael@paquier.xyz>
2025-01-08 03:22 ` Michael Paquier <michael@paquier.xyz>
2025-01-08 03:48 ` Thomas Munro <thomas.munro@gmail.com>
2025-02-05 16:36 ` Andres Freund <andres@anarazel.de>
2025-02-06 02:43 ` Thomas Munro <thomas.munro@gmail.com>
2025-04-06 18:16 ` Noah Misch <noah@leadboat.com>
2023-10-04 23:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-11 14:44 ` Robert Haas <robertmhaas@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