From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: masahiko.sawada@2ndquadrant.com
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Race condition in SyncRepGetSyncStandbysPriority
Date: Fri, 17 Apr 2020 02:20:04 +0900
Message-ID: <43ecec68-abff-6cdb-8412-204451d650ac@oss.nttdata.com> (raw)
In-Reply-To: <15616.1586872362@sss.pgh.pa.us>
References: <20200413.153101.688623432137444694.horikyota.ntt@gmail.com>
<8325.1586828060@sss.pgh.pa.us>
<CA+fd4k4f+QrGOpEMR3GA6yHakCr6ssJB_EbRrdC-=miwEZf3mQ@mail.gmail.com>
<20200414.183538.344904915964423903.horikyota.ntt@gmail.com>
<15616.1586872362@sss.pgh.pa.us>
On 2020/04/14 22:52, Tom Lane wrote:
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
>> SyncRepGetSyncStandbysPriority() is runing holding SyncRepLock so
>> sync_standby_priority of any walsender can be changed while the
>> function is scanning welsenders. The issue is we already have
>> inconsistent walsender information before we enter the function. Thus
>> how many times we scan on the array doesn't make any difference.
>
> *Yes it does*. The existing code can deliver entirely broken results
> if some walsender exits between where we examine the priorities and
> where we fetch the WAL pointers.
So, in this case, the oldest lsn that SyncRepGetOldestSyncRecPtr()
calculates may be based on also the lsn of already-exited walsender.
This is what you say "broken results"? If yes, ISTM that this issue still
remains even after applying your patch. No? The walsender marked
as sync may still exit just before SyncRepGetOldestSyncRecPtr()
calculates the oldest lsn.
IMO that the broken results can be delivered when walsender marked
as sync exits *and* new walsender comes at that moment. If this new
walsender uses the WalSnd slot that the exited walsender used,
SyncRepGetOldestSyncRecPtr() wronly calculates the oldest lsn based
on this new walsender (i.e., different walsender from one marked as sync).
If this is actually what you tried to say "broken results", your patch
seems fine and fixes the issue.
BTW, since the patch changes the API of SyncRepGetSyncStandbys(),
it should not be back-patched to avoid ABI break. Right?
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, tgl@sss.pgh.pa.us, horikyota.ntt@gmail.com, masahiko.sawada@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Race condition in SyncRepGetSyncStandbysPriority
In-Reply-To: <43ecec68-abff-6cdb-8412-204451d650ac@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