Re: WIP: Failover Slots - Mailing list pgsql-hackers
| From | Simon Riggs |
|---|---|
| Subject | Re: WIP: Failover Slots |
| Date | |
| Msg-id | CANP8+jJ+YFHO3Ow3HOhH8SAumGDhaRtt=bDtAvchcbdWySv+rw@mail.gmail.com Whole thread Raw |
| In response to | Re: WIP: Failover Slots (Andres Freund <andres@anarazel.de>) |
| List | pgsql-hackers |
On 6 April 2016 at 15:17, Andres Freund <andres@anarazel.de> wrote:
--
On 2016-04-06 14:30:21 +0100, Simon Riggs wrote:
> On 6 April 2016 at 14:15, Craig Ringer <craig@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
thread.
Thanks for the review. Rational technical comments are exactly why we are here and they are always welcome.
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),
As I observed above, the alternate solution doesn't sound particularly good either but the main point is that we wouldn't need to remove it, it can coexist happily. I would add that I did think of the alternate solution previously as well, this one seemed simpler, which is always key for me in code aimed at robustness.
there appears to have been very little code level review
That is potentially fixable. At this point I don't claim it is committable, I only say it is important and the alternate solution is not significantly better, therefore if the patch can be beaten into shape we should commit it.
I will spend some time on this and see if we have something viable. Which will be posted here for discussion, as would have happened even before our other discussions.
Thanks for the points below
(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
idea
* 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.
[1] http://archives.postgresql.org/message-id/CALLjQTSCHvcsF6y7%3DZhmdMjJUMGLqt1-6Pz2rtb7PfFLxFfBOw%40mail.gmail.com
[2] http://archives.postgresql.org/message-id/FA68178E-F0D1-47F6-9791-8A3E2136C119%40hintbits.com
[3] http://archives.postgresql.org/message-id/9B503EB5-676A-4258-9F78-27FC583713FE%40hintbits.com
[4] http://archives.postgresql.org/message-id/CAMsr+YE6LNy2e0tBuAQB+NTVb6W-dHJAfLq0-zbAL7G7hjhXBA@mail.gmail.com
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
pgsql-hackers by date: