Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On 2017-09-19 13:15:28 -0400, Tom Lane wrote:
> Andres Freund writes:
> > On 2017-09-19 13:00:33 -0400, Robert Haas wrote:
> >> You mean, in the postmaster?
>
> > Yes. We try to avoid touch shmem there, but it's not like we're
> > succeeding fully. See e.g. the pgstat_get_crashed_backend_activity()
> > calls (which do rely on shmem being ok to some extent), pmsignal,
> > BackgroundWorkerStateChange(), ...
>
> Well, the point is to avoid touching data structures that could be
> corrupted enough to confuse the postmaster. I don't have any problem with
> adding some more functionality to pmsignal, say.
Given that we're ok with reading pgstat shared memory entries, I think
adding a carefully coded variant of SendProcSignal() should be doable in
a safe manner.
Something roughly like
int
PostmasterSendProcSignal(pid_t pid, ProcSignalReason reason)
{
volatile ProcSignalSlot *slot;
/*
* As this is running in postmaster, be careful not to dereference
* any pointers from shared memory that could be corrupted, and to
* not to throw errors.
*/
for (i = 0; i < NumProcSignalSlots; i++)
{
slot = &ProcSignalSlots[i];
if (slot->pss_pid == pid)
{
/*
* The note about race conditions in SendProcSignal applies
* here, too
*/
/* Atomically set the proper flag */
slot->pss_signalFlags[reason] = true;
/* Send signal */
return kill(pid, SIGUSR1);
}
}
errno = ESRCH;
return -1;
}
As all the memory offsets are computed based on postmaster process-local
variables, this should be safe.
I'd rather like to avoid a copy of the procsignal infrastructure if we
don't need it...
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
Andres Freund writes: > On 2017-09-19 13:00:33 -0400, Robert Haas wrote: >> You mean, in the postmaster? > Yes. We try to avoid touch shmem there, but it's not like we're > succeeding fully. See e.g. the pgstat_get_crashed_backend_activity() > calls (which do rely on shmem being ok to some extent), pmsignal, > BackgroundWorkerStateChange(), ... Well, the point is to avoid touching data structures that could be corrupted enough to confuse the postmaster. I don't have any problem with adding some more functionality to pmsignal, say. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On 2017-09-19 13:00:33 -0400, Robert Haas wrote: > On Tue, Sep 19, 2017 at 12:51 PM, Andres Freund wrote: > > That'd not be that a crazy amount of > > shared memory that'd need to be touched in shared memory, ... > > You mean, in the postmaster? Yes. We try to avoid touch shmem there, but it's not like we're succeeding fully. See e.g. the pgstat_get_crashed_backend_activity() calls (which do rely on shmem being ok to some extent), pmsignal, BackgroundWorkerStateChange(), ... Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On Tue, Sep 19, 2017 at 12:51 PM, Andres Freund wrote: > That'd not be that a crazy amount of > shared memory that'd need to be touched in shared memory, ... You mean, in the postmaster? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On 2017-09-19 12:24:00 -0400, Tom Lane wrote: > Andres Freund writes: > > Unfortunately the backends themselves also react with inaccurate error > > messages to things like immediate shutdowns... > > Yeah, those signals are kind of overloaded these days. Not sure if > there's any good way to improve that. We could use the procsignal infrastructure (or some pared down equivalent with just one 'server-status' byte in shmem) for the non-crash immediate shutdown. That'd not be that a crazy amount of shared memory that'd need to be touched in shared memory, and we're not in an actual crash in that case, so no corrupted shmem. OTOH, that'd still leave us with some processes that aren't connected to shmem receiving the bare SIGQUIT - but I think they mostly already have more terse error messages anyway. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
Andres Freund writes: > Unfortunately the backends themselves also react with inaccurate error > messages to things like immediate shutdowns... Yeah, those signals are kind of overloaded these days. Not sure if there's any good way to improve that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On 2017-09-19 07:48:42 -0400, Robert Haas wrote: > Oh, I've not seen that. Mostly, what I think we should fix is the > fact that the libpq messages tend to report that the server crashed > even if it was an orderly shutdown. > > [rhaas ~]$ psql > psql (11devel) > Type "help" for help. > > rhaas=# select 1; > FATAL: terminating connection due to administrator command > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > The connection to the server was lost. Attempting reset: Failed. > > It's sort of funny (but also sort of sad) that we've got libpq talking > down the server's reliability (and even in the face of evidence which > manifestly contradicts it). Yea, I'm not very happy with that either. I really think we should stop emitting that if we got an actual message from the server. Unfortunately the backends themselves also react with inaccurate error messages to things like immediate shutdowns... Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On Mon, Sep 18, 2017 at 2:04 PM, Andres Freund wrote: > On 2017-09-18 12:16:42 -0400, Robert Haas wrote: >> On Mon, Sep 18, 2017 at 6:32 AM, Andres Freund wrote: >> > One thing that I've noticed for a while, but that I was reminded of >> > again here. We very frequently allow psql to reconnect in case of crash, >> > just for postmaster to notice a child has gone and kill that session. I >> > don't recall that frequently happening, but these days it happens nearly >> > every time. >> >> I don't understand what you're talking about here. > > I often see a backend crash, psql reacting to that crash by > reconnecting, successfully establish a new connection, just to be kicked > off by postmaster that does the crash restart cycle. I've not yet > figured out when exactly this happens and when not. Oh, I've not seen that. Mostly, what I think we should fix is the fact that the libpq messages tend to report that the server crashed even if it was an orderly shutdown. [rhaas ~]$ psql psql (11devel) Type "help" for help. rhaas=# select 1; FATAL: terminating connection due to administrator command server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: Failed. It's sort of funny (but also sort of sad) that we've got libpq talking down the server's reliability (and even in the face of evidence which manifestly contradicts it). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
Andres Freund writes: > On 2017-09-18 12:16:42 -0400, Robert Haas wrote: >> I don't understand what you're talking about here. > I often see a backend crash, psql reacting to that crash by > reconnecting, successfully establish a new connection, just to be kicked > off by postmaster that does the crash restart cycle. I've not yet > figured out when exactly this happens and when not. It seems like this must indicate that psql sees the connection drop significantly before the postmaster gets SIGCHLD. Which is weird, but if you have core dumps enabled, maybe your kernel is doing something like (1) close files, (2) dump core, (3) exit process (resulting in SIGCHLD)? I don't think I've ever seen this myself. Peculiarity of some kernels, perhaps. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On 2017-09-18 12:16:42 -0400, Robert Haas wrote: > On Mon, Sep 18, 2017 at 6:32 AM, Andres Freund wrote: > > One thing that I've noticed for a while, but that I was reminded of > > again here. We very frequently allow psql to reconnect in case of crash, > > just for postmaster to notice a child has gone and kill that session. I > > don't recall that frequently happening, but these days it happens nearly > > every time. > > I don't understand what you're talking about here. I often see a backend crash, psql reacting to that crash by reconnecting, successfully establish a new connection, just to be kicked off by postmaster that does the crash restart cycle. I've not yet figured out when exactly this happens and when not. - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On 16 September 2017 at 21:27, Andres Freund wrote: > On 2017-09-16 15:59:01 -0400, Tom Lane wrote: >> This does not seem like a problem that justifies a system-wide change >> that's much more delicate than you thought. +1 The change/reason ratio is too high. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On Mon, Sep 18, 2017 at 6:32 AM, Andres Freund wrote: > One thing that I've noticed for a while, but that I was reminded of > again here. We very frequently allow psql to reconnect in case of crash, > just for postmaster to notice a child has gone and kill that session. I > don't recall that frequently happening, but these days it happens nearly > every time. I don't understand what you're talking about here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On 2017-09-17 01:07:52 -0700, Andres Freund wrote:
> On 2017-09-16 13:27:05 -0700, Andres Freund wrote:
> > > This does not seem like a problem that justifies a system-wide change
> > > that's much more delicate than you thought.
> >
> > We need one more initialization call during crash-restart - that doesn't
> > seem particularly hard a fix.
>
> FWIW, attached is that simple fix. Not quite ready for commit - needs
> more comments, thoughts and a glass of wine less to commit.
>
> I'll try to come up with a tap test that tests crash restarts tomorrow -
> not sure if there's a decent way to trigger one on windows without
> writing a C function. Will play around with that tomorrow.
This took a bit longer than I anticipated. Attached. There's
surprisingly many issues with timing that make this not as easy as I
hoped. I think in later commits we should extend this to cover things
like the autovacuum / stats process / ... being killed - but that seems
like material for a separate commit.
One thing that I've noticed for a while, but that I was reminded of
again here. We very frequently allow psql to reconnect in case of crash,
just for postmaster to notice a child has gone and kill that session. I
don't recall that frequently happening, but these days it happens nearly
every time.
Regards,
Andres
>From d7cbdc120816410335d0da12197c31192657ca42 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Sun, 17 Sep 2017 23:05:58 -0700
Subject: [PATCH] Add tests for postmaster crash restarts.
Given that I managed to break this...
Author: Andres Freund
Discussion: https://postgr.es/m/20170917080752.rcmihzfmgbeuq...@alap3.anarazel.de
---
src/test/recovery/t/013_crash_restart.pl | 192 +++
1 file changed, 192 insertions(+)
create mode 100644 src/test/recovery/t/013_crash_restart.pl
diff --git a/src/test/recovery/t/013_crash_restart.pl b/src/test/recovery/t/013_crash_restart.pl
new file mode 100644
index 00..e8ad24941b
--- /dev/null
+++ b/src/test/recovery/t/013_crash_restart.pl
@@ -0,0 +1,192 @@
+#
+# Tests restarts of postgres due to crashes of a subprocess.
+#
+# Two longer-running psql subprocesses are used: One to kill a
+# backend, triggering a crash-restart cycle, one to detect when
+# postmaster noticed the backend died. The second backend is
+# necessary because it's otherwise hard to determine if postmaster is
+# still accepting new sessions (because it hasn't noticed that the
+# backend died), or because it's already restarted.
+#
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More;
+use Config;
+use Time::HiRes qw(usleep);
+
+if ($Config{osname} eq 'MSWin32')
+{
+ # some Windows Perls at least don't like IPC::Run's
+ # start/kill_kill regime.
+ plan skip_all => "Test fails on Windows perl";
+}
+else
+{
+ plan tests => 12;
+}
+
+my $node = get_new_node('master');
+$node->init(allows_streaming => 1);
+$node->start();
+
+# by default PostgresNode doesn't doesn't restart after a crash
+$node->safe_psql('postgres',
+ q[ALTER SYSTEM SET restart_after_crash = 1;
+ ALTER SYSTEM SET log_connections = 1;
+ SELECT pg_reload_conf();]);
+
+# Run psql, keeping session alive, so we have an alive backend to kill.
+my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+my $killme = IPC::Run::start(
+ [ 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d',
+ $node->connstr('postgres') ],
+ '<',
+ \$killme_stdin,
+ '>',
+ \$killme_stdout,
+ '2>',
+ \$killme_stderr);
+
+# Need a second psql to check if crash-restart happened.
+my ($monitor_stdin, $monitor_stdout, $monitor_stderr) = ('', '', '');
+my $monitor = IPC::Run::start(
+ [ 'psql', '-X', '-qAt', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d',
+ $node->connstr('postgres') ],
+ '<',
+ \$monitor_stdin,
+ '>',
+ \$monitor_stdout,
+ '2>',
+ \$monitor_stderr);
+
+#create table, insert row that should survive
+$killme_stdin .= q[
+CREATE TABLE alive(status text);
+INSERT INTO alive VALUES($$committed-before-sigquit$$);
+SELECT pg_backend_pid();
+];
+$killme->pump until $killme_stdout =~ /[[:digit:]]+[\r\n]$/;
+my $pid = $killme_stdout;
+chomp($pid);
+$killme_stdout = '';
+
+#insert a row that should *not* survive, due to in-progress xact
+$killme_stdin .= q[
+BEGIN;
+INSERT INTO alive VALUES($$in-progress-before-sigquit$$) RETURNING status;
+];
+$killme->pump until $killme_stdout =~ /in-progress-before-sigquit/;
+$killme_stdout = '';
+
+
+# Start longrunning query in second session, it's failure will signal
+# that crash-restart has occurred.
+$monitor_stdin .= q[
+SELECT pg_sleep(3600);
+];
+$monitor->pump;
+
+
+# kill once with QUIT - we expect psql to exit, while emitting error message first
+my $cnt = kill 'QUIT', $pid;
+
+# Exactly process should have been alive to be killed
+is($cnt, 1, "exactly one process killed with SIGQUIT");
+
+# Check that psql sees the killed backend as having been terminated
+$killme_stdin .= q[
+SELECT 1;
+];
+$killme->pump until $killme_st
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On 2017-09-16 13:27:05 -0700, Andres Freund wrote:
> > This does not seem like a problem that justifies a system-wide change
> > that's much more delicate than you thought.
>
> We need one more initialization call during crash-restart - that doesn't
> seem particularly hard a fix.
FWIW, attached is that simple fix. Not quite ready for commit - needs
more comments, thoughts and a glass of wine less to commit.
I'll try to come up with a tap test that tests crash restarts tomorrow -
not sure if there's a decent way to trigger one on windows without
writing a C function. Will play around with that tomorrow.
- Andres
>From 6728a5f4620270c1a116e295f316536861a317f4 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Sun, 17 Sep 2017 01:00:39 -0700
Subject: [PATCH] Preliminary fix for crash-restart.
---
src/backend/access/transam/xlog.c | 4 ++--
src/backend/postmaster/postmaster.c | 6 +-
src/backend/tcop/postgres.c | 2 +-
src/include/access/xlog.h | 2 +-
4 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b8f648927a..b475e3fb6b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4808,9 +4808,9 @@ check_wal_buffers(int *newval, void **extra, GucSource source)
* memory. XLOGShemInit() will then copy it to shared memory later.
*/
void
-LocalProcessControlFile(void)
+LocalProcessControlFile(bool reset)
{
- Assert(ControlFile == NULL);
+ Assert(reset || ControlFile == NULL);
ControlFile = palloc(sizeof(ControlFileData));
ReadControlFile();
}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index e4f8f597c6..7ccf8dbd9d 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -951,7 +951,7 @@ PostmasterMain(int argc, char *argv[])
CreateDataDirLockFile(true);
/* read control file (error checking and contains config) */
- LocalProcessControlFile();
+ LocalProcessControlFile(false);
/*
* Initialize SSL library, if specified.
@@ -3829,6 +3829,10 @@ PostmasterStateMachine(void)
ResetBackgroundWorkerCrashTimes();
shmem_exit(1);
+
+ /* re-read control file into local memory */
+ LocalProcessControlFile(true);
+
reset_shared(PostPortNumber);
StartupPID = StartupDataBase();
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 46b662266b..dfd52b3c87 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3718,7 +3718,7 @@ PostgresMain(int argc, char *argv[],
CreateDataDirLockFile(false);
/* read control file (error checking and contains config ) */
- LocalProcessControlFile();
+ LocalProcessControlFile(false);
/* Initialize MaxBackends (if under postmaster, was done already) */
InitializeMaxBackends();
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index e0635ab4e6..7213af0e81 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -261,7 +261,7 @@ extern XLogRecPtr GetFakeLSNForUnloggedRel(void);
extern Size XLOGShmemSize(void);
extern void XLOGShmemInit(void);
extern void BootStrapXLOG(void);
-extern void LocalProcessControlFile(void);
+extern void LocalProcessControlFile(bool reset);
extern void StartupXLOG(void);
extern void ShutdownXLOG(int code, Datum arg);
extern void InitXLOGAccess(void);
--
2.14.1.536.g6867272d5b.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On 2017-09-16 15:59:01 -0400, Tom Lane wrote:
> Andres Freund writes:
> > On 2017-09-16 14:30:21 -0400, Tom Lane wrote:
> >> I wonder whether we shouldn't just revert this patch altogether.
>
> > The problem is that the patch that makes the segment size configurable
> > also adds a bunch more ordering constraints due to the fact that the
> > contents of the control file influence how much shared buffers are
> > needed (via wal_buffers = -1, which requires the segment size, which is
> > read from the control file). Reading things in the wrong order leads to
> > bad results too.
>
> Maybe we could redefine wal_buffers to avoid that. Or read the control
> file at the time where we need it.
I'm curious - do you have a good idea how to do so? It's a bit too much
magic right now (assuming the wal_segment_size patch is applied),
depending on both wal_segment_size and shared_buffers.
> This does not seem like a problem that justifies a system-wide change
> that's much more delicate than you thought.
We need one more initialization call during crash-restart - that doesn't
seem particularly hard a fix. And see below, the alternatives aren't
pretty.
> >> I'm now quite worried about whether we aren't introducing
> >> hazards of using stale values from the file; if a system crash isn't
> >> enough to get it to flush its cache, then what is?
>
> > I don't think the problem here is stale values, it's "just" a stale
> > pointer pointing into shared memory that gets reiniitalized?
>
> The code that's crashing is attempting to install some pre-existing
> copy of pg_control into shared memory. Even if there were good reason
> to think the copy were up to date, I'm not sure we should trust it
> in a crash recovery context.
We definitely should re-read it, agreed. Note there's not really a copy
here, we just assume so because the ControlFile variable isn't NULL - in
hindsight a separate bool signalling the existance of the already read
data would've probably been a good idea.
> I'd rather see this hunk of code just playing dumb and reading the
> file (which, I'm pretty sure, is what it did up till this week).
The problem is that every process needs the results of the
ReadControlFile() call. Which adjusts GUCs and other process local
variables. We can either start ferrying a lot more knowledge through
the backend variable stuff, by adding a bunch more special-case code to
the restore patch, or we're going to have to execute ReadControlFile()
in EXEC_BACKEND processes. But we can't do so once shared memory is
set-up because that'd constantly happen while other processes write to
the ControlFile.
Therefore my approach was to do a ReadControlFile() into local memory at
postmaster start (missing the crash-restart case that should also have
done so), and only do other reads in the EXEC_BACKEND case, before it
uses shared memory.
Alternatively or additionally we could split ReadControlFile() into two
parts, one that does various SetConfigOption({data_checksums,
wal_segment_size})s, computes UsableBytesInSegment etc, and one that
actually reads the file. The former would then liberally sprinkled
everywhere, because it has only process-local effects.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
Andres Freund writes: > On 2017-09-16 14:30:21 -0400, Tom Lane wrote: >> I wonder whether we shouldn't just revert this patch altogether. > The problem is that the patch that makes the segment size configurable > also adds a bunch more ordering constraints due to the fact that the > contents of the control file influence how much shared buffers are > needed (via wal_buffers = -1, which requires the segment size, which is > read from the control file). Reading things in the wrong order leads to > bad results too. Maybe we could redefine wal_buffers to avoid that. Or read the control file at the time where we need it. This does not seem like a problem that justifies a system-wide change that's much more delicate than you thought. >> I'm now quite worried about whether we aren't introducing >> hazards of using stale values from the file; if a system crash isn't >> enough to get it to flush its cache, then what is? > I don't think the problem here is stale values, it's "just" a stale > pointer pointing into shared memory that gets reiniitalized? The code that's crashing is attempting to install some pre-existing copy of pg_control into shared memory. Even if there were good reason to think the copy were up to date, I'm not sure we should trust it in a crash recovery context. I'd rather see this hunk of code just playing dumb and reading the file (which, I'm pretty sure, is what it did up till this week). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
On 2017-09-16 14:30:21 -0400, Tom Lane wrote: > Andres Freund writes: > > Looking into it. > > I wonder whether we shouldn't just revert this patch altogether. > Certainly, extra reads of pg_control are not a noticeable performance > problem. The problem is that the patch that makes the segment size configurable also adds a bunch more ordering constraints due to the fact that the contents of the control file influence how much shared buffers are needed (via wal_buffers = -1, which requires the segment size, which is read from the control file). Reading things in the wrong order leads to bad results too. > I'm now quite worried about whether we aren't introducing > hazards of using stale values from the file; if a system crash isn't > enough to get it to flush its cache, then what is? I don't think the problem here is stale values, it's "just" a stale pointer pointing into shared memory that gets reiniitalized? Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only one ReadControlFile() during startup.
Andres Freund writes: > Looking into it. I wonder whether we shouldn't just revert this patch altogether. Certainly, extra reads of pg_control are not a noticeable performance problem. I'm now quite worried about whether we aren't introducing hazards of using stale values from the file; if a system crash isn't enough to get it to flush its cache, then what is? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
