Re: [HACKERS] Re: [COMMITTERS] pgsql: Perform only oneReadControlFile() during startup.

On 2017-09-16 15:59:01 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> 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