Thread: ReplicationSlotsComputeRequiredXmin seems pretty questionable
While trying to make sense of Adam Sjøgren's problem [1], I found
myself staring at ReplicationSlotsComputeRequiredXmin() in slot.c.
It seems to me that that is very shaky code, on two different
grounds:
1. Sometimes it's called with ProcArrayLock already held exclusively.
This means that any delay in acquiring the ReplicationSlotControlLock
translates directly into a hold on ProcArrayLock; in other words,
every acquisition of the ReplicationSlotControlLock is just as bad
for concurrency as an acquisition of ProcArrayLock. While I didn't
see any places that were doing really obviously slow things while
holding ReplicationSlotControlLock, this is disturbing. Do we really
need it to be like that?
2. On the other hand, the code is *releasing* the
ReplicationSlotControlLock before it calls
ProcArraySetReplicationSlotXmin, and that seems like a flat out
concurrency bug. How can we be sure that the values we're storing
into the shared xmin fields aren't stale by the time we acquire
the ProcArrayLock (in the case where we didn't hold it already)?
I'm concerned that in the worst case this code could make the
shared xmin fields go backwards.
Both of these issues could be solved, I think, if we got rid of
the provision for calling with ProcArrayLock already held and
moved the ProcArraySetReplicationSlotXmin call inside the hold
of ReplicationSlotControlLock. But maybe I'm missing something
about why that would be worse.
regards, tom lane
[1] https://www.postgresql.org/message-id/flat/87364kdsim.fsf%40tullinup.koldfront.dk
On Thu, Aug 20, 2020 at 10:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > While trying to make sense of Adam Sjøgren's problem [1], I found > myself staring at ReplicationSlotsComputeRequiredXmin() in slot.c. > It seems to me that that is very shaky code, on two different > grounds: > > 1. Sometimes it's called with ProcArrayLock already held exclusively. > This means that any delay in acquiring the ReplicationSlotControlLock > translates directly into a hold on ProcArrayLock; in other words, > every acquisition of the ReplicationSlotControlLock is just as bad > for concurrency as an acquisition of ProcArrayLock. While I didn't > see any places that were doing really obviously slow things while > holding ReplicationSlotControlLock, this is disturbing. Do we really > need it to be like that? > > 2. On the other hand, the code is *releasing* the > ReplicationSlotControlLock before it calls > ProcArraySetReplicationSlotXmin, and that seems like a flat out > concurrency bug. How can we be sure that the values we're storing > into the shared xmin fields aren't stale by the time we acquire > the ProcArrayLock (in the case where we didn't hold it already)? > I'm concerned that in the worst case this code could make the > shared xmin fields go backwards. > It is not clear to me how those values can go backward. Basically, we install those values in slots after getting it from GetOldestSafeDecodingTransactionId() and then those always seem to get advanced. And GetOldestSafeDecodingTransactionId() takes into account the already stored shared values of replication_slot_xmin and replication_slot_catalog_xmin for computing the xmin_horizon for slot. -- With Regards, Amit Kapila.
Amit Kapila <amit.kapila16@gmail.com> writes:
> On Thu, Aug 20, 2020 at 10:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 2. On the other hand, the code is *releasing* the
>> ReplicationSlotControlLock before it calls
>> ProcArraySetReplicationSlotXmin, and that seems like a flat out
>> concurrency bug.
> It is not clear to me how those values can go backward.
After releasing ReplicationSlotControlLock, that code is holding no
lock at all (in the already_locked=false case I'm concerned about).
Thus the scenario to consider is:
1. Walsender A runs ReplicationSlotsComputeRequiredXmin, computes
some perfectly-valid xmins, releases ReplicationSlotControlLock,
amd then gets swapped out to Timbuktu.
2. Time passes and the "true values" of those xmins advance thanks
to other walsender activity.
3. Walsender B runs ReplicationSlotsComputeRequiredXmin, computes
some perfectly-valid xmins, and successfully stores them in the
procarray.
4. Walsender A returns from never-never land, and stores its now
quite stale results in the procarray, causing the globally visible
xmins to go backwards from where they were after step 3.
I see no mechanism in the code that prevents this scenario.
On reflection I'm not even very sure that the code change
I'm suggesting would prevent it. It'd prevent walsenders
from entering or exiting until we've updated the procarray,
but there's nothing to stop the furthest-back walsender from
advancing its values.
There may be some argument why this can't lead to a problem,
but I don't see any comments making such an argument, either.
regards, tom lane
On Sat, Aug 22, 2020 at 8:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Kapila <amit.kapila16@gmail.com> writes: > > On Thu, Aug 20, 2020 at 10:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> 2. On the other hand, the code is *releasing* the > >> ReplicationSlotControlLock before it calls > >> ProcArraySetReplicationSlotXmin, and that seems like a flat out > >> concurrency bug. > > > It is not clear to me how those values can go backward. > > After releasing ReplicationSlotControlLock, that code is holding no > lock at all (in the already_locked=false case I'm concerned about). > Thus the scenario to consider is: > > 1. Walsender A runs ReplicationSlotsComputeRequiredXmin, computes > some perfectly-valid xmins, releases ReplicationSlotControlLock, > amd then gets swapped out to Timbuktu. > > 2. Time passes and the "true values" of those xmins advance thanks > to other walsender activity. > > 3. Walsender B runs ReplicationSlotsComputeRequiredXmin, computes > some perfectly-valid xmins, and successfully stores them in the > procarray. > > 4. Walsender A returns from never-never land, and stores its now > quite stale results in the procarray, causing the globally visible > xmins to go backwards from where they were after step 3. > > I see no mechanism in the code that prevents this scenario. > On reflection I'm not even very sure that the code change > I'm suggesting would prevent it. It'd prevent walsenders > from entering or exiting until we've updated the procarray, > but there's nothing to stop the furthest-back walsender from > advancing its values. > I think we can prevent that if we allow ProcArraySetReplicationSlotXmin to update the shared values only when new xmin values follows the shared values. I am not very sure if it is safe but I am not able to think of a problem with it. The other way could be to always acquire ProcArrayLock before calling ReplicationSlotsComputeRequiredXmin or before acquiring ReplicationSlotControlLock but that seems too restrictive. -- With Regards, Amit Kapila.