Re: pg_replication_slot_advance to return NULL instead of 0/0 if slot not advanced

On Thu, May 31, 2018 at 06:31:24PM +0100, Simon Riggs wrote:

Thanks for the input, Simon. I have been able to spend more time
monitoring the slot-related code.

> On 28 May 2018 at 09:57, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> Yes, this only returns InvalidXLogRecPtr if the location could not be
>> moved forward. Still, there is more going on here. For a physical
>> slot, confirmed_lsn is always 0/0, hence the backward check is never
>> applied for it. What I think should be used for value assigned to
>> startlsn is restart_lsn instead. Then what do we do if the position
>> cannot be moved: should we raise an error, as what my patch attached
>> does, or just report the existing position the slot is at?
>
> I don't see why an ERROR would be appropriate.

Okay, the caller can always compare if the returned position matches
what happened in the past or not, so that's fine for me. About that,
the LSN used as the initialization should then be startlsn instead of
InvalidXLogRecPtr.

>> A second error that I can see is in pg_logical_replication_slot_advance,
>> which does not take the mutex lock of MyReplicationSlot, so concurrent
>> callers like those of ReplicationSlotsComputeRequiredLSN (applies to
>> physical slot actually) and pg_get_replication_slots() may read false
>> data.
>>
>> On top of that, it seems to me that StartLogicalReplication is reading a
>> couple of fields from a slot without taking a lock, so at least
>> pg_get_replication_slots() may read incorrect data.
>> ReplicationSlotReserveWal also is missing a lock.. Those are older than
>> v11 though.

Actually, there are two extra problems:
- In CreateInitDecodingContext which can be called after the slot is
marked as in use so there could be inconsistencies with
pg_get_replication_slots() as well for catalog_xmin & co.
- In DecodingContextFindStartpoint where confirmed_lsn is updated
without the mutex taken.

> I think the problem here is there are no comments explaining how to
> access the various fields in the structure, so there was no way to
> check whether the code was good or not.
>
> If we add corrective code we should clarify that in comments the .h
> file also, as is done in XlogCtl

Yes, I agree with you. There are at least two LWLocks used for the
overall slot creation and handling, as well as a set of spin locks used
for the fields. The code also does not mention in its comments that
slots marked with in_use = false are not scanned at all by concurrent
backends, but this is strongly implied in the code, hence you don't need
to care about taking lock for them when working on fields for this slot
as long as its flag in_use has not been marked to true. There is one
code path which bypasses slots with in_use marked to true, but that's
for the startup process recovering the slot data, so there is no need to
care about locking in this case.

> Your points look correct to me, well spotted. I'd like to separate the
> correction of these issues from the change of behavior patch. Those
> earlier issues can be backpatched, but the change of behavior only
> affects PG11.

Definitely, while the previous patch was around mainly to show where
things are incorrect, I am attaching a set of patches for HEAD which can
be used for commit:
- One patch which addresses the several lock problems and adds some
instructions about the variables protected by spinlocks for slots in
use, which should be back-patched.
- A second patch for HEAD which addresses what has been noticed for the
new slot advance feature. This addresses as well the lock problems
introduced in the new advance code, hopefully the split makes sense to
others on this thread.
Once again those only apply to HEAD, please feel free to ping me if you
would like versions for back-branches (or anybody picking up those
patches).

Thanks,
--
Michael