Horton

Thread

Commits

  1. Fix race conditions in synchronous standby management.

  2. When WalSndCaughtUp, sleep only in WalSndWaitForWal().

  1. Race condition in SyncRepGetSyncStandbysPriority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-03-27T01:26:49Z

    Twice in the past month [1][2], buildfarm member hoverfly has managed
to reach the "unreachable" Assert(false) at the end of
SyncRepGetSyncStandbysPriority.

What seems likely to me, after quickly eyeballing the code, is that
hoverfly is hitting the blatantly-obvious race condition in that function.
Namely, that the second loop supposes that the state of the walsender
array hasn't changed since the first loop.

The minimum fix for this, I suppose, would have the first loop capture
the sync_standby_priority value for each walsender along with what it's
already capturing.  But I wonder if the whole function shouldn't be
rewritten from scratch, because it seems like the algorithm is both
expensively brute-force and unintelligible, which is a sad combination.
It's likely that the number of walsenders would never be high enough
that efficiency could matter, but then couldn't we use an algorithm
that is less complicated and more obviously correct?  (Because the
alternative conclusion, if you reject the theory that a race is happening,
is that the algorithm is just flat out buggy; something that's not too
easy to disprove either.)

Another fairly dubious thing here is that whether or not *am_sync
gets set depends not only on whether MyWalSnd is claiming to be
synchronous but on how many lower-numbered walsenders are too.
Is that really the right thing?

But worse than any of that is that the return value seems to be
a list of walsender array indexes, meaning that the callers cannot
use it without making even stronger assumptions about the array
contents not having changed since the start of this function.

It sort of looks like the design is based on the assumption that
the array contents can't change while SyncRepLock is held ... but
if that's the plan then why bother with the per-walsender spinlocks?
In any case this assumption seems to be failing, suggesting either
that there's a caller that's not holding SyncRepLock when it calls
this function, or that somebody is failing to take that lock while
modifying the array.

			regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2020-02-29%2001%3A34%3A55
[2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hoverfly&dt=2020-03-26%2013%3A51%3A15

  2. Re: Race condition in SyncRepGetSyncStandbysPriority

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-03-27T04:54:25Z

    
On 2020/03/27 10:26, Tom Lane wrote:
> Twice in the past month [1][2], buildfarm member hoverfly has managed
> to reach the "unreachable" Assert(false) at the end of
> SyncRepGetSyncStandbysPriority.

When I search the past discussions, I found that Noah Misch reported
the same issue.
https://www.postgresql.org/message-id/20200206074552.GB3326097@rfd.leadboat.com

> What seems likely to me, after quickly eyeballing the code, is that
> hoverfly is hitting the blatantly-obvious race condition in that function.
> Namely, that the second loop supposes that the state of the walsender
> array hasn't changed since the first loop.
> 
> The minimum fix for this, I suppose, would have the first loop capture
> the sync_standby_priority value for each walsender along with what it's
> already capturing.  But I wonder if the whole function shouldn't be
> rewritten from scratch, because it seems like the algorithm is both
> expensively brute-force and unintelligible, which is a sad combination.
> It's likely that the number of walsenders would never be high enough
> that efficiency could matter, but then couldn't we use an algorithm
> that is less complicated and more obviously correct?

+1 to rewrite the function with better algorithm.

> (Because the
> alternative conclusion, if you reject the theory that a race is happening,
> is that the algorithm is just flat out buggy; something that's not too
> easy to disprove either.)
> 
> Another fairly dubious thing here is that whether or not *am_sync
> gets set depends not only on whether MyWalSnd is claiming to be
> synchronous but on how many lower-numbered walsenders are too.
> Is that really the right thing?
> 
> But worse than any of that is that the return value seems to be
> a list of walsender array indexes, meaning that the callers cannot
> use it without making even stronger assumptions about the array
> contents not having changed since the start of this function.
> 
> It sort of looks like the design is based on the assumption that
> the array contents can't change while SyncRepLock is held ... but
> if that's the plan then why bother with the per-walsender spinlocks?
> In any case this assumption seems to be failing, suggesting either
> that there's a caller that's not holding SyncRepLock when it calls
> this function, or that somebody is failing to take that lock while
> modifying the array.

As far as I read the code, that assumption seems still valid. But the problem
is that each walsender updates MyWalSnd->sync_standby_priority at each
convenient timing, when SIGHUP is signaled. That is, at a certain moment,
some walsenders (also their WalSnd entries in shmem) work based on
the latest configuration but the others (also their WalSnd entries) work based
on the old one.

	lowest_priority = SyncRepConfig->nmembers;
	next_highest_priority = lowest_priority + 1;

SyncRepGetSyncStandbysPriority() calculates the lowest priority among
all running walsenders as the above, by using the configuration info that
this walsender is based on. But this calculated lowest priority would be
invalid if other walsender is based on different (e.g., old) configuraiton.
This can cause the (other) walsender to have lower priority than
the calculated lowest priority and the second loop in
SyncRepGetSyncStandbysPriority() to unexpectedly end.

Therefore, the band-aid fix seems to be to set the lowest priority to
very large number at the beginning of SyncRepGetSyncStandbysPriority().

Regards,

-- 
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

  3. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-03-30T07:53:10Z

    At Fri, 27 Mar 2020 13:54:25 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2020/03/27 10:26, Tom Lane wrote:
> > Twice in the past month [1][2], buildfarm member hoverfly has managed
> > to reach the "unreachable" Assert(false) at the end of
> > SyncRepGetSyncStandbysPriority.
> 
> When I search the past discussions, I found that Noah Misch reported
> the same issue.
> https://www.postgresql.org/message-id/20200206074552.GB3326097@rfd.leadboat.com
> 
> > What seems likely to me, after quickly eyeballing the code, is that
> > hoverfly is hitting the blatantly-obvious race condition in that
> > function.
> > Namely, that the second loop supposes that the state of the walsender
> > array hasn't changed since the first loop.
> > The minimum fix for this, I suppose, would have the first loop capture
> > the sync_standby_priority value for each walsender along with what
> > it's
> > already capturing.  But I wonder if the whole function shouldn't be
> > rewritten from scratch, because it seems like the algorithm is both
> > expensively brute-force and unintelligible, which is a sad
> > combination.
> > It's likely that the number of walsenders would never be high enough
> > that efficiency could matter, but then couldn't we use an algorithm
> > that is less complicated and more obviously correct?
> 
> +1 to rewrite the function with better algorithm.
> 
> > (Because the
> > alternative conclusion, if you reject the theory that a race is
> > happening,
> > is that the algorithm is just flat out buggy; something that's not too
> > easy to disprove either.)
> > Another fairly dubious thing here is that whether or not *am_sync
> > gets set depends not only on whether MyWalSnd is claiming to be
> > synchronous but on how many lower-numbered walsenders are too.
> > Is that really the right thing?
> > But worse than any of that is that the return value seems to be
> > a list of walsender array indexes, meaning that the callers cannot
> > use it without making even stronger assumptions about the array
> > contents not having changed since the start of this function.
> > It sort of looks like the design is based on the assumption that
> > the array contents can't change while SyncRepLock is held ... but
> > if that's the plan then why bother with the per-walsender spinlocks?
> > In any case this assumption seems to be failing, suggesting either
> > that there's a caller that's not holding SyncRepLock when it calls
> > this function, or that somebody is failing to take that lock while
> > modifying the array.
> 
> As far as I read the code, that assumption seems still valid. But the
> problem
> is that each walsender updates MyWalSnd->sync_standby_priority at each
> convenient timing, when SIGHUP is signaled. That is, at a certain
> moment,
> some walsenders (also their WalSnd entries in shmem) work based on
> the latest configuration but the others (also their WalSnd entries)
> work based
> on the old one.
> 
> 	lowest_priority = SyncRepConfig->nmembers;
> 	next_highest_priority = lowest_priority + 1;
> 
> SyncRepGetSyncStandbysPriority() calculates the lowest priority among
> all running walsenders as the above, by using the configuration info
> that
> this walsender is based on. But this calculated lowest priority would
> be
> invalid if other walsender is based on different (e.g., old)
> configuraiton.
> This can cause the (other) walsender to have lower priority than
> the calculated lowest priority and the second loop in
> SyncRepGetSyncStandbysPriority() to unexpectedly end.
> 
> Therefore, the band-aid fix seems to be to set the lowest priority to
> very large number at the beginning of
> SyncRepGetSyncStandbysPriority().

Or just ignore impossible priorities as non-sync standby. Anyway the
confused state is fixed after all walsenders have loaded the new
configuration.

I remember that I posted a bandaid for maybe the same issue.

https://www.postgresql.org/message-id/20200207.125251.146972241588695685.horikyota.ntt@gmail.com

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: Race condition in SyncRepGetSyncStandbysPriority

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-03-31T14:16:20Z

    On Fri, 27 Mar 2020 at 13:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/03/27 10:26, Tom Lane wrote:
> > Twice in the past month [1][2], buildfarm member hoverfly has managed
> > to reach the "unreachable" Assert(false) at the end of
> > SyncRepGetSyncStandbysPriority.
>
> When I search the past discussions, I found that Noah Misch reported
> the same issue.
> https://www.postgresql.org/message-id/20200206074552.GB3326097@rfd.leadboat.com
>
> > What seems likely to me, after quickly eyeballing the code, is that
> > hoverfly is hitting the blatantly-obvious race condition in that function.
> > Namely, that the second loop supposes that the state of the walsender
> > array hasn't changed since the first loop.
> >
> > The minimum fix for this, I suppose, would have the first loop capture
> > the sync_standby_priority value for each walsender along with what it's
> > already capturing.  But I wonder if the whole function shouldn't be
> > rewritten from scratch, because it seems like the algorithm is both
> > expensively brute-force and unintelligible, which is a sad combination.
> > It's likely that the number of walsenders would never be high enough
> > that efficiency could matter, but then couldn't we use an algorithm
> > that is less complicated and more obviously correct?
>
> +1 to rewrite the function with better algorithm.
>
> > (Because the
> > alternative conclusion, if you reject the theory that a race is happening,
> > is that the algorithm is just flat out buggy; something that's not too
> > easy to disprove either.)
> >
> > Another fairly dubious thing here is that whether or not *am_sync
> > gets set depends not only on whether MyWalSnd is claiming to be
> > synchronous but on how many lower-numbered walsenders are too.
> > Is that really the right thing?
> >
> > But worse than any of that is that the return value seems to be
> > a list of walsender array indexes, meaning that the callers cannot
> > use it without making even stronger assumptions about the array
> > contents not having changed since the start of this function.
> >
> > It sort of looks like the design is based on the assumption that
> > the array contents can't change while SyncRepLock is held ... but
> > if that's the plan then why bother with the per-walsender spinlocks?
> > In any case this assumption seems to be failing, suggesting either
> > that there's a caller that's not holding SyncRepLock when it calls
> > this function, or that somebody is failing to take that lock while
> > modifying the array.
>
> As far as I read the code, that assumption seems still valid. But the problem
> is that each walsender updates MyWalSnd->sync_standby_priority at each
> convenient timing, when SIGHUP is signaled. That is, at a certain moment,
> some walsenders (also their WalSnd entries in shmem) work based on
> the latest configuration but the others (also their WalSnd entries) work based
> on the old one.
>
>         lowest_priority = SyncRepConfig->nmembers;
>         next_highest_priority = lowest_priority + 1;
>
> SyncRepGetSyncStandbysPriority() calculates the lowest priority among
> all running walsenders as the above, by using the configuration info that
> this walsender is based on. But this calculated lowest priority would be
> invalid if other walsender is based on different (e.g., old) configuraiton.
> This can cause the (other) walsender to have lower priority than
> the calculated lowest priority and the second loop in
> SyncRepGetSyncStandbysPriority() to unexpectedly end.

I have the same understanding. Since sync_standby_priroity is
protected by SyncRepLock these values of each walsender are not
changed through two loops in SyncRepGetSyncStandbysPriority().
However, as Fujii-san already mentioned the true lowest priority can
be lower than lowest_priority, nmembers, when only part of walsenders
reloaded the configuration, which in turn could be the cause of
leaving entries in the pending list at the end of the function.

> Therefore, the band-aid fix seems to be to set the lowest priority to
> very large number at the beginning of SyncRepGetSyncStandbysPriority().

I think we can use max_wal_senders. And we can change the second loop
so that we exit from the function if the pending list is empty.

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  5. Re: Race condition in SyncRepGetSyncStandbysPriority

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-01T02:01:22Z

    On Tue, 31 Mar 2020 at 23:16, Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> On Fri, 27 Mar 2020 at 13:54, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >
> >
> >
> > On 2020/03/27 10:26, Tom Lane wrote:
> > > Twice in the past month [1][2], buildfarm member hoverfly has managed
> > > to reach the "unreachable" Assert(false) at the end of
> > > SyncRepGetSyncStandbysPriority.
> >
> > When I search the past discussions, I found that Noah Misch reported
> > the same issue.
> > https://www.postgresql.org/message-id/20200206074552.GB3326097@rfd.leadboat.com
> >
> > > What seems likely to me, after quickly eyeballing the code, is that
> > > hoverfly is hitting the blatantly-obvious race condition in that function.
> > > Namely, that the second loop supposes that the state of the walsender
> > > array hasn't changed since the first loop.
> > >
> > > The minimum fix for this, I suppose, would have the first loop capture
> > > the sync_standby_priority value for each walsender along with what it's
> > > already capturing.  But I wonder if the whole function shouldn't be
> > > rewritten from scratch, because it seems like the algorithm is both
> > > expensively brute-force and unintelligible, which is a sad combination.
> > > It's likely that the number of walsenders would never be high enough
> > > that efficiency could matter, but then couldn't we use an algorithm
> > > that is less complicated and more obviously correct?
> >
> > +1 to rewrite the function with better algorithm.
> >
> > > (Because the
> > > alternative conclusion, if you reject the theory that a race is happening,
> > > is that the algorithm is just flat out buggy; something that's not too
> > > easy to disprove either.)
> > >
> > > Another fairly dubious thing here is that whether or not *am_sync
> > > gets set depends not only on whether MyWalSnd is claiming to be
> > > synchronous but on how many lower-numbered walsenders are too.
> > > Is that really the right thing?
> > >
> > > But worse than any of that is that the return value seems to be
> > > a list of walsender array indexes, meaning that the callers cannot
> > > use it without making even stronger assumptions about the array
> > > contents not having changed since the start of this function.
> > >
> > > It sort of looks like the design is based on the assumption that
> > > the array contents can't change while SyncRepLock is held ... but
> > > if that's the plan then why bother with the per-walsender spinlocks?
> > > In any case this assumption seems to be failing, suggesting either
> > > that there's a caller that's not holding SyncRepLock when it calls
> > > this function, or that somebody is failing to take that lock while
> > > modifying the array.
> >
> > As far as I read the code, that assumption seems still valid. But the problem
> > is that each walsender updates MyWalSnd->sync_standby_priority at each
> > convenient timing, when SIGHUP is signaled. That is, at a certain moment,
> > some walsenders (also their WalSnd entries in shmem) work based on
> > the latest configuration but the others (also their WalSnd entries) work based
> > on the old one.
> >
> >         lowest_priority = SyncRepConfig->nmembers;
> >         next_highest_priority = lowest_priority + 1;
> >
> > SyncRepGetSyncStandbysPriority() calculates the lowest priority among
> > all running walsenders as the above, by using the configuration info that
> > this walsender is based on. But this calculated lowest priority would be
> > invalid if other walsender is based on different (e.g., old) configuraiton.
> > This can cause the (other) walsender to have lower priority than
> > the calculated lowest priority and the second loop in
> > SyncRepGetSyncStandbysPriority() to unexpectedly end.
>
> I have the same understanding. Since sync_standby_priroity is
> protected by SyncRepLock these values of each walsender are not
> changed through two loops in SyncRepGetSyncStandbysPriority().
> However, as Fujii-san already mentioned the true lowest priority can
> be lower than lowest_priority, nmembers, when only part of walsenders
> reloaded the configuration, which in turn could be the cause of
> leaving entries in the pending list at the end of the function.
>
> > Therefore, the band-aid fix seems to be to set the lowest priority to
> > very large number at the beginning of SyncRepGetSyncStandbysPriority().
>
> I think we can use max_wal_senders.

Sorry, that's not true. We need another number large enough.

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: Race condition in SyncRepGetSyncStandbysPriority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-11T22:30:30Z

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> writes:
> On Tue, 31 Mar 2020 at 23:16, Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
>>> Therefore, the band-aid fix seems to be to set the lowest priority to
>>> very large number at the beginning of SyncRepGetSyncStandbysPriority().

>> I think we can use max_wal_senders.

> Sorry, that's not true. We need another number large enough.

The buildfarm had another three failures of this type today, so that
motivated me to look at it some more.  I don't think this code needs
a band-aid fix; I think "nuke from orbit" is more nearly the right
level of response.

The point that I was trying to make originally is that it seems quite
insane to imagine that a walsender's sync_standby_priority value is
somehow more stable than the very existence of the process.  Yet we
only require a walsender to lock its own mutex while claiming or
disowning its WalSnd entry (by setting or clearing the pid field).
So I think it's nuts to define those fields as being protected by
the global SyncRepLock.

Even without considering the possibility that a walsender has just
started or stopped, we have the problem Fujii-san described that after
a change in the synchronous_standby_names GUC setting, different
walsenders will update their values of sync_standby_priority at
different instants.  (BTW, I now notice that Noah had previously
identified this problem at [1].)

Thus, even while holding SyncRepLock, we do not have a guarantee that
we'll see a consistent set of sync_standby_priority values.  In fact
we don't even know that the walsnd array entries still belong to the
processes that last set those values.  This is what is breaking
SyncRepGetSyncStandbysPriority, and what it means is that there's
really fundamentally no chance of that function producing trustworthy
results.  The "band aid" fixes discussed here might avoid crashing on
the Assert, but they won't fix the problems that (a) the result is
possibly wrong and (b) it can become stale immediately even if it's
right when returned.

Now, there are only two callers of SyncRepGetSyncStandbys:
SyncRepGetSyncRecPtr and pg_stat_get_wal_senders.  The latter is
mostly cosmetic (which is a good thing, because to add insult to
injury, it continues to use the list after releasing SyncRepLock;
not that continuing to hold that lock would make things much safer).
If I'm reading the code correctly, the former doesn't really care
exactly which walsenders are sync standbys: all it cares about is
to collect their WAL position pointers.

What I think we should do about this is, essentially, to get rid of
SyncRepGetSyncStandbys.  Instead, let's have each walsender advertise
whether *it* believes that it is a sync standby, based on its last
evaluation of the relevant GUCs.  This would be a bool that it'd
compute and set alongside sync_standby_priority.  (Hm, maybe we'd not
even need to have that field anymore?  Not sure.)  We should also
redefine that flag, and sync_standby_priority if it survives, as being
protected by the per-walsender mutex not SyncRepLock.  Then, what
SyncRepGetSyncRecPtr would do is just sweep through the walsender
array and collect WAL position pointers from the walsenders that
claim to be sync standbys at the instant that it's inspecting them.
pg_stat_get_wal_senders could also use those flags instead of the
list from SyncRepGetSyncStandbys.

It's likely that this definition would have slightly different
behavior from the current implementation during the period where
the system is absorbing a change in the set of synchronous
walsenders.  However, since the current implementation is visibly
wrong during that period anyway, I'm not sure how this could be
worse.  And at least we can be certain that SyncRepGetSyncRecPtr
will not include WAL positions from already-dead walsenders in
its calculations, which *is* a hazard in the code as it stands.

I also estimate that this would be noticeably more efficient than
the current code, since the logic to decide who's a sync standby
would only run when we're dealing with walsender start/stop or
SIGHUP, rather than every time SyncRepGetSyncRecPtr runs.

Don't especially want to code this myself, though.  Anyone?

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/20200206074552.GB3326097%40rfd.leadboat.com

  7. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-13T06:31:01Z

    At Sat, 11 Apr 2020 18:30:30 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Masahiko Sawada <masahiko.sawada@2ndquadrant.com> writes:
> > On Tue, 31 Mar 2020 at 23:16, Masahiko Sawada
> > <masahiko.sawada@2ndquadrant.com> wrote:
> >>> Therefore, the band-aid fix seems to be to set the lowest priority to
> >>> very large number at the beginning of SyncRepGetSyncStandbysPriority().
> 
> >> I think we can use max_wal_senders.
> 
> > Sorry, that's not true. We need another number large enough.
> 
> The buildfarm had another three failures of this type today, so that
> motivated me to look at it some more.  I don't think this code needs
> a band-aid fix; I think "nuke from orbit" is more nearly the right
> level of response.
> 
> The point that I was trying to make originally is that it seems quite
> insane to imagine that a walsender's sync_standby_priority value is
> somehow more stable than the very existence of the process.  Yet we
> only require a walsender to lock its own mutex while claiming or
> disowning its WalSnd entry (by setting or clearing the pid field).
> So I think it's nuts to define those fields as being protected by
> the global SyncRepLock.

Right. FWIW, furthermore, even SyncRepConfig->syncrep_method can be
inconsistent among walsenders.  I haven't thought that it can be
relied on as always consistent and it is enough that it makes a
consistent result only while the setting and the set of walsenders is
stable.

> Even without considering the possibility that a walsender has just
> started or stopped, we have the problem Fujii-san described that after
> a change in the synchronous_standby_names GUC setting, different
> walsenders will update their values of sync_standby_priority at
> different instants.  (BTW, I now notice that Noah had previously
> identified this problem at [1].)
> 
> Thus, even while holding SyncRepLock, we do not have a guarantee that
> we'll see a consistent set of sync_standby_priority values.  In fact
> we don't even know that the walsnd array entries still belong to the
> processes that last set those values.  This is what is breaking
> SyncRepGetSyncStandbysPriority, and what it means is that there's
> really fundamentally no chance of that function producing trustworthy
> results.  The "band aid" fixes discussed here might avoid crashing on
> the Assert, but they won't fix the problems that (a) the result is
> possibly wrong and (b) it can become stale immediately even if it's
> right when returned.

Agreed. And I thought that it's not a problem if we had wrong result
temporarily. And the unstability persists for the standby-reply
interval at most (unless the next cause of instability comes).

> Now, there are only two callers of SyncRepGetSyncStandbys:
> SyncRepGetSyncRecPtr and pg_stat_get_wal_senders.  The latter is
> mostly cosmetic (which is a good thing, because to add insult to
> injury, it continues to use the list after releasing SyncRepLock;
> not that continuing to hold that lock would make things much safer).
> If I'm reading the code correctly, the former doesn't really care
> exactly which walsenders are sync standbys: all it cares about is
> to collect their WAL position pointers.

Agreed.  To find the sync standby with the largest delay.

> What I think we should do about this is, essentially, to get rid of
> SyncRepGetSyncStandbys.  Instead, let's have each walsender advertise
> whether *it* believes that it is a sync standby, based on its last
> evaluation of the relevant GUCs.  This would be a bool that it'd
> compute and set alongside sync_standby_priority.  (Hm, maybe we'd not

Mmm..  SyncRepGetStandbyPriority returns the "priority" that a
walsender thinks it is at, among synchronous_standby_names.  Then to
decide "I am a sync standby" we need to know how many walsenders with
higher priority are alive now.  SyncRepGetSyncStandbyPriority does the
judgment now and suffers from the inconsistency of priority values.

In short, it seems to me like moving the problem into another
place. But I think that there might be a smarter way to find "I am
sync".

> even need to have that field anymore?  Not sure.)  We should also
> redefine that flag, and sync_standby_priority if it survives, as being
> protected by the per-walsender mutex not SyncRepLock.  Then, what
> SyncRepGetSyncRecPtr would do is just sweep through the walsender
> array and collect WAL position pointers from the walsenders that
> claim to be sync standbys at the instant that it's inspecting them.
> pg_stat_get_wal_senders could also use those flags instead of the
> list from SyncRepGetSyncStandbys.
> 
> It's likely that this definition would have slightly different
> behavior from the current implementation during the period where
> the system is absorbing a change in the set of synchronous
> walsenders.  However, since the current implementation is visibly
> wrong during that period anyway, I'm not sure how this could be
> worse.  And at least we can be certain that SyncRepGetSyncRecPtr
> will not include WAL positions from already-dead walsenders in
> its calculations, which *is* a hazard in the code as it stands.
> 
> I also estimate that this would be noticeably more efficient than
> the current code, since the logic to decide who's a sync standby
> would only run when we're dealing with walsender start/stop or
> SIGHUP, rather than every time SyncRepGetSyncRecPtr runs.
> 
> Don't especially want to code this myself, though.  Anyone?
> 
> 			regards, tom lane
> 
> [1] https://www.postgresql.org/message-id/flat/20200206074552.GB3326097%40rfd.leadboat.com

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  8. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-13T06:34:24Z

    At Mon, 13 Apr 2020 15:31:01 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Sat, 11 Apr 2020 18:30:30 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> > The point that I was trying to make originally is that it seems quite
> > insane to imagine that a walsender's sync_standby_priority value is
> > somehow more stable than the very existence of the process.  Yet we
> > only require a walsender to lock its own mutex while claiming or
> > disowning its WalSnd entry (by setting or clearing the pid field).
> > So I think it's nuts to define those fields as being protected by
> > the global SyncRepLock.
> 
> Right. FWIW, furthermore, even SyncRepConfig->syncrep_method can be
> inconsistent among walsenders.  I haven't thought that it can be
> relied on as always consistent and it is enough that it makes a
> consistent result only while the setting and the set of walsenders is
> stable.

Yes, the sentene "and (I haven't thought that) it is enough .." is a
mistake of "and I have thought that it is enough that..".

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  9. Re: Race condition in SyncRepGetSyncStandbysPriority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-14T01:34:20Z

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Sat, 11 Apr 2020 18:30:30 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
>> What I think we should do about this is, essentially, to get rid of
>> SyncRepGetSyncStandbys.  Instead, let's have each walsender advertise
>> whether *it* believes that it is a sync standby, based on its last
>> evaluation of the relevant GUCs.  This would be a bool that it'd
>> compute and set alongside sync_standby_priority.  (Hm, maybe we'd not

> Mmm..  SyncRepGetStandbyPriority returns the "priority" that a
> walsender thinks it is at, among synchronous_standby_names.  Then to
> decide "I am a sync standby" we need to know how many walsenders with
> higher priority are alive now.  SyncRepGetSyncStandbyPriority does the
> judgment now and suffers from the inconsistency of priority values.

Yeah.  After looking a bit closer, I think that the current definition
of sync_standby_priority (that is, as the result of local evaluation
of SyncRepGetStandbyPriority()) is OK.  The problem is what we're doing
with it.  I suggest that what we should do in SyncRepGetSyncRecPtr()
is make one sweep across the WalSnd array, collecting PID,
sync_standby_priority, *and* the WAL pointers from each valid entry.
Then examine that data and decide which WAL value we need, without assuming
that the sync_standby_priority values are necessarily totally consistent.
But in any case we must examine each entry just once while holding its
mutex, not go back to it later expecting it to still be the same.

Another thing that I'm finding interesting is that I now see this is
not at all new code.  It doesn't look like SyncRepGetSyncStandbysPriority
has changed much since 2016.  So how come we didn't detect this problem
long ago?  I searched the buildfarm logs for assertion failures in
syncrep.c, looking back one year, and here's what I found:

  sysname   |    branch     |      snapshot       |     stage     |                                                                           l                                                                           
------------+---------------+---------------------+---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------
 nightjar   | REL_10_STABLE | 2019-08-13 23:04:41 | recoveryCheck | TRAP: FailedAssertion("!(((bool) 0))", File: "/pgbuild/root/REL_10_STABLE/pgsql.build/../pgsql/src/backend/replication/syncrep.c", Line: 940)
 hoverfly   | REL9_6_STABLE | 2019-11-07 17:19:12 | recoveryCheck | TRAP: FailedAssertion("!(((bool) 0))", File: "syncrep.c", Line: 723)
 hoverfly   | HEAD          | 2019-11-22 12:15:08 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
 francolin  | HEAD          | 2020-01-16 23:10:06 | recoveryCheck | TRAP: FailedAssertion("false", File: "/home/andres/build/buildfarm-francolin/HEAD/pgsql.build/../pgsql/src/backend/replication/syncrep.c", Line: 951)
 hoverfly   | REL_11_STABLE | 2020-02-29 01:34:55 | recoveryCheck | TRAP: FailedAssertion("!(0)", File: "syncrep.c", Line: 946)
 hoverfly   | REL9_6_STABLE | 2020-03-26 13:51:15 | recoveryCheck | TRAP: FailedAssertion("!(((bool) 0))", File: "syncrep.c", Line: 723)
 hoverfly   | REL9_6_STABLE | 2020-04-07 21:52:07 | recoveryCheck | TRAP: FailedAssertion("!(((bool) 0))", File: "syncrep.c", Line: 723)
 curculio   | HEAD          | 2020-04-11 18:30:21 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
 sidewinder | HEAD          | 2020-04-11 18:45:39 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
 curculio   | HEAD          | 2020-04-11 20:30:26 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
 sidewinder | HEAD          | 2020-04-11 21:45:48 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
 sidewinder | HEAD          | 2020-04-13 10:45:35 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
 conchuela  | HEAD          | 2020-04-13 16:00:18 | recoveryCheck | TRAP: FailedAssertion("false", File: "/home/pgbf/buildroot/HEAD/pgsql.build/../pgsql/src/backend/replication/syncrep.c", Line: 951)
 sidewinder | HEAD          | 2020-04-13 18:45:34 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
(14 rows)

The line numbers vary in the back branches, but all of these crashes are
at that same Assert.  So (a) yes, this does happen in the back branches,
but (b) some fairly recent change has made it a whole lot more probable.
Neither syncrep.c nor 007_sync_rep.pl have changed much in some time,
so whatever the change was was indirect.  Curious.  Is it just timing?

I'm giving the side-eye to Noah's recent changes 328c70997 and 421685812,
but this isn't enough evidence to say definitely that that's what boosted
the failure rate.

			regards, tom lane

  10. Re: Race condition in SyncRepGetSyncStandbysPriority

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-14T04:06:14Z

    On Tue, 14 Apr 2020 at 10:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > At Sat, 11 Apr 2020 18:30:30 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> >> What I think we should do about this is, essentially, to get rid of
> >> SyncRepGetSyncStandbys.  Instead, let's have each walsender advertise
> >> whether *it* believes that it is a sync standby, based on its last
> >> evaluation of the relevant GUCs.  This would be a bool that it'd
> >> compute and set alongside sync_standby_priority.  (Hm, maybe we'd not
>
> > Mmm..  SyncRepGetStandbyPriority returns the "priority" that a
> > walsender thinks it is at, among synchronous_standby_names.  Then to
> > decide "I am a sync standby" we need to know how many walsenders with
> > higher priority are alive now.  SyncRepGetSyncStandbyPriority does the
> > judgment now and suffers from the inconsistency of priority values.
>
> Yeah.  After looking a bit closer, I think that the current definition
> of sync_standby_priority (that is, as the result of local evaluation
> of SyncRepGetStandbyPriority()) is OK.  The problem is what we're doing
> with it.  I suggest that what we should do in SyncRepGetSyncRecPtr()
> is make one sweep across the WalSnd array, collecting PID,
> sync_standby_priority, *and* the WAL pointers from each valid entry.
> Then examine that data and decide which WAL value we need, without assuming
> that the sync_standby_priority values are necessarily totally consistent.
> But in any case we must examine each entry just once while holding its
> mutex, not go back to it later expecting it to still be the same.

Can we have a similar approach of sync_standby_defined for
sync_standby_priority? That is, checkpionter is responsible for
changing sync_standby_priority of all walsenders when SIGHUP. That
way, all walsenders can see a consistent view of
sync_standby_priority. And when a walsender starts, it sets
sync_standby_priority by itself. The logic to decide who's a sync
standby doesn't change. SyncRepGetSyncRecPtr() gets all walsenders
having higher priority along with their WAL positions.

>
> Another thing that I'm finding interesting is that I now see this is
> not at all new code.  It doesn't look like SyncRepGetSyncStandbysPriority
> has changed much since 2016.  So how come we didn't detect this problem
> long ago?  I searched the buildfarm logs for assertion failures in
> syncrep.c, looking back one year, and here's what I found:
>
>   sysname   |    branch     |      snapshot       |     stage     |                                                                           l
> ------------+---------------+---------------------+---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------
>  nightjar   | REL_10_STABLE | 2019-08-13 23:04:41 | recoveryCheck | TRAP: FailedAssertion("!(((bool) 0))", File: "/pgbuild/root/REL_10_STABLE/pgsql.build/../pgsql/src/backend/replication/syncrep.c", Line: 940)
>  hoverfly   | REL9_6_STABLE | 2019-11-07 17:19:12 | recoveryCheck | TRAP: FailedAssertion("!(((bool) 0))", File: "syncrep.c", Line: 723)
>  hoverfly   | HEAD          | 2019-11-22 12:15:08 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
>  francolin  | HEAD          | 2020-01-16 23:10:06 | recoveryCheck | TRAP: FailedAssertion("false", File: "/home/andres/build/buildfarm-francolin/HEAD/pgsql.build/../pgsql/src/backend/replication/syncrep.c", Line: 951)
>  hoverfly   | REL_11_STABLE | 2020-02-29 01:34:55 | recoveryCheck | TRAP: FailedAssertion("!(0)", File: "syncrep.c", Line: 946)
>  hoverfly   | REL9_6_STABLE | 2020-03-26 13:51:15 | recoveryCheck | TRAP: FailedAssertion("!(((bool) 0))", File: "syncrep.c", Line: 723)
>  hoverfly   | REL9_6_STABLE | 2020-04-07 21:52:07 | recoveryCheck | TRAP: FailedAssertion("!(((bool) 0))", File: "syncrep.c", Line: 723)
>  curculio   | HEAD          | 2020-04-11 18:30:21 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
>  sidewinder | HEAD          | 2020-04-11 18:45:39 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
>  curculio   | HEAD          | 2020-04-11 20:30:26 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
>  sidewinder | HEAD          | 2020-04-11 21:45:48 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
>  sidewinder | HEAD          | 2020-04-13 10:45:35 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
>  conchuela  | HEAD          | 2020-04-13 16:00:18 | recoveryCheck | TRAP: FailedAssertion("false", File: "/home/pgbf/buildroot/HEAD/pgsql.build/../pgsql/src/backend/replication/syncrep.c", Line: 951)
>  sidewinder | HEAD          | 2020-04-13 18:45:34 | recoveryCheck | TRAP: FailedAssertion("false", File: "syncrep.c", Line: 951)
> (14 rows)
>
> The line numbers vary in the back branches, but all of these crashes are
> at that same Assert.  So (a) yes, this does happen in the back branches,
> but (b) some fairly recent change has made it a whole lot more probable.
> Neither syncrep.c nor 007_sync_rep.pl have changed much in some time,
> so whatever the change was was indirect.  Curious.  Is it just timing?

Interesting. It's happening on certain animals, not all. Especially
tests with HEAD on sidewinder and curculio, which are NetBSD 7 and
OpenBSD 5.9 respectively, started to fail at a high rate since a
couple of days ago.


Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-14T09:35:38Z

    At Tue, 14 Apr 2020 13:06:14 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in 
> On Tue, 14 Apr 2020 at 10:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > > At Sat, 11 Apr 2020 18:30:30 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> > >> What I think we should do about this is, essentially, to get rid of
> > >> SyncRepGetSyncStandbys.  Instead, let's have each walsender advertise
> > >> whether *it* believes that it is a sync standby, based on its last
> > >> evaluation of the relevant GUCs.  This would be a bool that it'd
> > >> compute and set alongside sync_standby_priority.  (Hm, maybe we'd not
> >
> > > Mmm..  SyncRepGetStandbyPriority returns the "priority" that a
> > > walsender thinks it is at, among synchronous_standby_names.  Then to
> > > decide "I am a sync standby" we need to know how many walsenders with
> > > higher priority are alive now.  SyncRepGetSyncStandbyPriority does the
> > > judgment now and suffers from the inconsistency of priority values.
> >
> > Yeah.  After looking a bit closer, I think that the current definition
> > of sync_standby_priority (that is, as the result of local evaluation
> > of SyncRepGetStandbyPriority()) is OK.  The problem is what we're doing
> > with it.  I suggest that what we should do in SyncRepGetSyncRecPtr()
> > is make one sweep across the WalSnd array, collecting PID,
> > sync_standby_priority, *and* the WAL pointers from each valid entry.
> > Then examine that data and decide which WAL value we need, without assuming
> > that the sync_standby_priority values are necessarily totally consistent.
> > But in any case we must examine each entry just once while holding its
> > mutex, not go back to it later expecting it to still be the same.

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.

I think we need to do one of the followings.

 A) prevent SyncRepGetSyncStandbysPriority from being entered while
    walsender priority is inconsistent.

 B) make SyncRepGetSyncStandbysPriority be tolerant of priority
    inconsistency.

 C) protect walsender priority array from beinig inconsistent.

The (B) is the band aids. To achieve A we need to central controller
of priority config handling.  C is:

> Can we have a similar approach of sync_standby_defined for
> sync_standby_priority? That is, checkpionter is responsible for
> changing sync_standby_priority of all walsenders when SIGHUP. That
> way, all walsenders can see a consistent view of
> sync_standby_priority. And when a walsender starts, it sets
> sync_standby_priority by itself. The logic to decide who's a sync
> standby doesn't change. SyncRepGetSyncRecPtr() gets all walsenders
> having higher priority along with their WAL positions.

Yeah, it works if we do , but the problem of that way is that to
determin priority of walsenders, we need to know what walsenders are
running. That is, when new walsender comes the process needs to aware
of the arrival (or leaving) right away and reassign the priority of
every wal senders again.

If we accept to share variable-length information among processes,
sharing sync_standby_names or parsed SyncRepConfigData among processes
would work.


> >
> > Another thing that I'm finding interesting is that I now see this is
> > not at all new code.  It doesn't look like SyncRepGetSyncStandbysPriority
> > has changed much since 2016.  So how come we didn't detect this problem
> > long ago?  I searched the buildfarm logs for assertion failures in
> > syncrep.c, looking back one year, and here's what I found:
...
> > The line numbers vary in the back branches, but all of these crashes are
> > at that same Assert.  So (a) yes, this does happen in the back branches,
> > but (b) some fairly recent change has made it a whole lot more probable.
> > Neither syncrep.c nor 007_sync_rep.pl have changed much in some time,
> > so whatever the change was was indirect.  Curious.  Is it just timing?
> 
> Interesting. It's happening on certain animals, not all. Especially
> tests with HEAD on sidewinder and curculio, which are NetBSD 7 and
> OpenBSD 5.9 respectively, started to fail at a high rate since a
> couple of days ago.

Coundn't this align the timing of config reloading? (I didn't checked
anything yet.)

| commit 421685812290406daea58b78dfab0346eb683bbb
| Author: Noah Misch <noah@leadboat.com>
| Date:   Sat Apr 11 10:30:00 2020 -0700
| 
|     When WalSndCaughtUp, sleep only in WalSndWaitForWal().
|    Before sleeping, WalSndWaitForWal() sends a keepalive if MyWalSnd->write
|    < sentPtr.  That is important in logical replication.  When the latest
|    physical LSN yields no logical replication messages (a common case),
|    that keepalive elicits a reply, and processing the reply updates
|    pg_stat_replication.replay_lsn.  WalSndLoop() lacks that; when
|    WalSndLoop() slept, replay_lsn advancement could stall until
|    wal_receiver_status_interval elapsed.  This sometimes stalled
|    src/test/subscription/t/001_rep_changes.pl for up to 10s.
|    
|    Discussion: https://postgr.es/m/20200406063649.GA3738151@rfd.leadboat.com

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  12. Re: Race condition in SyncRepGetSyncStandbysPriority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-14T13:52:42Z

    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.  While that doesn't seem to be the
exact issue we're seeing in the buildfarm, it's still another obvious
bug in this code.  I will not accept a "fix" that doesn't fix that.

> I think we need to do one of the followings.

>  A) prevent SyncRepGetSyncStandbysPriority from being entered while
>     walsender priority is inconsistent.
>  B) make SyncRepGetSyncStandbysPriority be tolerant of priority
>     inconsistency.
>  C) protect walsender priority array from beinig inconsistent.

(B) seems like the only practical solution from here.  We could
probably arrange for synchronous update of the priorities when
they change in response to a GUC change, but it doesn't seem to
me to be practical to do that in response to walsender exit.
You'd end up finding that an unexpected walsender exit results
in panic'ing the system, which is no better than where we are now.

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.

> If we accept to share variable-length information among processes,
> sharing sync_standby_names or parsed SyncRepConfigData among processes
> would work.

Not sure that we really need more than what's being shared now,
ie each process's last-known index in the sync_standby_names list.

			regards, tom lane

  13. Re: Race condition in SyncRepGetSyncStandbysPriority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-14T20:32:40Z

    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.

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.

			regards, tom lane

  14. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-15T01:21:27Z

    At Tue, 14 Apr 2020 09:52:42 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> 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.  While that doesn't seem to be the

Ah. I didn't take that as inconsistency. Actually walsender exit
inactivate the corresponding slot by setting pid = 0.  In a bad case
(as you mentioned upthread) the entry can be occupied by another wal
sender. However, sync_standby_priority cannot be updated until the
whole work is finished.

> exact issue we're seeing in the buildfarm, it's still another obvious
> bug in this code.  I will not accept a "fix" that doesn't fix that.

I think that the "inconsistency" that can be observed in a process is
disagreement between SyncRepConfig->nmembers and
<each_walsnd_entry>->sync_standby_priority.  If any one of walsenders
regards its priority as lower (larger in value) than nmembers in the
"current" process, the assertion fires.  If that is the issue, the
issue is not dynamic inconsistency.

# It's the assumption of my band-aid.

> > I think we need to do one of the followings.
> 
> >  A) prevent SyncRepGetSyncStandbysPriority from being entered while
> >     walsender priority is inconsistent.
> >  B) make SyncRepGetSyncStandbysPriority be tolerant of priority
> >     inconsistency.
> >  C) protect walsender priority array from beinig inconsistent.
> 
> (B) seems like the only practical solution from here.  We could
> probably arrange for synchronous update of the priorities when
> they change in response to a GUC change, but it doesn't seem to
> me to be practical to do that in response to walsender exit.
> You'd end up finding that an unexpected walsender exit results
> in panic'ing the system, which is no better than where we are now.

I agree to you as a whole.  I thought of several ways to keep the
consistency of the array but all of them looked too much.

> 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.

Mmm, the priority lists like 1,2,2,4 are not thought as inconsistency
at all in the context of walsender priority.  That happenes stablly if
any two or more walreceivers reported the same application_name. I
believe the existing code is already taking that case into
consideration.

> > If we accept to share variable-length information among processes,
> > sharing sync_standby_names or parsed SyncRepConfigData among processes
> > would work.
> 
> Not sure that we really need more than what's being shared now,
> ie each process's last-known index in the sync_standby_names list.

If we take the (B), we don't need anymore.  (A) and (C) would need
more.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  15. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-15T02:35:58Z

    At Tue, 14 Apr 2020 16:32:40 -0400, Tom Lane <tgl@sss.pgh.pa.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

  16. Re: Race condition in SyncRepGetSyncStandbysPriority

    Noah Misch <noah@leadboat.com> — 2020-04-15T03:14:02Z

    On Tue, Apr 14, 2020 at 04:32:40PM -0400, Tom Lane wrote:
> 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.
> 
> 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.

This passes the test battery I wrote in preparation for the 2020-02 thread.

  17. Re: Race condition in SyncRepGetSyncStandbysPriority

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-15T04:01:02Z

    On Tue, 14 Apr 2020 at 18:35, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> At Tue, 14 Apr 2020 13:06:14 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in
> > On Tue, 14 Apr 2020 at 10:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >
> > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > > > At Sat, 11 Apr 2020 18:30:30 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> > > >> What I think we should do about this is, essentially, to get rid of
> > > >> SyncRepGetSyncStandbys.  Instead, let's have each walsender advertise
> > > >> whether *it* believes that it is a sync standby, based on its last
> > > >> evaluation of the relevant GUCs.  This would be a bool that it'd
> > > >> compute and set alongside sync_standby_priority.  (Hm, maybe we'd not
> > >
> > > > Mmm..  SyncRepGetStandbyPriority returns the "priority" that a
> > > > walsender thinks it is at, among synchronous_standby_names.  Then to
> > > > decide "I am a sync standby" we need to know how many walsenders with
> > > > higher priority are alive now.  SyncRepGetSyncStandbyPriority does the
> > > > judgment now and suffers from the inconsistency of priority values.
> > >
> > > Yeah.  After looking a bit closer, I think that the current definition
> > > of sync_standby_priority (that is, as the result of local evaluation
> > > of SyncRepGetStandbyPriority()) is OK.  The problem is what we're doing
> > > with it.  I suggest that what we should do in SyncRepGetSyncRecPtr()
> > > is make one sweep across the WalSnd array, collecting PID,
> > > sync_standby_priority, *and* the WAL pointers from each valid entry.
> > > Then examine that data and decide which WAL value we need, without assuming
> > > that the sync_standby_priority values are necessarily totally consistent.
> > > But in any case we must examine each entry just once while holding its
> > > mutex, not go back to it later expecting it to still be the same.
>
> 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.
>
> I think we need to do one of the followings.
>
>  A) prevent SyncRepGetSyncStandbysPriority from being entered while
>     walsender priority is inconsistent.
>
>  B) make SyncRepGetSyncStandbysPriority be tolerant of priority
>     inconsistency.
>
>  C) protect walsender priority array from beinig inconsistent.
>
> The (B) is the band aids. To achieve A we need to central controller
> of priority config handling.  C is:
>
> > Can we have a similar approach of sync_standby_defined for
> > sync_standby_priority? That is, checkpionter is responsible for
> > changing sync_standby_priority of all walsenders when SIGHUP. That
> > way, all walsenders can see a consistent view of
> > sync_standby_priority. And when a walsender starts, it sets
> > sync_standby_priority by itself. The logic to decide who's a sync
> > standby doesn't change. SyncRepGetSyncRecPtr() gets all walsenders
> > having higher priority along with their WAL positions.
>
> Yeah, it works if we do , but the problem of that way is that to
> determin priority of walsenders, we need to know what walsenders are
> running. That is, when new walsender comes the process needs to aware
> of the arrival (or leaving) right away and reassign the priority of
> every wal senders again.

I think we don't need to reassign the priority when new walsender
comes or leaves. IIUC The priority is calculated based on only
synchronous_standby_names. Coming or leaving a walsender doesn't
affect other's priorities.

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  18. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-15T07:26:50Z

    At Wed, 15 Apr 2020 11:35:58 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> I'm looking this more closer.

It looks to be in the right direction to me.

As mentioned in the previous mail, I removed is_sync_standby from
SycnStandbyData. But just doing that breaks pg_stat_get_wal_senders.
It is an exsting issue but the logic for sync_state (values[10]) looks
odd. Fixed in the attached.

SyncRepInitConfig uses mutex instead of SyncRepLock. Since anyway the
integrity of sync_standby_priority is not guaranteed, it seems OK to
me. It seems fine to remove the assertion and requirement about
SyncRepLock from SyncRepGetSyncRecPtr for the same reason. (Actually
the lock is held, though.) 

SyncRepGetSyncStandbysPriority doesn't seem worth existing as a
function. Removed in the attached.

+	num_standbys = SyncRepGetSyncStandbys(&sync_standbys);

The list is no longer consists only of synchronous standbys.  I
changed the function name, variable name and tried to adjust related
comments.

It's not what the patch did, but I don't understand why
SyncRepGetNthLatestSyncRecPtr takes SyncRepConfig->num_sync but
SyncRepGetOldest.. accesses it directly.  Changed the function
*Oldest* in the attached.  I didn't do that but finally, the two
functions can be consolidated, just by moving the selection logic
currently in SyncRepGetSyncRecPtr into the new function.


The resulting patch is attached.

- removed is_sync_standby from SyncRepStandbyData
- Fixed the logic for values[10] in pg_stat_get_wal_senders
- Changed the signature of SyncRepGetOldestSyncRecPtr
- Adjusted some comments to the behavioral change of
  SyncRepGet(Sync)Standbys.

regards.


-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  19. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-15T07:33:30Z

    At Wed, 15 Apr 2020 13:01:02 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in 
> On Tue, 14 Apr 2020 at 18:35, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> >
> > At Tue, 14 Apr 2020 13:06:14 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in
> > > On Tue, 14 Apr 2020 at 10:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > >
> > > > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > > > > At Sat, 11 Apr 2020 18:30:30 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> > > > >> What I think we should do about this is, essentially, to get rid of
> > > > >> SyncRepGetSyncStandbys.  Instead, let's have each walsender advertise
> > > > >> whether *it* believes that it is a sync standby, based on its last
> > > > >> evaluation of the relevant GUCs.  This would be a bool that it'd
> > > > >> compute and set alongside sync_standby_priority.  (Hm, maybe we'd not
> > > >
> > > > > Mmm..  SyncRepGetStandbyPriority returns the "priority" that a
> > > > > walsender thinks it is at, among synchronous_standby_names.  Then to
> > > > > decide "I am a sync standby" we need to know how many walsenders with
> > > > > higher priority are alive now.  SyncRepGetSyncStandbyPriority does the
> > > > > judgment now and suffers from the inconsistency of priority values.
> > > >
> > > > Yeah.  After looking a bit closer, I think that the current definition
> > > > of sync_standby_priority (that is, as the result of local evaluation
> > > > of SyncRepGetStandbyPriority()) is OK.  The problem is what we're doing
> > > > with it.  I suggest that what we should do in SyncRepGetSyncRecPtr()
> > > > is make one sweep across the WalSnd array, collecting PID,
> > > > sync_standby_priority, *and* the WAL pointers from each valid entry.
> > > > Then examine that data and decide which WAL value we need, without assuming
> > > > that the sync_standby_priority values are necessarily totally consistent.
> > > > But in any case we must examine each entry just once while holding its
> > > > mutex, not go back to it later expecting it to still be the same.
> >
> > 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.
> >
> > I think we need to do one of the followings.
> >
> >  A) prevent SyncRepGetSyncStandbysPriority from being entered while
> >     walsender priority is inconsistent.
> >
> >  B) make SyncRepGetSyncStandbysPriority be tolerant of priority
> >     inconsistency.
> >
> >  C) protect walsender priority array from beinig inconsistent.
> >
> > The (B) is the band aids. To achieve A we need to central controller
> > of priority config handling.  C is:
> >
> > > Can we have a similar approach of sync_standby_defined for
> > > sync_standby_priority? That is, checkpionter is responsible for
> > > changing sync_standby_priority of all walsenders when SIGHUP. That
> > > way, all walsenders can see a consistent view of
> > > sync_standby_priority. And when a walsender starts, it sets
> > > sync_standby_priority by itself. The logic to decide who's a sync
> > > standby doesn't change. SyncRepGetSyncRecPtr() gets all walsenders
> > > having higher priority along with their WAL positions.
> >
> > Yeah, it works if we do , but the problem of that way is that to
> > determin priority of walsenders, we need to know what walsenders are
> > running. That is, when new walsender comes the process needs to aware
> > of the arrival (or leaving) right away and reassign the priority of
> > every wal senders again.
> 
> I think we don't need to reassign the priority when new walsender
> comes or leaves. IIUC The priority is calculated based on only
> synchronous_standby_names. Coming or leaving a walsender doesn't
> affect other's priorities.

Sorry, the "priority" in this area is a bit confusing.  The "priority"
defined by synchronous_standby_names is determined in isolation from
the presence of walsenders.  The "priority" in
walsnd->sync_standby_priority needs walsender presence to determine.
I thought of the latter in the discussion.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  20. Re: Race condition in SyncRepGetSyncStandbysPriority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-15T15:31:49Z

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Tue, 14 Apr 2020 16:32:40 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> +		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.

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 = (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

  21. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-16T07:22:41Z

    At Wed, 15 Apr 2020 11:31:49 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > At Tue, 14 Apr 2020 16:32:40 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> > +		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.
> 
> I don't follow that?  The existing coding of SyncRepGetSyncStandbysQuorum
> returns all the candidates in its list, so this is isomorphic to that.

The existing code actully does that. On the other hand
SyncRepGetSyncStandbysPriority returns standbys that *are known to be*
synchronous, but *Quorum returns standbys that *can be* synchronous.
What the two functions return are different from each other.  So it
should be is_sync_standby for -Priority and is_sync_candidate for
-Quorum.

> 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 = (i < SyncRepConfig->num_sync);
> >       break;
> >   }
> 
> I disagree with this, it will change the behavior in the quorum case.

Oops, you're right.  I find the whole thing there (and me) is a bit
confusing. syncrep_method affects how some values (specifically
am_sync and sync_standbys) are translated at several calling depths.
And the *am_sync informs nothing in quorum mode.

> 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.

Anyway the am_sync and is_sync_standby is utterly useless in quorum
mode.  That discussion is pretty persuasive if not, but actually the
upper layers (SyncRepReleaseWaiters and SyncRepGetSyncRecPtr) referes
to syncrep_method to differentiate the interpretation of the am_sync
flag and sync_standbys list.  So anyway the difference is actually a
part of API.

After thinking some more, I concluded that some of the variables are
wrongly named or considered, and redundant. The fucntion of am_sync is
covered by got_recptr in SyncRepReleaseWaiters, so it's enough that
SyncRepGetSyncRecPtr just reports to the caller whether the caller may
release some of the waiter processes. This simplifies the related
functions and make it (to me) clearer.

Please find the attached.


> (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.)

That seems accidentally. Sorting by priority is the disigned behavior
and documented, in contrast, entries of the same priority are ordered
in index order by accident and not documented, that means it can be
changed anytime.  I think we don't define everyting in such detail.

regards.

  22. Re: Race condition in SyncRepGetSyncStandbysPriority

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-16T07:48:28Z

    On Thu, 16 Apr 2020 at 16:22, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> At Wed, 15 Apr 2020 11:31:49 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > > At Tue, 14 Apr 2020 16:32:40 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> > > +           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.
> >
> > I don't follow that?  The existing coding of SyncRepGetSyncStandbysQuorum
> > returns all the candidates in its list, so this is isomorphic to that.
>
> The existing code actully does that. On the other hand
> SyncRepGetSyncStandbysPriority returns standbys that *are known to be*
> synchronous, but *Quorum returns standbys that *can be* synchronous.
> What the two functions return are different from each other.  So it
> should be is_sync_standby for -Priority and is_sync_candidate for
> -Quorum.
>
> > 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 = (i < SyncRepConfig->num_sync);
> > >       break;
> > >   }
> >
> > I disagree with this, it will change the behavior in the quorum case.
>
> Oops, you're right.  I find the whole thing there (and me) is a bit
> confusing. syncrep_method affects how some values (specifically
> am_sync and sync_standbys) are translated at several calling depths.
> And the *am_sync informs nothing in quorum mode.
>
> > 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.
>
> Anyway the am_sync and is_sync_standby is utterly useless in quorum
> mode.  That discussion is pretty persuasive if not, but actually the
> upper layers (SyncRepReleaseWaiters and SyncRepGetSyncRecPtr) referes
> to syncrep_method to differentiate the interpretation of the am_sync
> flag and sync_standbys list.  So anyway the difference is actually a
> part of API.
>
> After thinking some more, I concluded that some of the variables are
> wrongly named or considered, and redundant. The fucntion of am_sync is
> covered by got_recptr in SyncRepReleaseWaiters, so it's enough that
> SyncRepGetSyncRecPtr just reports to the caller whether the caller may
> release some of the waiter processes. This simplifies the related
> functions and make it (to me) clearer.
>
> Please find the attached.
>
>
> > (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.)
>
> That seems accidentally. Sorting by priority is the disigned behavior
> and documented, in contrast, entries of the same priority are ordered
> in index order by accident and not documented, that means it can be
> changed anytime.  I think we don't define everyting in such detail.
>

This is just a notice; I'm reading your latest patch but it seems to
include unrelated changes:

$ git diff --stat
 src/backend/replication/syncrep.c           | 475
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------------------------------------------------------------------------
 src/backend/replication/walsender.c         |  40 ++++++++++++++-----
 src/bin/pg_dump/compress_io.c               |  12 ++++++
 src/bin/pg_dump/pg_backup_directory.c       |  48 ++++++++++++++++++-----
 src/include/replication/syncrep.h           |  20 +++++++++-
 src/include/replication/walsender_private.h |  16 ++++----
 6 files changed, 274 insertions(+), 337 deletions(-)


Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  23. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-16T09:26:26Z

    At Thu, 16 Apr 2020 16:48:28 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in 
> This is just a notice; I'm reading your latest patch but it seems to
> include unrelated changes:
> 
> $ git diff --stat
>  src/backend/replication/syncrep.c           | 475
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------------------------------------------------------------------------
>  src/backend/replication/walsender.c         |  40 ++++++++++++++-----
>  src/bin/pg_dump/compress_io.c               |  12 ++++++
>  src/bin/pg_dump/pg_backup_directory.c       |  48 ++++++++++++++++++-----
>  src/include/replication/syncrep.h           |  20 +++++++++-
>  src/include/replication/walsender_private.h |  16 ++++----
>  6 files changed, 274 insertions(+), 337 deletions(-)

Ugg.  I failed to clean up working directory..  I didn't noticed as I
made the file by git diff. Thanks for noticing me of that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  24. Re: Race condition in SyncRepGetSyncStandbysPriority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-16T15:39:06Z

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> [ syncrep-fixes-4.patch ]

I agree that we could probably improve the clarity of this code with
further rewriting, but I'm still very opposed to the idea of having
callers know that the first num_sync array elements are the active
ones.  It's wrong (or at least different from current behavior) for
quorum mode, where there might be more than num_sync walsenders to
consider.  And it might not generalize very well to other syncrep
selection rules we might add in future, which might also not have
exactly num_sync interesting walsenders.  So I much prefer an API
definition that uses bool flags in an array that has no particular
ordering (so far as the callers know, anyway).  If you don't like
is_sync_standby, how about some more-neutral name like is_active
or is_interesting or include_position?

I dislike the proposed comment revisions in SyncRepReleaseWaiters,
too, particularly the change to say that what we're "announcing"
is the ability to release waiters.  You did not change the actual
log messages, and you would have gotten a lot of pushback if
you tried, because the current messages make sense to users and
something like that would not.  But by the same token this new
comment isn't too helpful to somebody reading the code.

(Actually, I wonder why we even have the restriction that only
sync standbys can release waiters.  It's not like they are
going to get different results from SyncRepGetSyncRecPtr than
any other walsender would.  Maybe we should just drop all the
am_sync logic?)

			regards, tom lane

  25. Re: Race condition in SyncRepGetSyncStandbysPriority

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-04-16T17:20:04Z

    
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

  26. Re: Race condition in SyncRepGetSyncStandbysPriority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-16T18:00:23Z

    Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> On 2020/04/14 22:52, Tom Lane wrote:
>> *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.

> 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).

Right, exactly, sorry that I was not more specific.

> BTW, since the patch changes the API of SyncRepGetSyncStandbys(),
> it should not be back-patched to avoid ABI break. Right?

Anything that is using that is just as broken as the core code is, for the
same reasons, so I don't have a problem with changing its API.  Maybe we
should rename it while we're at it, just to make it clear that we are
breaking any external callers.  (If there are any, which seems somewhat
unlikely.)

The only concession to ABI that I had in mind was to not re-order
the fields of WalSnd in the back branches.

			regards, tom lane

  27. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-17T05:58:34Z

    At Thu, 16 Apr 2020 11:39:06 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > [ syncrep-fixes-4.patch ]
> 
> I agree that we could probably improve the clarity of this code with
> further rewriting, but I'm still very opposed to the idea of having
> callers know that the first num_sync array elements are the active
> ones.  It's wrong (or at least different from current behavior) for
> quorum mode, where there might be more than num_sync walsenders to
> consider.  And it might not generalize very well to other syncrep
> selection rules we might add in future, which might also not have
> exactly num_sync interesting walsenders.  So I much prefer an API
> definition that uses bool flags in an array that has no particular
> ordering (so far as the callers know, anyway).  If you don't like
> is_sync_standby, how about some more-neutral name like is_active
> or is_interesting or include_position?

I'm convinced that each element has is_sync_standby.  I agree to the
name is_sync_standby since I don't come up with a better name.

> I dislike the proposed comment revisions in SyncRepReleaseWaiters,
> too, particularly the change to say that what we're "announcing"
> is the ability to release waiters.  You did not change the actual
> log messages, and you would have gotten a lot of pushback if
> you tried, because the current messages make sense to users and
> something like that would not.  But by the same token this new
> comment isn't too helpful to somebody reading the code.

The current log messages look perfect to me.  I don't insist on the
comment change since I might take the definition of "sync standby" too
strictly.

> (Actually, I wonder why we even have the restriction that only
> sync standbys can release waiters.  It's not like they are
> going to get different results from SyncRepGetSyncRecPtr than
> any other walsender would.  Maybe we should just drop all the
> am_sync logic?)

I thought the same thing, though I didn't do that in the last patch.

am_sync seems intending to reduce spurious wakeups but actually
spurious wakeup won't increase even without it.  Thus the only
remaining task of am_sync is the trigger for the log messages and that
fact is the sign that the log messages should be emitted within
SyncRepGetSyncRecPtr. That eliminates references to SyncRepConfig in
SyncRepReleaseWaiters, which make me feel ease.

The attached is baed on syncrep-fixes-1.patch + am_sync elimination.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  28. Re: Race condition in SyncRepGetSyncStandbysPriority

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-04-17T07:03:34Z

    
On 2020/04/17 14:58, Kyotaro Horiguchi wrote:
> At Thu, 16 Apr 2020 11:39:06 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
>> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
>>> [ syncrep-fixes-4.patch ]
>>
>> I agree that we could probably improve the clarity of this code with
>> further rewriting, but I'm still very opposed to the idea of having
>> callers know that the first num_sync array elements are the active
>> ones.  It's wrong (or at least different from current behavior) for
>> quorum mode, where there might be more than num_sync walsenders to
>> consider.  And it might not generalize very well to other syncrep
>> selection rules we might add in future, which might also not have
>> exactly num_sync interesting walsenders.  So I much prefer an API
>> definition that uses bool flags in an array that has no particular
>> ordering (so far as the callers know, anyway).  If you don't like
>> is_sync_standby, how about some more-neutral name like is_active
>> or is_interesting or include_position?
> 
> I'm convinced that each element has is_sync_standby.  I agree to the
> name is_sync_standby since I don't come up with a better name.
> 
>> I dislike the proposed comment revisions in SyncRepReleaseWaiters,
>> too, particularly the change to say that what we're "announcing"
>> is the ability to release waiters.  You did not change the actual
>> log messages, and you would have gotten a lot of pushback if
>> you tried, because the current messages make sense to users and
>> something like that would not.  But by the same token this new
>> comment isn't too helpful to somebody reading the code.
> 
> The current log messages look perfect to me.  I don't insist on the
> comment change since I might take the definition of "sync standby" too
> strictly.
> 
>> (Actually, I wonder why we even have the restriction that only
>> sync standbys can release waiters.  It's not like they are
>> going to get different results from SyncRepGetSyncRecPtr than
>> any other walsender would.  Maybe we should just drop all the
>> am_sync logic?)
> 
> I thought the same thing, though I didn't do that in the last patch.
> 
> am_sync seems intending to reduce spurious wakeups but actually
> spurious wakeup won't increase even without it.  Thus the only
> remaining task of am_sync is the trigger for the log messages and that
> fact is the sign that the log messages should be emitted within
> SyncRepGetSyncRecPtr. That eliminates references to SyncRepConfig in
> SyncRepReleaseWaiters, which make me feel ease.
> 
> The attached is baed on syncrep-fixes-1.patch + am_sync elimination.

I agree that it might be worth considering the removal of am_sync for
the master branch or v14. But I think that it should not be back-patched.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  29. Re: Race condition in SyncRepGetSyncStandbysPriority

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-04-17T07:31:36Z

    
On 2020/04/17 3:00, Tom Lane wrote:
> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
>> On 2020/04/14 22:52, Tom Lane wrote:
>>> *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.
> 
>> 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).
> 
> Right, exactly, sorry that I was not more specific.
> 
>> BTW, since the patch changes the API of SyncRepGetSyncStandbys(),
>> it should not be back-patched to avoid ABI break. Right?
> 
> Anything that is using that is just as broken as the core code is, for the
> same reasons, so I don't have a problem with changing its API.  Maybe we
> should rename it while we're at it, just to make it clear that we are
> breaking any external callers.  (If there are any, which seems somewhat
> unlikely.)

I agree to change the API if that's the only way to fix the bug. But ISTM that
we can fix the bug without changing the API, like the attached patch does.

Your patch changes the logic to pick up sync standbys, e.g., use qsort(),
in addition to the bug fix. This might be an improvement and I agree that
it's worth considering that idea for the master branch or v14. But I'm not
fan of adding such changes into the back branches if they are not
necessary for the bug fix. I like to basically keep the current logic as it is,
at least for the back branch, like the attached patch does.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  30. Re: Race condition in SyncRepGetSyncStandbysPriority

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-17T08:03:11Z

    On Fri, 17 Apr 2020 at 14:58, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> At Thu, 16 Apr 2020 11:39:06 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
> > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > > [ syncrep-fixes-4.patch ]
> >
> > I agree that we could probably improve the clarity of this code with
> > further rewriting, but I'm still very opposed to the idea of having
> > callers know that the first num_sync array elements are the active
> > ones.  It's wrong (or at least different from current behavior) for
> > quorum mode, where there might be more than num_sync walsenders to
> > consider.  And it might not generalize very well to other syncrep
> > selection rules we might add in future, which might also not have
> > exactly num_sync interesting walsenders.  So I much prefer an API
> > definition that uses bool flags in an array that has no particular
> > ordering (so far as the callers know, anyway).  If you don't like
> > is_sync_standby, how about some more-neutral name like is_active
> > or is_interesting or include_position?
>
> I'm convinced that each element has is_sync_standby.  I agree to the
> name is_sync_standby since I don't come up with a better name.
>
> > I dislike the proposed comment revisions in SyncRepReleaseWaiters,
> > too, particularly the change to say that what we're "announcing"
> > is the ability to release waiters.  You did not change the actual
> > log messages, and you would have gotten a lot of pushback if
> > you tried, because the current messages make sense to users and
> > something like that would not.  But by the same token this new
> > comment isn't too helpful to somebody reading the code.
>
> The current log messages look perfect to me.  I don't insist on the
> comment change since I might take the definition of "sync standby" too
> strictly.
>
> > (Actually, I wonder why we even have the restriction that only
> > sync standbys can release waiters.  It's not like they are
> > going to get different results from SyncRepGetSyncRecPtr than
> > any other walsender would.  Maybe we should just drop all the
> > am_sync logic?)
>
> I thought the same thing, though I didn't do that in the last patch.
>
> am_sync seems intending to reduce spurious wakeups but actually
> spurious wakeup won't increase even without it.  Thus the only
> remaining task of am_sync is the trigger for the log messages and that
> fact is the sign that the log messages should be emitted within
> SyncRepGetSyncRecPtr. That eliminates references to SyncRepConfig in
> SyncRepReleaseWaiters, which make me feel ease.
>
> The attached is baed on syncrep-fixes-1.patch + am_sync elimination.
>

Just for confirmation, since the new approach doesn't change that
walsenders reload new config at their convenient timing, it still can
happen that a walsender releases waiters according to the old config
that defines fewer number of sync standbys, during walsenders
absorbing a change in the set of synchronous walsenders. In the worst
case where the master crashes in the middle, we cannot be sure how
many sync servers the data has been replicated to. Is that right?

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  31. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-17T08:07:06Z

    At Fri, 17 Apr 2020 16:03:34 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> I agree that it might be worth considering the removal of am_sync for
> the master branch or v14. But I think that it should not be
> back-patched.

Ah! Agreed.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  32. Re: Race condition in SyncRepGetSyncStandbysPriority

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-04-17T08:41:24Z

    At Fri, 17 Apr 2020 17:03:11 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in 
> On Fri, 17 Apr 2020 at 14:58, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> > The attached is baed on syncrep-fixes-1.patch + am_sync elimination.
> >
> 
> Just for confirmation, since the new approach doesn't change that
> walsenders reload new config at their convenient timing, it still can
> happen that a walsender releases waiters according to the old config
> that defines fewer number of sync standbys, during walsenders

Right.

> absorbing a change in the set of synchronous walsenders. In the worst
> case where the master crashes in the middle, we cannot be sure how
> many sync servers the data has been replicated to. Is that right?

Wal senders can set a stupid value as priority or in a worse case the
shared walsender information might be of another walsender that is
launched just now. In any case SyncRepGetSyncStandbys can return a set
of walsenders with descending priority (in priority mode).  What can
be happen in the worst case is some transactions are released by a bit
wrong LSN information.  Such inconsistency also can be happen when the
oldest sync standby in priority mode goes out and sync-LSN goes back
even if the wal-sender list is strictly kept consistent.

In quorum mode, we cannot even know which servers that endorsed the
master's commit after a crash.

I don't come up of clean solution for such inconsistency or
unrecoverability(?) for now..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  33. Re: Race condition in SyncRepGetSyncStandbysPriority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-17T15:31:50Z

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Fri, 17 Apr 2020 16:03:34 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
>> I agree that it might be worth considering the removal of am_sync for
>> the master branch or v14. But I think that it should not be
>> back-patched.

> Ah! Agreed.

Yeah, that's not necessary to fix the bug.  I'd be inclined to leave
it for v14 at this point.

I don't much like the patch Fujii-san posted, though.  An important part
of the problem, IMO, is that SyncRepGetSyncStandbysPriority is too
complicated and it's unclear what dependencies it has on the set of
priorities in shared memory being consistent.  His patch does not improve
that situation; if anything it makes it worse.

If we're concerned about not breaking ABI in the back branches, what
I propose we do about that is just leave SyncRepGetSyncStandbys in
place but not used by the core code, and remove it only in HEAD.
We can do an absolutely minimal fix for the assertion failure, in
case anybody is calling that code, by just dropping the Assert and
letting SyncRepGetSyncStandbys return NIL if it falls out.  (Or we
could let it return the incomplete list, which'd be the behavior
you get today in a non-assert build.)

Also, I realized while re-reading my patch that Kyotaro-san is onto
something about the is_sync_standby flag not being necessary: instead
we can just have the new function SyncRepGetCandidateStandbys return
a reduced count.  I'd initially believed that it was necessary for
that function to return the rejected candidate walsenders along with
the accepted ones, but that was a misunderstanding.  I still don't
want its API spec to say anything about ordering of the result array,
but we don't need to.

So that leads me to the attached.  I propose applying this to the
back branches except for the rearrangement of WALSnd field order.
In HEAD, I'd remove SyncRepGetSyncStandbys and subroutines altogether.

			regards, tom lane

  34. Re: Race condition in SyncRepGetSyncStandbysPriority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-17T16:30:23Z

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Fri, 17 Apr 2020 17:03:11 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in 
>> On Fri, 17 Apr 2020 at 14:58, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>> Just for confirmation, since the new approach doesn't change that
>> walsenders reload new config at their convenient timing, it still can
>> happen that a walsender releases waiters according to the old config
>> that defines fewer number of sync standbys, during walsenders

> Right.

>> absorbing a change in the set of synchronous walsenders. In the worst
>> case where the master crashes in the middle, we cannot be sure how
>> many sync servers the data has been replicated to. Is that right?

> Wal senders can set a stupid value as priority or in a worse case the
> shared walsender information might be of another walsender that is
> launched just now. In any case SyncRepGetSyncStandbys can return a set
> of walsenders with descending priority (in priority mode).  What can
> be happen in the worst case is some transactions are released by a bit
> wrong LSN information.  Such inconsistency also can be happen when the
> oldest sync standby in priority mode goes out and sync-LSN goes back
> even if the wal-sender list is strictly kept consistent.

I don't really see a problem here.  It's true that transactions might
be released based on either the old or the new value of num_sync,
depending on whether the particular walsender executing the release
logic has noticed the SIGHUP yet.  But if a transaction was released,
then there were at least num_sync confirmed transmissions of data
to someplace, so it's not like you've got no redundancy at all.

The only thing that seems slightly odd is that there could in principle
be some transactions released on the basis of the new num_sync, and
then slightly later some transactions released on the basis of the old
num_sync.  But I don't think it's really going to be possible to avoid
that, given that the GUC update is propagated in an asynchronous
fashion.

I spent a few moments wondering if we could avoid such cases by having
SyncRepReleaseWaiters check for GUC updates after it's acquired
SyncRepLock.  But that wouldn't really guarantee much, since the
postmaster can't deliver SIGHUP to all the walsenders simultaneously.
I think the main practical effect would be to allow some possibly-slow
processing to happen while holding SyncRepLock, which surely isn't a
great idea.

BTW, it might be worth documenting in this thread that my proposed
patch intentionally doesn't move SyncRepReleaseWaiters' acquisition
of SyncRepLock.  With the patch, SyncRepGetSyncRecPtr does not require
SyncRepLock so one could consider acquiring that lock only while updating 
walsndctl and releasing waiters.  My concern about that is that then
it'd be possible for a later round of waiter-releasing to happen on the
basis of slightly older SyncRepGetSyncRecPtr results, if a walsender that
had done SyncRepGetSyncRecPtr first were only able to acquire the lock
second.  Perhaps that would be okay, but I'm not sure, so I left it
alone.

			regards, tom lane

  35. Re: Race condition in SyncRepGetSyncStandbysPriority

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-18T03:38:54Z

    On Sat, 18 Apr 2020 at 00:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > At Fri, 17 Apr 2020 16:03:34 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
> >> I agree that it might be worth considering the removal of am_sync for
> >> the master branch or v14. But I think that it should not be
> >> back-patched.
>
> > Ah! Agreed.
>
> Yeah, that's not necessary to fix the bug.  I'd be inclined to leave
> it for v14 at this point.
>
> I don't much like the patch Fujii-san posted, though.  An important part
> of the problem, IMO, is that SyncRepGetSyncStandbysPriority is too
> complicated and it's unclear what dependencies it has on the set of
> priorities in shared memory being consistent.  His patch does not improve
> that situation; if anything it makes it worse.
>
> If we're concerned about not breaking ABI in the back branches, what
> I propose we do about that is just leave SyncRepGetSyncStandbys in
> place but not used by the core code, and remove it only in HEAD.
> We can do an absolutely minimal fix for the assertion failure, in
> case anybody is calling that code, by just dropping the Assert and
> letting SyncRepGetSyncStandbys return NIL if it falls out.  (Or we
> could let it return the incomplete list, which'd be the behavior
> you get today in a non-assert build.)

+1

>
> Also, I realized while re-reading my patch that Kyotaro-san is onto
> something about the is_sync_standby flag not being necessary: instead
> we can just have the new function SyncRepGetCandidateStandbys return
> a reduced count.  I'd initially believed that it was necessary for
> that function to return the rejected candidate walsenders along with
> the accepted ones, but that was a misunderstanding.  I still don't
> want its API spec to say anything about ordering of the result array,
> but we don't need to.
>
> So that leads me to the attached.  I propose applying this to the
> back branches except for the rearrangement of WALSnd field order.
> In HEAD, I'd remove SyncRepGetSyncStandbys and subroutines altogether.
>

+   /* Quick out if not even configured to be synchronous */
+   if (SyncRepConfig == NULL)
+       return false;

I felt strange a bit that we do the above check in
SyncRepGetSyncRecPtr() because SyncRepReleaseWaiters() which is the
only caller says the following before calling it:

    /*
     * We're a potential sync standby. Release waiters if there are enough
     * sync standbys and we are considered as sync.
     */
    LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);

Can we either change it to an assertion, move it to before acquiring
SyncRepLock in SyncRepReleaseWaiters or just remove it?

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  36. Re: Race condition in SyncRepGetSyncStandbysPriority

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-18T16:00:19Z

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> writes:
> On Sat, 18 Apr 2020 at 00:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> +   /* Quick out if not even configured to be synchronous */
>> +   if (SyncRepConfig == NULL)
>> +       return false;

> I felt strange a bit that we do the above check in
> SyncRepGetSyncRecPtr() because SyncRepReleaseWaiters() which is the
> only caller says the following before calling it:

Notice there was such a test in SyncRepGetSyncRecPtr already --- I just
moved it to be before doing some work instead of after.

> Can we either change it to an assertion, move it to before acquiring
> SyncRepLock in SyncRepReleaseWaiters or just remove it?

I have no objection to that in principle, but it seems like it's a
change in SyncRepGetSyncRecPtr's API that is not necessary to fix
this bug.  So I'd rather leave it to happen along with the larger
API changes (getting rid of am_sync) that are proposed for v14.

			regards, tom lane

  37. Re: Race condition in SyncRepGetSyncStandbysPriority

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-04-20T05:35:23Z

    On Sun, 19 Apr 2020 at 01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Masahiko Sawada <masahiko.sawada@2ndquadrant.com> writes:
> > On Sat, 18 Apr 2020 at 00:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> +   /* Quick out if not even configured to be synchronous */
> >> +   if (SyncRepConfig == NULL)
> >> +       return false;
>
> > I felt strange a bit that we do the above check in
> > SyncRepGetSyncRecPtr() because SyncRepReleaseWaiters() which is the
> > only caller says the following before calling it:
>
> Notice there was such a test in SyncRepGetSyncRecPtr already --- I just
> moved it to be before doing some work instead of after.
>
> > Can we either change it to an assertion, move it to before acquiring
> > SyncRepLock in SyncRepReleaseWaiters or just remove it?
>
> I have no objection to that in principle, but it seems like it's a
> change in SyncRepGetSyncRecPtr's API that is not necessary to fix
> this bug.  So I'd rather leave it to happen along with the larger
> API changes (getting rid of am_sync) that are proposed for v14.

Agreed.

Regards,

-- 
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  38. Re: Race condition in SyncRepGetSyncStandbysPriority

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-04-20T06:05:46Z

    
On 2020/04/18 0:31, Tom Lane wrote:
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
>> At Fri, 17 Apr 2020 16:03:34 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>>> I agree that it might be worth considering the removal of am_sync for
>>> the master branch or v14. But I think that it should not be
>>> back-patched.
> 
>> Ah! Agreed.
> 
> Yeah, that's not necessary to fix the bug.  I'd be inclined to leave
> it for v14 at this point.
> 
> I don't much like the patch Fujii-san posted, though.  An important part
> of the problem, IMO, is that SyncRepGetSyncStandbysPriority is too
> complicated and it's unclear what dependencies it has on the set of
> priorities in shared memory being consistent.  His patch does not improve
> that situation; if anything it makes it worse.

Understood.

> 
> If we're concerned about not breaking ABI in the back branches, what
> I propose we do about that is just leave SyncRepGetSyncStandbys in
> place but not used by the core code, and remove it only in HEAD.
> We can do an absolutely minimal fix for the assertion failure, in
> case anybody is calling that code, by just dropping the Assert and
> letting SyncRepGetSyncStandbys return NIL if it falls out.  (Or we
> could let it return the incomplete list, which'd be the behavior
> you get today in a non-assert build.)
> 
> Also, I realized while re-reading my patch that Kyotaro-san is onto
> something about the is_sync_standby flag not being necessary: instead
> we can just have the new function SyncRepGetCandidateStandbys return
> a reduced count.  I'd initially believed that it was necessary for
> that function to return the rejected candidate walsenders along with
> the accepted ones, but that was a misunderstanding.  I still don't
> want its API spec to say anything about ordering of the result array,
> but we don't need to.
> 
> So that leads me to the attached.  I propose applying this to the
> back branches except for the rearrangement of WALSnd field order.
> In HEAD, I'd remove SyncRepGetSyncStandbys and subroutines altogether.

Thanks for making and committing the patch!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION