public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 1/4] bootstrap: convert Typ to a List*
11+ messages / 3 participants
[nested] [flat]
* [PATCH 1/4] bootstrap: convert Typ to a List*
@ 2020-11-20 02:48 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Justin Pryzby @ 2020-11-20 02:48 UTC (permalink / raw)
---
src/backend/bootstrap/bootstrap.c | 89 ++++++++++++++-----------------
1 file changed, 41 insertions(+), 48 deletions(-)
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 6f615e6622..1b940d9d27 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -58,7 +58,7 @@ static void BootstrapModeMain(void);
static void bootstrap_signals(void);
static void ShutdownAuxiliaryProcess(int code, Datum arg);
static Form_pg_attribute AllocateAttribute(void);
-static void populate_typ_array(void);
+static void populate_typ(void);
static Oid gettype(char *type);
static void cleanup(void);
@@ -159,7 +159,7 @@ struct typmap
FormData_pg_type am_typ;
};
-static struct typmap **Typ = NULL;
+static List *Typ = NIL; /* List of struct typmap* */
static struct typmap *Ap = NULL;
static Datum values[MAXATTR]; /* current row's attribute values */
@@ -595,10 +595,10 @@ boot_openrel(char *relname)
/*
* pg_type must be filled before any OPEN command is executed, hence we
- * can now populate the Typ array if we haven't yet.
+ * can now populate Typ if we haven't yet.
*/
- if (Typ == NULL)
- populate_typ_array();
+ if (Typ == NIL)
+ populate_typ();
if (boot_reldesc != NULL)
closerel(NULL);
@@ -688,7 +688,7 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
typeoid = gettype(type);
- if (Typ != NULL)
+ if (Typ != NIL)
{
attrtypes[attnum]->atttypid = Ap->am_oid;
attrtypes[attnum]->attlen = Ap->am_typ.typlen;
@@ -866,47 +866,36 @@ cleanup(void)
}
/* ----------------
- * populate_typ_array
+ * populate_typ
*
- * Load the Typ array by reading pg_type.
+ * Load Typ by reading pg_type.
* ----------------
*/
static void
-populate_typ_array(void)
+populate_typ(void)
{
Relation rel;
TableScanDesc scan;
HeapTuple tup;
- int nalloc;
- int i;
-
- Assert(Typ == NULL);
+ MemoryContext old;
- nalloc = 512;
- Typ = (struct typmap **)
- MemoryContextAlloc(TopMemoryContext, nalloc * sizeof(struct typmap *));
+ Assert(Typ == NIL);
rel = table_open(TypeRelationId, NoLock);
scan = table_beginscan_catalog(rel, 0, NULL);
- i = 0;
+ old = MemoryContextSwitchTo(TopMemoryContext);
while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL)
{
Form_pg_type typForm = (Form_pg_type) GETSTRUCT(tup);
+ struct typmap *newtyp;
- /* make sure there will be room for a trailing NULL pointer */
- if (i >= nalloc - 1)
- {
- nalloc *= 2;
- Typ = (struct typmap **)
- repalloc(Typ, nalloc * sizeof(struct typmap *));
- }
- Typ[i] = (struct typmap *)
- MemoryContextAlloc(TopMemoryContext, sizeof(struct typmap));
- Typ[i]->am_oid = typForm->oid;
- memcpy(&(Typ[i]->am_typ), typForm, sizeof(Typ[i]->am_typ));
- i++;
+ newtyp = (struct typmap *) palloc(sizeof(struct typmap));
+ Typ = lappend(Typ, newtyp);
+
+ newtyp->am_oid = typForm->oid;
+ memcpy(&newtyp->am_typ, typForm, sizeof(newtyp->am_typ));
}
- Typ[i] = NULL; /* Fill trailing NULL pointer */
+ MemoryContextSwitchTo(old);
table_endscan(scan);
table_close(rel, NoLock);
}
@@ -916,25 +905,26 @@ populate_typ_array(void)
*
* NB: this is really ugly; it will return an integer index into TypInfo[],
* and not an OID at all, until the first reference to a type not known in
- * TypInfo[]. At that point it will read and cache pg_type in the Typ array,
+ * TypInfo[]. At that point it will read and cache pg_type in Typ,
* and subsequently return a real OID (and set the global pointer Ap to
* point at the found row in Typ). So caller must check whether Typ is
- * still NULL to determine what the return value is!
+ * still NIL to determine what the return value is!
* ----------------
*/
static Oid
gettype(char *type)
{
- if (Typ != NULL)
+ if (Typ != NIL)
{
- struct typmap **app;
+ ListCell *lc;
- for (app = Typ; *app != NULL; app++)
+ foreach (lc, Typ)
{
- if (strncmp(NameStr((*app)->am_typ.typname), type, NAMEDATALEN) == 0)
+ struct typmap *app = lfirst(lc);
+ if (strncmp(NameStr(app->am_typ.typname), type, NAMEDATALEN) == 0)
{
- Ap = *app;
- return (*app)->am_oid;
+ Ap = app;
+ return app->am_oid;
}
}
}
@@ -949,7 +939,7 @@ gettype(char *type)
}
/* Not in TypInfo, so we'd better be able to read pg_type now */
elog(DEBUG4, "external type: %s", type);
- populate_typ_array();
+ populate_typ();
return gettype(type);
}
elog(ERROR, "unrecognized type \"%s\"", type);
@@ -977,17 +967,20 @@ boot_get_type_io_data(Oid typid,
Oid *typinput,
Oid *typoutput)
{
- if (Typ != NULL)
+ if (Typ != NIL)
{
/* We have the boot-time contents of pg_type, so use it */
- struct typmap **app;
- struct typmap *ap;
-
- app = Typ;
- while (*app && (*app)->am_oid != typid)
- ++app;
- ap = *app;
- if (ap == NULL)
+ struct typmap *ap = NULL;
+ ListCell *lc;
+
+ foreach (lc, Typ)
+ {
+ ap = lfirst(lc);
+ if (ap->am_oid == typid)
+ break;
+ }
+
+ if (!ap || ap->am_oid != typid)
elog(ERROR, "type OID %u not found in Typ list", typid);
*typlen = ap->am_typ.typlen;
--
2.26.2
--------------BCA1373AE29B32B57608F4EE
Content-Type: text/x-patch; charset=UTF-8;
name="0002-Allow-composite-types-in-bootstrap-20210307.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="0002-Allow-composite-types-in-bootstrap-20210307.patch"
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
@ 2023-01-31 01:09 Anton A. Melnikov <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Anton A. Melnikov @ 2023-01-31 01:09 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers
Hello!
On 24.11.2022 04:02, Thomas Munro wrote:
> On Thu, Nov 24, 2022 at 11:05 AM Tom Lane <[email protected]> wrote:
>> Thomas Munro <[email protected]> writes:
>
> ERROR: calculated CRC checksum does not match value stored in file
>
> The attached draft patch fixes it.
Tried to catch this error on my PC, but failed to do it within a reasonable time.
The 1s interval is probably too long for me.
It seems there are more durable way to reproduce this bug with 0001 patch applied:
At the first backend:
do $$ begin loop perform pg_update_control_file(); end loop; end; $$;
At the second one:
do $$ begin loop perform pg_control_system(); end loop; end; $$;
It will fails almost immediately with:
"ERROR: calculated CRC checksum does not match value stored in file"
both with fsync = off and fsync = on.
Checked it out for master and REL_11_STABLE.
Also checked for a few hours that the patch 0002 fixes this error,
but there are some questions to its logical structure.
The equality between the previous and newly calculated crc is checked only
if the last crc calculation was wrong, i.e not equal to the value stored in the file.
It is very unlikely that in this case the previous and new crc can match, so, in fact,
the loop will spin until crc is calculated correctly. In the other words,
this algorithm is practically equivalent to an infinite loop of reading from a file
and calculating crc while(EQ_CRC32C(crc, ControlFile->crc) != true).
But then it can be simplified significantly by removing checksums equality checks,
bool fist_try and by limiting the maximum number of iterations
with some constant in the e.g. for loop to avoid theoretically possible freeze.
Or maybe use the variant suggested by Tom Lane, i.e, as far as i understand,
repeat the file_open-read-close-calculate_crc sequence twice without a pause between
them and check the both calculated crcs for the equality. If they match, exit and return
the bool result of comparing between the last calculation with the value from the file,
if not, take a pause and repeat everything from the beginning.
In this case, no need to check *crc_ok_p inside get_controlfile()
as it was in the present version; i think it's more logically correct
since this variable is intended top-level functions and the logic
inside get_controlfile() should not depend on its value.
Also found a warning in 0001 patch for master branch. On my PC gcc gives:
xlog.c:2507:1: warning: no previous prototype for ‘pg_update_control_file’ [-Wmissing-prototypes]
2507 | pg_update_control_file()
Fixed it with #include "utils/fmgrprotos.h" to xlog.c and
add PG_FUNCTION_ARGS to pg_update_control_file().
With the best wishes,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
@ 2023-01-31 04:09 Thomas Munro <[email protected]>
parent: Anton A. Melnikov <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Thomas Munro @ 2023-01-31 04:09 UTC (permalink / raw)
To: Anton A. Melnikov <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Tue, Jan 31, 2023 at 2:10 PM Anton A. Melnikov <[email protected]> wrote:
> Also checked for a few hours that the patch 0002 fixes this error,
> but there are some questions to its logical structure.
Hi Anton,
Thanks for looking!
> The equality between the previous and newly calculated crc is checked only
> if the last crc calculation was wrong, i.e not equal to the value stored in the file.
> It is very unlikely that in this case the previous and new crc can match, so, in fact,
> the loop will spin until crc is calculated correctly. In the other words,
> this algorithm is practically equivalent to an infinite loop of reading from a file
> and calculating crc while(EQ_CRC32C(crc, ControlFile->crc) != true).
Maybe it's unlikely that two samples will match while running that
torture test, because it's overwriting the file as fast as it can.
But the idea is that a real system isn't writing the control file most
of the time.
> But then it can be simplified significantly by removing checksums equality checks,
> bool fist_try and by limiting the maximum number of iterations
> with some constant in the e.g. for loop to avoid theoretically possible freeze.
Yeah, I was thinking that we should also put a limit on the loop, just
to be cautious.
Primary servers write the control file very infrequently. Standybys
more frequently, while writing data out, maybe every few seconds on a
busy system writing out a lot of data. UpdateMinRecoveryPoint() makes
some effort to avoid updating the file too often. You definitely see
bursts of repeated flushes that might send this thing in a loop for a
while if the timings were exactly wrong, but usually with periodic
gaps; I haven't really studied the expected behaviour too closely.
> Or maybe use the variant suggested by Tom Lane, i.e, as far as i understand,
> repeat the file_open-read-close-calculate_crc sequence twice without a pause between
> them and check the both calculated crcs for the equality. If they match, exit and return
> the bool result of comparing between the last calculation with the value from the file,
> if not, take a pause and repeat everything from the beginning.
Hmm. Would it be good enough to do two read() calls with no sleep in
between? How sure are we that a concurrent write will manage to
change at least one bit that our second read can see? I guess it's
likely, but maybe hypervisors, preemptible kernels, I/O interrupts or
a glitch in the matrix could decrease our luck? I really don't know.
So I figured I should add a small sleep between the reads to change
the odds in our favour. But we don't want to slow down all reads of
the control file with a sleep, do we? So I thought we should only
bother doing this slow stuff if the actual CRC check fails, a low
probability event.
Clearly there is an element of speculation or superstition here. I
don't know what else to do if both PostgreSQL and ext4 decided not to
add interlocking. Maybe we should rethink that. How bad would it
really be if control file access used POSIX file locking? I mean, the
writer is going to *fsync* the file, so it's not like one more wafer
thin system call is going to hurt too much.
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
@ 2023-01-31 11:38 Thomas Munro <[email protected]>
parent: Thomas Munro <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Thomas Munro @ 2023-01-31 11:38 UTC (permalink / raw)
To: Anton A. Melnikov <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Tue, Jan 31, 2023 at 5:09 PM Thomas Munro <[email protected]> wrote:
> Clearly there is an element of speculation or superstition here. I
> don't know what else to do if both PostgreSQL and ext4 decided not to
> add interlocking. Maybe we should rethink that. How bad would it
> really be if control file access used POSIX file locking? I mean, the
> writer is going to *fsync* the file, so it's not like one more wafer
> thin system call is going to hurt too much.
Here's an experimental patch for that alternative. I wonder if
someone would want to be able to turn it off for some reason -- maybe
some NFS problem? It's less back-patchable, but maybe more
principled?
I don't know if Windows suffers from this type of problem.
Unfortunately its equivalent functionality LockFile() looks non-ideal
for this purpose: if your program crashes, the documentation is very
vague on when exactly it is released by the OS, but it's not
immediately on process exit. That seems non-ideal for a control file
you might want to access again very soon after a crash, to be able to
recover.
A thought in passing: if UpdateMinRecoveryPoint() performance is an
issue, maybe we should figure out how to use fdatasync() instead of
fsync().
Attachments:
[text/x-patch] 0001-Lock-pg_control-while-reading-or-writing.patch (3.0K, ../../CA+hUKGJBrKENOmq6zHGMOh637cjHL6VeOc1NF98Vtq9c962oBA@mail.gmail.com/2-0001-Lock-pg_control-while-reading-or-writing.patch)
download | inline diff:
From fe71350b0874adbec97c612e7b80e7179c6c48e9 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Tue, 31 Jan 2023 20:01:49 +1300
Subject: [PATCH 1/2] Lock pg_control while reading or writing.
Front-end programs that read pg_control and user-facing functions run
in the backend read pg_control without acquiring ControlFileLock. If
you're unlucky enough to read() while the backend is in write(), on at
least Linux ext4 you might see partial data. Use a POSIX advisory lock
to avoid this problem.
---
src/common/controldata_utils.c | 69 ++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index 9723587466..284895bdd9 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -39,6 +39,42 @@
#include "storage/fd.h"
#endif
+/*
+ * Advisory-lock the control file until closed.
+ */
+static int
+lock_file(int fd, bool exclusive)
+{
+#ifdef WIN32
+ /*
+ * LockFile() might work, but it seems dangerous because there are reports
+ * that the lock can sometimes linger if a program crashes without
+ * unlocking. That would prevent recovery after a PANIC, so we'd better
+ * not use it without more research. Because of the lack of portability,
+ * this function is kept private to controldata_utils.c.
+ */
+ return 0;
+#else
+ struct flock lock;
+ int rc;
+
+ memset(&lock, 0, sizeof(lock));
+ lock.l_type = exclusive ? F_WRLCK : F_RDLCK;
+ lock.l_start = 0;
+ lock.l_whence = SEEK_SET;
+ lock.l_len = 0;
+ lock.l_pid = -1;
+
+ do
+ {
+ rc = fcntl(fd, F_SETLKW, &lock);
+ }
+ while (rc < 0 && errno == EINTR);
+
+ return rc;
+#endif
+}
+
/*
* get_controlfile()
*
@@ -74,6 +110,20 @@ get_controlfile(const char *DataDir, bool *crc_ok_p)
ControlFilePath);
#endif
+ /* Make sure we can read the file atomically. */
+ if (lock_file(fd, false) < 0)
+ {
+#ifndef FRONTEND
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not lock file \"%s\" for reading: %m",
+ ControlFilePath)));
+#else
+ pg_fatal("could not lock file \"%s\" for reading: %m",
+ ControlFilePath);
+#endif
+ }
+
r = read(fd, ControlFile, sizeof(ControlFileData));
if (r != sizeof(ControlFileData))
{
@@ -186,6 +236,25 @@ update_controlfile(const char *DataDir,
pg_fatal("could not open file \"%s\": %m", ControlFilePath);
#endif
+ /*
+ * Make sure that any concurrent reader (including frontend programs) can
+ * read the file atomically. Note that this refers to atomicity of
+ * concurrent reads and writes. For our assumption of atomicity under
+ * power failure, see PG_CONTROL_MAX_SAFE_SIZE.
+ */
+ if (lock_file(fd, true) < 0)
+ {
+#ifndef FRONTEND
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not lock file \"%s\" for writing: %m",
+ ControlFilePath)));
+#else
+ pg_fatal("could not lock file \"%s\" for writing: %m",
+ ControlFilePath);
+#endif
+ }
+
errno = 0;
#ifndef FRONTEND
pgstat_report_wait_start(WAIT_EVENT_CONTROL_FILE_WRITE_UPDATE);
--
2.35.1
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
@ 2023-02-01 04:04 Anton A. Melnikov <[email protected]>
parent: Thomas Munro <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Anton A. Melnikov @ 2023-02-01 04:04 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
Hi, Thomas!
There are two variants of the patch now.
1) As for the first workaround:
On 31.01.2023 07:09, Thomas Munro wrote:
>
> Maybe it's unlikely that two samples will match while running that
> torture test, because it's overwriting the file as fast as it can.
> But the idea is that a real system isn't writing the control file most
> of the time.
>
........
> Yeah, I was thinking that we should also put a limit on the loop, just
> to be cautious.
At first i didn’t understand that the equality condition with the previous
calculated crc and the current one at the second+ attempts was intended
for the case when the pg_control file is really corrupted.
Indeed, by making a few debugging variables and running the tortue test,
i found that there were ~4000 crc errors (~0.0003%) in ~125 million reads from the file,
and there was no case when the crc error appeared twice in a row.
So the second and moreover the third successive occurrence of an crc error
can be neglected and for this workaround seems a simpler and maybe more clear
algorithm is possible.
For instance:
for(try = 0 ; try < 3; try++)
{
open, read from and close pg_control;
calculate crc;
*crc_ok_p = EQ_CRC32C(crc, ControlFile->crc);
if(*crc_ok_p)
break;
}
2) As for the second variant of the patch with POSIX locks:
On 31.01.2023 14:38, Thomas Munro wrote:
> On Tue, Jan 31, 2023 at 5:09 PM Thomas Munro <[email protected]> wrote:
>> Clearly there is an element of speculation or superstition here. I
>> don't know what else to do if both PostgreSQL and ext4 decided not to
>> add interlocking. Maybe we should rethink that. How bad would it
>> really be if control file access used POSIX file locking? I mean, the
>> writer is going to *fsync* the file, so it's not like one more wafer
>> thin system call is going to hurt too much.
>
> Here's an experimental patch for that alternative. I wonder if
> someone would want to be able to turn it off for some reason -- maybe
> some NFS problem? It's less back-patchable, but maybe more
> principled?
It looks very strange to me that there may be cases where the cluster data
is stored in NFS. Can't figure out how this can be useful.
i think this variant of the patch is a normal solution
of the problem, not workaround. Found no problems on Linux.
+1 for this variant.
Might add a custom error message for EDEADLK
since it absent in errcode_for_file_access()?
> I don't know if Windows suffers from this type of problem.
> Unfortunately its equivalent functionality LockFile() looks non-ideal
> for this purpose: if your program crashes, the documentation is very
> vague on when exactly it is released by the OS, but it's not
> immediately on process exit. That seems non-ideal for a control file
> you might want to access again very soon after a crash, to be able to
> recover.
Unfortunately i've not had time to reproduce the problem and apply this patch on
Windows yet but i'm going to do it soon on windows 10. If a crc error
will occur there, then we might use the workaround from the first
variant of the patch.
> A thought in passing: if UpdateMinRecoveryPoint() performance is an
> issue, maybe we should figure out how to use fdatasync() instead of
> fsync().
May be choose it in accordance with GUC wal_sync_method?
Sincerely yours,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
@ 2023-02-01 06:45 Thomas Munro <[email protected]>
parent: Anton A. Melnikov <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Thomas Munro @ 2023-02-01 06:45 UTC (permalink / raw)
To: Anton A. Melnikov <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Wed, Feb 1, 2023 at 5:04 PM Anton A. Melnikov <[email protected]> wrote:
> On 31.01.2023 14:38, Thomas Munro wrote:
> > Here's an experimental patch for that alternative. I wonder if
> > someone would want to be able to turn it off for some reason -- maybe
> > some NFS problem? It's less back-patchable, but maybe more
> > principled?
>
> It looks very strange to me that there may be cases where the cluster data
> is stored in NFS. Can't figure out how this can be useful.
Heh. There are many interesting failure modes, but people do it. I
guess my more general question when introducing any new system call
into this code is how some unusual system I can't even access is going
to break. Maybe some obscure filesystem will fail with EOPNOTSUPP, or
take 5 seconds and then fail because there is no lock server
configured or whatever, so that's why I don't think we can back-patch
it, and we probably need a way to turn it off.
> i think this variant of the patch is a normal solution
> of the problem, not workaround. Found no problems on Linux.
> +1 for this variant.
I prefer it too.
> Might add a custom error message for EDEADLK
> since it absent in errcode_for_file_access()?
Ah, good thought. I think it shouldn't happen™, so it's OK that
errcode_for_file_access() would classify it as ERRCODE_INTERNAL_ERROR.
Other interesting errors are:
ENOLCK: system limits exceeded; PANIC seems reasonable
EOPNOTSUPP: this file doesn't support locking (seen on FreeBSD man
pages, not on POSIX)
> > I don't know if Windows suffers from this type of problem.
> > Unfortunately its equivalent functionality LockFile() looks non-ideal
> > for this purpose: if your program crashes, the documentation is very
> > vague on when exactly it is released by the OS, but it's not
> > immediately on process exit. That seems non-ideal for a control file
> > you might want to access again very soon after a crash, to be able to
> > recover.
>
> Unfortunately i've not had time to reproduce the problem and apply this patch on
> Windows yet but i'm going to do it soon on windows 10. If a crc error
> will occur there, then we might use the workaround from the first
> variant of the patch.
Thank you for investigating. I am afraid to read your results.
One idea would be to proceed with LockFile() for Windows, with a note
suggesting you file a bug with your OS vendor if you ever need it to
get unstuck. Googling this subject, I read that MongoDB used to
suffer from stuck lock files, until an OS bug report led to recent
versions releasing locks more promptly. I find that sufficiently
scary that I would want to default the feature to off on Windows, even
if your testing shows that it does really need it.
> > A thought in passing: if UpdateMinRecoveryPoint() performance is an
> > issue, maybe we should figure out how to use fdatasync() instead of
> > fsync().
>
> May be choose it in accordance with GUC wal_sync_method?
Here's a patch like that. I don't know if it's a good idea for
wal_sync_method to affect other kinds of files or not, but, then, it
already does (fsync_writethough changes this behaviour). I would
only want to consider this if we also stop choosing "open_datasync" as
a default on at least Windows.
Attachments:
[text/x-patch] 0001-Apply-wal_sync_method-to-pg_control-file.patch (5.4K, ../../CA+hUKGKZoCCn+6Z_dmwQsKu5PVLbDu6G7fD6JU1qok84r1ry0A@mail.gmail.com/2-0001-Apply-wal_sync_method-to-pg_control-file.patch)
download | inline diff:
From f55c98daefc4eec7f05413edf92b3b1e7070e1ef Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Tue, 31 Jan 2023 21:08:12 +1300
Subject: [PATCH] Apply wal_sync_method to pg_control file.
When writing out the control file frequently on a standby, we can go a
little faster on some systems by using fdatasync() instead of fsync(),
with no loss of safety. We already respected the special
"wal_sync_method=fsync_writethrough" via a circuitous route, but we
might as well support the full range of methods.
---
src/backend/access/transam/xlog.c | 19 ++++++++-
src/common/controldata_utils.c | 59 ++++++++++++++++++++------
src/include/common/controldata_utils.h | 7 ++-
3 files changed, 70 insertions(+), 15 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index fb4c860bde..d0a7a3d7eb 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4165,7 +4165,24 @@ ReadControlFile(void)
static void
UpdateControlFile(void)
{
- update_controlfile(DataDir, ControlFile, true);
+ int sync_op;
+
+ switch (sync_method)
+ {
+ case SYNC_METHOD_FDATASYNC:
+ sync_op = UPDATE_CONTROLFILE_FDATASYNC;
+ break;
+ case SYNC_METHOD_OPEN:
+ sync_op = UPDATE_CONTROLFILE_O_SYNC;
+ break;
+ case SYNC_METHOD_OPEN_DSYNC:
+ sync_op = UPDATE_CONTROLFILE_O_DSYNC;
+ break;
+ default:
+ sync_op = UPDATE_CONTROLFILE_FSYNC;
+ }
+
+ update_controlfile(DataDir, ControlFile, sync_op);
}
/*
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index 9723587466..1bc1c6f8d4 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -136,18 +136,39 @@ get_controlfile(const char *DataDir, bool *crc_ok_p)
* update_controlfile()
*
* Update controlfile values with the contents given by caller. The
- * contents to write are included in "ControlFile". "do_sync" can be
- * optionally used to flush the updated control file. Note that it is up
- * to the caller to properly lock ControlFileLock when calling this
- * routine in the backend.
+ * contents to write are included in "ControlFile". "sync_op" can be set
+ * to 0 or a synchronization method UPDATE_CONTROLFILE_XXX. In frontend code,
+ * only 0 and non-zero (fsync) are meaningful.
+ * Note that it is up to the caller to properly lock ControlFileLock when
+ * calling this routine in the backend.
*/
void
update_controlfile(const char *DataDir,
- ControlFileData *ControlFile, bool do_sync)
+ ControlFileData *ControlFile,
+ int sync_op)
{
int fd;
char buffer[PG_CONTROL_FILE_SIZE];
char ControlFilePath[MAXPGPATH];
+ int open_flag;
+
+ switch (sync_op)
+ {
+#ifndef FRONTEND
+#ifdef O_SYNC
+ case UPDATE_CONTROLFILE_O_SYNC:
+ open_flag = O_SYNC;
+ break;
+#endif
+#ifdef O_DSYNC
+ case UPDATE_CONTROLFILE_O_DSYNC:
+ open_flag = O_DSYNC;
+ break;
+#endif
+#endif
+ default:
+ open_flag = 0;
+ }
/* Update timestamp */
ControlFile->time = (pg_time_t) time(NULL);
@@ -175,13 +196,13 @@ update_controlfile(const char *DataDir,
* All errors issue a PANIC, so no need to use OpenTransientFile() and to
* worry about file descriptor leaks.
*/
- if ((fd = BasicOpenFile(ControlFilePath, O_RDWR | PG_BINARY)) < 0)
+ if ((fd = BasicOpenFile(ControlFilePath, O_RDWR | PG_BINARY | open_flag)) < 0)
ereport(PANIC,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m",
ControlFilePath)));
#else
- if ((fd = open(ControlFilePath, O_WRONLY | PG_BINARY,
+ if ((fd = open(ControlFilePath, O_WRONLY | PG_BINARY | open_flag,
pg_file_create_mode)) == -1)
pg_fatal("could not open file \"%s\": %m", ControlFilePath);
#endif
@@ -209,17 +230,29 @@ update_controlfile(const char *DataDir,
pgstat_report_wait_end();
#endif
- if (do_sync)
+ if (sync_op != 0)
{
#ifndef FRONTEND
pgstat_report_wait_start(WAIT_EVENT_CONTROL_FILE_SYNC_UPDATE);
- if (pg_fsync(fd) != 0)
- ereport(PANIC,
- (errcode_for_file_access(),
- errmsg("could not fsync file \"%s\": %m",
- ControlFilePath)));
+ if (sync_op == UPDATE_CONTROLFILE_FDATASYNC)
+ {
+ if (fdatasync(fd) != 0)
+ ereport(PANIC,
+ (errcode_for_file_access(),
+ errmsg("could not fdatasync file \"%s\": %m",
+ ControlFilePath)));
+ }
+ else if (sync_op == UPDATE_CONTROLFILE_FSYNC)
+ {
+ if (pg_fsync(fd) != 0)
+ ereport(PANIC,
+ (errcode_for_file_access(),
+ errmsg("could not fdatasync file \"%s\": %m",
+ ControlFilePath)));
+ }
pgstat_report_wait_end();
#else
+ /* In frontend code, non-zero sync_op gets you plain fsync(). */
if (fsync(fd) != 0)
pg_fatal("could not fsync file \"%s\": %m", ControlFilePath);
#endif
diff --git a/src/include/common/controldata_utils.h b/src/include/common/controldata_utils.h
index 49e7c52d31..879455c8fb 100644
--- a/src/include/common/controldata_utils.h
+++ b/src/include/common/controldata_utils.h
@@ -12,8 +12,13 @@
#include "catalog/pg_control.h"
+#define UPDATE_CONTROLFILE_FSYNC 1
+#define UPDATE_CONTROLFILE_FDATASYNC 2
+#define UPDATE_CONTROLFILE_O_SYNC 3
+#define UPDATE_CONTROLFILE_O_DSYNC 4
+
extern ControlFileData *get_controlfile(const char *DataDir, bool *crc_ok_p);
extern void update_controlfile(const char *DataDir,
- ControlFileData *ControlFile, bool do_sync);
+ ControlFileData *ControlFile, int sync_op);
#endif /* COMMON_CONTROLDATA_UTILS_H */
--
2.35.1
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
@ 2023-02-14 03:38 Anton A. Melnikov <[email protected]>
parent: Thomas Munro <[email protected]>
0 siblings, 2 replies; 11+ messages in thread
From: Anton A. Melnikov @ 2023-02-14 03:38 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
Hi, Thomas!
Thanks for your rapid answer and sorry for my delay with reply.
On 01.02.2023 09:45, Thomas Munro wrote:
>> Might add a custom error message for EDEADLK
>> since it absent in errcode_for_file_access()?
>
> Ah, good thought. I think it shouldn't happen™, so it's OK that
> errcode_for_file_access() would classify it as ERRCODE_INTERNAL_ERROR.
Yes, i also think that is impossible since the lock is taken on
the entire file, so ERRCODE_INTERNAL_ERROR will be right here.
> Other interesting errors are:
>
> ENOLCK: system limits exceeded; PANIC seems reasonable
> EOPNOTSUPP: this file doesn't support locking (seen on FreeBSD man
> pages, not on POSIX)
Agreed that ENOLCK is a PANIC or at least FATAL. Maybe it's even better
to do it FATAL to allow other backends to survive?
As for EOPNOTSUPP, maybe make a fallback to the workaround from the
first variant of the patch? (In my previous letter i forgot the pause
after break;, of cause)
>>> I don't know if Windows suffers from this type of problem.
>>> Unfortunately its equivalent functionality LockFile() looks non-ideal
>>> for this purpose: if your program crashes, the documentation is very
>>> vague on when exactly it is released by the OS, but it's not
>>> immediately on process exit. That seems non-ideal for a control file
>>> you might want to access again very soon after a crash, to be able to
>>> recover.
>>
>> Unfortunately i've not had time to reproduce the problem and apply this patch on
>> Windows yet but i'm going to do it soon on windows 10. If a crc error
>> will occur there, then we might use the workaround from the first
>> variant of the patch.
>
> Thank you for investigating. I am afraid to read your results.
First of all it seemed to me that is not a problem at all since msdn
guarantees sector-by-sector atomicity.
"Physical Sector: The unit for which read and write operations to the device
are completed in a single operation. This is the unit of atomic write..."
https://learn.microsoft.com/en-us/windows/win32/fileio/file-buffering
https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-writefile
(Of course, only if the 512 bytes lays from the beginning of the file with a zero
offset, but this is our case. The current size of ControlFileData is
296 bytes at offset = 0.)
I tried to verify this fact experimentally and was very surprised.
Unfortunately it crashed in an hour during torture test:
2023-02-13 15:10:52.675 MSK [5704] LOG: starting PostgreSQL 16devel, compiled by Visual C++ build 1929, 64-bit
2023-02-13 15:10:52.768 MSK [5704] LOG: database system is ready to accept connections
@@@@@@ sizeof(ControlFileData) = 296
.........
2023-02-13 16:10:41.997 MSK [9380] ERROR: calculated CRC checksum does not match value stored in file
But fortunately, this only happens when fsync=off.
Also i did several experiments with fsync=on and found more appropriate behavior:
The stress test with sizeof(ControlFileData) = 512+8 = 520 bytes failed in a 4,5 hours,
but the other one with ordinary sizeof(ControlFileData) = 296 not crashed in more than 12 hours.
Seems in that case the behavior corresponds to msdn. So if it is possible
to use fsync() under windows when the GUC fsync is off it maybe a solution
for this problem. If so there is no need to lock the pg_control file under windows at all.
>> May be choose it in accordance with GUC wal_sync_method?
>
> Here's a patch like that. I don't know if it's a good idea for
> wal_sync_method to affect other kinds of files or not, but, then, it
> already does (fsync_writethough changes this behaviour).
+1. Looks like it needs a little fix:
+++ b/src/common/controldata_utils.c
@@ -316,7 +316,7 @@ update_controlfile(const char *DataDir,
if (pg_fsync(fd) != 0)
ereport(PANIC,
(errcode_for_file_access(),
- errmsg("could not fdatasync file \"%s\": %m",
+ errmsg("could not fsync file \"%s\": %m",
ControlFilePath)));
And it may be combined with 0001-Lock-pg_control-while-reading-or-writing.patch
> I would
> only want to consider this if we also stop choosing "open_datasync" as
> a default on at least Windows.
I didn't quite understand this point. Could you clarify it for me, please? If the performance
of UpdateMinRecoveryPoint() wasn't a problem we could just use fsync in all platforms.
Sincerely yours,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
@ 2023-02-14 15:52 Anton A. Melnikov <[email protected]>
parent: Anton A. Melnikov <[email protected]>
1 sibling, 0 replies; 11+ messages in thread
From: Anton A. Melnikov @ 2023-02-14 15:52 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
Hi, Thomas!
On 14.02.2023 06:38, Anton A. Melnikov wrote:
> Also i did several experiments with fsync=on and found more appropriate behavior:
> The stress test with sizeof(ControlFileData) = 512+8 = 520 bytes failed in a 4,5 hours,
> but the other one with ordinary sizeof(ControlFileData) = 296 not crashed in more than 12 hours.
Nonetheless it crashed after 18 hours:
2023-02-13 18:07:21.476 MSK [7640] LOG: starting PostgreSQL 16devel, compiled by Visual C++ build 1929, 64-bit
2023-02-13 18:07:21.483 MSK [7640] LOG: listening on IPv6 address "::1", port 5432
2023-02-13 18:07:21.483 MSK [7640] LOG: listening on IPv4 address "127.0.0.1", port 5432
2023-02-13 18:07:21.556 MSK [1940] LOG: database system was shut down at 2023-02-13 18:07:12 MSK
2023-02-13 18:07:21.590 MSK [7640] LOG: database system is ready to accept connections
@@@@@@@@@@@@@ sizeof(ControlFileData) = 296
2023-02-13 18:12:21.545 MSK [9532] LOG: checkpoint starting: time
2023-02-13 18:12:21.583 MSK [9532] LOG: checkpoint complete: wrote 3 buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled; write=0.003 s, sync=0.009 s, total=0.038 s; sync files=2, longest=0.005 s, average=0.005 s; distance=0 kB, estimate=0 kB; lsn=0/17AC388, redo lsn=0/17AC350
2023-02-14 12:12:21.738 MSK [8676] ERROR: calculated CRC checksum does not match value stored in file
2023-02-14 12:12:21.738 MSK [8676] CONTEXT: SQL statement "SELECT pg_control_system()"
PL/pgSQL function inline_code_block line 1 at PERFORM
2023-02-14 12:12:21.738 MSK [8676] STATEMENT: do $$ begin loop perform pg_control_system(); end loop; end; $$;
So all of the following is incorrect:
> Seems in that case the behavior corresponds to msdn. So if it is possible
> to use fsync() under windows when the GUC fsync is off it maybe a solution
> for this problem. If so there is no need to lock the pg_control file under windows at all.
and cannot be a solution.
Sincerely yours,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
@ 2023-02-17 03:21 Thomas Munro <[email protected]>
parent: Anton A. Melnikov <[email protected]>
1 sibling, 2 replies; 11+ messages in thread
From: Thomas Munro @ 2023-02-17 03:21 UTC (permalink / raw)
To: Anton A. Melnikov <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Tue, Feb 14, 2023 at 4:38 PM Anton A. Melnikov <[email protected]> wrote:
> First of all it seemed to me that is not a problem at all since msdn
> guarantees sector-by-sector atomicity.
> "Physical Sector: The unit for which read and write operations to the device
> are completed in a single operation. This is the unit of atomic write..."
> https://learn.microsoft.com/en-us/windows/win32/fileio/file-buffering
> https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-writefile
> (Of course, only if the 512 bytes lays from the beginning of the file with a zero
> offset, but this is our case. The current size of ControlFileData is
> 296 bytes at offset = 0.)
There are two kinds of atomicity that we rely on for the control file today:
* atomicity on power loss (= device property, in case of overwrite filesystems)
* atomicity of concurrent reads and writes (= VFS or kernel buffer
pool interlocking policy)
I assume that documentation is talking about the first thing (BTW, I
suspect that documentation is also now wrong in one special case: NTFS
filesystems mounted on DAX devices don't have sectors or sector-based
atomicity unless you turn on BTT and slow them down[1]; that might
eventually be something for PostgreSQL to think about, and it applies
to other OSes too).
With this patch we would stop relying on the second thing. Supposedly
POSIX requires read/write atomicity, and many file systems offer it in
a strong form (internal range locking) or maybe a weak accidental form
(page-level locking). Since some extremely popular implementations
just don't care, and Windows isn't even a POSIX, we just have to do it
ourselves.
BTW there are at least two other places where PostgreSQL already knows
that concurrent reads and writes are possibly non-atomic (and we also
don't even try to get the alignment right, making the question moot):
pg_basebackup, which enables full_page_writes implicitly if you didn't
have the GUC on already, and pg_rewind, which refuses to run if you
haven't enabled full_page_writes explicitly (as recently discussed on
another thread recently; that's an annoying difference, and also an
annoying behaviour if you know your storage doesn't really need it!)
> I tried to verify this fact experimentally and was very surprised.
> Unfortunately it crashed in an hour during torture test:
> 2023-02-13 15:10:52.675 MSK [5704] LOG: starting PostgreSQL 16devel, compiled by Visual C++ build 1929, 64-bit
> 2023-02-13 15:10:52.768 MSK [5704] LOG: database system is ready to accept connections
> @@@@@@ sizeof(ControlFileData) = 296
> .........
> 2023-02-13 16:10:41.997 MSK [9380] ERROR: calculated CRC checksum does not match value stored in file
Thanks. I'd seen reports of this in discussions on the 'net, but
those had no authoritative references or supporting test results. The
fact that fsync made it take longer (in your following email) makes
sense and matches Linux. It inserts a massive pause in the middle of
the experiment loop, affecting the probabilities.
Therefore, we need a solution for Windows too. I tried to write the
equivalent code, in the attached. I learned a few things: Windows
locks are mandatory (that is, if you lock a file, reads/writes can
fail, unlike Unix). Combined with async release, that means we
probably also need to lock the file in xlog.c, when reading it in
xlog.c:ReadControlFile() (see comments). Before I added that, on a CI
run, I found that the read in there would sometimes fail, and adding
the locking fixed that. I am a bit confused, though, because I
expected that to be necessary only if someone managed to crash while
holding the write lock, which the CI tests shouldn't do. Do you have
any ideas?
While contemplating what else a mandatory file lock might break, I
remembered that basebackup.c also reads the control file. Hrmph. Not
addressed yet; I guess it might need to acquire/release around
sendFile(sink, XLOG_CONTROL_FILE, ...)?
> > I would
> > only want to consider this if we also stop choosing "open_datasync" as
> > a default on at least Windows.
>
> I didn't quite understand this point. Could you clarify it for me, please? If the performance
> of UpdateMinRecoveryPoint() wasn't a problem we could just use fsync in all platforms.
The level of durability would be weakened on Windows. On all systems
except Linux and FreeBSD, we default to wal_sync_method=open_datasync,
so then we would start using FILE_FLAG_WRITE_THROUGH for the control
file too. We know from reading and experimentation that
FILE_FLAG_WRITE_THROUGH doesn't flush the drive cache on consumer
Windows hardware, but its fdatasync-like thing does[2]. I have not
thought too hard about the consequences of using different durability
levels for different categories of file, but messing with write
ordering can't be good for crash recovery, so I'd rather increase WAL
durability than decrease control file durability. If a Windows user
complains that it makes their fancy non-volatile cache slow down, they
can always adjust the settings in PostgreSQL, their OS, or their
drivers etc. I think we should just make fdatasync the default on all
systems.
Here's a patch like that.
[1] https://learn.microsoft.com/en-us/windows-server/storage/storage-spaces/persistent-memory-direct-acc...
[2] https://www.postgresql.org/message-id/flat/CA%2BhUKG%2Ba-7r4GpADsasCnuDBiqC1c31DAQQco2FayVtB9V3sQw%4...
Attachments:
[text/x-patch] 0001-Lock-pg_control-while-reading-or-writing.patch (6.6K, ../../CA+hUKGLkOYgHLmErZ1jAgR3WKzJRxCjJumFM4mkkba7+hv-NGA@mail.gmail.com/2-0001-Lock-pg_control-while-reading-or-writing.patch)
download | inline diff:
From 816760e52a1233b29674869205ce9283aab28a73 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Tue, 31 Jan 2023 20:01:49 +1300
Subject: [PATCH 1/3] Lock pg_control while reading or writing.
Front-end programs that read pg_control and user-facing functions run
in the backend read pg_control without acquiring ControlFileLock. If
you're unlucky enough to read() while the backend is in write(), on at
least ext4 or NTFS, you might see partial data. Use a POSIX advisory
file lock or a Windows mandatory file lock, to avoid this problem.
Since the semantics of POSIX and Windows file locks are different
and error-prone, don't try to provide a general purpose reusable
function for those operations; keep them close to the control file code.
Reviewed-by: Anton A. Melnikov <[email protected]>
Discussion: https://postgr.es/m/20221123014224.xisi44byq3cf5psi%40awork3.anarazel.de
---
src/backend/access/transam/xlog.c | 21 +++++
src/common/controldata_utils.c | 126 +++++++++++++++++++++++++
src/include/common/controldata_utils.h | 4 +
3 files changed, 151 insertions(+)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f9f0f6db8d..9b9c3294e8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3980,6 +3980,19 @@ ReadControlFile(void)
errmsg("could not open file \"%s\": %m",
XLOG_CONTROL_FILE)));
+ /*
+ * Since file locks are released asynchronously after a program crashes on
+ * Windows, there is a small chance that the write lock of a previously
+ * running postmaster hasn't been released yet. We need to wait for that
+ * here; if we didn't, our read might fail, because Windows file locks are
+ * mandatory (that is, not advisory).
+ */
+ if (lock_controlfile(fd, false) < 0)
+ ereport(PANIC,
+ (errcode_for_file_access(),
+ errmsg("could not lock file \"%s\": %m",
+ XLOG_CONTROL_FILE)));
+
pgstat_report_wait_start(WAIT_EVENT_CONTROL_FILE_READ);
r = read(fd, ControlFile, sizeof(ControlFileData));
if (r != sizeof(ControlFileData))
@@ -3997,6 +4010,14 @@ ReadControlFile(void)
}
pgstat_report_wait_end();
+#ifdef WIN32
+ if (unlock_controlfile(fd) < 0)
+ ereport(PANIC,
+ (errcode_for_file_access(),
+ errmsg("could not unlock file \"%s\": %m",
+ XLOG_CONTROL_FILE)));
+#endif
+
close(fd);
/*
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index 9723587466..45b43ae546 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -39,6 +39,91 @@
#include "storage/fd.h"
#endif
+/*
+ * Lock the control file until closed. This should only be used on file where
+ * only one descriptor is open in a given process (due to weird semantics of
+ * POSIX locks). On Windows, see unlock_controlfile(), but otherwise the lock
+ * is held until the file is closed.
+ */
+int
+lock_controlfile(int fd, bool exclusive)
+{
+#ifdef WIN32
+ OVERLAPPED overlapped = {0};
+ HANDLE handle;
+
+ handle = (HANDLE) _get_osfhandle(fd);
+ if (handle == INVALID_HANDLE_VALUE)
+ {
+ errno = EBADF;
+ return -1;
+ }
+
+ /* Lock first byte. */
+ if (!LockFileEx(handle,
+ exclusive ? LOCKFILE_EXCLUSIVE_LOCK : 0,
+ 0,
+ 1,
+ 0,
+ &overlapped))
+ {
+ _dosmaperr(GetLastError());
+ return -1;
+ }
+
+ return 0;
+#else
+ struct flock lock;
+ int rc;
+
+ memset(&lock, 0, sizeof(lock));
+ lock.l_type = exclusive ? F_WRLCK : F_RDLCK;
+ lock.l_start = 0;
+ lock.l_whence = SEEK_SET;
+ lock.l_len = 0;
+ lock.l_pid = -1;
+
+ do
+ {
+ rc = fcntl(fd, F_SETLKW, &lock);
+ }
+ while (rc < 0 && errno == EINTR);
+
+ return rc;
+#endif
+}
+
+#ifdef WIN32
+/*
+ * Release lock acquire with lock_controlfile(). On POSIX systems, we don't
+ * bother making an extra system call to release the lock, since it'll be
+ * released on close anyway. On Windows, explicit release is recommended by
+ * the documentation to make sure it is done synchronously.
+ */
+int
+unlock_controlfile(int fd)
+{
+ OVERLAPPED overlapped = {0};
+ HANDLE handle;
+
+ handle = (HANDLE) _get_osfhandle(fd);
+ if (handle == INVALID_HANDLE_VALUE)
+ {
+ errno = EBADF;
+ return -1;
+ }
+
+ /* Unlock first byte. */
+ if (!UnlockFileEx(handle, 0, 1, 0, &overlapped))
+ {
+ _dosmaperr(GetLastError());
+ return -1;
+ }
+
+ return 0;
+}
+#endif
+
/*
* get_controlfile()
*
@@ -74,6 +159,20 @@ get_controlfile(const char *DataDir, bool *crc_ok_p)
ControlFilePath);
#endif
+ /* Make sure we can read the file atomically. */
+ if (lock_controlfile(fd, false) < 0)
+ {
+#ifndef FRONTEND
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not lock file \"%s\" for reading: %m",
+ ControlFilePath)));
+#else
+ pg_fatal("could not lock file \"%s\" for reading: %m",
+ ControlFilePath);
+#endif
+ }
+
r = read(fd, ControlFile, sizeof(ControlFileData));
if (r != sizeof(ControlFileData))
{
@@ -97,6 +196,10 @@ get_controlfile(const char *DataDir, bool *crc_ok_p)
#endif
}
+#ifdef WIN32
+ unlock_controlfile(fd);
+#endif
+
#ifndef FRONTEND
if (CloseTransientFile(fd) != 0)
ereport(ERROR,
@@ -186,6 +289,25 @@ update_controlfile(const char *DataDir,
pg_fatal("could not open file \"%s\": %m", ControlFilePath);
#endif
+ /*
+ * Make sure that any concurrent reader (including frontend programs) can
+ * read the file atomically. Note that this refers to atomicity of
+ * concurrent reads and writes. For our assumption of atomicity under
+ * power failure, see PG_CONTROL_MAX_SAFE_SIZE.
+ */
+ if (lock_controlfile(fd, true) < 0)
+ {
+#ifndef FRONTEND
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not lock file \"%s\" for writing: %m",
+ ControlFilePath)));
+#else
+ pg_fatal("could not lock file \"%s\" for writing: %m",
+ ControlFilePath);
+#endif
+ }
+
errno = 0;
#ifndef FRONTEND
pgstat_report_wait_start(WAIT_EVENT_CONTROL_FILE_WRITE_UPDATE);
@@ -225,6 +347,10 @@ update_controlfile(const char *DataDir,
#endif
}
+#ifdef WIN32
+ unlock_controlfile(fd);
+#endif
+
if (close(fd) != 0)
{
#ifndef FRONTEND
diff --git a/src/include/common/controldata_utils.h b/src/include/common/controldata_utils.h
index 49e7c52d31..06825cad85 100644
--- a/src/include/common/controldata_utils.h
+++ b/src/include/common/controldata_utils.h
@@ -15,5 +15,9 @@
extern ControlFileData *get_controlfile(const char *DataDir, bool *crc_ok_p);
extern void update_controlfile(const char *DataDir,
ControlFileData *ControlFile, bool do_sync);
+extern int lock_controlfile(int fd, bool exclusive);
+#ifdef WIN32
+extern int unlock_controlfile(int fd);
+#endif
#endif /* COMMON_CONTROLDATA_UTILS_H */
--
2.35.1
[text/x-patch] 0002-Make-wal_sync_method-fdatasync-the-default-on-all-OS.patch (5.9K, ../../CA+hUKGLkOYgHLmErZ1jAgR3WKzJRxCjJumFM4mkkba7+hv-NGA@mail.gmail.com/3-0002-Make-wal_sync_method-fdatasync-the-default-on-all-OS.patch)
download | inline diff:
From fa594006c3ef7e958d870978c631b003aa680c18 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Thu, 16 Feb 2023 20:02:22 +1300
Subject: [PATCH 2/3] Make wal_sync_method=fdatasync the default on all OSes.
Previously, fdatasync was the default level on Linux and FreeBSD by
special rules, and on OpenBSD and DragonflyBSD because they didn't have
O_DSYNC != O_SYNC or O_DSYNC at all. For every other system, we'd
choose open_datasync.
Use fdatasync everywhere, for consistency. This became possible after
commit 9430fb40 added support for Windows, the last known system not to
have it. This means that we'll now flush caches on consumer drives by
default on Windows, where previously we didn't, which seems like a
better default for crash safety. Users who want the older behavior can
still request it with wal_sync_method=open_datasync.
---
doc/src/sgml/config.sgml | 5 ++---
doc/src/sgml/wal.sgml | 8 ++++----
src/bin/pg_test_fsync/pg_test_fsync.c | 4 ++--
src/include/access/xlogdefs.h | 14 +-------------
src/include/port/freebsd.h | 7 -------
src/include/port/linux.h | 8 --------
6 files changed, 9 insertions(+), 37 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index ecd9aa73ef..b78d374d7e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3107,9 +3107,8 @@ include_dir 'conf.d'
<para>
The <literal>open_</literal>* options also use <literal>O_DIRECT</literal> if available.
Not all of these choices are available on all platforms.
- The default is the first method in the above list that is supported
- by the platform, except that <literal>fdatasync</literal> is the default on
- Linux and FreeBSD. The default is not necessarily ideal; it might be
+ The default is <literal>fdatasync</literal>.
+ The default is not necessarily ideal; it might be
necessary to change this setting or other aspects of your system
configuration in order to create a crash-safe configuration or
achieve optimal performance.
diff --git a/doc/src/sgml/wal.sgml b/doc/src/sgml/wal.sgml
index ed7929cbcd..ffc22a7ded 100644
--- a/doc/src/sgml/wal.sgml
+++ b/doc/src/sgml/wal.sgml
@@ -106,12 +106,12 @@
<listitem>
<para>
On <productname>Windows</productname>, if <varname>wal_sync_method</varname> is
- <literal>open_datasync</literal> (the default), write caching can be disabled
+ <literal>open_datasync</literal>, write caching can be disabled
by unchecking <literal>My Computer\Open\<replaceable>disk drive</replaceable>\Properties\Hardware\Properties\Policies\Enable write caching on the disk</literal>.
Alternatively, set <varname>wal_sync_method</varname> to
- <literal>fdatasync</literal> (NTFS only), <literal>fsync</literal> or
- <literal>fsync_writethrough</literal>, which prevent
- write caching.
+ <literal>fdatasync</literal> (the default), <literal>fsync</literal> or
+ <literal>fsync_writethrough</literal>, which use system calls that
+ flush write caches.
</para>
</listitem>
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 3d5e8f30ab..45bd2daf2e 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -292,7 +292,7 @@ test_sync(int writes_per_op)
printf(_("\nCompare file sync methods using one %dkB write:\n"), XLOG_BLCKSZ_K);
else
printf(_("\nCompare file sync methods using two %dkB writes:\n"), XLOG_BLCKSZ_K);
- printf(_("(in wal_sync_method preference order, except fdatasync is Linux's default)\n"));
+ printf(_("(fdatasync is the default)\n"));
/*
* Test open_datasync if available
@@ -326,7 +326,7 @@ test_sync(int writes_per_op)
#endif
/*
- * Test fdatasync if available
+ * Test fdatasync
*/
printf(LABEL_FORMAT, "fdatasync");
fflush(stdout);
diff --git a/src/include/access/xlogdefs.h b/src/include/access/xlogdefs.h
index fe794c7740..a628976902 100644
--- a/src/include/access/xlogdefs.h
+++ b/src/include/access/xlogdefs.h
@@ -64,19 +64,7 @@ typedef uint32 TimeLineID;
*/
typedef uint16 RepOriginId;
-/*
- * This chunk of hackery attempts to determine which file sync methods
- * are available on the current platform, and to choose an appropriate
- * default method.
- *
- * Note that we define our own O_DSYNC on Windows, but not O_SYNC.
- */
-#if defined(PLATFORM_DEFAULT_SYNC_METHOD)
-#define DEFAULT_SYNC_METHOD PLATFORM_DEFAULT_SYNC_METHOD
-#elif defined(O_DSYNC) && (!defined(O_SYNC) || O_DSYNC != O_SYNC)
-#define DEFAULT_SYNC_METHOD SYNC_METHOD_OPEN_DSYNC
-#else
+/* Default synchronization method for WAL. */
#define DEFAULT_SYNC_METHOD SYNC_METHOD_FDATASYNC
-#endif
#endif /* XLOG_DEFS_H */
diff --git a/src/include/port/freebsd.h b/src/include/port/freebsd.h
index 0e3fde55d6..2e36d3da4f 100644
--- a/src/include/port/freebsd.h
+++ b/src/include/port/freebsd.h
@@ -1,8 +1 @@
/* src/include/port/freebsd.h */
-
-/*
- * Set the default wal_sync_method to fdatasync. xlogdefs.h's normal rules
- * would prefer open_datasync on FreeBSD 13+, but that is not a good choice on
- * many systems.
- */
-#define PLATFORM_DEFAULT_SYNC_METHOD SYNC_METHOD_FDATASYNC
diff --git a/src/include/port/linux.h b/src/include/port/linux.h
index 7a6e46cdbb..acd867606c 100644
--- a/src/include/port/linux.h
+++ b/src/include/port/linux.h
@@ -12,11 +12,3 @@
* to have a kernel version test here.
*/
#define HAVE_LINUX_EIDRM_BUG
-
-/*
- * Set the default wal_sync_method to fdatasync. With recent Linux versions,
- * xlogdefs.h's normal rules will prefer open_datasync, which (a) doesn't
- * perform better and (b) causes outright failures on ext4 data=journal
- * filesystems, because those don't support O_DIRECT.
- */
-#define PLATFORM_DEFAULT_SYNC_METHOD SYNC_METHOD_FDATASYNC
--
2.35.1
[text/x-patch] 0003-Apply-wal_sync_method-to-pg_control-file.patch (5.4K, ../../CA+hUKGLkOYgHLmErZ1jAgR3WKzJRxCjJumFM4mkkba7+hv-NGA@mail.gmail.com/4-0003-Apply-wal_sync_method-to-pg_control-file.patch)
download | inline diff:
From c9dcb77263a7f86b187231a83083bfbe3a0f30b4 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Tue, 31 Jan 2023 21:08:12 +1300
Subject: [PATCH 3/3] Apply wal_sync_method to pg_control file.
When writing out the control file frequently on a standby, we can go a
little faster on some systems by using fdatasync() instead of fsync(),
with no loss of safety. We already respected the special
"wal_sync_method=fsync_writethrough" via a circuitous route, but we
might as well support the full range of methods.
---
src/backend/access/transam/xlog.c | 19 ++++++++-
src/common/controldata_utils.c | 59 ++++++++++++++++++++------
src/include/common/controldata_utils.h | 7 ++-
3 files changed, 70 insertions(+), 15 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 9b9c3294e8..27c1cb8273 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4185,7 +4185,24 @@ ReadControlFile(void)
static void
UpdateControlFile(void)
{
- update_controlfile(DataDir, ControlFile, true);
+ int sync_op;
+
+ switch (sync_method)
+ {
+ case SYNC_METHOD_FDATASYNC:
+ sync_op = UPDATE_CONTROLFILE_FDATASYNC;
+ break;
+ case SYNC_METHOD_OPEN:
+ sync_op = UPDATE_CONTROLFILE_O_SYNC;
+ break;
+ case SYNC_METHOD_OPEN_DSYNC:
+ sync_op = UPDATE_CONTROLFILE_O_DSYNC;
+ break;
+ default:
+ sync_op = UPDATE_CONTROLFILE_FSYNC;
+ }
+
+ update_controlfile(DataDir, ControlFile, sync_op);
}
/*
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index 45b43ae546..3fc2a00d44 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -239,18 +239,39 @@ get_controlfile(const char *DataDir, bool *crc_ok_p)
* update_controlfile()
*
* Update controlfile values with the contents given by caller. The
- * contents to write are included in "ControlFile". "do_sync" can be
- * optionally used to flush the updated control file. Note that it is up
- * to the caller to properly lock ControlFileLock when calling this
- * routine in the backend.
+ * contents to write are included in "ControlFile". "sync_op" can be set
+ * to 0 or a synchronization method UPDATE_CONTROLFILE_XXX. In frontend code,
+ * only 0 and non-zero (fsync) are meaningful.
+ * Note that it is up to the caller to properly lock ControlFileLock when
+ * calling this routine in the backend.
*/
void
update_controlfile(const char *DataDir,
- ControlFileData *ControlFile, bool do_sync)
+ ControlFileData *ControlFile,
+ int sync_op)
{
int fd;
char buffer[PG_CONTROL_FILE_SIZE];
char ControlFilePath[MAXPGPATH];
+ int open_flag;
+
+ switch (sync_op)
+ {
+#ifndef FRONTEND
+#ifdef O_SYNC
+ case UPDATE_CONTROLFILE_O_SYNC:
+ open_flag = O_SYNC;
+ break;
+#endif
+#ifdef O_DSYNC
+ case UPDATE_CONTROLFILE_O_DSYNC:
+ open_flag = O_DSYNC;
+ break;
+#endif
+#endif
+ default:
+ open_flag = 0;
+ }
/* Update timestamp */
ControlFile->time = (pg_time_t) time(NULL);
@@ -278,13 +299,13 @@ update_controlfile(const char *DataDir,
* All errors issue a PANIC, so no need to use OpenTransientFile() and to
* worry about file descriptor leaks.
*/
- if ((fd = BasicOpenFile(ControlFilePath, O_RDWR | PG_BINARY)) < 0)
+ if ((fd = BasicOpenFile(ControlFilePath, O_RDWR | PG_BINARY | open_flag)) < 0)
ereport(PANIC,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m",
ControlFilePath)));
#else
- if ((fd = open(ControlFilePath, O_WRONLY | PG_BINARY,
+ if ((fd = open(ControlFilePath, O_WRONLY | PG_BINARY | open_flag,
pg_file_create_mode)) == -1)
pg_fatal("could not open file \"%s\": %m", ControlFilePath);
#endif
@@ -331,17 +352,29 @@ update_controlfile(const char *DataDir,
pgstat_report_wait_end();
#endif
- if (do_sync)
+ if (sync_op != 0)
{
#ifndef FRONTEND
pgstat_report_wait_start(WAIT_EVENT_CONTROL_FILE_SYNC_UPDATE);
- if (pg_fsync(fd) != 0)
- ereport(PANIC,
- (errcode_for_file_access(),
- errmsg("could not fsync file \"%s\": %m",
- ControlFilePath)));
+ if (sync_op == UPDATE_CONTROLFILE_FDATASYNC)
+ {
+ if (fdatasync(fd) != 0)
+ ereport(PANIC,
+ (errcode_for_file_access(),
+ errmsg("could not fdatasync file \"%s\": %m",
+ ControlFilePath)));
+ }
+ else if (sync_op == UPDATE_CONTROLFILE_FSYNC)
+ {
+ if (pg_fsync(fd) != 0)
+ ereport(PANIC,
+ (errcode_for_file_access(),
+ errmsg("could not fdatasync file \"%s\": %m",
+ ControlFilePath)));
+ }
pgstat_report_wait_end();
#else
+ /* In frontend code, non-zero sync_op gets you plain fsync(). */
if (fsync(fd) != 0)
pg_fatal("could not fsync file \"%s\": %m", ControlFilePath);
#endif
diff --git a/src/include/common/controldata_utils.h b/src/include/common/controldata_utils.h
index 06825cad85..de6de5571e 100644
--- a/src/include/common/controldata_utils.h
+++ b/src/include/common/controldata_utils.h
@@ -12,9 +12,14 @@
#include "catalog/pg_control.h"
+#define UPDATE_CONTROLFILE_FSYNC 1
+#define UPDATE_CONTROLFILE_FDATASYNC 2
+#define UPDATE_CONTROLFILE_O_SYNC 3
+#define UPDATE_CONTROLFILE_O_DSYNC 4
+
extern ControlFileData *get_controlfile(const char *DataDir, bool *crc_ok_p);
extern void update_controlfile(const char *DataDir,
- ControlFileData *ControlFile, bool do_sync);
+ ControlFileData *ControlFile, int sync_op);
extern int lock_controlfile(int fd, bool exclusive);
#ifdef WIN32
extern int unlock_controlfile(int fd);
--
2.35.1
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
@ 2023-02-22 00:10 Thomas Munro <[email protected]>
parent: Thomas Munro <[email protected]>
1 sibling, 0 replies; 11+ messages in thread
From: Thomas Munro @ 2023-02-22 00:10 UTC (permalink / raw)
To: Anton A. Melnikov <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
On Fri, Feb 17, 2023 at 4:21 PM Thomas Munro <[email protected]> wrote:
> While contemplating what else a mandatory file lock might break, I
> remembered that basebackup.c also reads the control file. Hrmph. Not
> addressed yet; I guess it might need to acquire/release around
> sendFile(sink, XLOG_CONTROL_FILE, ...)?
If we go this way, I suppose, in theory at least, someone with
external pg_backup_start()-based tools might also want to hold the
lock while copying pg_control. Otherwise they might fail to open it
on Windows (where that patch uses a mandatory lock) or copy garbage on
Linux (as they can today, I assume), with non-zero probability -- at
least when copying files from a hot standby. Or backup tools might
want to get the file contents through some entirely different
mechanism that does the right interlocking (whatever that might be,
maybe inside the server). Perhaps this is not so much the localised
systems programming curiosity I thought it was, and has implications
that'd need to be part of the documented low-level backup steps. It
makes me like the idea a bit less. It'd be good to hear from backup
gurus what they think about that.
One cute hack I thought of to make the file lock effectively advisory
on Windows is to lock a byte range *past the end* of the file (the
documentation says you can do that). That shouldn't stop programs
that want to read the file without locking and don't know/care about
our scheme (that is, pre-existing backup tools that haven't considered
this problem and remain oblivious or accept the (low) risk of torn
reads), but will block other participants in our scheme.
If we went back to the keep-rereading-until-it-stops-changing model,
then an external backup tool would need to be prepared to do that too,
in theory at least. Maybe some already do something like that?
Or maybe the problem is/was too theoretical before...
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"
@ 2023-02-24 10:12 Anton A. Melnikov <[email protected]>
parent: Thomas Munro <[email protected]>
1 sibling, 0 replies; 11+ messages in thread
From: Anton A. Melnikov @ 2023-02-24 10:12 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Tom Lane <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers
Hi, Thomas!
On 17.02.2023 06:21, Thomas Munro wrote:
> There are two kinds of atomicity that we rely on for the control file today:
>
> * atomicity on power loss (= device property, in case of overwrite filesystems)
> * atomicity of concurrent reads and writes (= VFS or kernel buffer
> pool interlocking policy)
>
> I assume that documentation is talking about the first thing
I think this is true, as documentation says about write operations only,
but not about the read ones.
> (BTW, I
> suspect that documentation is also now wrong in one special case: NTFS
> filesystems mounted on DAX devices don't have sectors or sector-based
> atomicity unless you turn on BTT and slow them down[1]; that might
> eventually be something for PostgreSQL to think about, and it applies
> to other OSes too).
Very interesting find! For instance, the volume of Intel® Optane™ Persistent Memory
already reaches 512GB and can be potentially used for cluster data. As the
first step it would be good to understand what Microsoft means by
large memory pages, what size are they talking about, that is, where
is the reasonable boundary for using BTT; i suppose, this will help choose
whether to use BTT or have to write own DAX-aware code.
> With this patch we would stop relying on the second thing. Supposedly
> POSIX requires read/write atomicity, and many file systems offer it in
> a strong form (internal range locking) or maybe a weak accidental form
> (page-level locking). Since some extremely popular implementations
> just don't care, and Windows isn't even a POSIX, we just have to do it
> ourselves.
Yes. indeed. But unless it's an atomic or transactional filesystem. In
such a case there is almost nothing to do. Another thing is that
it seems such a systems do not exist in reality although it has been
discussed many times I've googled some information on this topic e.g
[1], [2], [3] but all of project were abandoned or deprecated as
Microsoft's own development.
> BTW there are at least two other places where PostgreSQL already knows
> that concurrent reads and writes are possibly non-atomic (and we also
> don't even try to get the alignment right, making the question moot):
> pg_basebackup, which enables full_page_writes implicitly if you didn't
> have the GUC on already, and pg_rewind, which refuses to run if you
> haven't enabled full_page_writes explicitly (as recently discussed on
> another thread recently; that's an annoying difference, and also an
> annoying behaviour if you know your storage doesn't really need it!)
It seems a good topic for a separate thread patch. Would you provide a
link to the thread you mentioned please?
> Therefore, we need a solution for Windows too. I tried to write the
> equivalent code, in the attached. I learned a few things: Windows
> locks are mandatory (that is, if you lock a file, reads/writes can
> fail, unlike Unix). Combined with async release, that means we
> probably also need to lock the file in xlog.c, when reading it in
> xlog.c:ReadControlFile() (see comments). Before I added that, on a CI
> run, I found that the read in there would sometimes fail, and adding
> the locking fixed that. I am a bit confused, though, because I
> expected that to be necessary only if someone managed to crash while
> holding the write lock, which the CI tests shouldn't do. Do you have
> any ideas?
Unfortunately, no ideas so far. Was it a pg_control CRC or I/O errors?
Maybe logs of such a fail were saved somewhere? I would like to see
them if possible.
> While contemplating what else a mandatory file lock might break, I
> remembered that basebackup.c also reads the control file. Hrmph. Not
> addressed yet; I guess it might need to acquire/release around
> sendFile(sink, XLOG_CONTROL_FILE, ...)?
Here, possibly pass a boolean flag into sendFile()? When it is true,
then take a lock after OpenTransientFile() and release it before
CloseTransientFile() if under Windows.
There are also two places where read or write from/to the pg_control
occur. These are functions WriteControlFile() in xlog.c and
read_controlfile() in pg_resetwal.c.
For the second case, locking definitely not necessary as
the server is stopped. For the first case seems too as BootStrapXLOG()
where WriteControlFile() will be called inside must be called
only once on system install.
Since i've smoothly moved on to the code review here there is a
suggestion at your discretion to add error messages to get_controlfile()
and update_controlfile() if unlock_controlfile() fails.
> I think we should just make fdatasync the default on all
> systems.
Agreed. And maybe choose the UPDATE_CONTROLFILE_FDATASYNC as the default
case in UpdateControlFile() since fdatasync now the default on all
systems and its metadata are of no interest?
On 22.02.2023 03:10, Thomas Munro wrote:
> If we go this way, I suppose, in theory at least, someone with
> external pg_backup_start()-based tools might also want to hold the
> lock while copying pg_control. Otherwise they might fail to open it
> on Windows (where that patch uses a mandatory lock)
As for external pg_backup_start()-based tools if somebody want to take the
lock while copying pg_control i suppose it's a normal case. He may have to wait
a bit until we release the lock, like in lock_controlfile(). Moreover
this is a very good desire, as it guarantees the integrity of pg_control if
only someone is going to use F_SETLKW rather than non-waiting F_SETLK.
Backup of locked files is the common place in Windows and all standard
backup tools can do it well via VSS (volume shadow copy) including
embedded windows backup tool. Just in case, i tested it experimentally.
During the torture test first try to copy pg_control and predictably caught:
Error: Cannot read C:\pgbins\master\data\global\pg_control!
"The process cannot access the file because another process has locked a portion of the file."
But copy using own windows backup utility successfully copied it with VSS.
> > One cute hack I thought of to make the file lock effectively advisory
> on Windows is to lock a byte range *past the end* of the file (the
> documentation says you can do that). That shouldn't stop programs
> that want to read the file without locking and don't know/care about
> our scheme (that is, pre-existing backup tools that haven't considered
> this problem and remain oblivious or accept the (low) risk of torn
> reads), but will block other participants in our scheme.
A very interesting idea. It makes sense when someone using external
backup tools that can not work with VSS. But the fact of using such a tools
under Windows is highly doubtful, i guess. It will not allow to backup
many other applications and windows system itself.
Let me to join you suggestion that it'd be good to hear from backup
gurus what they think about that.
> or copy garbage on
> Linux (as they can today, I assume), with non-zero probability -- at
> least when copying files from a hot standby.
> Or backup tools might
> want to get the file contents through some entirely different
> mechanism that does the right interlocking (whatever that might be,
> maybe inside the server). Perhaps this is not so much the localised
> systems programming curiosity I thought it was, and has implications
> that'd need to be part of the documented low-level backup steps. It
> makes me like the idea a bit less. It'd be good to hear from backup
> gurus what they think about that.
> If we went back to the keep-rereading-until-it-stops-changing model,
> then an external backup tool would need to be prepared to do that too,
> in theory at least. Maybe some already do something like that?
>
> Or maybe the problem is/was too theoretical before...
As far as i understand, this problem has always been, but the probability of
this is extremely small in practice, which is directly pointed in
the docs [4]:
"So while it is theoretically a weak spot, pg_control does not seem
to be a problem in practice."
> Here's a patch like that.
In this patch, the problem is solved for the live database and
maybe remains for some possible cases of an external backup. In a whole,
i think it can be stated that this is a sensible step forward.
Just like last time, i ran a long stress test under windows with current patch.
There were no errors for more than 3 days even with fsync=off.
[1] https://lwn.net/Articles/789600/
[2] https://github.com/ut-osa/txfs
[3] https://en.wikipedia.org/wiki/Transactional_NTFS[4] https://www.postgresql.org/docs/devel/wal-internals.html
With the best wishes!
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2023-02-24 10:12 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 02:48 [PATCH 1/4] bootstrap: convert Typ to a List* Justin Pryzby <[email protected]>
2023-01-31 01:09 Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt" Anton A. Melnikov <[email protected]>
2023-01-31 04:09 ` Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt" Thomas Munro <[email protected]>
2023-01-31 11:38 ` Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt" Thomas Munro <[email protected]>
2023-02-01 04:04 ` Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt" Anton A. Melnikov <[email protected]>
2023-02-01 06:45 ` Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt" Thomas Munro <[email protected]>
2023-02-14 03:38 ` Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt" Anton A. Melnikov <[email protected]>
2023-02-14 15:52 ` Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt" Anton A. Melnikov <[email protected]>
2023-02-17 03:21 ` Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt" Thomas Munro <[email protected]>
2023-02-22 00:10 ` Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt" Thomas Munro <[email protected]>
2023-02-24 10:12 ` Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt" Anton A. Melnikov <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox