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
> source.
> => 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 [1], two by Oleksii mostly focusing on error messages [2] [3]). The
whole patch was submitted late to the 9.6 cycle.

Quickly skimming 0001 in [4] 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.



Andres Freund

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to