Re: what to revert

Hi,

On 2016-05-04 16:22:41 -0500, Kevin Grittner wrote:
> On Wed, May 4, 2016 at 3:42 PM, Andres Freund <andres@anarazel.de> wrote:
> 
> > My concern isn't performing checks at snapshot build time and at buffer
> > access time. That seems fairly reasonable.  My concern is that the
> > time->xid mapping used to determine the xid horizon is built at snapshot
> > access time. The relevant function for that is
> > MaintainOldSnapshotTimeMapping() called by GetSnapshotData(), if you
> > want to look at it.  Building the mapping when building the snapshot
> > requires that an exclusive lwlock is taken.  Given that there's very few
> > access patterns in which more xids are assigned than snapshots taken,
> > that just doesn't make much sense; even leaving the fact that adding an
> > exclusive lock in a function that's already one of the biggest
> > bottlenecks in postgres, isn't a good idea.
> 
> It seems to me that you are confusing the structure with the point
> from which the function to call it is filled (and how often).

Well, you say tomato I say tomato. Sure we'd still use some form of
mapping, but filling it from somewhere else, with somewhat different
contents (xact timestamp presumably), with a somewhat different
replacement policy wouldn't leave all that much of the current structure
in place.  But ok, let's then call it a question of where from and how
the mapping is maintained.


> Some of the proposals involve fairly small tweaks to call
> MaintainOldSnapshotTimeMapping() from elsewhere or only when
> something changes (like crossing a minute boundary or seeing that a
> new TransactionId has been assigned).  If you can disentangle those
> ideas, it might not look so bad.

Yea, if we can do that, I'm ok. I'm doubtful about releasing with the
current state, even leaving performance aside, since fixing this will
result in somewhat changed semantics, and I'm doubtful about significant
development at this point of the release process. If it comes down to
either one of those I'm clearly in favor of the latter.


> >> Surely nobody here is blind to the fact that holding back xmin often
> >> creates a lot of bloat for no reason - many or all of the retained old
> >> row versions may never be accessed.
> >
> > Definitely. I *like* the feature.
> 
> And I think we have agreed in off-list discussions that even with
> these NUMA issues the patch, as it stands, would help some -- the
> poor choice of a call site for MaintainOldSnapshotTimeMapping()
> just unnecessarily limits how many workloads can benefit.

Yes, totally.


> Hopefully you can understand the reasons I chose to focus on the
> performance issues when it is disabled, and the hash index issue
> before moving on to this.

Well, somewhat. For me addressing architectural issues (and where to
fill the mapping from is that, independent of whether you call it a data
structure issue) is pretty important too, because I personally don't
believe we can release or even go to beta before that. But I'd not be
all that bothered to release beta1 with a hash index issue that we know
how to address.


> > Sure, it's not a guarantee. But I think a "promise" (signed in blood of
> > course) of a senior contributor makes quite the difference.
> 
> How about we see where we are as we get closer to a go/no go point
> and see where performance has settled and what kind of promise
> would satisfy you.

Hm. We're pretty close to a go/no go point, namely beta1.  I don't want
to be in the situation that we don't fix the issue before release, just
because beta has passed.


> I'm uncomfortable with the demand for a rather
> non-specific promise, and find myself flashing back to some
> sections of the Catch 22 novel.

Gotta read that one day (ordered).


Greetings,

Andres Freund