Re: Copy function for logical replication slots

On Wed, Feb 20, 2019 at 12:26 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Tue, Feb 19, 2019 at 05:09:33PM +0900, Masahiko Sawada wrote:
> > On Tue, Feb 19, 2019 at 1:28 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> >> Well, I'd not thought we'd do it without acquiring the other slot. But
> >> that still seems to be easy enough to address, we just need to recheck
> >> whether the slot still exists (with the right name) the second time we
> >> acquire the spinlock?
> >
> > Yeah, I think that would work. The attached patch takes this
> > direction. Please review it.
>
> + if (XLogRecPtrIsInvalid(copy_restart_lsn) ||
> + copy_restart_lsn < src_restart_lsn ||
> + src_islogical != copy_islogical ||
> + strcmp(copy_name, NameStr(*src_name)) != 0)
> + ereport(ERROR,
> + (errmsg("could not copy logical replication slot \"%s\"",
> + NameStr(*src_name)),
> + errdetail("The source replication slot has been dropped during copy")));
> +
> + /* Install copied values again */
> + SpinLockAcquire(&MyReplicationSlot->mutex);
>
> Worth worrying about this window not reduced to zero? If the slot is
> dropped between both then the same issue would arise.

You meant that the destination slot (i.e. MyReplicationSlot) could be
dropped before acquiring its lock? Since we're holding the new slot it
cannot be dropped.

BTW, XLogRecPtrIsInvalid(copy_restart_lsn) || copy_restart_lsn <
src_restart_lsn is redundant, the former should be removed.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center