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

On 2016-05-02 18:15:40 +0300, Ants Aasma wrote:
> On Mon, May 2, 2016 at 5:21 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-05-02 09:03:19 -0400, Robert Haas wrote:
> >> On Fri, Apr 29, 2016 at 6:08 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> >> > Now to continue with the performance benchmarks. I'm pretty sure
> >> > we've fixed the problems when the feature is disabled
> >> > (old_snapshot_threshold = -1), and there are several suggestions
> >> > for improving performance while it is on that need to be compared
> >> > and benchmarked.
> >>
> >> If anyone thinks that the issue with the feature disabled is NOT
> >> fixed, please speak up! I'm moving the corresponding open item to
> >> CLOSE_WAIT status, meaning that it will be closed if nobody shows up
> >> to say that there is still an issue.
> >
> > Well, I don't agree that the feature is in a releaseable state. The
> > datastructure is pretty much non-scalable, and maintained on the wrong
> > side (every read, instead of once in writing writing xacts). There's no
> > proposal actually addressing the scalability issues.
>
> Unless I'm missing something fundamental the feature only requires
> tracking an upper bound on xmin observed by snapshots between clock
> ticks.

I'm not saying that there's no datastructure that can make the whole
thing efficient - just that current datastructure doesn't look viable
and that I've not seen that point addressed seriously.

> The simplest way to do this would be a periodic process that
> increments a clock counter (32bit counter would be plenty) and then
> calculates xmin for the preceding range. With this scheme
> GetSnapshotData would need two atomic fetches to get current LSN and
> the timestamp. Test for old snapshot can also run completely lock free
> with a single atomic fetch of threshold timestamp. The negative side
> is that we need to have a process running that runs the clock ticks
> and the ticks may sometimes be late. Giving something like autovacuum
> launcher this task doesn't seem too bad and the consequence of falling
> behind is just delayed timing out of old snapshots.

That'd be one way, yes. I suspect it'd even be sufficient to move
maintaining the map around GetNewTransactionId(); so only writers pay
the overhead. Given that writes obviously are slower than reads that
might make the problem disappear for a long while.

> As far as I can see this approach would get rid of any scalability
> issues, but it is a pretty significant change and requires 64bit
> atomic reads to get rid of contention on xlog insert lock.

Yea.

Andres