Re: Copy function for logical replication slots

On Tue, Jul 3, 2018 at 1:01 PM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Mon, Jul 02, 2018 at 04:31:32PM +0900, Masahiko Sawada wrote:
>> Attached an updated patch including copy function support for logical
>> slots as well as physical slots. Please review it.
>
> I had a look at this patch.

Thank you for the reviews.

>
> As the output plugin can be changed for logical slots, having two
> functions is a good thing.
>
> +# Basic decoding works using the copied slot
> +$result = $node_master->safe_psql('postgres',
> + qq[SELECT pg_logical_slot_get_changes('copy_slot', NULL, NULL);]);
> +is(scalar(@foobar = split /^/m, $result),
> + 12, 'Decoding produced 12 rows inc BEGIN/COMMIT using the copied slot');
>
> This could live as well as part of the test suite for test_decoding, and
> that would be faster as well. There are more scenarios that could be
> tested as well:
> - Copy a temporary slot to become permanent and switch its plugin type,
> then check the state of the slot in pg_replication_slots.
> - Do the reverse, aka copy a permanent slot and make it temporary.
> - Checking that the default behaviors are preserved: if persistent is
> not changed then the new slot should have the same persistence as the
> origin. The same applies for the output plugin, and for both logical
> and replication slots.
> - Check that the reversed LSN is the same for both the origin and the
> target.

Will fix.

>
> + Copy a existing <parameter>src_slot_name</parameter> physical slot
> + to <parameter>dst_slot_name</parameter>. The copied physical slot starts
> + to reserve WAL from the same LSN as the source slot if the source slot
> + already reserves WAL. <parameter>temporary</parameter> is optional. If
> + <parameter>temporary</parameter> is omitted, the same value of the
> + source slot is used.
>
> Typo here. Copy AN existing slot. I would reword that a bit:
> Copies an existing physical replication slot name src_slot_name to a
> physical replication slot named dst_slot_name.
> Extra one: "the same value AS the source slot is used."
>
> + Copy a existing <parameter>src_slot_name</parameter> logical (decoding) slot
> + to <parameter>dst_slot_name</parameter> while changing the output plugin
> + and persistence.
>
> There may be no need for "decoding" here, the same phrasing as suggested
> above would be nicer I think. For LSN I would suggest to add an
> <acronym> markup.

Will fix.

>
> I am not sure if it makes much sense to be able to copy from a slot
> which has not yet been used to reserve WAL, but to change the properties
> of a slot I could get that forbidding the behavior is not really
> intuitive either.
>
> - ReplicationSlotReserveWal();
> + /* Find start location to read WAL if not specified */
> + if (XLogRecPtrIsInvalid(start_lsn))
> + ReplicationSlotReserveWal();
> + else
> + {
> + SpinLockAcquire(&slot->mutex);
> + slot->data.restart_lsn = start_lsn;
> + SpinLockRelease(&slot->mutex);
> + }
> Hmm. Instead of all this stanza in CreateInitDecodingContext(), I would
> have imagined that what should happen is that the new fresh slot gets
> created with the same status data as the origin. This saves quite a bit
> of extra post-creation computing, and we are also sure that the origin
> slot has consistent data as it is owned by the process doing the copy.

Hmm, such post-creation computing is not necessary for the copied
slot? I'm concerned we might miss some operations required for for
logical replication slot.

>
> One property which seems important to me is to make sure that the target
> slot has the same data as the origin slot once the caller knows that the
> copy has completed, and not that the target slot may perhaps have the
> same data as the origin while creating the target. Do you see the
> difference? Your patch does the latter, because it creates the new slot
> after releasing the lock it held on the origin, while I would think that
> the former is more important, aka keep the lock for the duration of the
> copy.

Did you mean "the caller" is clients who call
pg_copy_logical_replication_slot function? If so, I think it's very
difficult to ensure the former because the origin slot can advance
during returning the result to the client. Also if we keep the lock on
the origin slot during the copy I think that one problem is that we
will own two replication slots at a time. But there are a lot of code
that premise a process holds at most one replication slot.

>I am not completely sure if that's a property we want to keep,
> but that deserves discussion as we should not do a copy while the origin
> slot may still be consumed in parallel. For physical slot the copy is
> straight-forward, less for logical slots.

I think this copy functions ensure that the target slot has the same
values as the origin slot at the time when the function is called.
Isn't it clear?

Regards,

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