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

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.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Kevin Grittner <kgrittn@gmail.com> writes:
> On Tue, Apr 12, 2016 at 12:08 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> 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 --

> 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.

It'd be good if you document the problems you found somewhere, before
you forget them, just in case somebody does want to try to lift the
restriction.  I agree that scattered code comments wouldn't be the way.
Just a quick email to -hackers to get the info into the archives
might be enough.
        regards, tom lane

Kevin Grittner wrote:

> 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.

Okay, that's fair.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 4/12/16 12:30 PM, Tom Lane wrote:
> It'd be good if you document the problems you found somewhere, before
> you forget them, just in case somebody does want to try to lift the
> restriction.  I agree that scattered code comments wouldn't be the way.
> Just a quick email to -hackers to get the info into the archives
> might be enough.

I think a code comment pointing at the archived message would be good 
though...
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com