On Tue, 2011-01-11 at 13:04 +0100, Magnus Hagander wrote: > On Tue, Jan 11, 2011 at 12:58, Simon Riggs <si...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers