Re: An example of bugs for Hot Standby

On Wednesday 20 January 2010 17:30:04 Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On Wednesday 20 January 2010 06:30:28 Tom Lane wrote:
> >> Er ... what? I believe there are live platforms with sig_atomic_t =
> >> char. If we're assuming more that's a must-fix.
> >
> > The reason I have asked is that the code is doing things like:
> > [ grabbing a spinlock to read a single integer ]
>
> Yes, I think we probably actually need that. The problem is not so
> much whether the read is an atomic operation as whether you can rely
> on getting an up-to-date value. On multiprocessors with weak memory
> ordering you need some type of "sync" instruction to be sure you will
> see a value that was recently written by another processor. Currently,
> we embed such instructions in the spinlock acquire/release code.
> There's been some discussion of exposing memory sync independently
> of lock acquisition; perhaps that would be enough here, but I haven't
> looked at the surrounding logic enough to say.
I think it should be enough.

I realize its way too late in the cycle for that, but why dont we start using
some library for easy cross platform atomic ops? I think libatomic or such
should support the required platforms.

> My complaint at the top was responding to the idea that someone might
> be supposing the specific type sig_atomic_t was at least as wide as
> int. That's a different matter altogether. We do assume in some places
> that we can read or write the specific type TransactionId indivisibly,
> but we don't try to declare it as sig_atomic_t.
So we already assume that? Fine.

(yes, the sig_atomic_t was a sidetrack - I had memorized it wrongly as "the
biggest value that can be read/written atomically which is *clearly* wrong)

> > or similar things with LWLockAcquire in a signal handler
>
> [ grows visibly pale ] *Please* tell me we are not trying to take
> locks in a signal handler. What happens if it interrupts code that
> is already holding that lock?
Yes the patch does that at two places. Thats what I was complaining about and
what triggered my sig_atomic_t question because of the above explained
misunderstanding.

Andres