On Tue, 2011-01-11 at 13:04 +0100, Magnus Hagander wrote:
> On Tue, Jan 11, 2011 at 12:58, Simon Riggs <simon@2ndquadrant.com> wrote:
> > On Tue, 2011-01-11 at 12:41 +0100, Magnus Hagander wrote:
> >
> >> Just to be clear, you're objecting to the *name* of the state, right,
> >> not how/where it's set?
> >
> > Yes
> >
> >> In particular, how the catchup/streaming
> >> things are set?
> >
> > You've set it in the right places.
> >
> > I would personally constrain the state transitions, so that we can't
> > ever make illegal changes, such as CATCHUP -> BACKUP.
>
> Well, once we enter the walsender loop we can never get out of it, so
> it simply cannot happen...
Accidentally it can, so I would like to protect against that.
Putting those checks in are like Asserts(), they help document what is
and is not possible by design.
> > I would also check the state locally before grabbing the spinlock, as is
> > typical in other xlog code. Continually resetting shared state to
> > exactly what it is already seems strange, to me. If we make the rule
> > that the state can only be changed by the WALSender itself, it won't
> > need to grab spinlock. If you don't like that, a local variable works.
>
> Hmm. I don't see why anybody other than the walsender should change
> it, so yeah. You mean I can just drop the spinlock calls completely,
> and then do an
> if (walsnd->state != state)
> walsnd->state = state;
>
>
> ? Do I need to keep the volatile pointer, or should I drop that as well?
No, do this at top
if (walsnd->state == state) return;
Keep spinlocks when actually setting it.
> > Also, mixing upper camel case and uppercase for the STATe NamES looKS
> > weIRD. Uppercase makes them look more clearly like enum/states as used
> > elsewhere in similar code.
>
> Yeah, I'll change that.
>
-- Simon Riggs http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services
