On Tue, Jan 11, 2011 at 13:18, Simon Riggs <simon@2ndquadrant.com> wrote:
> 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.
Aha. Thanks for the pointers, pfa a new version.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
