Re: WIP: Failover Slots

On 2 January 2016 at 08:50, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> Patch is WIP, posted for comment, so you can see where I'm going.
>

I've applied this on a branch of master and posted it, with some comment
editorialization, as
https://github.com/2ndQuadrant/postgres/tree/dev/failover-slots . The tree
will be subject to rebasing.

At present the patch does not appear to work. No slots are visible in the
replica's pg_replication_slots before or after promotion and no slot
information is written to the xlog according to pg_xlogdump:

$ ~/pg/96/bin/pg_xlogdump -r ReplicationSlot 000000010000000000000001
000000010000000000000003
pg_xlogdump: FATAL: error in WAL record at 0/301DDE0: invalid record
length at 0/301DE10

so it's very much a WIP. I've read through it and think the idea makes
sense, it's just still missing some pieces...

So. Initial review comments.

This looks pretty incomplete:

+ if (found_duplicate)
+ {
+ LWLockRelease(ReplicationSlotAllocationLock);
+
+ /*
+ * Do something nasty to the sinful duplicants, but
+ * take with locking.
+ */
+
+ LWLockAcquire(ReplicationSlotAllocationLock, LW_SHARED);
+ }

... and I'm not sure I understand how the possibility of a duplicate slot
can arise in the first place, since you cannot create a slot on a read
replica. This seems unnecessary.

I'm not sure I understand why, in ReplicationSlotRedoCreate, it's
especially desirable to prevent blocking iteration over
pg_replication_slots or acquiring a slot. When redoing a slot create isn't
that exactly what we should do? This looks like it's been copied & pasted
verbatim from CheckPointReplicationSlots . There it makes sense, since the
slots may be in active use. During redo it seems reasonable to just
acquire ReplicationSlotControlLock.

I'm not a fan of the ReplicationSlotInWAL typedef
for ReplicationSlotPersistentData. Especially as it's only used in the redo
functions but *not* when xlog is written out. I'd like to just replace it.

Purely for convenient testing there's a shell script in the tree -
https://github.com/2ndQuadrant/postgres/blob/dev/failover-slots/failover-slot-test.sh
.
Assuming a patched 9.6 in $HOME/pg/96 it'll do a run-through of the patch.
I'll attempt to convert this to use the new test infrastructure, but needed
something ASAP for development. Posted in case it's useful to others.

Now it's time to look into why this doesn't seem to be generating any xlog
when by rights it seems like it should. Also into at what point exactly we
purge existing slots on start / promotion of a read-replica.

TL;DR: this doesn't work yet, working on it.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services