From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
cc: masahiko.sawada@2ndquadrant.com, masao.fujii@oss.nttdata.com,
        pgsql-hackers@lists.postgresql.org
Subject: Re: Race condition in SyncRepGetSyncStandbysPriority
In-reply-to: <20200415.113558.363461930251464527.horikyota.ntt@gmail.com>
References: <20200414.183538.344904915964423903.horikyota.ntt@gmail.com>
 <15616.1586872362@sss.pgh.pa.us> <13059.1586896360@sss.pgh.pa.us>
 <20200415.113558.363461930251464527.horikyota.ntt@gmail.com>
Comments: In-reply-to Kyotaro Horiguchi <horikyota.ntt@gmail.com>
	message dated "Wed, 15 Apr 2020 11:35:58 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <22257.1586964709.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 15 Apr 2020 11:31:49 -0400
Message-ID: <22258.1586964709@sss.pgh.pa.us>
Precedence: bulk

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Tue, 14 Apr 2020 16:32:40 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote i=
n =

> +		stby->is_sync_standby =3D true;	/* might change below */

> I'm uneasy with that.  In quorum mode all running standbys are marked
> as "sync" and that's bogus.

I don't follow that?  The existing coding of SyncRepGetSyncStandbysQuorum
returns all the candidates in its list, so this is isomorphic to that.

Possibly a different name for the flag would be more suited?

> On the other hand sync_standbys is already sorted in priority order so I=
 think we can get rid of the member by setting *am_sync as the follows.

> SyncRepGetSyncRecPtr:
>   if (sync_standbys[i].is_me)
>   {
>       *am_sync =3D (i < SyncRepConfig->num_sync);
>       break;
>   }

I disagree with this, it will change the behavior in the quorum case.

In any case, a change like this will cause callers to know way more than
they ought to about the ordering of the array.  In my mind, the fact that
SyncRepGetSyncStandbysPriority is sorting the array is an internal
implementation detail; I do not want it to be part of the API.

(Apropos to that, I realized from working on this patch that there's
another, completely undocumented assumption in the existing code, that
the integer list will be sorted by walsender index for equal priorities.
I don't like that either, and not just because it's undocumented.)

			regards, tom lane