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

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.

This patch has not been benchmarked yet on a big NUMA machine, but
it seems like a good idea on general principle, and it seemed to
prevent an apparent 2.2% regression on a single-socket i7 box
running 200 connections at saturation load.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/2201d801b03c2d1b0bce4d6580b718dc34d38b3e

Modified Files
--------------
src/backend/storage/ipc/procarray.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)

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(at)2ndquadrant(dot)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(at)gmail(dot)com> writes:
> On Tue, Apr 12, 2016 at 12:08 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)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 2016-04-12 16:49:25 +0000, Kevin Grittner wrote:
> 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.

FWIW, I could see massive regressions with just 64 connections.

I'm a bit scared of having an innoccuous sounding option regress things
by a factor of 10. I think, in addition to this fix, we need to actually
solve the scalability issue here to a good degree. One way to do so is
to apply the parts of 0001 in
http://archives.postgresql.org/message-id/20160330230914.GH13305%40awork2.anarazel.de
defining PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY and rely on that. Another
to apply the whole patch and simply put the lsn in an 8 byte atomic.

- Andres

On Tue, Apr 12, 2016 at 12:38 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-04-12 16:49:25 +0000, Kevin Grittner wrote:
>> 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.
>
> FWIW, I could see massive regressions with just 64 connections.

With what settings? With or without the patch to avoid the locks when off?

> I'm a bit scared of having an innoccuous sounding option regress things
> by a factor of 10. I think, in addition to this fix, we need to actually
> solve the scalability issue here to a good degree. One way to do so is
> to apply the parts of 0001 in
> http://archives.postgresql.org/message-id/20160330230914.GH13305%40awork2.anarazel.de
> defining PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY and rely on that. Another
> to apply the whole patch and simply put the lsn in an 8 byte atomic.

I think that we are well due for atomic access to aligned 8-byte
values. That would eliminate one potential hot spot in the
"snapshot too old" code, for sure.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-04-12 13:44:00 -0500, Kevin Grittner wrote:
> On Tue, Apr 12, 2016 at 12:38 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-04-12 16:49:25 +0000, Kevin Grittner wrote:
> >> 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.
> >
> > FWIW, I could see massive regressions with just 64 connections.
>
> With what settings?

You mean pgbench or postgres? The former -M prepared -c 64 -j 64 -S. The
latter just a large enough shared buffers to contains the scale 300
database, and adapted maintenance_work_mem. Nothing special.

> With or without the patch to avoid the locks when off?

Without. Your commit message made it sound like you need unrealistic or
at least unusual numbers of connections, and that's afaics not the case.

> > I'm a bit scared of having an innoccuous sounding option regress things
> > by a factor of 10. I think, in addition to this fix, we need to actually
> > solve the scalability issue here to a good degree. One way to do so is
> > to apply the parts of 0001 in
> > http://archives.postgresql.org/message-id/20160330230914.GH13305%40awork2.anarazel.de
> > defining PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY and rely on that. Another
> > to apply the whole patch and simply put the lsn in an 8 byte atomic.
>
> I think that we are well due for atomic access to aligned 8-byte
> values. That would eliminate one potential hot spot in the
> "snapshot too old" code, for sure.

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.

FWIW, accessing a frequently changing value from a significant number of
connections, at a high frequency, isn't exactly free without a spinlock
either. But it should be much less bad.

Greetings,

Andres Freund

On Tue, Apr 12, 2016 at 1:56 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-04-12 13:44:00 -0500, Kevin Grittner wrote:
>> On Tue, Apr 12, 2016 at 12:38 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>> On 2016-04-12 16:49:25 +0000, Kevin Grittner wrote:
>>>> 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.
>>>
>>> FWIW, I could see massive regressions with just 64 connections.
>>
>> With what settings?
>
> You mean pgbench or postgres? The former -M prepared -c 64 -j 64 -S. The
> latter just a large enough shared buffers to contains the scale 300
> database, and adapted maintenance_work_mem. Nothing special.

Well, something is different between your environment and mine,
since I saw no difference at scale 100 and 2.2% at scale 200. So,
knowing more about your hardware, OS, configuration, etc., might
allow me to duplicate a problem so I can fix it. For example, I
used a "real" pg config, like I would for a production machine
(because that seems to me to be the environment that is most
important): the kernel is 3.13 (not one with pessimal scheduling)
and has tuning for THP, the deadline scheduler, the vm.*dirty*
settings, etc. Without knowing even the kernel and what tuning the
OS and pg have had on your box, I could take a lot of shots in the
dark without hitting anything. Oh, and the output of `numactl
--hardware` would be good to have. Thanks for all information you
can provide.

>> With or without the patch to avoid the locks when off?
>
> Without. Your commit message made it sound like you need unrealistic or
> at least unusual numbers of connections, and that's afaics not the case.

It was the only reported case to that point, so the additional data
point is valuable, if I can tell where that point is. And you
don't have any evidence that even with your configuration that any
performance regression remains for those who have the default value
for old_snapshot_threshold?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2016-04-12 14:17:12 -0500, Kevin Grittner wrote:
> Well, something is different between your environment and mine,
> since I saw no difference at scale 100 and 2.2% at scale 200.

In a readonly test or r/w? A lot of this will be different between
single-socket and multi-socket servers; as soon as you have the latter
the likelihood of contention being bad goes up dramatically.

> So,
> knowing more about your hardware, OS, configuration, etc., might
> allow me to duplicate a problem so I can fix

> For example, I used a "real" pg config, like I would for a production
> machine (because that seems to me to be the environment that is most
> important): the kernel is 3.13 (not one with pessimal scheduling) and
> has tuning for THP, the deadline scheduler, the vm.*dirty* settings,
> etc. Without knowing even the kernel and what tuning the OS and pg
> have had on your box, I could take a lot of shots in the dark without
> hitting anything.

That shouldn't really matter much for a read-only, shared_buffer
resident, test? There's no IO and THP pretty much plays no role because
there's very few memory allocations (removing the pressure causing the
well known degradations).

> Oh, and the output of `numactl --hardware` would be good to have.
> Thanks for all information you can provide.

That was on Alexander's/PgPro's machine. Numactl wasn't installed, and I
didn't have root. But it has four numa domains (gathered via /sys/).

> It was the only reported case to that point, so the additional data
> point is valuable, if I can tell where that point is. And you
> don't have any evidence that even with your configuration that any
> performance regression remains for those who have the default value
> for old_snapshot_threshold?

I haven't tested yet.

Greetings,

Andres Freund

On Tue, Apr 12, 2016 at 2:28 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-04-12 14:17:12 -0500, Kevin Grittner wrote:
>> Well, something is different between your environment and mine,
>> since I saw no difference at scale 100 and 2.2% at scale 200.
>
> In a readonly test or r/w?

Readonly with client and job counts matching scale.

> A lot of this will be different between
> single-socket and multi-socket servers; as soon as you have the latter
> the likelihood of contention being bad goes up dramatically.

Yeah, I know, and 4 socket has been at least an order of magnitude
more problematic in my experience than 2 socket. And the problems
are far, far, far worse on kernels prior to 3.8, especially on 3.x
before 3.8, so it's hard to know how to take any report of problems
on a 4 node NUMA machine without knowing the kernel version.

>> knowing more about your hardware, OS, configuration, etc., might
>> allow me to duplicate a problem so I can fix
>
>> For example, I used a "real" pg config, like I would for a production
>> machine (because that seems to me to be the environment that is most
>> important): the kernel is 3.13 (not one with pessimal scheduling) and
>> has tuning for THP, the deadline scheduler, the vm.*dirty* settings,
>> etc. Without knowing even the kernel and what tuning the OS and pg
>> have had on your box, I could take a lot of shots in the dark without
>> hitting anything.
>
> That shouldn't really matter much for a read-only, shared_buffer
> resident, test? There's no IO and THP pretty much plays no role because
> there's very few memory allocations (removing the pressure causing the
> well known degradations).

I hate to assume which differences matter without trying, but some
of them seem less probable than others.

>> Oh, and the output of `numactl --hardware` would be good to have.
>> Thanks for all information you can provide.
>
> That was on Alexander's/PgPro's machine. Numactl wasn't installed, and I
> didn't have root. But it has four numa domains (gathered via /sys/).

On the machines I've used, it will give you the hardware report
without being root. But of course, it can't do that if it's not
installed. I hadn't yet seen a machine with multiple NUMA memory
segments that didn't have the numactl executable installed; I'll
keep in mind that can happen.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Apr 12, 2016 at 2:53 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> Readonly with client and job counts matching scale.

Single-socket i7, BTW.

>> A lot of this will be different between
>> single-socket and multi-socket servers; as soon as you have the latter
>> the likelihood of contention being bad goes up dramatically.
>
> Yeah, I know, and 4 socket has been at least an order of magnitude
> more problematic in my experience than 2 socket. And the problems
> are far, far, far worse on kernels prior to 3.8, especially on 3.x
> before 3.8, so it's hard to know how to take any report of problems
> on a 4 node NUMA machine without knowing the kernel version.

Also, with 4 node NUMA I have seen far better scaling with
hyper-threading turned off. I know there are environments where it
helps, but high-concurrency on multi-node NUMA is not one of them.

So, anyway, mentioning the HT setting is important, too.

Kevin Grittner

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.
It might as well decide to revert the whole patch.

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

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?

Andres

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> 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.

What will you do on 32-bit platforms (or, more generally, anything
lacking 64-bit-wide atomics)?

regards, tom lane

Robert Haas wrote:
> On Tue, Apr 12, 2016 at 11:05 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

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

Agreed.

> but I disagree that an issue which only affects people using the
> feature is a must-fix.

Agreed.

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

Agreed.

The patch being proposed for commit is fiddly architecture-specific
stuff which is likely to destabilize the tree for quite some time, and
cause lots of additional work to Andres and anyone else likely to work
on such low-level details, such as Robert, both of which already have
plenty to do.

The snapshot-too-old feature is said to be great and shows lots of
improvement in certain cases, and no regression can be measured for
those who have it turned off. The regression only seems to show up if
you turn it on and have a crazily high rate of read-only transactions.
I think this can wait for 9.7.

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

On Wed, Apr 13, 2016 at 9:52 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> 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.
>
> What will you do on 32-bit platforms (or, more generally, anything
> lacking 64-bit-wide atomics)?

We fall back to emulating it using spinlocks. This isn't really an
issue in practice because 32-bit x86 has native 64-bit atomics, and
it's hard to point to another 32-bit platform that is likely to be
have enough concurrency for the lack of 64-bit atomics to matter.
Actually, it looks like we have 64-bit atomics in the tree already;
it's only the fallback implementation that is missing (so anything you
do using 64-bit atomics would need an alternate implementation that
did not rely on them).

But the really interesting that the patch to which Andres linked does
is introduce machinery to try to determine whether a platform has
8-byte single-copy atomicity; that is, whether a load or store of an
aligned 8-byte value is guaranteed not to be torn. We currently avoid
assuming that, but this requires additional spinlocks in a significant
number of places; the regression seen using "snapshot too old" at high
concurrency is merely the tip of the iceberg. And the annoying thing
about avoiding that assumption is that it actually is true on pretty
much every modern platform. Look at this gem Andres wrote in that
patch:

+/*
+ * 8 byte reads / writes have single-copy atomicity on 32 bit x86 platforms
+ * since at least the 586. As well as on all x86-64 cpus.
+ */
+#if defined(__i568__) || defined(__i668__) || /* gcc i586+ */ \
+ (defined(_M_IX86) && _M_IX86 >= 500) || /* msvc i586+ */ \
+ defined(__x86_64__) || defined(__x86_64) || defined(_M_X64) /*
gcc, sunpro, msvc */
+#define PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY
+#endif /* 8 byte single-copy atomicity */

I don't know if that test is actually correct, and I wonder about
compile-time environment vs. run-time environment, but I have my
doubts about how well PostgreSQL 9.6 would run on an i486. I doubt
that is the platform for which we should be optimizing.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Apr 13, 2016 at 9:52 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> 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.

>> What will you do on 32-bit platforms (or, more generally, anything
>> lacking 64-bit-wide atomics)?

> We fall back to emulating it using spinlocks.

That's what I thought you were going to say, and it means that any
"performance improvement" patch that relies on 64-bit atomics in hotspot
code paths is going to be a complete disaster on anything but modern Intel
hardware. I'm not sure that's a direction we want to go in. We need to
stick to a set of atomics that's pretty widely portable.

> This isn't really an
> issue in practice because 32-bit x86 has native 64-bit atomics, and
> it's hard to point to another 32-bit platform that is likely to be
> have enough concurrency for the lack of 64-bit atomics to matter.

It's not concurrency I'm worried about, it's the sheer overhead of
going through the spinlock code.

I'd be okay with atomics that were defined as "pointer width", if
we have a need for that, but I'm suspicious of 64-bits-exactly.

regards, tom lane

On Wed, Apr 13, 2016 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Apr 13, 2016 at 9:52 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> 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.
>
>>> What will you do on 32-bit platforms (or, more generally, anything
>>> lacking 64-bit-wide atomics)?
>
>> We fall back to emulating it using spinlocks.
>
> That's what I thought you were going to say, and it means that any
> "performance improvement" patch that relies on 64-bit atomics in hotspot
> code paths is going to be a complete disaster on anything but modern Intel
> hardware. I'm not sure that's a direction we want to go in. We need to
> stick to a set of atomics that's pretty widely portable.

I think 64-bit atomics *are* pretty widely portable. Can you name a
system with more than 4 CPU cores that doesn't support them?

>> This isn't really an
>> issue in practice because 32-bit x86 has native 64-bit atomics, and
>> it's hard to point to another 32-bit platform that is likely to be
>> have enough concurrency for the lack of 64-bit atomics to matter.
>
> It's not concurrency I'm worried about, it's the sheer overhead of
> going through the spinlock code.

I'm not sure I understand exactly what the concern is here. I agree
that there is a possibility that any patch which uses 64-bit atomics
could regress performance on platforms that do not support 64-bit
atomics. That's why I argued initially against having fallbacks for
*any* atomic operations; I was of the opinion that we should be
prepared to carry two implementations of anything that was going to
depend on atomics. I lost that argument, perhaps for the best. I
think one of the problems here is that very few of us have any
hardware available which we could even use to test performance on
systems that lack support for both 32 and 64 bit atomics. We can
compile without atomics on the hardware we do have and see how that
goes, but that's not necessarily indicative of what will happen on
some altogether different CPU architecture. In some cases there might
be an emulator, like the VAX emulator Greg Stark was playing with, but
that's not necessarily indicative either, and also, really, who cares?

I think it would be cool if somebody started a project to try to
optimize the performance of PostgreSQL on, say, a Raspberry Pi. Then
we might learn whether any of this stuff actually matters there or
whether the problems are completely elsewhere (like too much
per-backend memory consumption). However, for reasons that are
probably sort of obvious, I doubt I'll have much luck getting
EnterpriseDB to fund work on that project - if it ever happens, it
will probably have to be the work of a dedicated hobbiest, or somebody
who has a tangible need to build an embedded system using PostgreSQL.

> I'd be okay with atomics that were defined as "pointer width", if
> we have a need for that, but I'm suspicious of 64-bits-exactly.

I think LSNs are an important case, and they are not pointer width.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Apr 13, 2016 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> That's what I thought you were going to say, and it means that any
>> "performance improvement" patch that relies on 64-bit atomics in hotspot
>> code paths is going to be a complete disaster on anything but modern Intel
>> hardware. I'm not sure that's a direction we want to go in. We need to
>> stick to a set of atomics that's pretty widely portable.

> I think 64-bit atomics *are* pretty widely portable. Can you name a
> system with more than 4 CPU cores that doesn't support them?

No, you're ignoring my point, which is what happens on single-CPU
32-bit machines, and whether we aren't going to destroy performance
on low-end machines in pursuit of better performance on high-end.

Now, to the extent that a patch uses a 64-bit atomic op to replace
a spinlock acquisition, it might be pretty much a wash if low-end
machines have to use a spinlock to emulate the atomic op. But it
would be really easy for the translation to replace one spinlock
acquisition with multiple spinlock acquisitions, and that would hurt.

regards, tom lane

On 2016-04-13 08:36:47 -0400, Robert Haas wrote:
> 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 problem with that is that snapshot-too-old is essentially a
efficiency feature for busy and large databases. Regressing noticeably
when it's enabled in it's natural habitat seems sad.

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

Well, I'm less likely to write a patch when there's no chance that it's
going to be applied. Which the rest of the thread sounds like...

Greetings,

Andres Freund

On Wed, Apr 13, 2016 at 10:42 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Apr 13, 2016 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> That's what I thought you were going to say, and it means that any
>>> "performance improvement" patch that relies on 64-bit atomics in hotspot
>>> code paths is going to be a complete disaster on anything but modern Intel
>>> hardware. I'm not sure that's a direction we want to go in. We need to
>>> stick to a set of atomics that's pretty widely portable.
>
>> I think 64-bit atomics *are* pretty widely portable. Can you name a
>> system with more than 4 CPU cores that doesn't support them?
>
> No, you're ignoring my point, which is what happens on single-CPU
> 32-bit machines, and whether we aren't going to destroy performance
> on low-end machines in pursuit of better performance on high-end.
>
> Now, to the extent that a patch uses a 64-bit atomic op to replace
> a spinlock acquisition, it might be pretty much a wash if low-end
> machines have to use a spinlock to emulate the atomic op. But it
> would be really easy for the translation to replace one spinlock
> acquisition with multiple spinlock acquisitions, and that would hurt.

One of us is confused, or we're just talking past each other, because
I don't think I'm ignoring your point at all. In fact, I think I just
responded to it rather directly. I agree that the exact risk you are
describing exists. However, the multiple spinlock cycles that you are
concerned about will only occur on a platform that doesn't support
64-bit atomics. In order to test whether there is a performance
problem on such hardware, or how serious that problem is, we'd need to
have access to such hardware, and I don't know where to find any such
hardware. Do you?

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

On Wed, Apr 13, 2016 at 11:01 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Well, I'm less likely to write a patch when there's no chance that it's
> going to be applied. Which the rest of the thread sounds like...

I hope somebody writes it at some point, because we surely want to fix
this for 9.7. However, I agree that there seems to be a tangible lack
of enthusiasm for doing anything about it right now. I'm slightly
surprised by that, but that's OK: I just work here.

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

On 2016-04-13 11:08:21 -0300, Alvaro Herrera wrote:
> The patch being proposed for commit is fiddly architecture-specific
> stuff which is likely to destabilize the tree for quite some time, and
> cause lots of additional work to Andres and anyone else likely to work
> on such low-level details, such as Robert, both of which already have
> plenty to do.

Personally I think this is an 9.6 open-item, and primarily Kevin has to
work on it.

Note that there really shouldn't be too much fiddly bits, we've already
had 64bit atomics, just no fallback. This is just copying the fallback
code from 32bit atomics to 64bit atomics.

But what I'm actually proposing isn't even using the 64bit atomics from
that patch, just to add

--- a/src/include/port/atomics/arch-ppc.h
+++ b/src/include/port/atomics/arch-ppc.h
@@ -24,3 +24,6 @@
#define pg_read_barrier_impl() __asm__ __volatile__ ("lwsync" : : : "memory")
#define pg_write_barrier_impl() __asm__ __volatile__ ("lwsync" : : : "memory")
#endif
+
+/* per architecture manual doubleword accesses have single copy atomicity */
+#define PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY

to the appropriate files (ia64, ppc, x86) and then add an #ifndef
PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY to GetXLogInsertRecPtr's acquisition
of the spinlock. I.e.
#ifndef PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY
SpinLockAcquire(&Insert->insertpos_lck);
#endif
current_bytepos = Insert->CurrBytePos;
#ifndef PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY
SpinLockRelease(&Insert->insertpos_lck);
#endif

not because I think it's perfectly pretty that way, but because it's
very easy to demonstrate that there's no regressions for anybody.

> The regression only seems to show up if you turn it on and have a
> crazily high rate of read-only transactions. I think this can wait
> for 9.7.

I don't think 120k read tps is all that high anymore these days. And you
can easily create scenarios that are *much* worse than pgbench. E.g. a
loop in a volatile plpgsql function will acquire

Greetings,

Andres Freund

On 2016-04-13 10:42:03 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Wed, Apr 13, 2016 at 10:20 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> That's what I thought you were going to say, and it means that any
> >> "performance improvement" patch that relies on 64-bit atomics in hotspot
> >> code paths is going to be a complete disaster on anything but modern Intel
> >> hardware. I'm not sure that's a direction we want to go in. We need to
> >> stick to a set of atomics that's pretty widely portable.
>
> > I think 64-bit atomics *are* pretty widely portable. Can you name a
> > system with more than 4 CPU cores that doesn't support them?
>
> No, you're ignoring my point, which is what happens on single-CPU
> 32-bit machines, and whether we aren't going to destroy performance
> on low-end machines in pursuit of better performance on high-end.

I think generally the only platform of concern wrt is arm (< armv8),
which doesn't have 64bit atomicity and doesn't have
single-copy-atomicity for 8 byte values either (C.f.
https://wiki.postgresql.org/wiki/Atomics). But:

> Now, to the extent that a patch uses a 64-bit atomic op to replace
> a spinlock acquisition, it might be pretty much a wash if low-end
> machines have to use a spinlock to emulate the atomic op. But it
> would be really easy for the translation to replace one spinlock
> acquisition with multiple spinlock acquisitions, and that would hurt.

Which is why I'm actually proposing to *not* use a pg_atomic_uint64, just a
single define to remove the spinlock acquisition:
http://archives.postgresql.org/message-id/20160413150839.mevdlgekizxyjhc5%40alap3.anarazel.de

I think there are a number of LSNs which we'd be better of replacing LSN
manipulations with an actual atomic operation (including fallback to the
spinlock) might be beneficial. But that'd be a larger patch, and would
require more testing; which is why I'm proposing the above.

Greetings,

Andres Freund

On Wed, Apr 13, 2016 at 11:18 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> I think generally the only platform of concern wrt is arm (< armv8),
> which doesn't have 64bit atomicity and doesn't have
> single-copy-atomicity for 8 byte values either (C.f.
> https://wiki.postgresql.org/wiki/Atomics).

That page is sort of confusing, because it says that platform has
those things but then says ***, which is footnoted to mean "linux
kernel emulation available", but it's not too clear whether that
applies to all atomics or just 8-byte atomics. The operator
precedence of / (used as a separator) vs. footnotes is not stated.
It's also not clear what "linux kernel emulation available" actually
means. Should we think of those things being fast, or slow?

At any rate, I do actually have a Raspberry Pi 2 here so if we ever
commit a patch that might suck without real 64-bit atomics we might be
able to actuall test whether it does or not. But as you say, no such
patch is being proposed at the moment.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Apr 13, 2016 at 10:42 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> No, you're ignoring my point, which is what happens on single-CPU
>> 32-bit machines, and whether we aren't going to destroy performance
>> on low-end machines in pursuit of better performance on high-end.

> One of us is confused, or we're just talking past each other, because
> I don't think I'm ignoring your point at all. In fact, I think I just
> responded to it rather directly. I agree that the exact risk you are
> describing exists. However, the multiple spinlock cycles that you are
> concerned about will only occur on a platform that doesn't support
> 64-bit atomics. In order to test whether there is a performance
> problem on such hardware, or how serious that problem is, we'd need to
> have access to such hardware, and I don't know where to find any such
> hardware. Do you?

As Andres says, low-end ARM machines are probably the most common such
hardware right now. I have two non-ARM machines in the buildfarm that
certainly haven't got such instructions (prairiedog and gaur/pademelon).
Now I wouldn't propose that we need to concern ourselves very much with
performance on those two decade-plus-old platforms, but I do think that
performance on small ARM machines is still of interest.

regards, tom lane

On Wed, Apr 13, 2016 at 10:01 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> My problem with that is that snapshot-too-old is essentially a
> efficiency feature for busy and large databases. Regressing noticeably
> when it's enabled in it's natural habitat seems sad.

With a real-world application with realistic simulated user load
there was no such regression and a big gain in performance over
time, so we're talking about adjusting how broad a range of
workloads it benefits. I don't have a strong opinion yet, since I
haven't run the benchmarks on big machines (scheduled for the day
after tomorrow); but as an example, if I only see such regression
on a Linux kernel with version a version < 3.8 I am going to be
less concerned about getting something into 9.6, since IMO it is
completely irresponsible to run a NUMA machine with 4 or more nodes
on an OS with a substandard NUMA scheduler. I'm not sure when 3.8
became available, but according to Wikipedia Version 3.10 of the
Linux kernel was released in June 2013, so it's not like you need
to be on the bleeding edge to have a decent scheduler.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-04-13 11:27:09 -0400, Robert Haas wrote:
> That page is sort of confusing, because it says that platform has
> those things but then says ***, which is footnoted to mean "linux
> kernel emulation available", but it's not too clear whether that
> applies to all atomics or just 8-byte atomics. The operator
> precedence of / (used as a separator) vs. footnotes is not stated.

/ has a higher precedence than footnotes. Not sure how to make that
easily clear. I'm not exactly a mediawiki expert.

> It's also not clear what "linux kernel emulation available" actually
> means. Should we think of those things being fast, or slow?

Slow. It means that the compiler generates a syscall to perform the
atomic. The syscall disables preemption, then performs the actual math,
re-enables preemption, and returns. That's a lot more expensive than a
spinlock. There's

/*
* 64 bit atomics on arm are implemented using kernel fallbacks and might be
* slow, so disable entirely for now.
* XXX: We might want to change that at some point for AARCH64
*/
#define PG_DISABLE_64_BIT_ATOMICS

for that reason (in the current tree, not patch).

The whole fallback facility exists to make it easier to port software to
arm; but I wouldn't want to rely on it if not necessary.

Greetings,

Andres Freund

On 2016-04-13 10:31:19 -0500, Kevin Grittner wrote:
> With a real-world application with realistic simulated user load
> there was no such regression and a big gain in performance over
> time, so we're talking about adjusting how broad a range of
> workloads it benefits.

I think it depends very heavily on the type of application. To be
affected you need a high rate of snapshot acquisitions. So lots of small
statements, or possibly longer running stuff involving volatile
functions (which IIRC get new snapshots continually).

> but as an example, if I only see such regression on a Linux kernel
> with version a version < 3.8 I am going to be less concerned about
> getting something into 9.6, since IMO it is completely irresponsible
> to run a NUMA machine with 4 or more nodes on an OS with a substandard
> NUMA scheduler. I'm not sure when 3.8 became available, but according
> to Wikipedia Version 3.10 of the Linux kernel was released in June
> 2013, so it's not like you need to be on the bleeding edge to have a
> decent scheduler.

I don't think effect of adding a single spinlock (an exclusive lock!) in
a hot path is likely to be hugely dependant on the kernel version.
We've had such cases before, and felt the pain. E.g. the spinlock in the
ProcArrayLock used to be a *HUGE* contention point, and it has pretty
much the same acquisition pattern as this spinlock now.

Greetings,

Andres Freund

On Wed, Apr 13, 2016 at 10:59 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:

>> but as an example, if I only see such regression on a Linux kernel
>> with version a version < 3.8 I am going to be less concerned about
>> getting something into 9.6, since IMO it is completely irresponsible
>> to run a NUMA machine with 4 or more nodes on an OS with a substandard
>> NUMA scheduler. I'm not sure when 3.8 became available, but according
>> to Wikipedia Version 3.10 of the Linux kernel was released in June
>> 2013, so it's not like you need to be on the bleeding edge to have a
>> decent scheduler.
>
> I don't think effect of adding a single spinlock (an exclusive lock!) in
> a hot path is likely to be hugely dependant on the kernel version.

My experience is that is easily can be. We had a customer who
could not scale beyond a certain point due to spinlock contention
on a single spinlock already present in stock pg. We tried lots of
config tweaks and a some custom patches to no avail. Then we had
them upgrade from RHEL 6.latest to RHEL 7.latest, and they could
scale much, much farther. No OS or pg config changes were made at
the same time. The difference is that they went from kernel
version kernel 2.6.32 to kernel version 3.10.0. The early version
3 kernels had a NUMA scheduler rewrite that was a disaster compared
to 2.6.32. They rewrote it again in 3.8, with dramatic effect.

> We've had such cases before, and felt the pain. E.g. the spinlock in the
> ProcArrayLock used to be a *HUGE* contention point, and it has pretty
> much the same acquisition pattern as this spinlock now.

It would be great to have improvements in such access patterns, no
doubt. I'll be happy if we get there. I don't have a problem
trying to contribute to the effort, either, if people think that
might actually be a net gain. But if we have a point where those
not using the new feature are unaffected, and the question is about
the range of workloads where the new feature will be helpful in
9.6, it doesn't seem to me to rise to the level of a bug or a
release blocker.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-04-12 14:53:57 -0500, Kevin Grittner wrote:
> On Tue, Apr 12, 2016 at 2:28 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-04-12 14:17:12 -0500, Kevin Grittner wrote:
> >> Well, something is different between your environment and mine,
> >> since I saw no difference at scale 100 and 2.2% at scale 200.
> >
> > In a readonly test or r/w?
>
> Readonly with client and job counts matching scale.
>
> > A lot of this will be different between
> > single-socket and multi-socket servers; as soon as you have the latter
> > the likelihood of contention being bad goes up dramatically.
>
> Yeah, I know, and 4 socket has been at least an order of magnitude
> more problematic in my experience than 2 socket. And the problems
> are far, far, far worse on kernels prior to 3.8, especially on 3.x
> before 3.8, so it's hard to know how to take any report of problems
> on a 4 node NUMA machine without knowing the kernel version.

On an EC2 m4.10xlarge (dedicated, but still a VM) - sorry I don't have
anything better at hand right now, and it was already running.

postgres config:
postgres -D /srv/data/dev/
-c shared_buffers=64GB \
-c max_wal_size=64GB \
-c maintenance_work_mem=32GB \
-c huge_pages=on \
-c max_connections=400 \
-c logging_collector=on -c log_filename='postgresql.log' \
-c log_checkpoints=on -c autovacuum=off \
-c autovacuum_freeze_max_age=80000000 \
-c synchronous_commit=off

Initialized with pgbench -q -i -s 300

Before each run I prewarmed with
psql -c "create extension if not exists pg_prewarm;select sum(x.x) from (select pg_prewarm(oid) as x from pg_class where relkind in ('i', 'r') order by oid) x;" > /dev/null 2>&1;

running pgbench -M prepared -c 128 -j 128 -n -P 1 -T 100 -S

With -c old_snapshot_threshold=0:

latency average = 0.218 ms
latency stddev = 0.154 ms
tps = 584666.289753 (including connections establishing)
tps = 584867.785569 (excluding connections establishing)

With -c old_snapshot_threshold=10:

latency average = 1.112 ms
latency stddev = 1.246 ms
tps = 114883.528964 (including connections establishing)
tps = 114905.555943 (excluding connections establishing)

With 848ef42bb8c7909c9d7baa38178d4a209906e7c1 (and followups) reverted:
latency average = 0.210 ms
latency stddev = 0.050 ms
tps = 607734.407158 (including connections establishing)
tps = 607918.118566 (excluding connections establishing)

A quicker (each -T 10) test, without restarts between scale reuns, of
other scales:

scale thres=0 thresh=10
1 15377.761645 15017.789751
1 16285.111754 14829.493870
2 29563.478651 28790.462964
4 62649.628931 50935.364141
8 84557.464387 85631.348766
16 101475.002295 93908.910894
32 347435.607586 167702.527893
64 575640.880911 150139.375351
128 594782.154256 112183.933956
196 584290.957806 92080.129402
256 583921.995839 79345.378887
398 582138.372414 58100.798609

- Andres

On Wed, Apr 13, 2016 at 1:19 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On an EC2 m4.10xlarge (dedicated, but still a VM) - sorry I don't have
> anything better at hand right now, and it was already running.
>
> postgres config:
> postgres -D /srv/data/dev/
> -c shared_buffers=64GB \
> -c max_wal_size=64GB \
> -c maintenance_work_mem=32GB \
> -c huge_pages=on \
> -c max_connections=400 \
> -c logging_collector=on -c log_filename='postgresql.log' \
> -c log_checkpoints=on -c autovacuum=off \
> -c autovacuum_freeze_max_age=80000000 \
> -c synchronous_commit=off
>
> Initialized with pgbench -q -i -s 300
>
> Before each run I prewarmed with
> psql -c "create extension if not exists pg_prewarm;select sum(x.x) from (select pg_prewarm(oid) as x from pg_class where relkind in ('i', 'r') order by oid) x;" > /dev/null 2>&1;
>
> running pgbench -M prepared -c 128 -j 128 -n -P 1 -T 100 -S
>
> With -c old_snapshot_threshold=0:
>
> latency average = 0.218 ms
> latency stddev = 0.154 ms
> tps = 584666.289753 (including connections establishing)
> tps = 584867.785569 (excluding connections establishing)
>
>
> With -c old_snapshot_threshold=10:
>
> latency average = 1.112 ms
> latency stddev = 1.246 ms
> tps = 114883.528964 (including connections establishing)
> tps = 114905.555943 (excluding connections establishing)
>
>
> With 848ef42bb8c7909c9d7baa38178d4a209906e7c1 (and followups) reverted:
> latency average = 0.210 ms
> latency stddev = 0.050 ms
> tps = 607734.407158 (including connections establishing)
> tps = 607918.118566 (excluding connections establishing)

Yuck. Aside from the fact that performance tanks when the feature is
turned on, it seems that there is a significant effect even with it
turned off.

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

On 2016-04-13 13:25:14 -0400, Robert Haas wrote:
> > With -c old_snapshot_threshold=0:
> >
> > latency average = 0.218 ms
> > latency stddev = 0.154 ms
> > tps = 584666.289753 (including connections establishing)
> > tps = 584867.785569 (excluding connections establishing)
> >
> >
> > With -c old_snapshot_threshold=10:
> >
> > latency average = 1.112 ms
> > latency stddev = 1.246 ms
> > tps = 114883.528964 (including connections establishing)
> > tps = 114905.555943 (excluding connections establishing)
> >
> >
> > With 848ef42bb8c7909c9d7baa38178d4a209906e7c1 (and followups) reverted:
> > latency average = 0.210 ms
> > latency stddev = 0.050 ms
> > tps = 607734.407158 (including connections establishing)
> > tps = 607918.118566 (excluding connections establishing)
>
> Yuck. Aside from the fact that performance tanks when the feature is
> turned on

A quick look at the former shows that it's primarily contention around
the new OldSnapshotTimeMapLock not, on that hardware in that workload,
the spinlock. Which isn't that surprising because it adds an exclusive
lock to a path which doesn't contain any other exclusive locks these
days...

I have to say, I'm *highly* doubtful that it's ok to add an exclusive
lock in a readonly workload to such an hot path, without any clear path
forward how to fix that scalability issue. This doesn't apear to be
requiring just a bit of elbow grease, but a fair bit more.

> it seems that there is a significant effect even with it turned off.

It looks that way, but I'd rather run a bit more careful and repeated
tests to make sure about that part. At a factor of 5, as with the on/off
tests, per-run varitions don't play a large role, but at smaller
percentages it's worthwhile to put more care into it. If possible it'd
be helpful to avoid a VM too...

Andres

On Wed, Apr 13, 2016 at 12:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> [test results with old_snapshot_threshold = 0 and 10]

From the docs:

| A value of -1 disables this feature, and is the default.

> Yuck. Aside from the fact that performance tanks when the feature is
> turned on, it seems that there is a significant effect even with it
> turned off.

No evidence of that has been provided. -1 is off; 0 is for testing
very fast expiration.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-04-13 13:52:15 -0500, Kevin Grittner wrote:
> On Wed, Apr 13, 2016 at 12:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> > [test results with old_snapshot_threshold = 0 and 10]
>
> From the docs:
>
> | A value of -1 disables this feature, and is the default.

Hm, ok, let me run that as well then. The reason for the massive
performance difference presumably is that
MaintainOldSnapshotTimeMapping() is cut short due to
/* No further tracking needed for 0 (used for testing). */
if (old_snapshot_threshold == 0)
return;
which means that OldSnapshotTimeMap isn't acquired exclusively.

> > Yuck. Aside from the fact that performance tanks when the feature is
> > turned on, it seems that there is a significant effect even with it
> > turned off.
>
> No evidence of that has been provided. -1 is off; 0 is for testing
> very fast expiration.

I'll run with -1 once the current (longer) run has finished.

Greetings,

Andres Freund

On Wed, Apr 13, 2016 at 1:56 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> I'll run with -1 once the current (longer) run has finished.

Just for the record, were any of the other results purporting to be
with the feature "off" also actually running with the feature set
for its fastest possible timeout?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Wed, Apr 13, 2016 at 3:08 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Wed, Apr 13, 2016 at 1:56 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
>> I'll run with -1 once the current (longer) run has finished.
>
> Just for the record, were any of the other results purporting to be
> with the feature "off" also actually running with the feature set
> for its fastest possible timeout?

Mine were testing something else entirely, so I didn't touch
old_snapshot_threshold at all.

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

On 2016-04-13 13:52:15 -0500, Kevin Grittner wrote:
> On Wed, Apr 13, 2016 at 12:25 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> > [test results with old_snapshot_threshold = 0 and 10]
>
> From the docs:
>
> | A value of -1 disables this feature, and is the default.
>
> > Yuck. Aside from the fact that performance tanks when the feature is
> > turned on, it seems that there is a significant effect even with it
> > turned off.
>
> No evidence of that has been provided. -1 is off; 0 is for testing
> very fast expiration.

Longer tests are running, but, again on the previous hardware with only
two sockets, the results for 128 clients are:

0:
progress: 100.0 s, 593351.0 tps, lat 0.215 ms stddev 0.118
progress: 200.0 s, 594035.9 tps, lat 0.215 ms stddev 0.118
progress: 300.0 s, 594013.3 tps, lat 0.215 ms stddev 0.117

-1:
progress: 100.0 s, 600835.3 tps, lat 0.212 ms stddev 0.049
progress: 200.0 s, 601466.1 tps, lat 0.212 ms stddev 0.048
progress: 300.0 s, 601529.5 tps, lat 0.212 ms stddev 0.047

reverted:
progress: 100.0 s, 612676.6 tps, lat 0.208 ms stddev 0.048
progress: 200.0 s, 613214.3 tps, lat 0.208 ms stddev 0.047
progress: 300.0 s, 613384.3 tps, lat 0.208 ms stddev 0.047

This is all on virtualized (though using a dedicated instance)
hardware. So they numbers are to be taken with a grain of salt. But I
did run shorter tests in various orders, and the runtime difference
apears to be very small.

- Andres

On 2016-04-13 14:08:49 -0500, Kevin Grittner wrote:
> On Wed, Apr 13, 2016 at 1:56 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > I'll run with -1 once the current (longer) run has finished.
>
> Just for the record, were any of the other results purporting to be
> with the feature "off" also actually running with the feature set
> for its fastest possible timeout?

Yes, I'd only used 0 / 10. I think that shows that the contention, for
me, is primarily the lwlock, not the spinlock.

Greetings,

Andres Freund

Hi Kevin,

On 2016-04-13 12:21:10 -0700, Andres Freund wrote:
> 0:
> progress: 100.0 s, 593351.0 tps, lat 0.215 ms stddev 0.118
> progress: 200.0 s, 594035.9 tps, lat 0.215 ms stddev 0.118
> progress: 300.0 s, 594013.3 tps, lat 0.215 ms stddev 0.117
>
> -1:
> progress: 100.0 s, 600835.3 tps, lat 0.212 ms stddev 0.049
> progress: 200.0 s, 601466.1 tps, lat 0.212 ms stddev 0.048
> progress: 300.0 s, 601529.5 tps, lat 0.212 ms stddev 0.047
>
> reverted:
> progress: 100.0 s, 612676.6 tps, lat 0.208 ms stddev 0.048
> progress: 200.0 s, 613214.3 tps, lat 0.208 ms stddev 0.047
> progress: 300.0 s, 613384.3 tps, lat 0.208 ms stddev 0.047

Setting it to 1 gives:
progress: 100.0 s, 115413.7 tps, lat 1.107 ms stddev 1.240
progress: 200.0 s, 114907.4 tps, lat 1.113 ms stddev 1.244
progress: 300.0 s, 115621.4 tps, lat 1.106 ms stddev 1.238

If you want me to rn some other tests I can, but ISTM we have the data
we need?

- Andres

On Wed, Apr 13, 2016 at 3:01 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> If you want me to rn some other tests I can, but ISTM we have the
> data we need?

Thanks for the additional detail on how this was run. I think I
still need a little more context, though:

What is the kernel on which these tests were run?

Which pg commit were these tests run against?

If 2201d801 was not included in your -1 tests, have you identified
where the 2% extra run time is going on -1 versus reverted? Since
several other threads lately have reported bigger variation than
that based on random memory alignment issues, can we confirm that
this is a real difference in what is at master's HEAD? Of course,
I'm still scheduled to test on bare metal machines in a couple
days, on two different architectures, so we'll have a few more data
points after that.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-04-13 15:21:31 -0500, Kevin Grittner wrote:
> On Wed, Apr 13, 2016 at 3:01 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > If you want me to rn some other tests I can, but ISTM we have the
> > data we need?
>
> Thanks for the additional detail on how this was run. I think I
> still need a little more context, though:
>
> What is the kernel on which these tests were run?

3.16. I can upgrade to 4.4 if necessary. But I still believe very
strongly that this is side-tracking the issue. An exclusive lock (or
spinlock) in a very hot path, which previously didn't have a specific
exclusively locked lock, will present scalability issues, regardless of
kernel.

> Which pg commit were these tests run against?

85e00470. + some reverts (the whitespace commits make this harder...) in
the reverted case.

> If 2201d801 was not included in your -1 tests, have you identified
> where the 2% extra run time is going on -1 versus reverted?

No. It's hard to do good profiles on most virtualized hardware, since
hardware performance counters are disabled. So you only can do OS
sampling; which has a pretty big performance influence.

I'm not entirely sure what you mean with "2201d801 was not included in
your -1 tests". The optimization was present.

> Since several other threads lately have reported bigger variation than
> that based on random memory alignment issues, can we confirm that this
> is a real difference in what is at master's HEAD?

It's unfortunately hard to measure this conclusively here (and in
general). I guess we'll have to look, on native hardware, where the
difference comes from. The difference is smaller on my laptop, and my
workstation is somewhere on a container ship, other physical hardware I
do not have.

Greetings,

Andres Freund

On Wed, Apr 13, 2016 at 3:47 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-04-13 15:21:31 -0500, Kevin Grittner wrote:

>> What is the kernel on which these tests were run?
>
> 3.16. I can upgrade to 4.4 if necessary.

No, I'm not aware of any problems from 3.8 on.

> But I still believe very strongly that this is side-tracking the issue.

As long as I know it isn't a broken NUMA scheduler, or that there
were fewer than four NUMA memory nodes, I consider it a non-issue.
I just need to know whether it fits that problem profile to feel
comfortable that I can interpret the results correctly.

>> Which pg commit were these tests run against?
>
> 85e00470. + some reverts (the whitespace commits make this harder...) in
> the reverted case.
>
>
>> If 2201d801 was not included in your -1 tests, have you identified
>> where the 2% extra run time is going on -1 versus reverted?
>
> No. It's hard to do good profiles on most virtualized hardware, since
> hardware performance counters are disabled. So you only can do OS
> sampling; which has a pretty big performance influence.
>
> I'm not entirely sure what you mean with "2201d801 was not included in
> your -1 tests". The optimization was present.

Sorry, the "not" was accidental -- I hate reverse logic errors like that.

Based on the commit you used, I have my answer. Thanks.

>> Since several other threads lately have reported bigger variation than
>> that based on random memory alignment issues, can we confirm that this
>> is a real difference in what is at master's HEAD?
>
> It's unfortunately hard to measure this conclusively here (and in
> general). I guess we'll have to look, on native hardware, where the
> difference comes from. The difference is smaller on my laptop, and my
> workstation is somewhere on a container ship, other physical hardware I
> do not have.

OK, thanks. I can't think of anything else to ask for at this
point. If you feel that you have enough to press for some
particular course of action, go for it. Personally, I want to do
some more investigation on those big machines.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-04-13 16:05:25 -0500, Kevin Grittner wrote:
> OK, thanks. I can't think of anything else to ask for at this
> point. If you feel that you have enough to press for some
> particular course of action, go for it.

I think we, at the very least, need a clear proposal how to resolve the
scalability issue around OldSnapshotTimeMapLock in 9.6. Personally I
think we shouldn't release with such a large regression due to a
performance oriented feature; but if we do, we need to be confident that
we can easily resolve it for 9.7. In contrast to the spinlock issue I
don't see an easy way unfortunately. Without such a plan it seems too
likely to go unfixed for a long time otherwise.

> Personally, I want to do some more investigation on those big
> machines.

Sounds good, especially around the regression with the feature disabled.

Andres

On Thu, Apr 14, 2016 at 12:23 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2016-04-13 16:05:25 -0500, Kevin Grittner wrote:
> > OK, thanks. I can't think of anything else to ask for at this
> > point. If you feel that you have enough to press for some
> > particular course of action, go for it.
>
> I think we, at the very least, need a clear proposal how to resolve the
> scalability issue around OldSnapshotTimeMapLock in 9.6. Personally I
> think we shouldn't release with such a large regression due to a
> performance oriented feature; but if we do, we need to be confident that
> we can easily resolve it for 9.7. In contrast to the spinlock issue I
> don't see an easy way unfortunately. Without such a plan it seems too
> likely to go unfixed for a long time otherwise.
>
>
> > Personally, I want to do some more investigation on those big
> > machines.
>
> Sounds good, especially around the regression with the feature disabled.
>

I've also run read-only test on 4x18 Intel machine between master and
snapshot_too_old reverted. In particular, I've reverted following commits:
8b65cf4c5edabdcae45ceaef7b9ac236879aae50
848ef42bb8c7909c9d7baa38178d4a209906e7c1
80647bf65a03e232c995c0826ef394dad8d685fe
a6f6b78196a701702ec4ff6df56c346bdcf9abd2
2201d801b03c2d1b0bce4d6580b718dc34d38b3e

I've obtained following results.

clients master sto-reverted
1 13918 12997
2 26143 26728
4 50521 52539
8 104330 103785
10 129067 132606
20 255561 255844
30 368472 371359
40 444486 450429
50 489950 497705
60 563606 564385
70 710579 718860
80 916480 934170
90 1089917 1152961
100 1201337 1240055
110 1147208 1207727
120 1116256 1167681
130 1066475 1120891
140 1040379 1085904
150 974064 1022160
160 938396 976487
170 953636 978120
180 920772 953843

We can see small but certain regression after snapshot too old feature.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Wed, Apr 13, 2016 at 03:21:31PM -0500, Kevin Grittner wrote:
> If 2201d801 was not included in your -1 tests, have you identified
> where the 2% extra run time is going on -1 versus reverted? Since
> several other threads lately have reported bigger variation than
> that based on random memory alignment issues, can we confirm that
> this is a real difference in what is at master's HEAD?

If anyone wishes to confirm that, I recommend this method:
http://www.postgresql.org/message-id/87vbitb2zp.fsf@news-spur.riddles.org.uk

PostgreSQL has not required that from contributors, though. For putative
regressions this small, we've either analyzed them theoretically or just
dismissed them.

The key judgment to finalize here is whether it's okay to release this feature
given its current effect[1], when enabled, on performance. That is more
controversial than the potential ~2% regression for old_snapshot_threshold=-1.
Alvaro[2] and Robert[3] are okay releasing that way, and Andres[4] is not. If
anyone else wants to weigh in, now is the time.

[1] http://www.postgresql.org/message-id/20160413192110.fogwesjti3kxycnu@alap3.anarazel.de
[2] http://www.postgresql.org/message-id/20160413140821.GA6568@alvherre.pgsql
[3] http://www.postgresql.org/message-id/CA+TgmoZqN0xevR+1pZ6j-99-ZCBoOphr-23tiREb+QW1Eu=KOA@mail.gmail.com
[4] http://www.postgresql.org/message-id/20160413212356.uv4velailmivnihh@alap3.anarazel.de

On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
> That is more controversial than the potential ~2% regression for
> old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing
> that way, and Andres[4] is not.

FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
a clear proposal on the table how to solve the scalability issue around
MaintainOldSnapshotTimeMapping(). Postponing the optimization around
something as trivial as a spinlock around reading an LSN is one thing,
postponing something we don't know the solution to is anohter.

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
>> That is more controversial than the potential ~2% regression for
>> old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing
>> that way, and Andres[4] is not.

> FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
> a clear proposal on the table how to solve the scalability issue around
> MaintainOldSnapshotTimeMapping(). Postponing the optimization around
> something as trivial as a spinlock around reading an LSN is one thing,
> postponing something we don't know the solution to is anohter.

The message Noah cited mentions only a 4% regression, but this one
seems far worse:

http://www.postgresql.org/message-id/20160413200148.bawmwjdmggbllhha@alap3.anarazel.de

That's more than a 5X penalty, which seems like it would make the
feature unusable; unless there is an argument that that's an extreme
case that wouldn't be representative of most real-world usage.
Which there may well be; I've not been following this thread carefully.

regards, tom lane

On 2016-04-16 17:52:44 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
> >> That is more controversial than the potential ~2% regression for
> >> old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing
> >> that way, and Andres[4] is not.
>
> > FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
> > a clear proposal on the table how to solve the scalability issue around
> > MaintainOldSnapshotTimeMapping(). Postponing the optimization around
> > something as trivial as a spinlock around reading an LSN is one thing,
> > postponing something we don't know the solution to is anohter.
>
> The message Noah cited mentions only a 4% regression, but this one
> seems far worse:
>
> http://www.postgresql.org/message-id/20160413200148.bawmwjdmggbllhha@alap3.anarazel.de
>
> That's more than a 5X penalty, which seems like it would make the
> feature unusable; unless there is an argument that that's an extreme
> case that wouldn't be representative of most real-world usage.
> Which there may well be; I've not been following this thread carefully.

The 4 % was with the feature disabled (in comparison to before it's
introduction), we're not sure where that's coming from. But the 5x - and
that was just on a mid-sized box - is with the feature enabled.

- Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-04-16 17:52:44 -0400, Tom Lane wrote:
>> That's more than a 5X penalty, which seems like it would make the
>> feature unusable; unless there is an argument that that's an extreme
>> case that wouldn't be representative of most real-world usage.
>> Which there may well be; I've not been following this thread carefully.

> The 4 % was with the feature disabled (in comparison to before it's
> introduction), we're not sure where that's coming from. But the 5x - and
> that was just on a mid-sized box - is with the feature enabled.

128 processors is a mid-sized box? Or if you didn't have 128 processors,
why are you testing "-c 128 -j 128" cases?

More seriously, the complaints here seem to center on performance in a
read-only workload; but I don't actually see why you'd want to turn on
this feature in a read-only, or even read-mostly, workload. It exists for
the benefit of people who are trying to keep their pg_xlog/ directories
reasonably sized, no? That doesn't sound very read-only-ish to me.

regards, tom lane

Hi,

On 2016-04-16 18:27:06 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2016-04-16 17:52:44 -0400, Tom Lane wrote:
> >> That's more than a 5X penalty, which seems like it would make the
> >> feature unusable; unless there is an argument that that's an extreme
> >> case that wouldn't be representative of most real-world usage.
> >> Which there may well be; I've not been following this thread carefully.
>
> > The 4 % was with the feature disabled (in comparison to before it's
> > introduction), we're not sure where that's coming from. But the 5x - and
> > that was just on a mid-sized box - is with the feature enabled.
>
> 128 processors is a mid-sized box?

It has fewer.

> Or if you didn't have 128 processors, why are you testing "-c 128 -j
> 128" cases?

I tried 128, because it's a random number I picket out of my hat. I've
posted various different client numbers elsewhere in the thread. The
machine (a VM, this isn't the best test!), has 20 cores / 40 hw threads
afaik.

But 128 connections on 40 "cpus" isn't unrealistic. Many workloads have
a lot more connections and concurrent queries than cores - besides often
being operationally easier, it's also sensible from a hardware
utilization perspective. Due to latency effects individual connections
frequently are idle; even if the client were issuing queries as fast as
possible.

> More seriously, the complaints here seem to center on performance in a
> read-only workload; but I don't actually see why you'd want to turn on
> this feature in a read-only, or even read-mostly, workload.

In a purely read-only workload it's surely pointless. But I don't see
why the results would be any benefit in a 75 read/25 write mix; which is
probably already more write heavy than a lot of the actual workloads out
there.

> It exists for
> the benefit of people who are trying to keep their pg_xlog/ directories
> reasonably sized, no? That doesn't sound very read-only-ish to me.

pg_xlog size? By my understanding it's there to cope with the bloat
introduced by longrunning readonly transactions? Isn't the idea that
"old snapshots" basically don't enforce their xmin anymore, allowing
vacuum/hot pruning? Such old snapshots continue to work as long as
they're not used to make visibility decisions about pages which have
been modified "recently". The whole feature only is interesting if such
old snapshots are likely to only access data that's not frequently
modified.

- Andres

On 4/16/16 4:44 PM, Noah Misch wrote:

> The key judgment to finalize here is whether it's okay to release this feature
> given its current effect[1], when enabled, on performance. That is more
> controversial than the potential ~2% regression for old_snapshot_threshold=-1.
> Alvaro[2] and Robert[3] are okay releasing that way, and Andres[4] is not. If
> anyone else wants to weigh in, now is the time.

I'm in favor of releasing the feature even with the performance
regression when enabled. First, there are use cases where a feature
like this is absolutely critical. Second, I don't think it will improve
and become performant without exposure to a wider audience.

I think it's entirely within the PostgreSQL philosophy to release a
feature that has warts and doesn't perform as well as we'd like as long
as it is stable and does not corrupt data.

In my opinion this feature meets these criteria and it is an important
capability to add to PostgreSQL.

--
-David
david(at)pgmasters(dot)net

> Second, I don't think it will improve and become performant without
> exposure to a wider audience.

Huh? The issue is a relatively simple to spot architectural issue
(taking a single exclusive lock during snapshot acquiration which only
needs shared locks otherwise) - I don't see how any input it's needed.
And for that matter, I don't see why such a lock got through review.

On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
> > That is more controversial than the potential ~2% regression for
> > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing
> > that way, and Andres[4] is not.
>
> FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
> a clear proposal on the table how to solve the scalability issue around
> MaintainOldSnapshotTimeMapping().
>

It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
takes EXCLUSIVE LWLock which seems to be a probable reason for a
performance regression. Now, here the question is do we need to acquire
that lock if xmin is not changed since the last time value of
oldSnapshotControl->latest_xmin is updated or xmin is lesser than equal to
oldSnapshotControl->latest_xmin?
If we don't need it for above cases, I think it can address the performance
regression to a good degree for read-only workloads when the feature is
enabled.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>
>> On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
>> > That is more controversial than the potential ~2% regression for
>> > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing
>> > that way, and Andres[4] is not.
>>
>> FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
>> a clear proposal on the table how to solve the scalability issue around
>> MaintainOldSnapshotTimeMapping().
>
> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
> takes EXCLUSIVE LWLock which seems to be a probable reason for a performance
> regression. Now, here the question is do we need to acquire that lock if
> xmin is not changed since the last time value of
> oldSnapshotControl->latest_xmin is updated or xmin is lesser than equal to
> oldSnapshotControl->latest_xmin?
> If we don't need it for above cases, I think it can address the performance
> regression to a good degree for read-only workloads when the feature is
> enabled.

Thanks, Amit -- I think something along those lines is the right
solution to the scaling issues when the feature is enabled. For
now I'm focusing on the back-patching issues and the performance
regression when the feature is disabled, but I'll shift focus to
this once the "killer" issues are in hand.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Apr 19, 2016 at 11:11 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>>
>>> On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
>>> > That is more controversial than the potential ~2% regression for
>>> > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing
>>> > that way, and Andres[4] is not.
>>>
>>> FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
>>> a clear proposal on the table how to solve the scalability issue around
>>> MaintainOldSnapshotTimeMapping().
>>
>> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
>> takes EXCLUSIVE LWLock which seems to be a probable reason for a performance
>> regression. Now, here the question is do we need to acquire that lock if
>> xmin is not changed since the last time value of
>> oldSnapshotControl->latest_xmin is updated or xmin is lesser than equal to
>> oldSnapshotControl->latest_xmin?
>> If we don't need it for above cases, I think it can address the performance
>> regression to a good degree for read-only workloads when the feature is
>> enabled.
>
> Thanks, Amit -- I think something along those lines is the right
> solution to the scaling issues when the feature is enabled. For
> now I'm focusing on the back-patching issues and the performance
> regression when the feature is disabled, but I'll shift focus to
> this once the "killer" issues are in hand.

Maybe Amit could try his idea in parallel.

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

On Tue, Apr 19, 2016 at 10:14 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Apr 19, 2016 at 11:11 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>> On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>>>
>>>> On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
>>>> > That is more controversial than the potential ~2% regression for
>>>> > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing
>>>> > that way, and Andres[4] is not.
>>>>
>>>> FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
>>>> a clear proposal on the table how to solve the scalability issue around
>>>> MaintainOldSnapshotTimeMapping().
>>>
>>> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
>>> takes EXCLUSIVE LWLock which seems to be a probable reason for a performance
>>> regression. Now, here the question is do we need to acquire that lock if
>>> xmin is not changed since the last time value of
>>> oldSnapshotControl->latest_xmin is updated or xmin is lesser than equal to
>>> oldSnapshotControl->latest_xmin?
>>> If we don't need it for above cases, I think it can address the performance
>>> regression to a good degree for read-only workloads when the feature is
>>> enabled.
>>
>> Thanks, Amit -- I think something along those lines is the right
>> solution to the scaling issues when the feature is enabled. For
>> now I'm focusing on the back-patching issues and the performance
>> regression when the feature is disabled, but I'll shift focus to
>> this once the "killer" issues are in hand.
>
> Maybe Amit could try his idea in parallel.

That would be great!

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Apr 19, 2016 at 8:44 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Tue, Apr 19, 2016 at 11:11 AM, Kevin Grittner <kgrittn(at)gmail(dot)com>
wrote:
> > On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
>
> >> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
> >> takes EXCLUSIVE LWLock which seems to be a probable reason for a
performance
> >> regression. Now, here the question is do we need to acquire that lock
if
> >> xmin is not changed since the last time value of
> >> oldSnapshotControl->latest_xmin is updated or xmin is lesser than
equal to
> >> oldSnapshotControl->latest_xmin?
> >> If we don't need it for above cases, I think it can address the
performance
> >> regression to a good degree for read-only workloads when the feature is
> >> enabled.
> >
> > Thanks, Amit -- I think something along those lines is the right
> > solution to the scaling issues when the feature is enabled. For
> > now I'm focusing on the back-patching issues and the performance
> > regression when the feature is disabled, but I'll shift focus to
> > this once the "killer" issues are in hand.
>
> Maybe Amit could try his idea in parallel.
>

Okay, will look into it.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 2016-04-19 20:27:31 +0530, Amit Kapila wrote:
> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
> > > That is more controversial than the potential ~2% regression for
> > > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing
> > > that way, and Andres[4] is not.
> >
> > FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
> > a clear proposal on the table how to solve the scalability issue around
> > MaintainOldSnapshotTimeMapping().
> >
>
> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
> takes EXCLUSIVE LWLock which seems to be a probable reason for a
> performance regression.

Yes, that's the major problem.

> Now, here the question is do we need to acquire that lock if xmin is
> not changed since the last time value of
> oldSnapshotControl->latest_xmin is updated or xmin is lesser than
> equal to oldSnapshotControl->latest_xmin? If we don't need it for
> above cases, I think it can address the performance regression to a
> good degree for read-only workloads when the feature is enabled.

I think the more fundamental issue is that the time->xid mapping is
built at GetSnapshotData() time (via MaintainOldSnapshotTimeMapping()),
and not when xids are assigned. Snapshots are created a lot more
frequently in nearly all use-cases than xids are assigned. That's what
forces the exclusive lock to be in the read path, rather than the write
path.

What's the reason for this?

Greetings,

Andres Freund

On Tue, Apr 19, 2016 at 6:11 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>>
>>> On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
>>> > That is more controversial than the potential ~2% regression for
>>> > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay releasing
>>> > that way, and Andres[4] is not.
>>>
>>> FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
>>> a clear proposal on the table how to solve the scalability issue around
>>> MaintainOldSnapshotTimeMapping().
>>
>> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
>> takes EXCLUSIVE LWLock which seems to be a probable reason for a performance
>> regression. Now, here the question is do we need to acquire that lock if
>> xmin is not changed since the last time value of
>> oldSnapshotControl->latest_xmin is updated or xmin is lesser than equal to
>> oldSnapshotControl->latest_xmin?
>> If we don't need it for above cases, I think it can address the performance
>> regression to a good degree for read-only workloads when the feature is
>> enabled.
>
> Thanks, Amit -- I think something along those lines is the right
> solution to the scaling issues when the feature is enabled. For
> now I'm focusing on the back-patching issues and the performance
> regression when the feature is disabled, but I'll shift focus to
> this once the "killer" issues are in hand.

I had an idea I wanted to test out. The gist of it is to effectively
have the last slot of timestamp to xid map stored in the latest_xmin
field and only update the mapping when slot boundaries are crossed.
See attached WIP patch for details. This way the exclusive lock only
needs to be acquired once per minute. The common case is a spinlock
that could be replaced with atomics later. And it seems to me that the
mutex_threshold taken in TestForOldSnapshot() can also get pretty hot
under some workloads, so that may also need some tweaking.

I think a better approach would be to base the whole mechanism on a
periodically updated counter, instead of timestamps. Autovacuum
launcher looks like a good candidate to play the clock keeper, without
it the feature has little point anyway. AFAICS only the clock keeper
needs to have the timestamp xid mapping, others can make do with a
couple of periodically updated values. I haven't worked it out in
detail, but it feels like the code would be simpler. But this was a
larger change than I felt comfortable trying out, so I went with the
simple change first.

However, while checking out if my proof of concept patch actually
works I hit another issue. I couldn't get my test for the feature to
actually work. The test script I used is attached. Basically I have a
table with 1000 rows, one high throughput worker deleting old rows and
inserting new ones, one long query that acquires a snapshot and sleeps
for 30min, and one worker that has a repeatable read snapshot and
periodically does count(*) on the table. Based on documentation I
would expect the following:

* The interfering query gets cancelled
* The long running query gets to run
* Old rows will start to be cleaned up after the threshold expires.

However, testing on commit 9c75e1a36b6b2f3ad9f76ae661f42586c92c6f7c,
I'm seeing that the old rows do not get cleaned up, and that I'm only
seeing the interfering query get cancelled when old_snapshot_threshold
= 0. Larger values do not result in cancellation. Am I doing something
wrong or is the feature just not working at all?

Regards,
Ants Aasma

On Wed, Apr 20, 2016 at 8:08 PM, Ants Aasma <ants(dot)aasma(at)eesti(dot)ee> wrote:

> However, while checking out if my proof of concept patch actually
> works I hit another issue. I couldn't get my test for the feature to
> actually work. The test script I used is attached.

Could you provide enough to make that a self-contained reproducible
test case (i.e., that I don't need to infer or re-write any steps
or guess how to call it)? In previous cases people have given me
where they felt that the feature wasn't working there have have
been valid reasons for it to behave as it was (e.g., a transaction
with a transaction ID and an xmin which prevented cleanup from
advancing). I'll be happy to look at your case and see whether
it's another such case or some bug, but it seems a waste to reverse
engineer or rewrite parts of the test case to do so.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Apr 21, 2016 at 5:16 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Wed, Apr 20, 2016 at 8:08 PM, Ants Aasma <ants(dot)aasma(at)eesti(dot)ee> wrote:
>
>> However, while checking out if my proof of concept patch actually
>> works I hit another issue. I couldn't get my test for the feature to
>> actually work. The test script I used is attached.
>
> Could you provide enough to make that a self-contained reproducible
> test case (i.e., that I don't need to infer or re-write any steps
> or guess how to call it)? In previous cases people have given me
> where they felt that the feature wasn't working there have have
> been valid reasons for it to behave as it was (e.g., a transaction
> with a transaction ID and an xmin which prevented cleanup from
> advancing). I'll be happy to look at your case and see whether
> it's another such case or some bug, but it seems a waste to reverse
> engineer or rewrite parts of the test case to do so.

Just to be sure I didn't have anything screwy in my build environment
I redid the test on a freshly installed Fedora 23 VM. Steps to
reproduce:

1. Build postgresql from git. I used ./configure --enable-debug
--enable-cassert --prefix=/home/ants/pg-master
2. Set up database:

cat << EOF > test-settings.conf
old_snapshot_threshold = 1min

logging_collector = on
log_directory = 'pg_log'
log_filename = 'postgresql.log'
log_line_prefix = '[%m] '
log_autovacuum_min_duration = 0
EOF

pg-master/bin/initdb data/
cat test-settings.conf >> data/postgresql.conf
pg-master/bin/pg_ctl -D data/ start
pg-master/bin/createdb

3. Install python-psycopg2 and get the test script from my earlier e-mail [1]
4. Run the test:

python test_oldsnapshot.py "host=/tmp"

5. Observe that the table keeps growing even after the old snapshot
threshold is exceeded and autovacuum has run. Autovacuum log shows 0
tuples removed.

Only the write workload has a xid assigned, the other two backends
only have snapshot held:

[ants(at)localhost ~]$ pg-master/bin/psql -c "SELECT application_name,
backend_xid, backend_xmin, NOW()-xact_start AS tx_age, state FROM
pg_stat_activity"
application_name | backend_xid | backend_xmin | tx_age |
state
----------------------+-------------+--------------+-----------------+---------------------
write-workload | 95637 | | 00:00:00.009314 | active
long-unrelated-query | | 1806 | 00:04:33.914048 | active
interfering-query | | 2444 | 00:04:32.910742 |
idle in transaction
psql | | 95637 | 00:00:00 | active

Output from the test tool attached. After killing the test tool and
the long running query autovacuum cleans stuff as expected.

I'm too tired right now to chase this down myself. The mental toll
that two small kids can take is pretty staggering. But I might find
the time to fire up a debugger sometime tomorrow.

Regards,
Ants Aasma

[1] http://www.postgresql.org/message-id/attachment/43859/test_oldsnapshot.py

On Thu, Apr 21, 2016 at 2:10 PM, Ants Aasma <ants(dot)aasma(at)eesti(dot)ee> wrote:
> On Thu, Apr 21, 2016 at 5:16 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:

>> Could you provide enough to make that a self-contained
>> reproducible test case [?]

> [provided]

Thanks! I have your test case running, and it is not immediately
clear why old rows are not being vacuumed away. Will investigate.

> I'm too tired right now to chase this down myself. The mental
> toll that two small kids can take is pretty staggering.

Been there, done that; so I know just what you mean. :-) It is
rewarding though, eh?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Apr 21, 2016 at 4:13 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:

> I have your test case running, and it is not immediately
> clear why old rows are not being vacuumed away.

I have not found the reason that the vacuuming is not as aggressive
as it should be with this old_snapshot_threshold, but I left your
test case running overnight and found that it eventually did kick
in. So the question is why it was not nearly as aggressive as one
would expect.

From the server log:

kgrittn(at)Kevin-Desktop:~/pg/master$ grep -B2 -A3 'tuples: [1-9]'
Debug/data/pg_log/postgresql.log
[2016-04-21 16:21:29.658 CDT] LOG: automatic vacuum of table
"kgrittn.public.high_throughput": index scans: 1
pages: 0 removed, 2759 remain, 0 skipped due to pins, 0 skipped frozen
tuples: 94 removed, 159928 remain, 158935 are dead but not yet removable
buffer usage: 6005 hits, 0 misses, 8 dirtied
avg read rate: 0.000 MB/s, avg write rate: 0.090 MB/s
system usage: CPU 0.00s/0.08u sec elapsed 0.69 sec
--
[2016-04-21 16:55:31.971 CDT] LOG: automatic vacuum of table
"kgrittn.pg_catalog.pg_statistic": index scans: 1
pages: 0 removed, 23 remain, 0 skipped due to pins, 0 skipped frozen
tuples: 2 removed, 515 remain, 128 are dead but not yet removable
buffer usage: 50 hits, 11 misses, 14 dirtied
avg read rate: 4.048 MB/s, avg write rate: 5.152 MB/s
system usage: CPU 0.00s/0.00u sec elapsed 0.02 sec
--
[2016-04-22 00:33:11.978 CDT] LOG: automatic vacuum of table
"kgrittn.pg_catalog.pg_statistic": index scans: 1
pages: 0 removed, 68 remain, 0 skipped due to pins, 0 skipped frozen
tuples: 1016 removed, 409 remain, 22 are dead but not yet removable
buffer usage: 89 hits, 127 misses, 111 dirtied
avg read rate: 1.478 MB/s, avg write rate: 1.292 MB/s
system usage: CPU 0.00s/0.00u sec elapsed 0.67 sec
[2016-04-22 00:33:18.572 CDT] LOG: automatic vacuum of table
"kgrittn.public.high_throughput": index scans: 1
pages: 0 removed, 20196 remain, 0 skipped due to pins, 0 skipped frozen
tuples: 292030 removed, 3941 remain, 3553 are dead but not yet removable
buffer usage: 68541 hits, 14415 misses, 20638 dirtied
avg read rate: 1.674 MB/s, avg write rate: 2.396 MB/s
system usage: CPU 0.23s/1.09u sec elapsed 67.29 sec
--
[2016-04-22 00:52:13.013 CDT] LOG: automatic vacuum of table
"kgrittn.public.high_throughput": index scans: 1
pages: 0 removed, 20233 remain, 0 skipped due to pins, 19575
skipped frozen
tuples: 8463 removed, 30533 remain, 28564 are dead but not yet removable
buffer usage: 8136 hits, 4 misses, 158 dirtied
avg read rate: 0.027 MB/s, avg write rate: 1.065 MB/s
system usage: CPU 0.00s/0.03u sec elapsed 1.15 sec
--
[2016-04-22 01:28:22.812 CDT] LOG: automatic vacuum of table
"kgrittn.pg_catalog.pg_statistic": index scans: 1
pages: 0 removed, 68 remain, 0 skipped due to pins, 44 skipped frozen
tuples: 26 removed, 760 remain, 108 are dead but not yet removable
buffer usage: 37 hits, 27 misses, 12 dirtied
avg read rate: 4.963 MB/s, avg write rate: 2.206 MB/s
system usage: CPU 0.00s/0.00u sec elapsed 0.04 sec
--
[2016-04-22 06:51:23.042 CDT] LOG: automatic vacuum of table
"kgrittn.pg_catalog.pg_statistic": index scans: 1
pages: 0 removed, 68 remain, 0 skipped due to pins, 0 skipped frozen
tuples: 692 removed, 403 remain, 16 are dead but not yet removable
buffer usage: 90 hits, 109 misses, 76 dirtied
avg read rate: 1.646 MB/s, avg write rate: 1.148 MB/s
system usage: CPU 0.00s/0.00u sec elapsed 0.51 sec
[2016-04-22 06:52:45.174 CDT] LOG: automatic vacuum of table
"kgrittn.public.high_throughput": index scans: 1
pages: 0 removed, 28152 remain, 0 skipped due to pins, 0 skipped frozen
tuples: 928116 removed, 14021 remain, 14021 are dead but not
yet removable
buffer usage: 88738 hits, 33068 misses, 45857 dirtied
avg read rate: 1.811 MB/s, avg write rate: 2.511 MB/s
system usage: CPU 0.43s/1.93u sec elapsed 142.68 sec
--
[2016-04-22 06:53:23.665 CDT] LOG: automatic vacuum of table
"kgrittn.public.high_throughput": index scans: 1
pages: 0 removed, 28313 remain, 0 skipped due to pins, 27853
skipped frozen
tuples: 43 removed, 9002 remain, 8001 are dead but not yet removable
buffer usage: 9795 hits, 22 misses, 28 dirtied
avg read rate: 0.159 MB/s, avg write rate: 0.202 MB/s
system usage: CPU 0.00s/0.03u sec elapsed 1.08 sec
--
[2016-04-22 07:22:25.240 CDT] LOG: automatic vacuum of table
"kgrittn.public.high_throughput": index scans: 1
pages: 0 removed, 28313 remain, 0 skipped due to pins, 25627
skipped frozen
tuples: 51 removed, 149639 remain, 147733 are dead but not yet removable
buffer usage: 14227 hits, 18 misses, 15 dirtied
avg read rate: 0.089 MB/s, avg write rate: 0.074 MB/s
system usage: CPU 0.01s/0.10u sec elapsed 1.58 sec

From the output stream of the python test script:

00:32:00] Counted 1000 rows with max 1094 in high_throughput table
[00:32:05] High throughput table size @ 31240s. Size 161624kB Last
vacuum 0:00:42.664285 ago
[00:32:10] Interfering query got error snapshot too old

[00:32:10] Waiting 3min to restart interfering query

[06:50:00] Counted 1000 rows with max 1176984 in high_throughput table
[06:50:05] High throughput table size @ 53920s. Size 225088kB Last
vacuum 0:00:23.685084 ago
[06:50:10] Interfering query got error snapshot too old

[06:50:10] Waiting 3min to restart interfering query

I don't see any evidence that it returned incorrect query results
at any point, and it did eventually limit bloat; what we have is
that it is not limiting it nearly as tightly as one would expect in
this particular test case.

I'm continuing to investigate.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Apr 20, 2016 at 7:39 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> On 2016-04-19 20:27:31 +0530, Amit Kapila wrote:
> > On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres(at)anarazel(dot)de>
wrote:
> > >
> > > On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
> > > > That is more controversial than the potential ~2% regression for
> > > > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay
releasing
> > > > that way, and Andres[4] is not.
> > >
> > > FWIW, I could be kinda convinced that it's temporarily ok, if there'd
be
> > > a clear proposal on the table how to solve the scalability issue
around
> > > MaintainOldSnapshotTimeMapping().
> > >
> >
> > It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
> > takes EXCLUSIVE LWLock which seems to be a probable reason for a
> > performance regression.
>
> Yes, that's the major problem.
>
>
> > Now, here the question is do we need to acquire that lock if xmin is
> > not changed since the last time value of
> > oldSnapshotControl->latest_xmin is updated or xmin is lesser than
> > equal to oldSnapshotControl->latest_xmin? If we don't need it for
> > above cases, I think it can address the performance regression to a
> > good degree for read-only workloads when the feature is enabled.
>
> I think the more fundamental issue is that the time->xid mapping is
> built at GetSnapshotData() time (via MaintainOldSnapshotTimeMapping()),
> and not when xids are assigned. Snapshots are created a lot more
> frequently in nearly all use-cases than xids are assigned. That's what
> forces the exclusive lock to be in the read path, rather than the write
> path.
>
> What's the reason for this?
>

I don't see any particular reason for doing so, but not sure if it will be
beneficial in all kind of cases if we build that mapping when xids are
assigned. As an example, consider the case where couple of write
transactions start at same time and immediately after that a read statement
is executed, now for all-those write-transactions we need to take Exclusive
lock to build an oldsnaphot entry, whereas with the above optimization
suggested by me, it needs to take Exclusive lock just once for
read-statement.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 21, 2016 at 6:38 AM, Ants Aasma <ants(dot)aasma(at)eesti(dot)ee> wrote:
>
> On Tue, Apr 19, 2016 at 6:11 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> > On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> >> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres(at)anarazel(dot)de>
wrote:
> >>>
> >>> FWIW, I could be kinda convinced that it's temporarily ok, if there'd
be
> >>> a clear proposal on the table how to solve the scalability issue
around
> >>> MaintainOldSnapshotTimeMapping().
> >>
> >> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
> >> takes EXCLUSIVE LWLock which seems to be a probable reason for a
performance
> >> regression. Now, here the question is do we need to acquire that lock
if
> >> xmin is not changed since the last time value of
> >> oldSnapshotControl->latest_xmin is updated or xmin is lesser than
equal to
> >> oldSnapshotControl->latest_xmin?
> >> If we don't need it for above cases, I think it can address the
performance
> >> regression to a good degree for read-only workloads when the feature is
> >> enabled.
> >
> > Thanks, Amit -- I think something along those lines is the right
> > solution to the scaling issues when the feature is enabled. For
> > now I'm focusing on the back-patching issues and the performance
> > regression when the feature is disabled, but I'll shift focus to
> > this once the "killer" issues are in hand.
>
> I had an idea I wanted to test out. The gist of it is to effectively
> have the last slot of timestamp to xid map stored in the latest_xmin
> field and only update the mapping when slot boundaries are crossed.
> See attached WIP patch for details. This way the exclusive lock only
> needs to be acquired once per minute.
>

Why at all do we need to acquire Exclusive lock if xmin is not changing at
all? Also, I think your proposed patch can effect the update of xid's for
existing mappings. In particular, I am talking about below code:

else if (ts <= (oldSnapshotControl->head_timestamp
+((oldSnapshotControl->count_used - 1)* USECS_PER_MINUTE)))

{

/* existing mapping; advance xid if possible */

After your patch, it might skip the update to existing mappings which
doesn't seem to be a good thing.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 19, 2016 at 8:41 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>
> On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres(at)anarazel(dot)de>
wrote:
> >>
> >> On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
> >> > That is more controversial than the potential ~2% regression for
> >> > old_snapshot_threshold=-1. Alvaro[2] and Robert[3] are okay
releasing
> >> > that way, and Andres[4] is not.
> >>
> >> FWIW, I could be kinda convinced that it's temporarily ok, if there'd
be
> >> a clear proposal on the table how to solve the scalability issue around
> >> MaintainOldSnapshotTimeMapping().
> >
> > It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
> > takes EXCLUSIVE LWLock which seems to be a probable reason for a
performance
> > regression. Now, here the question is do we need to acquire that lock
if
> > xmin is not changed since the last time value of
> > oldSnapshotControl->latest_xmin is updated or xmin is lesser than equal
to
> > oldSnapshotControl->latest_xmin?
> > If we don't need it for above cases, I think it can address the
performance
> > regression to a good degree for read-only workloads when the feature is
> > enabled.
>
> Thanks, Amit -- I think something along those lines is the right
> solution to the scaling issues when the feature is enabled.
>

I have tried attached patch along the above lines and it seems that it
addresses performance regression to a good degree when feature is enabled
at moderate client-count like 32, but still more needs to be done for
somewhat higher client-count like 64.

Performance data is for median of 3, 5 min runs of read-only workload -
pgbench -c $client_count -j $client_count -T 300 -M prepared -S postgres

o_s_t - old_snapshot_threshold

Client_Count/Patch_Ver 32 64
HEAD (o_s_t = -1) 354077 552063
HEAD (o_s_t = 1) 92809 55847
Patch (o_s_t = 1) 319759 191741

If you think that attached patch is correct functionality wise, then I
think we can go-ahead with it and then investigate what more can be
improved. I think newly introduced spinlocks might be the reason of
performance degradation at higher client-count, if that turns out to be
true, then I think we can replace them with atomics, once Andres's patch
for completing the 64-bit atomics implementation is committed.

m/c details used for performance testing
Architecture: ppc64le
Byte Order: Little Endian
CPU(s): 192
On-line CPU(s) list: 0-191
Thread(s) per core: 8
Core(s) per socket: 1
Socket(s): 24
NUMA node(s): 4
Model: IBM,8286-42A
L1d cache: 64K
L1i cache: 32K
L2 cache: 512K
L3 cache: 8192K
NUMA node0 CPU(s): 0-47
NUMA node1 CPU(s): 48-95
NUMA node2 CPU(s): 96-143
NUMA node3 CPU(s): 144-191

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

What is the recommended procedure for replying to a pgsql-committers
messsage? Is cross-posting to hackers really the right approach, as it
causes duplicate messages. (pgsql-committers CC removed.)

---------------------------------------------------------------------------

On Tue, Apr 12, 2016 at 10:38:56AM -0700, Andres Freund wrote:
> On 2016-04-12 16:49:25 +0000, Kevin Grittner wrote:
> > 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.
>
> FWIW, I could see massive regressions with just 64 connections.
>
> I'm a bit scared of having an innoccuous sounding option regress things
> by a factor of 10. I think, in addition to this fix, we need to actually
> solve the scalability issue here to a good degree. One way to do so is
> to apply the parts of 0001 in
> http://archives.postgresql.org/message-id/20160330230914.GH13305%40awork2.anarazel.de
> defining PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY and rely on that. Another
> to apply the whole patch and simply put the lsn in an 8 byte atomic.
>
> - Andres
>
>
> --
> Sent via pgsql-committers mailing list (pgsql-committers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-committers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

Bruce Momjian wrote:
>
> What is the recommended procedure for replying to a pgsql-committers
> messsage? Is cross-posting to hackers really the right approach, as it
> causes duplicate messages. (pgsql-committers CC removed.)

CCing pgsql-hackers when replying to -committers was discussed some
time ago and the consensus seemed to be that that's preferrable than
keeping the discussion in -committers only, because that one is so much
smaller. Whether -committers is kept in cc/to or not seems not to be
important. This is all in the archives somewhere ...

You must be new to this email thing.

Any millennial will tell you that there's no duplicate because Gmail
already de-duplicates them. You would only see one copy, ever.

Now, if you still live in a cave and don't use Gmail (like, say, me),
you could still change the options in Majordomo to send a unique copy of
each message, that is to say change the delivery class to "unique"
rather than "each". Then it will see that you have the same message in
two lists and send you only one.

Now, whenever you're in the CC list of a message (something which I'm
told is somewhat common around here) you would additionally get that
copy too! There's nothing majordomo could do about that of course.
Again the solution is to use Gmail (what else!). If you won't do that,
you can install a procmail recipe to remove the dupes, say

:0 Wh: msgid.lock
| formail -D 65536 $HOME/.msgid.cache

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

On 04/29/2016 10:20 AM, Alvaro Herrera wrote:

> :0 Wh: msgid.lock
> | formail -D 65536 $HOME/.msgid.cache
>

And Alvaro drowns in the drool of his own sarcasm.

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

Joshua D. Drake wrote:
> On 04/29/2016 10:20 AM, Alvaro Herrera wrote:
>
> >:0 Wh: msgid.lock
> >| formail -D 65536 $HOME/.msgid.cache
>
> And Alvaro drowns in the drool of his own sarcasm.

I was going to add an apology that this wasn't supposed to be insulting,
only funny, but refrained. There was no offense meant.

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

On Fri, Apr 29, 2016 at 02:49:38PM -0300, Alvaro Herrera wrote:
> Joshua D. Drake wrote:
> > On 04/29/2016 10:20 AM, Alvaro Herrera wrote:
> >
> > >:0 Wh: msgid.lock
> > >| formail -D 65536 $HOME/.msgid.cache
> >
> > And Alvaro drowns in the drool of his own sarcasm.
>
> I was going to add an apology that this wasn't supposed to be insulting,
> only funny, but refrained. There was no offense meant.

I took it as humorous. I didn't turn on duplicate removal on Majordomo
so I could police people who cross-posted unnecessarily. Are you saying
I shouldn't worry about that and just turn on duplicate removal?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

Bruce Momjian wrote:
> On Fri, Apr 29, 2016 at 02:49:38PM -0300, Alvaro Herrera wrote:
> > Joshua D. Drake wrote:
> > > On 04/29/2016 10:20 AM, Alvaro Herrera wrote:
> > >
> > > >:0 Wh: msgid.lock
> > > >| formail -D 65536 $HOME/.msgid.cache
> > >
> > > And Alvaro drowns in the drool of his own sarcasm.
> >
> > I was going to add an apology that this wasn't supposed to be insulting,
> > only funny, but refrained. There was no offense meant.
>
> I took it as humorous.

Great, thanks.

> I didn't turn on duplicate removal on Majordomo so I could police
> people who cross-posted unnecessarily. Are you saying I shouldn't
> worry about that and just turn on duplicate removal?

Yeah, I've done that for years. I notice duplicates by looking at CCs
anyway. Note that our archives system is perfectly prepared to deal
with messages cross-posted to several mailing lists, as a first-class
feature. I don't think there's a strong need to police them
specifically.

If consensus is that we should completely forbid cross-posting, we can
talk about that.

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

On 2016-04-29 15:27:15 -0300, Alvaro Herrera wrote:
> If consensus is that we should completely forbid cross-posting, we can
> talk about that.

I find xposts rather useful. WRT committers vs. hackers thing, I'll
e.g. be far more likely to be able to keep up with committers than
hackers.

Andres

On Fri, Apr 29, 2016 at 02:20:40PM -0300, Alvaro Herrera wrote:
> Now, if you still live in a cave and don't use Gmail (like, say, me),
> you could still change the options in Majordomo to send a unique copy of
> each message, that is to say change the delivery class to "unique"
> rather than "each". Then it will see that you have the same message in
> two lists and send you only one.

OK, I set every Majordomo email list to "each unduplicated message" ---
who says I am behind the times. :-)

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On Fri, Apr 22, 2016 at 8:06 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Thu, Apr 21, 2016 at 4:13 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>
>> I have your test case running, and it is not immediately
>> clear why old rows are not being vacuumed away.
>
> I have not found the reason that the vacuuming is not as aggressive
> as it should be with this old_snapshot_threshold, but I left your
> test case running overnight and found that it eventually did kick
> in. So the question is why it was not nearly as aggressive as one
> would expect.

Once I found it, it turned out to be a bit of a "forehead-slapper".
Because the array of entries mapping time to TransactionId was
exactly the same size as old_snapshot_threshold, the override of
the xmin for pruning or vacuum would not be seen if another
transaction got in fast enough, and this python test case was
pounding hard enough that the override was rarely seen. By
expanding the array by 10 entries, we will only miss the more
aggressive cleanup if the thread stalls at that point for more than
10 minutes, which seems like a reasonable degree of patience, given
that there is no correctness problem if that does happen.

Ants, I think you'll find your test case behaving as you expected
now.

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.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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.

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

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.

Andres

On Mon, May 2, 2016 at 10:21 AM, 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.

You are certainly welcome to add a new open item to cover those
complaints. But I do not want to blur together the discussion of
whether the feature is well-designed with the question of whether it
regresses performance when it is turned off. Those are severable
issues, meriting separate discussion (and probably separate threads).

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

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

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.

Regards,
Ants Aasma

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

On Mon, May 2, 2016 at 07:21:21AM -0700, Andres Freund 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.

I also strongly question whether we should revert this feature and try
again in 9.7.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +

On 2016-05-02 10:32:28 -0400, Robert Haas wrote:
> You are certainly welcome to add a new open item to cover those
> complaints.

Done that.

> But I do not want to blur together the discussion of
> whether the feature is well-designed with the question of whether it
> regresses performance when it is turned off. Those are severable
> issues, meriting separate discussion (and probably separate threads).

The current thread already contains a lot of information about both, so
separating doesn't seem beneficial.

Greetings,

Andres Freund

On Wed, Apr 20, 2016 at 8:08 PM, Ants Aasma <ants(dot)aasma(at)eesti(dot)ee> wrote:

> I had an idea I wanted to test out. The gist of it is to effectively
> have the last slot of timestamp to xid map stored in the latest_xmin
> field and only update the mapping when slot boundaries are crossed.
> See attached WIP patch for details. This way the exclusive lock only
> needs to be acquired once per minute. The common case is a spinlock
> that could be replaced with atomics later.

I rebased the patch Ants posted (attached), and am running
benchmarks on a cthulhu (a big NUMA machine with 8 memory nodes).
Normally I wouldn't post results without a lot more data points
with multiple samples at each, but the initial results have me
wondering whether people would like to see this pushed later today
so that it has some time in the buildfarm and then into beta1.

Running the r/w TPC-B (sort of) load with scale, jobs, and threads
at 1000, and the database configured as I would for a production
server of that size, preliminary TPS results are:

master, -1: 8158
master, 10min: 2019
Ants' patch, 10min: 7804

Basically it just skips the maintenance of the time/xid mapping
unless current time has advanced to a new minute.

I can see arguments for tuning this far in time for the beta, as
well as the argument to wait until after the beta, so I'm just
throwing it out there to see what other people think. I wouldn't
do it unless I have three runs at -1 and 10min with the patch, all
showing similar numbers. If the BF chokes on it I would revert
this optimization attempt.

Thoughts?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2016-05-06 14:18:22 -0500, Kevin Grittner wrote:
> I rebased the patch Ants posted (attached), and am running
> benchmarks on a cthulhu (a big NUMA machine with 8 memory nodes).
> Normally I wouldn't post results without a lot more data points
> with multiple samples at each, but the initial results have me
> wondering whether people would like to see this pushed later today
> so that it has some time in the buildfarm and then into beta1.

I think that generally would make sense. We quite possibly need some
further changes, but it seems more likely that we can find them if the
patch runs close to the disabled performance.

> Running the r/w TPC-B (sort of) load with scale, jobs, and threads
> at 1000, and the database configured as I would for a production
> server of that size, preliminary TPS results are:
>
> master, -1: 8158
> master, 10min: 2019
> Ants' patch, 10min: 7804

That's rather nice. Did you test read-only as well?

If you'd feel more comfortable committing after I've run some
performance tests, I could kick off some soon.

> I can see arguments for tuning this far in time for the beta, as
> well as the argument to wait until after the beta, so I'm just
> throwing it out there to see what other people think. I wouldn't
> do it unless I have three runs at -1 and 10min with the patch, all
> showing similar numbers. If the BF chokes on it I would revert
> this optimization attempt.

+1 for going forward. I'm still doubtful that it's a good idea to the
map maintenance from GetSnapshotData(), but the issue becomes much less
severe when addressed like this.

The primary reasons why I'd like to move it is because of the
significant amount of added gettimeofday() calls which are a real hog in
some virtualized environments, and because I'm doubtful of tying the
current time to the xmin horizon.

> diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
> index e1551a3..b7d965a 100644
> --- a/src/backend/utils/time/snapmgr.c
> +++ b/src/backend/utils/time/snapmgr.c
> @@ -80,8 +80,11 @@ typedef struct OldSnapshotControlData
> */
> slock_t mutex_current; /* protect current timestamp */
> int64 current_timestamp; /* latest snapshot timestamp */
> - slock_t mutex_latest_xmin; /* protect latest snapshot xmin */
> + slock_t mutex_latest_xmin; /* protect latest snapshot xmin
> + * and next_map_update
> + */
> TransactionId latest_xmin; /* latest snapshot xmin */
> + int64 next_map_update; /* latest snapshot valid up to */
> slock_t mutex_threshold; /* protect threshold fields */
> int64 threshold_timestamp; /* earlier snapshot is old */
> TransactionId threshold_xid; /* earlier xid may be gone */

Overly nitpickily I'd refer to the actual variable name (instead of
"latest snapshot xmin") in the mutex_latest_xmin comment.

> if (!same_ts_as_threshold)
> {
> + if (ts == update_ts)
> + {
> + xlimit = latest_xmin;
> + if (NormalTransactionIdFollows(xlimit, recentXmin))
> + SetOldSnapshotThresholdTimestamp(ts, xlimit);
> + }
> + else
> + {
> LWLockAcquire(OldSnapshotTimeMapLock, LW_SHARED);
>
> if (oldSnapshotControl->count_used > 0

I guess it's just an issue in my mail-reader, but the indentation looks
funky here.

Looks roughly sensible.

Greetings,

Andres Freund

On Fri, May 6, 2016 at 5:07 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2016-05-06 14:18:22 -0500, Kevin Grittner wrote:
>> I rebased the patch Ants posted (attached), and am running
>> benchmarks on a cthulhu (a big NUMA machine with 8 memory nodes).
>> Normally I wouldn't post results without a lot more data points
>> with multiple samples at each, but the initial results have me
>> wondering whether people would like to see this pushed later today
>> so that it has some time in the buildfarm and then into beta1.
>
> I think that generally would make sense. We quite possibly need some
> further changes, but it seems more likely that we can find them if the
> patch runs close to the disabled performance.

ok

>> Running the r/w TPC-B (sort of) load with scale, jobs, and threads
>> at 1000, and the database configured as I would for a production
>> server of that size, preliminary TPS results are:
>>
>> master, -1: 8158
>> master, 10min: 2019
>> Ants' patch, 10min: 7804
>
> That's rather nice. Did you test read-only as well?

Not yet. I don't trust short runs, so I've been going with -T2400;
with setup times and so on, that limits me to one run per hour of
time I book the machine, and I'm competing with others for that. I
do plan to run read-only, too.

From the 40 minute tests so far with Ants' patch (alternating settings):

old_snapshot_threshold = 10
7804
9524
9512

old_snapshot_threshold = -1
10421
8691
8977

It's disappointing that I am not getting more consistent numbers,
but NUMA can be hard to manage that way.

> If you'd feel more comfortable committing after I've run some
> performance tests, I could kick off some soon.

I think I should get it onto the buildfarm if we're going for
beta2, so there's time to recognize any problem (unlikely as that
*seems*) and back this out before beta if needed. That said, all
additional data points welcome!

>> I can see arguments for tuning this far in time for the beta, as
>> well as the argument to wait until after the beta, so I'm just
>> throwing it out there to see what other people think. I wouldn't
>> do it unless I have three runs at -1 and 10min with the patch, all
>> showing similar numbers. If the BF chokes on it I would revert
>> this optimization attempt.
>
> +1 for going forward. I'm still doubtful that it's a good idea to the
> map maintenance from GetSnapshotData(), but the issue becomes much less
> severe when addressed like this.
>
> The primary reasons why I'd like to move it is because of the
> significant amount of added gettimeofday() calls which are a real hog in
> some virtualized environments, and because I'm doubtful of tying the
> current time to the xmin horizon.

When I initially presented the proof of concept patch during the
9.5 development cycle it was based on transaction counts, and that
was the biggest criticism, and it came from many quarters. Using
time was the big demand from just about everyone, and I'm not sure
how you do that without a mapping of time to xmin horizon. If you
have some other idea, I'm all ears.

> Looks roughly sensible.

Will push shortly with the nit-pick fixes you requested.

Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-05-06 19:43:24 -0500, Kevin Grittner wrote:
> It's disappointing that I am not getting more consistent numbers,
> but NUMA can be hard to manage that way.

FWIW, in my experience, unless you disable autovacuum (or rather
auto-analyze), the effects from non-predicable analyze runs with
long-running snapshots are worse. I mean the numa effects suck, but in
r/w workload effects of analyze are often much worse.

That comment reminds me of a question I had: Did you consider the effect
of this patch on analyze? It uses a snapshot, and by memory you've not
built in a defense against analyze being cancelled.

> Will push shortly with the nit-pick fixes you requested.

Cool.

Greetings,

Andres Freund

On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-05-06 19:43:24 -0500, Kevin Grittner wrote:
>> It's disappointing that I am not getting more consistent numbers,
>> but NUMA can be hard to manage that way.
>
> FWIW, in my experience, unless you disable autovacuum (or rather
> auto-analyze), the effects from non-predicable analyze runs with
> long-running snapshots are worse. I mean the numa effects suck, but in
> r/w workload effects of analyze are often much worse.

Hm. But the benefits of the patch are not there if autovacuum is
off. I'm gonna need to ponder the best way to test given all that.

> That comment reminds me of a question I had: Did you consider the effect
> of this patch on analyze? It uses a snapshot, and by memory you've not
> built in a defense against analyze being cancelled.

Will need to check on that.

>> Will push shortly with the nit-pick fixes you requested.
>
> Cool.

Done.

I will be checking in on the buildfarm, but if it starts causing
problems while I'm, say, sleeping -- I won't be offended if someone
else reverts 7e3da1c4737fd6582e12c80983987e4d2cbc1d17.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-05-06 20:28:27 -0500, Kevin Grittner wrote:
> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-05-06 19:43:24 -0500, Kevin Grittner wrote:
> >> It's disappointing that I am not getting more consistent numbers,
> >> but NUMA can be hard to manage that way.
> >
> > FWIW, in my experience, unless you disable autovacuum (or rather
> > auto-analyze), the effects from non-predicable analyze runs with
> > long-running snapshots are worse. I mean the numa effects suck, but in
> > r/w workload effects of analyze are often much worse.
>
> Hm. But the benefits of the patch are not there if autovacuum is
> off. I'm gonna need to ponder the best way to test given all that.

It's sufficient to set the threshhold for analyze very high, as vacuum
itself doesn't have that problem. I basically just set
autovacuum_analyze_threshold to INT_MAX , that alleviates the problem
for normal runs.

Andres

On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

>> That comment reminds me of a question I had: Did you consider the effect
>> of this patch on analyze? It uses a snapshot, and by memory you've not
>> built in a defense against analyze being cancelled.
>
> Will need to check on that.

With a 1min threshold, after loading a table "v" with a million
rows, beginning a repeatable read transaction on a different
connection and opening a cursor against that table, deleting almost
all rows on the original connection, and waiting a few minutes I
see this in the open transaction:

test=# analyze verbose v;
INFO: analyzing "public.v"
INFO: "v": scanned 4425 of 4425 pages, containing 1999 live rows and
0 dead rows; 1999 rows in sample, 1999 estimated total rows
ANALYZE
test=# select count(*) from v;
ERROR: snapshot too old

Meanwhile, no errors appeared in the log from autovacuum.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote:
> On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> > On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> >> That comment reminds me of a question I had: Did you consider the effect
> >> of this patch on analyze? It uses a snapshot, and by memory you've not
> >> built in a defense against analyze being cancelled.
> >
> > Will need to check on that.
>
> With a 1min threshold, after loading a table "v" with a million
> rows, beginning a repeatable read transaction on a different
> connection and opening a cursor against that table, deleting almost
> all rows on the original connection, and waiting a few minutes I
> see this in the open transaction:
>
> test=# analyze verbose v;
> INFO: analyzing "public.v"
> INFO: "v": scanned 4425 of 4425 pages, containing 1999 live rows and
> 0 dead rows; 1999 rows in sample, 1999 estimated total rows
> ANALYZE
> test=# select count(*) from v;
> ERROR: snapshot too old
>
> Meanwhile, no errors appeared in the log from autovacuum.

I'd guess that that problem could only be reproduced if autoanalyze
takes longer than the timeout, and there's rows pruned after it has
started? Analyze IIRC acquires a new snapshot when getting sample rows,
so it'll not necessarily trigger in the above scenario, right?

Is there anything preventing this from becoming a problem?

Greetings,

Andres Freund

On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> Analyze IIRC acquires a new snapshot when getting sample rows,

I could not find anything like that, and a case-insensitive search
of analyze.c finds no occurrences of "snap". Can you remember
where you think you saw something that would cause the ANALYZE
command in my test to use a snapshot other than the one from the
REPEATABLE READ transaction in which it was run?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner wrote:
> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > Analyze IIRC acquires a new snapshot when getting sample rows,
>
> I could not find anything like that, and a case-insensitive search
> of analyze.c finds no occurrences of "snap". Can you remember
> where you think you saw something that would cause the ANALYZE
> command in my test to use a snapshot other than the one from the
> REPEATABLE READ transaction in which it was run?

For ANALYZE, the snapshot is set in vacuum() prior to calling
analyze_rel(); see vacuum.c.

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

Kevin Grittner <kgrittn(at)gmail(dot)com> writes:
> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> Analyze IIRC acquires a new snapshot when getting sample rows,

> I could not find anything like that, and a case-insensitive search
> of analyze.c finds no occurrences of "snap". Can you remember
> where you think you saw something that would cause the ANALYZE
> command in my test to use a snapshot other than the one from the
> REPEATABLE READ transaction in which it was run?

The control logic concerned with that is in vacuum.c, not analyze.c.
AFAICS a manual ANALYZE that's within a transaction block would use
the prevailing snapshot (see in_outer_xact tests). There are a lot
of cases that wouldn't, though.

regards, tom lane

On 2016-05-24 13:04:09 -0500, Kevin Grittner wrote:
> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > Analyze IIRC acquires a new snapshot when getting sample rows,
>
> I could not find anything like that, and a case-insensitive search
> of analyze.c finds no occurrences of "snap". Can you remember
> where you think you saw something that would cause the ANALYZE
> command in my test to use a snapshot other than the one from the
> REPEATABLE READ transaction in which it was run?

It's outside of analyze.c:

autovacuum_do_vac_analyze() -> vacuum() ->
if (options & VACOPT_VACUUM)
use_own_xacts = true;
else
{
Assert(options & VACOPT_ANALYZE);
if (IsAutoVacuumWorkerProcess())
use_own_xacts = true;
...
if (options & VACOPT_ANALYZE)
{
/*
* If using separate xacts, start one for analyze. Otherwise,
* we can use the outer transaction.
*/
if (use_own_xacts)
{
StartTransactionCommand();
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
}

analyze_rel(relid, relation, options, params,
va_cols, in_outer_xact, vac_strategy);

if (use_own_xacts)
{
PopActiveSnapshot();
CommitTransactionCommand();
}
}

Andres

On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote:
>> On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>
>>>> That comment reminds me of a question I had: Did you consider the effect
>>>> of this patch on analyze? It uses a snapshot, and by memory you've not
>>>> built in a defense against analyze being cancelled.

The primary defense is not considering a cancellation except in 30
of the 500 places a page is used. None of those 30 are, as far as
I can see (upon review in response to your question), used in the
analyze process.

>> With a 1min threshold, after loading a table "v" with a million
>> rows, beginning a repeatable read transaction on a different
>> connection and opening a cursor against that table, deleting almost
>> all rows on the original connection, and waiting a few minutes I
>> see this in the open transaction:
>>
>> test=# analyze verbose v;
>> INFO: analyzing "public.v"
>> INFO: "v": scanned 4425 of 4425 pages, containing 1999 live rows and
>> 0 dead rows; 1999 rows in sample, 1999 estimated total rows
>> ANALYZE
>> test=# select count(*) from v;
>> ERROR: snapshot too old
>>
>> Meanwhile, no errors appeared in the log from autovacuum.
>
> I'd guess that that problem could only be reproduced if autoanalyze
> takes longer than the timeout, and there's rows pruned after it has
> started? Analyze IIRC acquires a new snapshot when getting sample rows,
> so it'll not necessarily trigger in the above scenario, right?

Per Tom's recollection and my review of the code, the transaction
snapshot would be used in the test I show above, and the
combination of the verbose output the subsequent query show clearly
that if one of the page references capable of throwing the error
were encountered with this snapshot, the error would be thrown. So
at least in this ANALYZE run, there is empirical support for what I
found in reviewing the code -- none of the places where we check
for an old snapshot are exercised during an ANALYZE.

> Is there anything preventing this from becoming a problem?

The fundamental approach that the error can only appear on
user-facing scans, not internal reads and positioning.

Unless there is some code path that uses a scan where the snapshot
age is checked, this cannot be a problem. I don't see any such
path, but if you do, please let me know, and I'll fix things.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner wrote:
> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote:
> >> On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> >>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >>
> >>>> That comment reminds me of a question I had: Did you consider the effect
> >>>> of this patch on analyze? It uses a snapshot, and by memory you've not
> >>>> built in a defense against analyze being cancelled.
>
> The primary defense is not considering a cancellation except in 30
> of the 500 places a page is used. None of those 30 are, as far as
> I can see (upon review in response to your question), used in the
> analyze process.

I think what this means is that vacuum might remove tuples that would
otherwise be visible to analyze's snapshot. I suppose that's
acceptable. I wondered if it could cause harm to the size of the
sample, but after looking at acquire_sample_rows briefly I think it'd be
unharmed.

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

On 2016-05-24 14:48:35 -0500, Kevin Grittner wrote:
> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote:
> >> On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> >>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >>
> >>>> That comment reminds me of a question I had: Did you consider the effect
> >>>> of this patch on analyze? It uses a snapshot, and by memory you've not
> >>>> built in a defense against analyze being cancelled.
>
> The primary defense is not considering a cancellation except in 30
> of the 500 places a page is used. None of those 30 are, as far as
> I can see (upon review in response to your question), used in the
> analyze process.

Uh. How's that a defense? That seems like a recipe for corruption, not a
protection. That might be acceptable in the analyze case, but what about
e.g. concurrent index builds? E.g. IndexBuildHeapRangeScan() doesn't
seem to contain any checks against outdated blocks, and that's certainly
not ok? It appears that concurrent index builds are currently broken
from a quick skim?

> > Is there anything preventing this from becoming a problem?
>
> The fundamental approach that the error can only appear on
> user-facing scans, not internal reads and positioning.

> Unless there is some code path that uses a scan where the snapshot
> age is checked, this cannot be a problem. I don't see any such
> path, but if you do, please let me know, and I'll fix things.

That scares me. Not throwing an error, and not being broken are entirely
different things.

Andres

On Tue, May 24, 2016 at 3:54 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> what about e.g. concurrent index builds? E.g. IndexBuildHeapRangeScan() doesn't
> seem to contain any checks against outdated blocks

Why would it? We're talking about blocks where there were dead
tuples, with the transaction which updated or deleted the tuples
being complete, where those dead tuples were vacuumed away. These
should not appear in the index, should they?

> and that's certainly not ok?

Why not?

> It appears that concurrent index builds are currently broken
> from a quick skim?

Either you don't understand this feature very well, or I don't
understand concurrent index build very well. I thought we burned a
lot of time going through the index an extra time just to get rid
of dead tuples -- why would it be a problem not to add them in the
first place?

>>> Is there anything preventing this from becoming a problem?
>>
>> The fundamental approach that the error can only appear on
>> user-facing scans, not internal reads and positioning.
>
>> Unless there is some code path that uses a scan where the snapshot
>> age is checked, this cannot be a problem. I don't see any such
>> path, but if you do, please let me know, and I'll fix things.
>
> That scares me. Not throwing an error, and not being broken are entirely
> different things.

Absolutely, but let's not start pointing at random chunks of code
and asking why an error isn't thrown there without showing that you
get bad results otherwise, or at least some plausible argument why
you might.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, May 24, 2016 at 3:48 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote:
>>> On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>>>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>>
>>>>> That comment reminds me of a question I had: Did you consider the effect
>>>>> of this patch on analyze? It uses a snapshot, and by memory you've not
>>>>> built in a defense against analyze being cancelled.
>
> The primary defense is not considering a cancellation except in 30
> of the 500 places a page is used. None of those 30 are, as far as
> I can see (upon review in response to your question), used in the
> analyze process.

It's not obvious to me how this is supposed to work. If ANALYZE's
snapshot is subject to being ignored for xmin purposes because of
snapshot_too_old, then I would think it would need to consider
cancelling itself if it reads a page with possibly-removed data, just
like in any other case. It seems that we might not actually need a
snapshot set for ANALYZE in all cases, because the comments say:

/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());

If we can get away with it, it would be a rather large win to only set
a snapshot when the table has an expression index. For purposes of
"snapshot too old", though, it will be important that a function in an
index which tries to read data from some other table which has been
pruned cancels itself when necessary.

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

On Tue, May 24, 2016 at 4:10 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> For purposes of
> "snapshot too old", though, it will be important that a function in an
> index which tries to read data from some other table which has been
> pruned cancels itself when necessary.

Hm. I'll try to work up a test case for this. If you have one,
please send it along to me.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, May 24, 2016 at 4:09 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Tue, May 24, 2016 at 3:54 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

>> It appears that concurrent index builds are currently broken
>> from a quick skim?
>
> Either you don't understand this feature very well, or I don't
> understand concurrent index build very well.

And it may be the latter. On closer review I think some adjustment
may be needed in IndexBuildHeapRangeScan(). I'm not sure that
throwing the error is necessary, since there is a flag to say that
the index is unsafe for existing snapshots -- it may be enough to
set that flag.

Sorry for my earlier email.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-05-24 16:09:27 -0500, Kevin Grittner wrote:
> On Tue, May 24, 2016 at 3:54 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > what about e.g. concurrent index builds? E.g. IndexBuildHeapRangeScan() doesn't
> > seem to contain any checks against outdated blocks
>
> Why would it? We're talking about blocks where there were dead
> tuples, with the transaction which updated or deleted the tuples
> being complete, where those dead tuples were vacuumed away. These
> should not appear in the index, should they?

(it appears I had switched around the concerns for concurrent and
!concurrent in my head. But I think both might be negatively affected.)

For concurrent indexes we'll actually use a normal mvcc snapshot, which
means heap_getnext() in IndexBuildHeapRangeScan() and
validate_index_heapscan() will error out when encountering removed
tuples. Which means it'll be hard to ever perform a concurrent reindex
where an individual phase takes more than old_snapshot_threshold -
problematic from my POV, given that old_snapshot_threshold cannot be
changed at runtime.

For normal index scans the following appears to be problematic:
case HEAPTUPLE_RECENTLY_DEAD:
/*
* If tuple is recently deleted then we must index it
* anyway to preserve MVCC semantics. (Pre-existing
* transactions could try to use the index after we finish
* building it, and may need to see such tuples.)
*
* However, if it was HOT-updated then we must only index
* the live tuple at the end of the HOT-chain. Since this
* breaks semantics for pre-existing snapshots, mark the
* index as unusable for them.
*/

afaics that detection is broken if we advance the xmin horizon more
aggressively than the snapshot. The LSNs of the created index pages will
be new, and we'll thus not necessarily error out when requried. At the
very least we'd have to set indexInfo->ii_BrokenHotChain in that case.
As far as I can see normal index builds will allow concurrent hot prunes
and everything; since those only require page-level exclusive locks.

So for !concurrent builds we might end up with a corrupt index.

I think many of the places relying on heap scans with !mvcc snapshots
are problematic in that way. Outdated reads will not be detected by
TestForOldSnapshot() (given the (snapshot)->satisfies ==
HeapTupleSatisfiesMVCC condition therein), and rely on the snapshot to
be actually working. E.g. CLUSTER/ VACUUM FULL rely on accurate
HEAPTUPLE_RECENTLY_DEAD
switch (HeapTupleSatisfiesVacuum(tuple, OldestXmin, buf))
{
...
case HEAPTUPLE_RECENTLY_DEAD:
tups_recently_dead += 1;
/* fall through */
case HEAPTUPLE_LIVE:
/* Live or recently dead, must copy it */
isdead = false;
break;

If an old session with >= repeatable read accesses a clustered table
(after the cluster committed), they'll now not see any errors, because
all the LSNs look new.

> >>> Is there anything preventing this from becoming a problem?
> >>
> >> The fundamental approach that the error can only appear on
> >> user-facing scans, not internal reads and positioning.
> >
> >> Unless there is some code path that uses a scan where the snapshot
> >> age is checked, this cannot be a problem. I don't see any such
> >> path, but if you do, please let me know, and I'll fix things.
> >
> > That scares me. Not throwing an error, and not being broken are entirely
> > different things.
>
> Absolutely, but let's not start pointing at random chunks of code
> and asking why an error isn't thrown there without showing that you
> get bad results otherwise, or at least some plausible argument why
> you might.

This attitude is disturbing. You've evidently not at all looked at the
snapshot issues around analyze before; even though snapshot too old at
the very least introduces a behavioural issue there (if not a bug at
least in the case of expression based stuff). I've asked you about
principled defenses, and your answer was "we don't error out". Now I
point to another place, and you respond with a relatively strong
dismissal. Independent of me being right or wrong, that seems to be the
completely wrong way round.

Andres

On Tue, May 24, 2016 at 4:56 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> The LSNs of the created index pages will be new, and we'll thus
> not necessarily error out when requried.

It is *old* LSNs that are *safe* -- *new* LSNs are what trigger the
"snapshot too old" error. So your observation may be a reason that
there is not, in fact, any bug here. That said, even if there is
no chance that using the index could generate incorrect results, it
may be worth looking at whether avoiding index use (as mentioned
below) might avoid false positives, and have enough value as an
optimization to add. Of course, if that is the case, there is the
question of whether that is appropriate for 9.6 or material for the
first version 10 CF.

> At the very least we'd have to set indexInfo->ii_BrokenHotChain
> in that case.

If there is a bug, this is what I will look at first -- having
queries avoid the index if they are using an old snapshot, rather
than throwing an error.

> As far as I can see normal index builds will allow concurrent hot
> prunes and everything; since those only require page-level
> exclusive locks.
>
> So for !concurrent builds we might end up with a corrupt index.

... by which you mean an index that omits certainly-dead heap
tuples which have been the subject of early pruning or vacuum, even
if there are still registered snapshots that include those tuples?
Or do you see something else?

Again, since both the heap pages involved and all the new index
pages would have LSNs recent enough to trigger the old snapshot
check, I'm not sure where the problem is, but will review closely
to see what I might have missed.

> I think many of the places relying on heap scans with !mvcc
> snapshots are problematic in that way. Outdated reads will not be
> detected by TestForOldSnapshot() (given the (snapshot)->satisfies
> == HeapTupleSatisfiesMVCC condition therein), and rely on the
> snapshot to be actually working. E.g. CLUSTER/ VACUUM FULL rely
> on accurate HEAPTUPLE_RECENTLY_DEAD

Don't the "RECENTLY" values imply that the transaction is still
running which cause the tuple to be dead? Since tuples are not
subject to early pruning or vacuum when that is the case, where do
you see a problem? The snapshot itself has the usual xmin et al.,
so I'm not sure what you might mean by "the snapshot to be actually
working" if not the override at the time of pruning/vacuuming.

> If an old session with >= repeatable read accesses a clustered
> table (after the cluster committed), they'll now not see any
> errors, because all the LSNs look new.

Again, it is new LSNs that trigger errors; if the page has not been
written recently the LSN is old and there is no error. I think you
may be seeing problems based on getting the basics of this
backwards.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 27, 2016 at 9:58 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Tue, May 24, 2016 at 4:56 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

>> If an old session with >= repeatable read accesses a clustered
>> table (after the cluster committed), they'll now not see any
>> errors, because all the LSNs look new.
>
> Again, it is new LSNs that trigger errors; if the page has not been
> written recently the LSN is old and there is no error. I think you
> may be seeing problems based on getting the basics of this
> backwards.

I am reviewing the suggestion of a possible bug now, and will make
it my top priority until resolved. By the end of 1 June I will
either have committed a fix or posted an explanation of why the
concern is mistaken, with test results to demonstrate correct
behavior.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, May 24, 2016 at 4:10 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, May 24, 2016 at 3:48 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>> On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote:
>>>> On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>>>>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>>>>
>>>>>> That comment reminds me of a question I had: Did you consider the effect
>>>>>> of this patch on analyze? It uses a snapshot, and by memory you've not
>>>>>> built in a defense against analyze being cancelled.
>>
>> The primary defense is not considering a cancellation except in 30
>> of the 500 places a page is used. None of those 30 are, as far as
>> I can see (upon review in response to your question), used in the
>> analyze process.
>
> It's not obvious to me how this is supposed to work. If ANALYZE's
> snapshot is subject to being ignored for xmin purposes because of
> snapshot_too_old, then I would think it would need to consider
> cancelling itself if it reads a page with possibly-removed data, just
> like in any other case. It seems that we might not actually need a
> snapshot set for ANALYZE in all cases, because the comments say:
>
> /* functions in indexes may want a snapshot set */
> PushActiveSnapshot(GetTransactionSnapshot());
>
> If we can get away with it, it would be a rather large win to only set
> a snapshot when the table has an expression index. For purposes of
> "snapshot too old", though, it will be important that a function in an
> index which tries to read data from some other table which has been
> pruned cancels itself when necessary.

I will make this my top priority after resolving the question of whether
there is an issue with CREATE INDEX. I expect to have a resolution,
probably involving some patch, by 3 June.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, May 27, 2016 at 10:58 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>> As far as I can see normal index builds will allow concurrent hot
>> prunes and everything; since those only require page-level
>> exclusive locks.
>>
>> So for !concurrent builds we might end up with a corrupt index.
>
> ... by which you mean an index that omits certainly-dead heap
> tuples which have been the subject of early pruning or vacuum, even
> if there are still registered snapshots that include those tuples?
> Or do you see something else?

I think that is the danger.

> Again, since both the heap pages involved and all the new index
> pages would have LSNs recent enough to trigger the old snapshot
> check, I'm not sure where the problem is, but will review closely
> to see what I might have missed.

This is a good point, though, I think.

>> I think many of the places relying on heap scans with !mvcc
>> snapshots are problematic in that way. Outdated reads will not be
>> detected by TestForOldSnapshot() (given the (snapshot)->satisfies
>> == HeapTupleSatisfiesMVCC condition therein), and rely on the
>> snapshot to be actually working. E.g. CLUSTER/ VACUUM FULL rely
>> on accurate HEAPTUPLE_RECENTLY_DEAD
>
> Don't the "RECENTLY" values imply that the transaction is still
> running which cause the tuple to be dead? Since tuples are not
> subject to early pruning or vacuum when that is the case, where do
> you see a problem? The snapshot itself has the usual xmin et al.,
> so I'm not sure what you might mean by "the snapshot to be actually
> working" if not the override at the time of pruning/vacuuming.

Anybody who calls HeapTupleSatisfiesVacuum() with an xmin value that
is newer that the oldest registered snapshot in the system (based on
some snapshots being ignored) might get a return value of
HEAPTUPLE_DEAD rather than HEAPTUPLE_RECENTLY_DEAD. It seems
necessary to carefully audit all calls to HeapTupleSatisfiesVacuum()
to see whether that difference matters. I took a quick look and
here's what I see:

statapprox_heap(): Statistical information for the DBA. The
difference is non-critical.

heap_prune_chain(): Seeing the tuple as dead might cause it to be
removed early. This should be OK. Removing the tuple early will
cause the page LSN to be bumped unless RelationNeedsWAL() returns
false, and TransactionIdLimitedForOldSnapshots() includes that as a
condition for disabling early pruning.

IndexBuildHeapRangeScan(): We might end up with indexIt = false
instead of indexIt = true. That should be OK because anyone using the
old snapshot will see a new page LSN and error out. We might also
fail to set indexInfo->ii_BrokenHotChain = true. I suspect that's a
problem, but I'm not certain of it.

acquire_sample_rows: Both return values are treated in the same way.
No problem.

copy_heap_data: We'll end up setting isdead = true instead of
tups_recently_dead += 1. That means that the new heap won't include
the tuple, which is OK because old snapshots can't read the new heap
without erroring out, assuming that the new heap has LSNs. The xmin
used here comes from vacuum_set_xid_limits() which goes through
TransactionIdLimitedForOldSnapshots() so this should be OK for the
same reasons as heap_prune_chain(). Another effect of seeing the
tuple as prematurely dead is that we'll call rewrite_heap_dead_tuple()
on it; rewrite_heap_dead_tuple() will presume that if this tuple is
dead, its predecessor in the ctid chain is also dead. I don't see any
obvious problem with that.

lazy_scan_heap(): Basically, the same thing as heap_prune_chain().

CheckForSerializableConflictOut: Maybe a problem? If the tuple is
dead, there's no issue, but if it's recently-dead, there might be.

We might want to add comments to some of these places addressing
snapshot_too_old specifically.

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

On Tue, May 31, 2016 at 10:03 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, May 27, 2016 at 10:58 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>>> As far as I can see normal index builds will allow concurrent hot
>>> prunes and everything; since those only require page-level
>>> exclusive locks.
>>>
>>> So for !concurrent builds we might end up with a corrupt index.
>>
>> ... by which you mean an index that omits certainly-dead heap
>> tuples which have been the subject of early pruning or vacuum, even
>> if there are still registered snapshots that include those tuples?
>> Or do you see something else?
>
> I think that is the danger.

Well, the *perceived* danger -- since every page in the new index
would be new enough to be recognized as too recently modified to be
safe for a snapshot that could still see the omitted rows, the only
question I'm sorting out on this is how safe it might be to cause
the index to be ignored in planning when using such a snapshot.
That and demonstrating the safe behavior to those not following
closely enough to see what will happen without a demonstration.

>> Again, since both the heap pages involved and all the new index
>> pages would have LSNs recent enough to trigger the old snapshot
>> check, I'm not sure where the problem is,

> This is a good point, though, I think.

The whole perception of risk in this area seems to be based on
getting that wrong; although the review of this area may yield some
benefit in terms of minimizing false positives.

>>> I think many of the places relying on heap scans with !mvcc
>>> snapshots are problematic in that way. Outdated reads will not be
>>> detected by TestForOldSnapshot() (given the (snapshot)->satisfies
>>> == HeapTupleSatisfiesMVCC condition therein), and rely on the
>>> snapshot to be actually working. E.g. CLUSTER/ VACUUM FULL rely
>>> on accurate HEAPTUPLE_RECENTLY_DEAD
>>
>> Don't the "RECENTLY" values imply that the transaction is still
>> running which cause the tuple to be dead? Since tuples are not
>> subject to early pruning or vacuum when that is the case, where do
>> you see a problem? The snapshot itself has the usual xmin et al.,
>> so I'm not sure what you might mean by "the snapshot to be actually
>> working" if not the override at the time of pruning/vacuuming.
>
> Anybody who calls HeapTupleSatisfiesVacuum() with an xmin value that
> is newer that the oldest registered snapshot in the system (based on
> some snapshots being ignored) might get a return value of
> HEAPTUPLE_DEAD rather than HEAPTUPLE_RECENTLY_DEAD.

Since the override xmin cannot advance past the earliest
transaction which is still active, HEAPTUPLE_DEAD indicates that
the transaction causing the tuple to be dead has completed and the
tuple is irrevocably dead -- even if there are still snapshots
registered which can see it. Seeing HEAPTUPLE_DEAD rather than
HEAPTUPLE_RECENTLY_DEAD is strictly limited to tuples which are
certainly and permanently dead and for which the only possible
references are non-MVCC snapshots or existing snapshots subject to
"snapshot too old" monitoring.