Re: Copy function for logical replication slots

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.

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.

+ 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.

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.

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. 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.
--
Michael