From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Michael Paquier <michael@paquier.xyz>
To: Bossart, Nathan <bossartn@amazon.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: race condition when writing pg_control
Date: Fri, 29 May 2020 16:24:12 +0900
Message-ID: <fc796148-7d63-47bb-e91d-e09b62a502e9@oss.nttdata.com> (raw)
In-Reply-To: <20200527071053.GD103662@paquier.xyz>
References: <70BF24D6-DC51-443F-B55A-95735803842A@amazon.com>
<CA+hUKGLO3jStOdDgq72u=P6rDjLfwZT3Fawzd1PyLb9R2+Peow@mail.gmail.com>
<CA+hUKGJ+rud16GvO1Gg9V0P26tz7Rvz3cYVfW144b8=8ENtJ0g@mail.gmail.com>
<637e17dd-e90b-0dab-f215-dbbe9c6c726a@oss.nttdata.com>
<20200523053935.GB266978@paquier.xyz>
<D162D812-D724-4B79-8D6C-CB1EFC9A796E@amazon.com>
<20200527071053.GD103662@paquier.xyz>
On 2020/05/27 16:10, Michael Paquier wrote:
> On Tue, May 26, 2020 at 07:30:54PM +0000, Bossart, Nathan wrote:
>> While an assertion in UpdateControlFile() would not have helped us
>> catch the problem I initially reported, it does seem worthwhile to add
>> it. I have attached a patch that adds this assertion and also
>> attempts to fix XLogReportParameters(). Since there is only one place
>> where we feel it is safe to call UpdateControlFile() without a lock, I
>> just changed it to take the lock. I don't think this adds any sort of
>> significant contention risk, and IMO it is a bit cleaner than the
>> boolean flag.
>
> Let's see what Fujii-san and Thomas think about that. I'd rather
> avoid taking a lock here because we don't need it and because it makes
> things IMO confusing with the beginning of StartupXLOG() where a lot
> of the fields are read, even if we go without this extra assertion.
I have no strong opinion about this, but I tend to agree with Michael here.
>> For the XLogReportParameters() fix, I simply added an exclusive lock
>> acquisition for the portion that updates the values in shared memory
>> and calls UpdateControlFile(). IIUC the first part of this function
>> that accesses several ControlFile values should be safe, as none of
>> them can be updated after server start.
>
> They can get updated when replaying a XLOG_PARAMETER_CHANGE record.
> But you are right as all of this happens in the startup process, so
> your patch looks right to me here.
LGTM.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, michael@paquier.xyz, bossartn@amazon.com, thomas.munro@gmail.com
Subject: Re: race condition when writing pg_control
In-Reply-To: <fc796148-7d63-47bb-e91d-e09b62a502e9@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox