Re: Race condition in SyncRepGetSyncStandbysPriority

At Tue, 14 Apr 2020 16:32:40 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in
> I wrote:
> > It doesn't seem to me to be that hard to implement the desired
> > semantics for synchronous_standby_names with inconsistent info.
> > In FIRST mode you basically just need to take the N smallest
> > priorities you see in the array, but without assuming there are no
> > duplicates or holes. It might be a good idea to include ties at the
> > end, that is if you see 1,2,2,4 or 1,3,3,4 and you want 2 sync
> > standbys, include the first three of them in the calculation until
> > the inconsistency is resolved. In ANY mode I don't see that
> > inconsistent priorities matter at all.
>
> Concretely, I think we ought to do the attached, or something pretty
> close to it.

Looking SyncRepGetSyncStandbys, I agree that it's good not assuming
lowest_priority, which I thought as the culprit of the assertion
failure. The current code intends to use less memory. I don't think
there is a case where only 3 out of 1000 standbys are required to be
sync-standby so collecting all wal senders then sorting them seems
reasonable strategy. The new code looks clearer.

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

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

The only users of the flag seems to be:

SyncRepGetSyncRecPtr:
+ *am_sync = sync_standbys[i].is_sync_standby;

and

SyncRepGetOldestSyncRecPtr:
+ /* Ignore candidates that aren't considered synchronous */
+ if (!sync_standbys[i].is_sync_standby)
+ continue;

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 = (i < SyncRepConfig->num_sync);
break;
}

And the second user can be as the follows.

SyncRepGetOldestSyncRecPtr:
/* Ignore candidates that aren't considered synchronous */
if (i >= SyncRepConfig->num_sync)
break;

> I'm not really happy about breaking ties based on walsnd_index,
> but I see that there are several TAP test cases that fail if we
> do something else. I'm inclined to think those tests are bogus ...
> but I won't argue to change them right now.

Agreed about the tie-breaker.

I'm looking this more closer.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center