Re: Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <

On Tue, Apr 12, 2016 at 11:05 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-04-12 23:52:14 -0300, Alvaro Herrera wrote:
>> Andres Freund wrote:
>> > I'm kinda inclined to apply that portion (or just the whole patch with
>> > the spurious #ifdef 0 et al fixed) into 9.6; and add the necessary
>> > checks in a few places. Because I really think this is likely to hit
>> > unsuspecting users.
>>
>> !!!
>>
>> Be sure to consult with the RMT before doing anything of the sort.
>
> I didn't plan to do anything without a few +1's. I don't think we can
> release with the state of things as is though. I don't see a less
> intrusive way than to get rid of that spinlock on all platforms capable
> of significant concurrency.
>
> So, RMT, what are your thoughts on this?

I think that a significant performance regression which affects people
not using snapshot_too_old would be a stop-ship issue, but I disagree
that an issue which only affects people using the feature is a
must-fix. It may be desirable to fix it, but I don't think we should
regard it as a hard requirement. It's reasonable to fix some kinds of
issues after feature freeze, but not at the price of accepting
arbitrary amounts of new code that may have problems of its own.
Every release will have some warts.

My testing yesterday of latest master, specifically
deb71fa9713dfe374a74fc58a5d298b5f25da3f5, last night did not show
evidence of a regression under heavy concurrency, as per
http://www.postgresql.org/message-id/CA+TgmobpHAqsOeHc-ooRsjzTKw1H4s4P1VBtwh1KkKO+6Mp8_Q@mail.gmail.com
- that test was of course run without enabling "snapshot too old".
My guess is that 2201d801b03c2d1b0bce4d6580b718dc34d38b3e was
sufficient to put things right, and that we now have a problem only
when "snapshot too old" is enabled.

I have never understood why you didn't include 64-bit atomics in the
original atomics implementation, and I really think we should have
committed a patch to add them long before now. Also noteworthy is the
fact that, by itself, such a patch cannot break anything except
perhaps the build, for, lo!, unused macros and functions do not do
anything. On the whole, I think that putting such a patch into
PostgreSQL 9.6 is likely to save us more pain than it causes us. I
would be disinclined to endorse applying part of it, because that
seems likely to complicate back-patching for no real gain.

Of course, the real fly in the ointment here is what we're going to do
with the atomics once we have them. But AFAICS, there's no patch for
that, yet. I don't think that I wish to take a position on whether a
patch that hasn't been written yet should be applied. So I think the
next step is that you should post the patches that you think should be
applied in final form and those should be reviewed by knowledgeable
people. Then, based on those reviews, the RMT can decide what to do.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company