On Fri, May 25, 2018 at 02:12:32PM +0200, Magnus Hagander wrote:
> I agree that returning 0/0 on this is wrong.
>
> However, can this actually occour for any case other than exactly the case
> of "moving the position to where the position already is"? If I look at the
> physical slot path at least that seems to be the only case, and in that
> case I think the correct thing to return would be the new position, and not
> NULL. If we actually *fail* to move the position, we give an error.
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?
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, isn't there also a race there? That is, if we try to move it, we
> check that we're not trying to move it backwards, and throw an error, but
> that's checked outside the lock. Then later we actually move it, and check
> *again* if we try to move it backwards, but if that one fails we return
> InvalidXLogRecPtr (which can happen in the case of concurrent activity on
> the slot, I think)? In this case, maybe we should just re-check that and
> raise an error appropriately?
Er, isn't the take here that ReplicationSlotAcquire is used to lock any
replication slot update to happen from other backends? It seems to me
that what counts at the end if if a backend PID is associated to a slot
in memory. If you look at the code paths updating a logical or physical
slot then those imply that the slot is owned, still a spin lock needs to
be taken for concurrent readers.
> (I haven't looked at the logical slot path, but I assume it would have
> something similar in it)
Found one. All the things I have spotted are in the patch attached.
--
Michael
