On 2016-04-06 14:30:21 +0100, Simon Riggs wrote:
> On 6 April 2016 at 14:15, Craig Ringer <cr...@2ndquadrant.com> wrote:
> Nice summary
> Failover slots are optional. And they work on master.
> While the other approach could also work, it will work later and still
> require a slot on the master.
> => I don't see why having Failover Slots in 9.6 would prevent us from
> having something else later, if someone else writes it.
> We don't need to add this to core. Each plugin can independently write is
> own failover code. Works, but doesn't seem like the right approach for open
> => I think we should add Failover Slots to 9.6.
Simon, please don't take this personal; because of the other ongoing
I don't think this is commit-ready. For one I think this is
architecturally the wrong choice. But even leaving that fact aside, and
considering this a temporary solution (we can't easily remove), there
appears to have been very little code level review (one early from Petr
in , two by Oleksii mostly focusing on error messages  ). The
whole patch was submitted late to the 9.6 cycle.
Quickly skimming 0001 in  there appear to be a number of issues:
* LWLockHeldByMe() is only for debugging, not functional differences
* ReplicationSlotPersistentData is now in an xlog related header
* The code and behaviour around name conflicts of slots seems pretty
raw, and not discussed
* Taking spinlocks dependant on InRecovery() seems like a seriously bad
* I doubt that the archive based switches around StartupReplicationSlots
do what they intend. Afaics that'll not work correctly for basebackups
taken with -X, without recovery.conf
That's from a ~5 minute skim, of one patch in the series.
Sent via pgsql-hackers mailing list (email@example.com)
To make changes to your subscription: