On Tue, Apr 12, 2016 at 12:08 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Kevin Grittner wrote:
>> Avoid extra locks in GetSnapshotData if old_snapshot_threshold < 0
>>
>> On a big NUMA machine with 1000 connections in saturation load
>> there was a performance regression due to spinlock contention, for
>> acquiring values which were never used. Just fill with dummy
>> values if we're not going to use them.
>
> old_snapshot_threshold is PGC_POSTMASTER, so this is okay AFAICS, but
> perhaps it'd be a good idea to add a oneline comment to guc.c indicating
> to verify this code if there's an intention to lift that limitation --
> snapshots taken before the reload would have invalid lsn/timestamp, so
> the current check would simply skip the check, which would be the wrong
> thing to do.
>
> I think it's reasonable to want to enable this feature with a simple
> reload, so if we ever do that it'd be good to have a pointer about that
> gotcha. (I'm not proposing you do that, only add the comment for a
> future hacker.)
Perhaps, but this would be one of at least a dozen land mines that
exist for trying to modify this setting to be read on reload.
FWIW, I spent a fair amount of time trying to make it PGC_SIGHUP,
since it would be very nice to allow that; but I kept running into
one problem after another with it, some of which were very hard to
see how to fix. My inclination is that trying to comment all the
places that would need something done if we do this in some later
release would be distracting until such time as we get there, and
might give a false sense of security to anyone who fixes all the
places the comments were scattered.
If there is a consensus that the comments would be worthwhile, I
can do a pass over the code I had before I gave up on PGC_SIGHUP
and try to add comments to all the appropriate spots based on
differences due to when the GUC was changed. If we don't want
that, I'm not sure why this one spot is a better place for such a
comment than all the others.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
