Thread: 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(-)

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@anarazel.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@anarazel.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@anarazel.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@anarazel.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@anarazel.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@gmail.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

On 2016-04-12 14:53:57 -0500, Kevin Grittner wrote:
> On Tue, Apr 12, 2016 at 2:28 PM, Andres Freund <andres@anarazel.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
whererelkind 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@anarazel.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
whererelkind 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@gmail.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@gmail.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@anarazel.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 Wed, Apr 13, 2016 at 3:08 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Wed, Apr 13, 2016 at 1:56 PM, Andres Freund <andres@anarazel.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@gmail.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@anarazel.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@anarazel.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@anarazel.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@anarazel.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 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

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@pgmasters.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 Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres@anarazel.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@gmail.com> wrote:
> On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres@anarazel.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@gmail.com> wrote:
> On Tue, Apr 19, 2016 at 11:11 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
>> On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres@anarazel.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 2016-04-19 20:27:31 +0530, Amit Kapila wrote:
> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres@anarazel.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@gmail.com> wrote:
> On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund <andres@anarazel.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.aasma@eesti.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@gmail.com> wrote:
> On Wed, Apr 20, 2016 at 8:08 PM, Ants Aasma <ants.aasma@eesti.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@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.aasma@eesti.ee> wrote:
> On Thu, Apr 21, 2016 at 5:16 PM, Kevin Grittner <kgrittn@gmail.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

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 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 2016-05-06 20:28:27 -0500, Kevin Grittner wrote:
> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres@anarazel.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@gmail.com> wrote:
> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres@anarazel.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@gmail.com> wrote:
> > On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres@anarazel.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@anarazel.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@anarazel.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@gmail.com> writes:
> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres@anarazel.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@anarazel.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@anarazel.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@gmail.com> wrote:
>>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres@anarazel.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@anarazel.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@gmail.com> wrote:
> >>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres@anarazel.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@anarazel.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@gmail.com> wrote:
> >>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres@anarazel.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@anarazel.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@gmail.com> wrote:
> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres@anarazel.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@gmail.com> wrote:
>>>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres@anarazel.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@gmail.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@gmail.com> wrote:
> On Tue, May 24, 2016 at 3:54 PM, Andres Freund <andres@anarazel.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@anarazel.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@anarazel.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@gmail.com> wrote:
> On Tue, May 24, 2016 at 4:56 PM, Andres Freund <andres@anarazel.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@gmail.com> wrote:
> On Tue, May 24, 2016 at 3:48 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
>> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres@anarazel.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@gmail.com> wrote:
>>>>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres@anarazel.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@gmail.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@gmail.com> wrote:
> On Fri, May 27, 2016 at 10:58 AM, Kevin Grittner <kgrittn@gmail.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.

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

The latter flag is what I'm currently digging at; but my hope is
that whenever old_snapshot_threshold >= 0 we can set
indexInfo->ii_BrokenHotChain = true to cause the planner to skip
consideration of the index if the snapshot is an "old" one.  That
will avoid some false positives (seeing the error when not strictly
necessary).  If that works out the way I hope, the only down side
is that a scan using a snapshot from an old transaction or cursor
would use some other index or a heap scan; but we already have that
possibility in some cases -- that seems to be the point of the
flag.

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

If the tuple is not visible to the scan, the behavior is unchanged
(a simple return from the function on either HEAPTUPLE_DEAD or
HEAPTUPLE_RECENTLY_DEAD with !visible) and (thus) clearly correct.
If the tuple is visible to us it is currently subject to early
pruning or vacuum (since those operations would get the same
modified xmin) but has not yet had any such treatment since we made
it to this function in the first place.  The processing for SSI
purposes would be unaffected by the possibility that there could
later be early pruning/vacuuming.

Thanks for the review and feedback!

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

On Fri, May 27, 2016 at 10:18 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Fri, May 27, 2016 at 9:58 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
>> On Tue, May 24, 2016 at 4:56 PM, Andres Freund <andres@anarazel.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.

This got set back by needing to fix a bug in the 9.5 release.  I am
back on this and have figured out that everyone who commented on
this specific issue was wrong about a very important fact -- the
LSNs in index pages after CREATE INDEX (with or without
CONCURRENTLY) and for REINDEX are always == InvalidXLogRecPtr (0).

That means that a snapshot from before an index build does not
always generate errors when it should on the use of the new index.
(Any early pruning/vacuuuming from before the index build is
missed; activity subsequent to the index build is recognized.)
Consequently, causing the index to be ignored in planning when
using the old index is not a nice optimization, but necessary for
correctness.  We already have logic to do this for other cases
(like HOT updates), so it is a matter of tying in to that existing
code correctly.  This won't be all that novel.

I now expect to push a fix along those lines by Tuesday, 6 June.

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

On Fri, May 27, 2016 at 10:35 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Tue, May 24, 2016 at 4:10 PM, Robert Haas <robertmhaas@gmail.com> wrote:

>> [ANALYZE of index with expression may fail to update statistics
>> if ANALYZE runs longer than old_snapshot_threshold]

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

Due to 9.5 bug-fixing and the index issue taking a bit longer than
I expected, this is now looking like a 7 June resolution.

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

On Sat, Jun 4, 2016 at 4:21 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Fri, Jun 3, 2016 at 4:24 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
>> Consequently, causing the index to be ignored in planning when
>> using the old index
>
> That last line should have read "using an old snapshot"
>
>> is not a nice optimization, but necessary for
>> correctness.  We already have logic to do this for other cases
>> (like HOT updates), so it is a matter of tying in to that existing
>> code correctly.  This won't be all that novel.
>
> Just to demonstrate that, the minimal patch to fix behavior in this
> area would be:
>
> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
> index 31a1438..6c379da 100644
> --- a/src/backend/catalog/index.c
> +++ b/src/backend/catalog/index.c
> @@ -2268,6 +2268,9 @@ IndexBuildHeapRangeScan(Relation heapRelation,
>         Assert(numblocks == InvalidBlockNumber);
>     }
>
> +   if (old_snapshot_threshold >= 0)
> +       indexInfo->ii_BrokenHotChain = true;
> +
>     reltuples = 0;
>
>     /*
>
> Of course, ii_BrokenHotChain should be renamed to something like
> ii_UnsafeForOldSnapshots, and some comments need to be updated; but
> the above is the substance of it.

I don't know why we'd want to rename it like that...

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

On Tue, Jun 7, 2016 at 10:40 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Jun 4, 2016 at 4:21 PM, Kevin Grittner <kgrittn@gmail.com> wrote:

>> the minimal patch to fix behavior in this area would be:
>>
>> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
>> index 31a1438..6c379da 100644
>> --- a/src/backend/catalog/index.c
>> +++ b/src/backend/catalog/index.c
>> @@ -2268,6 +2268,9 @@ IndexBuildHeapRangeScan(Relation heapRelation,
>>         Assert(numblocks == InvalidBlockNumber);
>>     }
>>
>> +   if (old_snapshot_threshold >= 0)
>> +       indexInfo->ii_BrokenHotChain = true;
>> +
>>     reltuples = 0;
>>
>>     /*
>>
>> Of course, ii_BrokenHotChain should be renamed to something like
>> ii_UnsafeForOldSnapshots, and some comments need to be updated; but
>> the above is the substance of it.
>
> I don't know why we'd want to rename it like that...

If we made the above change, the old name would be misleading, but
I've thought better of that and attach a slightly different
approach (tested but not yet with comment adjustments); attached.

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

On Wed, Jun 8, 2016 at 9:40 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
>>> Of course, ii_BrokenHotChain should be renamed to something like
>>> ii_UnsafeForOldSnapshots, and some comments need to be updated; but
>>> the above is the substance of it.
>>
>> I don't know why we'd want to rename it like that...
>
> If we made the above change, the old name would be misleading, but
> I've thought better of that and attach a slightly different
> approach (tested but not yet with comment adjustments); attached.

Kevin asked me to look at this patch, and maybe update it, but after
some further study, I am not at all convinced that there's any actual
bug here.  Here's why: in order for the HeapTupleSatisfiesVacuum() in
IndexBuildHeapRangeScan() to return HEAPTUPLE_DEAD instead of
HEAPTUPLE_RECENTLY_DEAD, it would have to be using an OldestXmin value
that doesn't include all of the snapshots in the system.  But that
will never happen, because that xmin comes directly from
GetOldestXmin(heapRelation, true), which knows nothing about
snapshot_too_old and will therefore never exclude any snapshots.  If
we were to pass the output of HeapTupleSatisfiesVacuum() through
TransactionIdLimitedForOldSnapshots() before using it here, we would
have a bug.  But we don't do that.

Do you have a test case that demonstrates a problem, or an explanation
of why you think there is one?

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

On Wed, Jun 8, 2016 at 2:49 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> Do you have a test case that demonstrates a problem, or an explanation
> of why you think there is one?

With old_snapshot_threshold = '1min'

-- connection 1
drop table if exists t1;
create table t1 (c1 int not null);
insert into t1 select generate_series(1, 1000000);
begin transaction isolation level repeatable read;
select 1;

-- connection 2
insert into t2 values (1);
delete from t1 where c1 between 200000 and 299999;
delete from t1 where c1 = 1000000;
vacuum analyze verbose t1;
select pg_sleep_for('2min');
vacuum analyze verbose t1;  -- repeat if needed until dead rows vacuumed

-- connection 1
select c1 from t1 where c1 = 100;
select c1 from t1 where c1 = 250000;

The problem occurs when an index is built while an old snapshot
exists which can't see the effects of early pruning/vacuuming.  The
fix prevents use of such an index until all snapshots early enough
to have a problem have been released.

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

On Wed, Jun 8, 2016 at 4:04 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Wed, Jun 8, 2016 at 2:49 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Do you have a test case that demonstrates a problem, or an explanation
>> of why you think there is one?
>
> With old_snapshot_threshold = '1min'
>
> -- connection 1
> drop table if exists t1;
> create table t1 (c1 int not null);
> insert into t1 select generate_series(1, 1000000);
> begin transaction isolation level repeatable read;
> select 1;
>
> -- connection 2
> insert into t2 values (1);
> delete from t1 where c1 between 200000 and 299999;
> delete from t1 where c1 = 1000000;
> vacuum analyze verbose t1;
> select pg_sleep_for('2min');
> vacuum analyze verbose t1;  -- repeat if needed until dead rows vacuumed
>
> -- connection 1
> select c1 from t1 where c1 = 100;
> select c1 from t1 where c1 = 250000;
>
> The problem occurs when an index is built while an old snapshot
> exists which can't see the effects of early pruning/vacuuming.  The
> fix prevents use of such an index until all snapshots early enough
> to have a problem have been released.

This example doesn't seem to involve any CREATE INDEX or REINDEX
operations, so I'm a little confused.

Generally, I think I see the hazard you're concerned about: I had
failed to realize, as you mentioned upthread, that new index pages
would have an LSN of 0. So if a tuple is pruned early and then the
index is reindexed, old snapshots won't realize that data is missing.
What I'm less certain about is whether you can actually get by with
reusing ii_BrokenHotChain to handle this case.  For example, note this
comment:

     * However, when reindexing an existing index, we should do nothing here.
     * Any HOT chains that are broken with respect to the index must predate
     * the index's original creation, so there is no need to change the
     * index's usability horizon.  Moreover, we *must not* try to change the
     * index's pg_index entry while reindexing pg_index itself, and this
     * optimization nicely prevents that.

This logic doesn't apply to the old snapshot case; there, you'd need
to distrust the index whether it was an initial build or a REINDEX,
but it doesn't look like that's what the patch does.  I'm worried
there could be other places where we rely on ii_BrokenHotChain to
detect only one specific condition that isn't quite the same as what
you're trying to use it for here.

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

On Fri, Jun 03, 2016 at 04:29:40PM -0500, Kevin Grittner wrote:
> On Fri, May 27, 2016 at 10:35 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> > On Tue, May 24, 2016 at 4:10 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> >> [ANALYZE of index with expression may fail to update statistics
> >> if ANALYZE runs longer than old_snapshot_threshold]
>
> >> 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.
>
> Due to 9.5 bug-fixing and the index issue taking a bit longer than
> I expected, this is now looking like a 7 June resolution.

This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com

[Thanks to Robert to stepping up to keep this moving while I was
down yesterday with a minor injury.  I'm back on it today.]

On Wed, Jun 8, 2016 at 3:11 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Jun 8, 2016 at 4:04 PM, Kevin Grittner <kgrittn@gmail.com> wrote:

>> -- connection 1
>> drop table if exists t1;
>> create table t1 (c1 int not null);
>> insert into t1 select generate_series(1, 1000000);
>> begin transaction isolation level repeatable read;
>> select 1;
>>
>> -- connection 2
>> insert into t2 values (1);
>> delete from t1 where c1 between 200000 and 299999;
>> delete from t1 where c1 = 1000000;
>> vacuum analyze verbose t1;
>> select pg_sleep_for('2min');
>> vacuum analyze verbose t1;  -- repeat if needed until dead rows vacuumed
>>
>> -- connection 1
>> select c1 from t1 where c1 = 100;
>> select c1 from t1 where c1 = 250000;
>>
>> The problem occurs when an index is built while an old snapshot
>> exists which can't see the effects of early pruning/vacuuming.  The
>> fix prevents use of such an index until all snapshots early enough
>> to have a problem have been released.
>
> This example doesn't seem to involve any CREATE INDEX or REINDEX
> operations, so I'm a little confused.

Sorry; pasto -- the index build is supposed to be on connection 2
right after the dead rows are confirmed vacuumed away:

create index t1_c1 on t1 (c1);

> Generally, I think I see the hazard you're concerned about: I had
> failed to realize, as you mentioned upthread, that new index pages
> would have an LSN of 0. So if a tuple is pruned early and then the
> index is reindexed, old snapshots won't realize that data is missing.
> What I'm less certain about is whether you can actually get by with
> reusing ii_BrokenHotChain to handle this case.

v2 and later does not do that.  v1 did, but that was a more blunt
instrument.

>  For example, note this comment:
>
>      * However, when reindexing an existing index, we should do nothing here.
>      * Any HOT chains that are broken with respect to the index must predate
>      * the index's original creation, so there is no need to change the
>      * index's usability horizon.  Moreover, we *must not* try to change the
>      * index's pg_index entry while reindexing pg_index itself, and this
>      * optimization nicely prevents that.
>
> This logic doesn't apply to the old snapshot case; there, you'd need
> to distrust the index whether it was an initial build or a REINDEX,
> but it doesn't look like that's what the patch does.  I'm worried
> there could be other places where we rely on ii_BrokenHotChain to
> detect only one specific condition that isn't quite the same as what
> you're trying to use it for here.

Well spotted.  I had used a lot of discreet calls to get that
reindex logic right, but it was verbose and ugly, so I had just
added the new macros in this patch and started to implement them
before I knocked off for the day.  At handover I was too distracted
to remember where I was with it.  :-(  See if it looks right to you
now.

Attached is v3.  I will commit this patch to resolve this issue
tomorrow, barring any objections before then.

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

On Thu, Jun 9, 2016 at 10:28 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> [Thanks to Robert to stepping up to keep this moving while I was
> down yesterday with a minor injury.  I'm back on it today.]
>> Generally, I think I see the hazard you're concerned about: I had
>> failed to realize, as you mentioned upthread, that new index pages
>> would have an LSN of 0. So if a tuple is pruned early and then the
>> index is reindexed, old snapshots won't realize that data is missing.
>> What I'm less certain about is whether you can actually get by with
>> reusing ii_BrokenHotChain to handle this case.
>
> v2 and later does not do that.  v1 did, but that was a more blunt
> instrument.
>
>>  For example, note this comment:
>>
>>      * However, when reindexing an existing index, we should do nothing here.
>>      * Any HOT chains that are broken with respect to the index must predate
>>      * the index's original creation, so there is no need to change the
>>      * index's usability horizon.  Moreover, we *must not* try to change the
>>      * index's pg_index entry while reindexing pg_index itself, and this
>>      * optimization nicely prevents that.
>>
>> This logic doesn't apply to the old snapshot case; there, you'd need
>> to distrust the index whether it was an initial build or a REINDEX,
>> but it doesn't look like that's what the patch does.  I'm worried
>> there could be other places where we rely on ii_BrokenHotChain to
>> detect only one specific condition that isn't quite the same as what
>> you're trying to use it for here.
>
> Well spotted.  I had used a lot of discreet calls to get that
> reindex logic right, but it was verbose and ugly, so I had just
> added the new macros in this patch and started to implement them
> before I knocked off for the day.  At handover I was too distracted
> to remember where I was with it.  :-(  See if it looks right to you
> now.
>
> Attached is v3.  I will commit this patch to resolve this issue
> tomorrow, barring any objections before then.

So I like the idea of centralizing checks in
RelationAllowsEarlyVacuum, but shouldn't it really be called
RelationAllowsEarlyPruning?

Will look at this a bit more if I get time.

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

On Thu, Jun 9, 2016 at 6:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> So I like the idea of centralizing checks in
> RelationAllowsEarlyVacuum, but shouldn't it really be called
> RelationAllowsEarlyPruning?

Since vacuum calls the pruning function, and not the other way
around, the name you suggest would be technically more correct.
Committed using "Pruning" instead of "Vacuum" in both new macros.

I have closed the CREATE INDEX versus "snapshot too old" issue in
the "PostgreSQL 9.6 Open Items" Wiki page.

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

On Fri, Jun 10, 2016 at 10:45 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Thu, Jun 9, 2016 at 6:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
>> So I like the idea of centralizing checks in
>> RelationAllowsEarlyVacuum, but shouldn't it really be called
>> RelationAllowsEarlyPruning?
>
> Since vacuum calls the pruning function, and not the other way
> around, the name you suggest would be technically more correct.
> Committed using "Pruning" instead of "Vacuum" in both new macros.
>
> I have closed the CREATE INDEX versus "snapshot too old" issue in
> the "PostgreSQL 9.6 Open Items" Wiki page.

You've still got an early_vacuum_enabled variable name floating around there.

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

On Fri, Jun 10, 2016 at 10:26 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jun 10, 2016 at 10:45 AM, Kevin Grittner <kgrittn@gmail.com> wrote:

>> Since vacuum calls the pruning function, and not the other way
>> around, the name you suggest would be technically more correct.
>> Committed using "Pruning" instead of "Vacuum" in both new macros.

> You've still got an early_vacuum_enabled variable name floating around there.

Gah!  Renamed for consistency.

Thanks!

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

On Tue, May 24, 2016 at 4:10 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, May 24, 2016 at 3:48 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
>> On Tue, May 24, 2016 at 12:00 PM, Andres Freund <andres@anarazel.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@gmail.com> wrote:
>>>>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund <andres@anarazel.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 have reviewed the code and run tests to try to find something
here which could be considered a bug, without finding any problem.
When reading pages for the random sample for ANALYZE (or
auto-analyze) there is not an age check; so ANALYZE completes
without error, keeping statistics up-to-date.

There really is no difference in behavior except in the case that:

(1)  old_snapshot_threshold >= 0 to enable the "snapshot too old"
       feature, and
(2)  there were tuples that were dead as the result of completed
       transactions, and
(3)  those tuples became older than the threshold, and
(4)  those tuples were pruned or vacuumed away, and
(5)  an ANALYZE process would have read those dead tuples had they
       not been removed.

In such a case the irrevocably dead, permanently removed tuples are
not counted in the statistics.  I have trouble seeing a better
outcome than that.  Among my tests, I specifically checked for an
ANALYZE of a table having an index on an expression, using an old
snapshot:

-- connection 1
drop table if exists t1;
create table t1 (c1 int not null);
drop table if exists t2;
create table t2 (c1 int not null);
insert into t1 select generate_series(1, 10000);
drop function mysq(i int);
create function mysq(i int)
  returns int
  language plpgsql
  immutable
as $mysq$
begin
  return (i * i);
end
$mysq$;
create index t1_c1sq on t1 ((mysq(c1)));
begin transaction isolation level repeatable read;
select 1;

-- connection 2
vacuum analyze verbose t1;
delete from t1 where c1 between 1000 and 1999;
delete from t1 where c1 = 8000;
insert into t2 values (1);
select pg_sleep_for('2min');
vacuum verbose t1;  -- repeat if necessary to see the dead rows
disappear

-- connection 1
analyze verbose t1;

This runs to completion, as I would want and expect.

I am closing this item on the "PostgreSQL 9.6 Open Items" page.  If
anyone feels that I've missed something, please provide a test to
show the problem, or a clear description of the problem and how you
feel behavior should be different.

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

On Sat, Jun 11, 2016 at 11:29 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> I have reviewed the code and run tests to try to find something
> here which could be considered a bug, without finding any problem.
> When reading pages for the random sample for ANALYZE (or
> auto-analyze) there is not an age check; so ANALYZE completes
> without error, keeping statistics up-to-date.
>
> There really is no difference in behavior except in the case that:
>
> (1)  old_snapshot_threshold >= 0 to enable the "snapshot too old"
>        feature, and
> (2)  there were tuples that were dead as the result of completed
>        transactions, and
> (3)  those tuples became older than the threshold, and
> (4)  those tuples were pruned or vacuumed away, and
> (5)  an ANALYZE process would have read those dead tuples had they
>        not been removed.
>
> In such a case the irrevocably dead, permanently removed tuples are
> not counted in the statistics.  I have trouble seeing a better
> outcome than that.  Among my tests, I specifically checked for an
> ANALYZE of a table having an index on an expression, using an old
> snapshot:
>
> -- connection 1
> drop table if exists t1;
> create table t1 (c1 int not null);
> drop table if exists t2;
> create table t2 (c1 int not null);
> insert into t1 select generate_series(1, 10000);
> drop function mysq(i int);
> create function mysq(i int)
>   returns int
>   language plpgsql
>   immutable
> as $mysq$
> begin
>   return (i * i);
> end
> $mysq$;
> create index t1_c1sq on t1 ((mysq(c1)));
> begin transaction isolation level repeatable read;
> select 1;
>
> -- connection 2
> vacuum analyze verbose t1;
> delete from t1 where c1 between 1000 and 1999;
> delete from t1 where c1 = 8000;
> insert into t2 values (1);
> select pg_sleep_for('2min');
> vacuum verbose t1;  -- repeat if necessary to see the dead rows
> disappear
>
> -- connection 1
> analyze verbose t1;
>
> This runs to completion, as I would want and expect.
>
> I am closing this item on the "PostgreSQL 9.6 Open Items" page.  If
> anyone feels that I've missed something, please provide a test to
> show the problem, or a clear description of the problem and how you
> feel behavior should be different.

So what happens in this scenario:

1. ANALYZE runs really slowly - maybe the user-defined function it's
running for the expression index is extremely long-running.
2. Eventually, the snapshot for ANALYZE is older than the configured
value of snapshot_too_old.
3. Then, ANALYZE selects a page with an LSN new enough that it might
have been pruned.

Presumably, the ANALYZE ought to error out in this scenario, just as
it would in any other situation where an old snapshot sees a new page.
No?

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

On Wed, Jun 15, 2016 at 10:46 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Jun 11, 2016 at 11:29 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
>> I have reviewed the code and run tests to try to find something
>> here which could be considered a bug, without finding any problem.
>> When reading pages for the random sample for ANALYZE (or
>> auto-analyze) there is not an age check; so ANALYZE completes
>> without error, keeping statistics up-to-date.
>>
>> There really is no difference in behavior except in the case that:
>>
>> (1)  old_snapshot_threshold >= 0 to enable the "snapshot too old"
>>        feature, and
>> (2)  there were tuples that were dead as the result of completed
>>        transactions, and
>> (3)  those tuples became older than the threshold, and
>> (4)  those tuples were pruned or vacuumed away, and
>> (5)  an ANALYZE process would have read those dead tuples had they
>>        not been removed.
>>
>> In such a case the irrevocably dead, permanently removed tuples are
>> not counted in the statistics.  I have trouble seeing a better
>> outcome than that.  Among my tests, I specifically checked for an
>> ANALYZE of a table having an index on an expression, using an old
>> snapshot:
>>
>> -- connection 1
>> drop table if exists t1;
>> create table t1 (c1 int not null);
>> drop table if exists t2;
>> create table t2 (c1 int not null);
>> insert into t1 select generate_series(1, 10000);
>> drop function mysq(i int);
>> create function mysq(i int)
>>   returns int
>>   language plpgsql
>>   immutable
>> as $mysq$
>> begin
>>   return (i * i);
>> end
>> $mysq$;
>> create index t1_c1sq on t1 ((mysq(c1)));
>> begin transaction isolation level repeatable read;
>> select 1;
>>
>> -- connection 2
>> vacuum analyze verbose t1;
>> delete from t1 where c1 between 1000 and 1999;
>> delete from t1 where c1 = 8000;
>> insert into t2 values (1);
>> select pg_sleep_for('2min');
>> vacuum verbose t1;  -- repeat if necessary to see the dead rows
>> disappear
>>
>> -- connection 1
>> analyze verbose t1;
>>
>> This runs to completion, as I would want and expect.
>>
>> I am closing this item on the "PostgreSQL 9.6 Open Items" page.  If
>> anyone feels that I've missed something, please provide a test to
>> show the problem, or a clear description of the problem and how you
>> feel behavior should be different.
>
> So what happens in this scenario:
>
> 1. ANALYZE runs really slowly - maybe the user-defined function it's
> running for the expression index is extremely long-running.
> 2. Eventually, the snapshot for ANALYZE is older than the configured
> value of snapshot_too_old.
> 3. Then, ANALYZE selects a page with an LSN new enough that it might
> have been pruned.
>
> Presumably, the ANALYZE ought to error out in this scenario, just as
> it would in any other situation where an old snapshot sees a new page.
> No?

The test I showed creates a situation which (to ANALYZE) is
identical to what you describe -- ANALYZE sees a page with an LSN
recent enough that it could have been (and actually has been)
pruned.  Why would it be better for the ANALYZE to fail than to
complete?

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

On Wed, Jun 15, 2016 at 1:44 PM, Kevin Grittner <kgrittn@gmail.com>
wrote:>> So what happens in this scenario:
>> 1. ANALYZE runs really slowly - maybe the user-defined function it's
>> running for the expression index is extremely long-running.
>> 2. Eventually, the snapshot for ANALYZE is older than the configured
>> value of snapshot_too_old.
>> 3. Then, ANALYZE selects a page with an LSN new enough that it might
>> have been pruned.
>>
>> Presumably, the ANALYZE ought to error out in this scenario, just as
>> it would in any other situation where an old snapshot sees a new page.
>> No?
>
> The test I showed creates a situation which (to ANALYZE) is
> identical to what you describe -- ANALYZE sees a page with an LSN
> recent enough that it could have been (and actually has been)
> pruned.  Why would it be better for the ANALYZE to fail than to
> complete?

As I understand it, the reason we need to sometimes give "ERROR:
snapshot too old" after early pruning is because we might otherwise
give the wrong answer.

Maybe I'm confused.

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

On Wed, Jun 15, 2016 at 1:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Jun 15, 2016 at 1:44 PM, Kevin Grittner <kgrittn@gmail.com>
> wrote:>> So what happens in this scenario:
>>> 1. ANALYZE runs really slowly - maybe the user-defined function it's
>>> running for the expression index is extremely long-running.
>>> 2. Eventually, the snapshot for ANALYZE is older than the configured
>>> value of snapshot_too_old.
>>> 3. Then, ANALYZE selects a page with an LSN new enough that it might
>>> have been pruned.
>>>
>>> Presumably, the ANALYZE ought to error out in this scenario, just as
>>> it would in any other situation where an old snapshot sees a new page.
>>> No?
>>
>> The test I showed creates a situation which (to ANALYZE) is
>> identical to what you describe -- ANALYZE sees a page with an LSN
>> recent enough that it could have been (and actually has been)
>> pruned.  Why would it be better for the ANALYZE to fail than to
>> complete?
>
> As I understand it, the reason we need to sometimes give "ERROR:
> snapshot too old" after early pruning is because we might otherwise
> give the wrong answer.
>
> Maybe I'm confused.

In the scenario that you describe, ANALYZE is coming up with
statistics to use in query planning, and the numbers are not
expected to always be 100% accurate.  I can think of conditions
which might prevail when seeing the recent LSN.

(1)  The recent LSN is from a cause having nothing to do with the
STO feature, like DML.  As things stand, the behavior is the same
as without the patch -- the rows are counted just the same as
always.  If we did as you suggest, we instead would abort ANALYZE
and have stale statistics.

(2)  The recent LSN is related to STO pruning.  The dead rows are
gone forever, and will not be counted.  This seems more correct
than counting them, even if it were possible, and also superior to
aborting the ANALYZE and leaving stale statistics.

Of course, a subset of (1) is the case that the rows can be
early-pruned at the next opportunity.  In such a case ANALYZE is
still counting them according to the rules that we had before the
snapshot too old feature.  If someone wants to argue that those
rules are wrong, that seems like material for a separate patch.

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

Robert Haas wrote:
> On Wed, Jun 15, 2016 at 1:44 PM, Kevin Grittner <kgrittn@gmail.com>
> wrote:

> > The test I showed creates a situation which (to ANALYZE) is
> > identical to what you describe -- ANALYZE sees a page with an LSN
> > recent enough that it could have been (and actually has been)
> > pruned.  Why would it be better for the ANALYZE to fail than to
> > complete?
>
> As I understand it, the reason we need to sometimes give "ERROR:
> snapshot too old" after early pruning is because we might otherwise
> give the wrong answer.

So what constitutes "the wrong answer"?  A regular transaction reading a
page and not finding a tuple that should have been there but was
removed, is a serious problem and should be aborted.  For ANALYZE it may
not matter a great deal.  Some very old tuple that might have been
chosen for the sample is not there; a different tuple is chosen instead,
so the stats might be slightly difference.  No big deal.

Maybe it is possible to get into trouble if you're taking a sample for
an expression index.

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

On Wed, Jun 15, 2016 at 1:45 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

> Maybe it is possible to get into trouble if you're taking a sample for
> an expression index.

Maybe -- if you are using a function for an index expression which
does an explicit SELECT against some database table rather than
only using values from the row itself.  In such a case you would
have had to mark a function as IMMUTABLE which depends on table
contents.  I say you get to keep both pieces.

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

On Wed, Jun 15, 2016 at 2:45 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Robert Haas wrote:
>> On Wed, Jun 15, 2016 at 1:44 PM, Kevin Grittner <kgrittn@gmail.com>
>> wrote:
>> > The test I showed creates a situation which (to ANALYZE) is
>> > identical to what you describe -- ANALYZE sees a page with an LSN
>> > recent enough that it could have been (and actually has been)
>> > pruned.  Why would it be better for the ANALYZE to fail than to
>> > complete?
>>
>> As I understand it, the reason we need to sometimes give "ERROR:
>> snapshot too old" after early pruning is because we might otherwise
>> give the wrong answer.
>
> So what constitutes "the wrong answer"?  A regular transaction reading a
> page and not finding a tuple that should have been there but was
> removed, is a serious problem and should be aborted.  For ANALYZE it may
> not matter a great deal.  Some very old tuple that might have been
> chosen for the sample is not there; a different tuple is chosen instead,
> so the stats might be slightly difference.  No big deal.
>
> Maybe it is possible to get into trouble if you're taking a sample for
> an expression index.

The expression index case is the one to worry about; if there is a
problem, that's where it is.  What bothers me is that a function used
in an expression index could do anything at all - it can read any
table in the database.

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

Robert Haas wrote:
> On Wed, Jun 15, 2016 at 2:45 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:

> > Maybe it is possible to get into trouble if you're taking a sample for
> > an expression index.
>
> The expression index case is the one to worry about; if there is a
> problem, that's where it is.  What bothers me is that a function used
> in an expression index could do anything at all - it can read any
> table in the database.

Hmm, but if it does that, then the code that actually implements that
query would run the STO checks, right?  The analyze code doesn't need
to.

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

On Wed, Jun 15, 2016 at 1:50 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> The expression index case is the one to worry about; if there is a
> problem, that's where it is.  What bothers me is that a function used
> in an expression index could do anything at all - it can read any
> table in the database.

It *can*, but then you are lying to the database when you call it
IMMUTABLE.  Such an index can easily become corrupted through
normal DML.  Without DML the ANALYZE has no problem.  So you seem
to be concerned that if someone is lying to the database engine to
force it accept a function as IMMUTABLE when it actually isn't, and
then updating the referenced rows (which is very likely to render
the index corrupted), that statistics might also become stale.

They might.

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

Kevin Grittner wrote:
> On Wed, Jun 15, 2016 at 1:50 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> > The expression index case is the one to worry about; if there is a
> > problem, that's where it is.  What bothers me is that a function used
> > in an expression index could do anything at all - it can read any
> > table in the database.
>
> It *can*, but then you are lying to the database when you call it
> IMMUTABLE.  Such an index can easily become corrupted through
> normal DML.  Without DML the ANALYZE has no problem.  So you seem
> to be concerned that if someone is lying to the database engine to
> force it accept a function as IMMUTABLE when it actually isn't, and
> then updating the referenced rows (which is very likely to render
> the index corrupted), that statistics might also become stale.

We actually go quite some lengths to support this case, even when it's
the opinion of many that we shouldn't.  For example VACUUM doesn't try
to find index entries using the values in each deleted tuple; instead we
remember the TIDs and then scan the indexes (possibly many times) to
find entries that match those TIDs -- which is much slower.  Yet we do
it this way to protect the case that somebody is doing the
not-really-IMMUTABLE function.

In other words, I don't think we consider the position you argued as
acceptable.

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

On Wed, Jun 15, 2016 at 1:54 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Robert Haas wrote:

>> The expression index case is the one to worry about; if there is a
>> problem, that's where it is.  What bothers me is that a function used
>> in an expression index could do anything at all - it can read any
>> table in the database.
>
> Hmm, but if it does that, then the code that actually implements that
> query would run the STO checks, right?  The analyze code doesn't need
> to.

Right.  In the described case, you would get a "snapshot too old"
error inside the expression which is trying to generate the index
value.

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

On Wed, Jun 15, 2016 at 1:59 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Kevin Grittner wrote:
>> On Wed, Jun 15, 2016 at 1:50 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>>> The expression index case is the one to worry about; if there is a
>>> problem, that's where it is.  What bothers me is that a function used
>>> in an expression index could do anything at all - it can read any
>>> table in the database.
>>
>> It *can*, but then you are lying to the database when you call it
>> IMMUTABLE.  Such an index can easily become corrupted through
>> normal DML.  Without DML the ANALYZE has no problem.  So you seem
>> to be concerned that if someone is lying to the database engine to
>> force it accept a function as IMMUTABLE when it actually isn't, and
>> then updating the referenced rows (which is very likely to render
>> the index corrupted), that statistics might also become stale.
>
> We actually go quite some lengths to support this case, even when it's
> the opinion of many that we shouldn't.  For example VACUUM doesn't try
> to find index entries using the values in each deleted tuple; instead we
> remember the TIDs and then scan the indexes (possibly many times) to
> find entries that match those TIDs -- which is much slower.  Yet we do
> it this way to protect the case that somebody is doing the
> not-really-IMMUTABLE function.
>
> In other words, I don't think we consider the position you argued as
> acceptable.

What are you saying is unacceptable, and what behavior would be
acceptable instead?

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

On 2016-06-15 14:50:46 -0400, Robert Haas wrote:
> On Wed, Jun 15, 2016 at 2:45 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > Robert Haas wrote:
> >> On Wed, Jun 15, 2016 at 1:44 PM, Kevin Grittner <kgrittn@gmail.com>
> >> wrote:
> >> > The test I showed creates a situation which (to ANALYZE) is
> >> > identical to what you describe -- ANALYZE sees a page with an LSN
> >> > recent enough that it could have been (and actually has been)
> >> > pruned.  Why would it be better for the ANALYZE to fail than to
> >> > complete?
> >>
> >> As I understand it, the reason we need to sometimes give "ERROR:
> >> snapshot too old" after early pruning is because we might otherwise
> >> give the wrong answer.
> >
> > So what constitutes "the wrong answer"?  A regular transaction reading a
> > page and not finding a tuple that should have been there but was
> > removed, is a serious problem and should be aborted.  For ANALYZE it may
> > not matter a great deal.  Some very old tuple that might have been
> > chosen for the sample is not there; a different tuple is chosen instead,
> > so the stats might be slightly difference.  No big deal.
> >
> > Maybe it is possible to get into trouble if you're taking a sample for
> > an expression index.
>
> The expression index case is the one to worry about; if there is a
> problem, that's where it is.  What bothers me is that a function used
> in an expression index could do anything at all - it can read any
> table in the database.

Isn't that also a problem around fetching toast tuples? As we don't
TestForOldSnapshot_impl() for toast, We might fetch a toast tuple which
since have been re-purposed for a datum of a different type. Which can
have arbitrarily bad consequences afaics.

Andres

On Wed, Jun 15, 2016 at 2:20 PM, Andres Freund <andres@anarazel.de> wrote:

> We might fetch a toast tuple which
> since have been re-purposed for a datum of a different type.

How would that happen?

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

Kevin Grittner wrote:
> On Wed, Jun 15, 2016 at 1:59 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:

> > We actually go quite some lengths to support this case, even when it's
> > the opinion of many that we shouldn't.  For example VACUUM doesn't try
> > to find index entries using the values in each deleted tuple; instead we
> > remember the TIDs and then scan the indexes (possibly many times) to
> > find entries that match those TIDs -- which is much slower.  Yet we do
> > it this way to protect the case that somebody is doing the
> > not-really-IMMUTABLE function.
> >
> > In other words, I don't think we consider the position you argued as
> > acceptable.
>
> What are you saying is unacceptable, and what behavior would be
> acceptable instead?

The answer "we don't support the situation where you have an index using
an IMMUTABLE function that isn't actually immutable" is not acceptable.
The acceptable solution would be a design that doesn't have that
property as a requisite.

I think having various executor(/heapam) checks that raise errors when
queries are executed from within ANALYZE is acceptable.  I don't know
about the TOAST related angle Andres just raised.

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

On Wed, Jun 15, 2016 at 2:59 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Kevin Grittner wrote:
>> On Wed, Jun 15, 2016 at 1:50 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> > The expression index case is the one to worry about; if there is a
>> > problem, that's where it is.  What bothers me is that a function used
>> > in an expression index could do anything at all - it can read any
>> > table in the database.
>>
>> It *can*, but then you are lying to the database when you call it
>> IMMUTABLE.  Such an index can easily become corrupted through
>> normal DML.  Without DML the ANALYZE has no problem.  So you seem
>> to be concerned that if someone is lying to the database engine to
>> force it accept a function as IMMUTABLE when it actually isn't, and
>> then updating the referenced rows (which is very likely to render
>> the index corrupted), that statistics might also become stale.
>
> We actually go quite some lengths to support this case, even when it's
> the opinion of many that we shouldn't.  For example VACUUM doesn't try
> to find index entries using the values in each deleted tuple; instead we
> remember the TIDs and then scan the indexes (possibly many times) to
> find entries that match those TIDs -- which is much slower.  Yet we do
> it this way to protect the case that somebody is doing the
> not-really-IMMUTABLE function.
>
> In other words, I don't think we consider the position you argued as
> acceptable.

Well, I actually don't think there's a giant problem here.  I'm just
trying to follow the chain of the argument to its (illogical)
conclusion.  I mean, if ANALYZE itself fails to see a tuple subjected
to early pruning, that should be fine.  And if some query run by a
supposedly-but-not-actually immutable function errors out because
snapshot_too_old is set, that also seems more or less fine.  The
statistics might not get updated, but oh well: either make your
supposedly-immutable function actually immutable, or else turn off
snapshot_too_old, or else live with the fact that ANALYZE will fail
some percentage of the time.  Supporting people who cheat and do
things that technically aren't allowed is one thing; saying that every
new feature must never have any downsides for such people is something
else.  If we took the latter approach, parallel query would be right
out, because you sure can break things by mislabeling functions as
PARALLEL SAFE.  I *do* think it *must* be possible to get an ANALYZE
to do something funky - either error out, or give wrong answers - if
you set up a strange enough set of circumstances, but I don't think
that's necessarily something we need to do anything about.

I think this whole discussion of snapshot too old has provoked far too
many bitter emails -- on all sides.  I entirely believe that there are
legitimate reasons to have concerns about this feature, and I entirely
suspect that it has problems we haven't found yet, and I also entirely
believe that there will be some future bugs that stem from this
feature that we would not have had otherwise.  I think it is entirely
legitimate to have concerns about those things.  On the other hand, I
*also* believe that Kevin is a pretty careful guy and that he's done
his best to make this patch safe and that he did not just go off and
commit something half-baked without due reflection.  We have to expect
that if people who are committers don't get much review of their
patches, they will eventually commit them anyway.  The "I can't
believe you committed this" reactions seem out of line to me.  This
feature is not perfect.  Nor is it the worst thing anybody's ever
committed.  But it's definitely provoked more ire than most.

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

On 2016-06-15 14:24:58 -0500, Kevin Grittner wrote:
> On Wed, Jun 15, 2016 at 2:20 PM, Andres Freund <andres@anarazel.de> wrote:
>
> > We might fetch a toast tuple which
> > since have been re-purposed for a datum of a different type.
>
> How would that happen?

Autovac vacuums toast and heap tables independently. Once a toast datum
isn't used anymore, the oid used can be reused (because it doesn't
conflict via GetNewOidWithIndex() anymore. If analyze then detoasts a
datum, which hasn't been removed, the contents of that toast id, might
actually be for something different.

That's not super likely to happen (given how rare oid wraparounds
usually are), but it appears to be possible.

Regards,

Andres

On Wed, Jun 15, 2016 at 3:25 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-06-15 14:24:58 -0500, Kevin Grittner wrote:
>> On Wed, Jun 15, 2016 at 2:20 PM, Andres Freund <andres@anarazel.de> wrote:
>>
>> > We might fetch a toast tuple which
>> > since have been re-purposed for a datum of a different type.
>>
>> How would that happen?
>
> Autovac vacuums toast and heap tables independently. Once a toast datum
> isn't used anymore, the oid used can be reused (because it doesn't
> conflict via GetNewOidWithIndex() anymore. If analyze then detoasts a
> datum, which hasn't been removed, the contents of that toast id, might
> actually be for something different.

What prevents that from happening now, without STO?

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

On Wed, Jun 15, 2016 at 2:40 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Kevin Grittner wrote:
>> On Wed, Jun 15, 2016 at 1:59 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
>> > We actually go quite some lengths to support this case, even when it's
>> > the opinion of many that we shouldn't.  For example VACUUM doesn't try
>> > to find index entries using the values in each deleted tuple; instead we
>> > remember the TIDs and then scan the indexes (possibly many times) to
>> > find entries that match those TIDs -- which is much slower.  Yet we do
>> > it this way to protect the case that somebody is doing the
>> > not-really-IMMUTABLE function.
>> >
>> > In other words, I don't think we consider the position you argued as
>> > acceptable.
>>
>> What are you saying is unacceptable, and what behavior would be
>> acceptable instead?
>
> The answer "we don't support the situation where you have an index using
> an IMMUTABLE function that isn't actually immutable" is not acceptable.
> The acceptable solution would be a design that doesn't have that
> property as a requisite.
>
> I think having various executor(/heapam) checks that raise errors when
> queries are executed from within ANALYZE is acceptable.

Here is an example of a test case showing that:

-- connection 1
drop table if exists t1;
create table t1 (c1 int not null);
drop table if exists t2;
create table t2 (c1 int not null);
insert into t1 select generate_series(1, 10000);
drop function mysq(i int);
create function mysq(i int)
  returns int
  language plpgsql
  immutable
as $mysq$
begin
  return (i * (select c1 from t2));
end
$mysq$;
insert into t2 values (1);
create index t1_c1sq on t1 ((mysq(c1)));
begin transaction isolation level repeatable read;
select 1;

-- connection 2
vacuum analyze verbose t1;
delete from t1 where c1 between 1000 and 1999;
delete from t1 where c1 = 8000;
update t2 set c1 = 1;

-- connection 1
analyze verbose t1;  -- when run after threshold, STO error occurs

The tail end of that, running the analyze once immediately and once
after the threshold is:

test=# -- connection 1
test=# analyze verbose t1;  -- when run after threshold, STO error occurs
INFO:  analyzing "public.t1"
INFO:  "t1": scanned 45 of 45 pages, containing 8999 live rows and
1001 dead rows; 8999 rows in sample, 8999 estimated total rows
ANALYZE
test=# -- connection 1
analyze verbose t1;  -- when run after threshold, STO error occurs
INFO:  analyzing "public.t1"
INFO:  "t1": scanned 45 of 45 pages, containing 8999 live rows and
1001 dead rows; 8999 rows in sample, 8999 estimated total rows
ERROR:  snapshot too old
CONTEXT:  SQL statement "SELECT (i * (select c1 from t2))"
PL/pgSQL function mysq(integer) line 3 at RETURN

Is there some other behavior which would be preferred?

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

On 2016-06-15 16:58:25 -0500, Kevin Grittner wrote:
> On Wed, Jun 15, 2016 at 3:25 PM, Andres Freund <andres@anarazel.de> wrote:
> > On 2016-06-15 14:24:58 -0500, Kevin Grittner wrote:
> >> On Wed, Jun 15, 2016 at 2:20 PM, Andres Freund <andres@anarazel.de> wrote:
> >>
> >> > We might fetch a toast tuple which
> >> > since have been re-purposed for a datum of a different type.
> >>
> >> How would that happen?
> >
> > Autovac vacuums toast and heap tables independently. Once a toast datum
> > isn't used anymore, the oid used can be reused (because it doesn't
> > conflict via GetNewOidWithIndex() anymore. If analyze then detoasts a
> > datum, which hasn't been removed, the contents of that toast id, might
> > actually be for something different.
>
> What prevents that from happening now, without STO?

Afaics we shouldn't ever look (i.e. detoast) at a "dead for everyone"
tuple in autovacuum (or anywhere else). There's one minor exception to
that, and that's enum datums in indexes, which is why we currently have
weird transactional requirements for them.  I'm not entirely sure this
can be hit, but it's worth checking.

Andres

On Wed, Jun 15, 2016 at 5:34 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-06-15 16:58:25 -0500, Kevin Grittner wrote:
>> On Wed, Jun 15, 2016 at 3:25 PM, Andres Freund <andres@anarazel.de> wrote:
>>> On 2016-06-15 14:24:58 -0500, Kevin Grittner wrote:
>>>> On Wed, Jun 15, 2016 at 2:20 PM, Andres Freund <andres@anarazel.de> wrote:
>>>>
>>>>> We might fetch a toast tuple which
>>>>> since have been re-purposed for a datum of a different type.
>>>>
>>>> How would that happen?
>>>
>>> Autovac vacuums toast and heap tables independently. Once a toast datum
>>> isn't used anymore, the oid used can be reused (because it doesn't
>>> conflict via GetNewOidWithIndex() anymore. If analyze then detoasts a
>>> datum, which hasn't been removed, the contents of that toast id, might
>>> actually be for something different.
>>
>> What prevents that from happening now, without STO?
>
> Afaics we shouldn't ever look (i.e. detoast) at a "dead for everyone"
> tuple in autovacuum (or anywhere else). There's one minor exception to
> that, and that's enum datums in indexes, which is why we currently have
> weird transactional requirements for them.  I'm not entirely sure this
> can be hit, but it's worth checking.

I'm not clear where you see this as being in any way different with
STO.  Above it seemed that you saw this as an issue related to
ANALYZE.  If there is not early pruning for the table being
analyzed, nothing is at all different.  If there is early pruning
the rows are not seen and there could be no detoasting.  If there
is a function that lies about IMMUTABLE and reads from a table, it
either functions as before or throws a STO error on page access
(long before any detoasting).  Am I missing something?

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

On 2016-06-15 20:22:24 -0500, Kevin Grittner wrote:
> I'm not clear where you see this as being in any way different with
> STO.  Above it seemed that you saw this as an issue related to
> ANALYZE.  If there is not early pruning for the table being
> analyzed, nothing is at all different.  If there is early pruning
> the rows are not seen and there could be no detoasting.  If there
> is a function that lies about IMMUTABLE and reads from a table, it
> either functions as before or throws a STO error on page access
> (long before any detoasting).  Am I missing something?

I'm not sure, I might be missing something myself. Given the frequency
of confusion of all senior hackers involved in this discussion...

I previously was thinking of this in the context of ANALYZE, but I now
think it's a bigger problem (and might not really affect ANALYZE
itself).

The simplest version of the scenario I'm concerned about is that a tuple
in a tuple is *not* vacuumed, even though it's elegible to be removed
due to STO. If that tuple has toasted columns, it could be the that the
toast table was independently vacuumed (autovac considers main/toast
tables separately, or the horizon could change between the two heap
scans, or pins could prevent vacuuming of one page, or ...).  Toast
accesses via tuptoaster.c currently don't perform
TestForOldSnapshot_impl(), because they use SnapshotToastData, not
SnapshotMVCC.

That seems to means two things:

1) You might get 'missing chunk number ...' errors on access to toasted
   columns. Misleading error, but ok.

2) Because the tuple has been pruned from the toast table, it's possible
   that the toast oid/va_valueid is reused, because now there's no
   conflict with GetNewOidWithIndex() anymore. In that case the
   toast_fetch_datum() might return a tuple from another column & type
   (all columns in a table share the same toast table), which could lead
   to almost arbitrary problems.  That's not super likely to happen, but
   could have quite severe consequences once it starts.


It seems the easiest way to fix this would be to make
TestForOldSnapshot() "accept" SnapshotToast as well.

Andres

Kevin Grittner wrote:

> test=# -- connection 1
> analyze verbose t1;  -- when run after threshold, STO error occurs
> INFO:  analyzing "public.t1"
> INFO:  "t1": scanned 45 of 45 pages, containing 8999 live rows and
> 1001 dead rows; 8999 rows in sample, 8999 estimated total rows
> ERROR:  snapshot too old
> CONTEXT:  SQL statement "SELECT (i * (select c1 from t2))"
> PL/pgSQL function mysq(integer) line 3 at RETURN
>
> Is there some other behavior which would be preferred?

The fact that the ERROR is being thrown seems okay to me.  I was a bit
surprised that the second INFO line is shown, but there's a simple
explanation: we first acquire the sample rows (using
acquire_sample_rows) and only after that's done we try to compute the
stats from them.

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

On Wed, Jun 15, 2016 at 8:57 PM, Andres Freund <andres@anarazel.de> wrote:

> The simplest version of the scenario I'm concerned about is that a tuple
> in a tuple is *not* vacuumed, even though it's elegible to be removed
> due to STO. If that tuple has toasted columns, it could be the that the
> toast table was independently vacuumed (autovac considers main/toast
> tables separately,

If that were really true, why would we not have the problem in
current production versions that the toast table could be vacuumed
before the heap, leading to exactly the issue you are talking
about?  It seems to me that a simple test shows that it is not the
case that the heap is vacuumed without considering toast:

test=# create table tt (c1 text not null);
CREATE TABLE
test=# insert into tt select repeat(md5(n::text),100000) from (select
generate_series(1,100)) x(n);
INSERT 0 100
test=# delete from tt;
DELETE 100
test=# vacuum verbose tt;
INFO:  vacuuming "public.tt"
INFO:  "tt": removed 100 row versions in 1 pages
INFO:  "tt": found 100 removable, 0 nonremovable row versions in 1 out
of 1 pages
DETAIL:  0 dead row versions cannot be removed yet.
There were 0 unused item pointers.
Skipped 0 pages due to buffer pins.
0 pages are entirely empty.
CPU 0.00s/0.00u sec elapsed 0.00 sec.
INFO:  vacuuming "pg_toast.pg_toast_16552"
INFO:  scanned index "pg_toast_16552_index" to remove 1900 row versions
DETAIL:  CPU 0.00s/0.00u sec elapsed 0.00 sec.
INFO:  "pg_toast_16552": removed 1900 row versions in 467 pages
DETAIL:  CPU 0.00s/0.00u sec elapsed 0.00 sec.
INFO:  index "pg_toast_16552_index" now contains 0 row versions in 8 pages
DETAIL:  1900 index row versions were removed.
5 index pages have been deleted, 0 are currently reusable.
CPU 0.00s/0.00u sec elapsed 0.00 sec.
INFO:  "pg_toast_16552": found 1900 removable, 0 nonremovable row
versions in 467 out of 467 pages
DETAIL:  0 dead row versions cannot be removed yet.
There were 0 unused item pointers.
Skipped 0 pages due to buffer pins.
0 pages are entirely empty.
CPU 0.00s/0.00u sec elapsed 0.00 sec.
VACUUM

> or the horizon could change between the two heap scans,

Not a problem in current production why?

> or pins could prevent vacuuming of one page, or ...).

Not a problem in current production why?

So far I am not seeing any way for TOAST tuples to be pruned in
advance of the referencing heap tuples, nor any way for the problem
you describe to happen in the absence of that.  If you see such,
could you provide a more detailed description or a reproducible
test case?

> Toast accesses via tuptoaster.c currently don't perform
> TestForOldSnapshot_impl(), because they use SnapshotToastData, not
> SnapshotMVCC.
>
> That seems to means two things:
>
> 1) You might get 'missing chunk number ...' errors on access to toasted
>    columns. Misleading error, but ok.
>
> 2) Because the tuple has been pruned from the toast table, it's possible
>    that the toast oid/va_valueid is reused, because now there's no
>    conflict with GetNewOidWithIndex() anymore. In that case the
>    toast_fetch_datum() might return a tuple from another column & type
>    (all columns in a table share the same toast table), which could lead
>    to almost arbitrary problems.  That's not super likely to happen, but
>    could have quite severe consequences once it starts.
>
> It seems the easiest way to fix this would be to make
> TestForOldSnapshot() "accept" SnapshotToast as well.

I don't think that would be appropriate without testing the
characteristics of the underlying table to see whether it should be
excluded.  But is the TOAST data ever updated without an update to
the referencing heap tuple?  If not, I don't see any benefit.  And
we certainly don't want to add some new way to prune TOAST tuples
which might still have referencing heap tuples; that could provide
a route to *create* the problem you describe.

I am on vacation tomorrow (Friday the 17th) through Monday the
27th, so I will be unable to respond to further issues during that
time.  Hopefully I can get this particular issue sorted out before
I go.

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

On 2016-06-16 09:50:09 -0500, Kevin Grittner wrote:
> On Wed, Jun 15, 2016 at 8:57 PM, Andres Freund <andres@anarazel.de> wrote:
>
> > The simplest version of the scenario I'm concerned about is that a tuple
> > in a tuple is *not* vacuumed, even though it's elegible to be removed
> > due to STO. If that tuple has toasted columns, it could be the that the
> > toast table was independently vacuumed (autovac considers main/toast
> > tables separately,
>
> If that were really true, why would we not have the problem in
> current production versions that the toast table could be vacuumed
> before the heap, leading to exactly the issue you are talking
> about?

The issue isn't there without the feature, because we (should) never
access a tuple/detoast a column when it's invisible enough for the
corresponding toast tuple to be vacuumed away. But with
old_snapshot_timeout that's obviously (intentionally) not the case
anymore.  Due to old_snapshot_threshold we'll prune tuples which,
without it, would still be considered HEAPTUPLE_RECENTLY_DEAD.


> It seems to me that a simple test shows that it is not the
> case that the heap is vacuumed without considering toast:

That's why I mentioned autovacuum:
    /*
     * Scan pg_class to determine which tables to vacuum.
     *
     * We do this in two passes: on the first one we collect the list of plain
     * relations and materialized views, and on the second one we collect
     * TOAST tables. The reason for doing the second pass is that during it we
     * want to use the main relation's pg_class.reloptions entry if the TOAST
     * table does not have any, and we cannot obtain it unless we know
     * beforehand what's the main  table OID.
     *
     * We need to check TOAST tables separately because in cases with short,
     * wide tables there might be proportionally much more activity in the
     * TOAST table than in its parent.
     */
...
        tab->at_vacoptions = VACOPT_SKIPTOAST |
            (dovacuum ? VACOPT_VACUUM : 0) |
            (doanalyze ? VACOPT_ANALYZE : 0) |
            (!wraparound ? VACOPT_NOWAIT : 0);
(note the skiptoast)
...
    /*
     * Remember the relation's TOAST relation for later, if the caller asked
     * us to process it.  In VACUUM FULL, though, the toast table is
     * automatically rebuilt by cluster_rel so we shouldn't recurse to it.
     */
    if (!(options & VACOPT_SKIPTOAST) && !(options & VACOPT_FULL))
        toast_relid = onerel->rd_rel->reltoastrelid;
    else
        toast_relid = InvalidOid;
...
    if (toast_relid != InvalidOid)
        vacuum_rel(toast_relid, relation, options, params);


> > or the horizon could change between the two heap scans,
>
> Not a problem in current production why?

Because the horizon will never go to a value which allows "surely dead"
tuples to be read, thus we never detoast columns from a tuple for which
we'd removed toast data. That's why we're performing visibility tests
(hopefully) everywhere, before accessing tuple contents (as opposed to
inspecting the header).


> > or pins could prevent vacuuming of one page, or ...).
>
> Not a problem in current production why?

Same reason.


> So far I am not seeing any way for TOAST tuples to be pruned in
> advance of the referencing heap tuples, nor any way for the problem
> you describe to happen in the absence of that.

Didn't I just list three different ways, only one of which you doubted
the veracity of? Saying "Not a problem in current production why"
doesn't change it being a problem.


> > It seems the easiest way to fix this would be to make
> > TestForOldSnapshot() "accept" SnapshotToast as well.
>
> I don't think that would be appropriate without testing the
> characteristics of the underlying table to see whether it should be
> excluded.

You mean checking whether it's a toast table? We could check that, but
since we never use a toast scan outside of toast, it doesn't seem
necessary.


> But is the TOAST data ever updated without an update to
> the referencing heap tuple?

It shouldn't.


> If not, I don't see any benefit.

Huh?


Greetings,

Andres Freund

On Thu, Jun 16, 2016 at 11:37 AM, Andres Freund <andres@anarazel.de> wrote:
>> If that were really true, why would we not have the problem in
>> current production versions that the toast table could be vacuumed
>> before the heap, leading to exactly the issue you are talking
>> about?
>
> The issue isn't there without the feature, because we (should) never
> access a tuple/detoast a column when it's invisible enough for the
> corresponding toast tuple to be vacuumed away. But with
> old_snapshot_timeout that's obviously (intentionally) not the case
> anymore.  Due to old_snapshot_threshold we'll prune tuples which,
> without it, would still be considered HEAPTUPLE_RECENTLY_DEAD.

Is there really an assumption that the heap and the TOAST heap are
only ever vacuumed with the same OldestXmin value?  Because that seems
like it would be massively flaky.

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

On 2016-06-16 12:02:39 -0400, Robert Haas wrote:
> On Thu, Jun 16, 2016 at 11:37 AM, Andres Freund <andres@anarazel.de> wrote:
> >> If that were really true, why would we not have the problem in
> >> current production versions that the toast table could be vacuumed
> >> before the heap, leading to exactly the issue you are talking
> >> about?
> >
> > The issue isn't there without the feature, because we (should) never
> > access a tuple/detoast a column when it's invisible enough for the
> > corresponding toast tuple to be vacuumed away. But with
> > old_snapshot_timeout that's obviously (intentionally) not the case
> > anymore.  Due to old_snapshot_threshold we'll prune tuples which,
> > without it, would still be considered HEAPTUPLE_RECENTLY_DEAD.
>
> Is there really an assumption that the heap and the TOAST heap are
> only ever vacuumed with the same OldestXmin value?  Because that seems
> like it would be massively flaky.

There's not. They can be vacuumed days apart. But if we vacuum the toast
table with an OldestXmin, and encounter a dead toast tuple, by the
definition of OldestXmin (excluding STO), there cannot be a session
reading the referencing tuple anymore - so that shouldn't matter.

IIRC we actually reverted a patch that caused significant problems
around this. I think there's a small race condition around
ProcessStandbyHSFeedbackMessage(), and you can restart with a different
vacuum_defer_cleanup_age (we should just remove that), but other than
that we shouldn't run into any issues without STO.

On Thu, Jun 16, 2016 at 12:14 PM, Andres Freund <andres@anarazel.de> wrote:
>> > The issue isn't there without the feature, because we (should) never
>> > access a tuple/detoast a column when it's invisible enough for the
>> > corresponding toast tuple to be vacuumed away. But with
>> > old_snapshot_timeout that's obviously (intentionally) not the case
>> > anymore.  Due to old_snapshot_threshold we'll prune tuples which,
>> > without it, would still be considered HEAPTUPLE_RECENTLY_DEAD.
>>
>> Is there really an assumption that the heap and the TOAST heap are
>> only ever vacuumed with the same OldestXmin value?  Because that seems
>> like it would be massively flaky.
>
> There's not. They can be vacuumed days apart. But if we vacuum the toast
> table with an OldestXmin, and encounter a dead toast tuple, by the
> definition of OldestXmin (excluding STO), there cannot be a session
> reading the referencing tuple anymore - so that shouldn't matter.

I don't understand how STO changes that.  I'm not saying it doesn't
change it, but I don't understand why it would.

The root of my confusion is: if we prune a tuple, we'll bump the page
LSN, so any session that is still referencing that tuple will error
out as soon as it touches the page on which that tuple used to exist.
It won't even survive long enough to care that the tuple isn't there
any more.

Maybe it would help if you lay out the whole sequence of events, like:

S1: Does this.
S2: Does that.
S1: Now does something else.

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

On 2016-06-16 12:43:34 -0400, Robert Haas wrote:
> On Thu, Jun 16, 2016 at 12:14 PM, Andres Freund <andres@anarazel.de> wrote:
> >> > The issue isn't there without the feature, because we (should) never
> >> > access a tuple/detoast a column when it's invisible enough for the
> >> > corresponding toast tuple to be vacuumed away. But with
> >> > old_snapshot_timeout that's obviously (intentionally) not the case
> >> > anymore.  Due to old_snapshot_threshold we'll prune tuples which,
> >> > without it, would still be considered HEAPTUPLE_RECENTLY_DEAD.
> >>
> >> Is there really an assumption that the heap and the TOAST heap are
> >> only ever vacuumed with the same OldestXmin value?  Because that seems
> >> like it would be massively flaky.
> >
> > There's not. They can be vacuumed days apart. But if we vacuum the toast
> > table with an OldestXmin, and encounter a dead toast tuple, by the
> > definition of OldestXmin (excluding STO), there cannot be a session
> > reading the referencing tuple anymore - so that shouldn't matter.
>
> I don't understand how STO changes that.  I'm not saying it doesn't
> change it, but I don't understand why it would.

Because we advance OldestXmin more aggressively, while allowing
snapshots that are *older* than OldestXmin to access old tuples on pages
which haven't been touched.


> The root of my confusion is: if we prune a tuple, we'll bump the page
> LSN, so any session that is still referencing that tuple will error
> out as soon as it touches the page on which that tuple used to exist.

Right. On the main table. But we don't peform that check on the toast
table/pages. So if we prune toast tuples, which are still referenced by
(unvacuumed) main relation, we can get into trouble.


> It won't even survive long enough to care that the tuple isn't there
> any more.
>
> Maybe it would help if you lay out the whole sequence of events, like:
>
> S1: Does this.
> S2: Does that.
> S1: Now does something else.

I presume it'd be something like:

Assuming a 'toasted' table, which contains one row, with a 1GB field.

S1: BEGIN REPEATABLE READ;
S1: SELECT SUM(length(one_gb_record)) FROM toasted;
S2: DELETE FROM toasted;
AUTOVAC: vacuum toasted's toast table, it's large. skip toasted, it's small
S1: SELECT SUM(length(one_gb_record)) FROM toasted;
<missing chunk error>

On Thu, Jun 16, 2016 at 12:54 PM, Andres Freund <andres@anarazel.de> wrote:
>> The root of my confusion is: if we prune a tuple, we'll bump the page
>> LSN, so any session that is still referencing that tuple will error
>> out as soon as it touches the page on which that tuple used to exist.
>
> Right. On the main table. But we don't peform that check on the toast
> table/pages. So if we prune toast tuples, which are still referenced by
> (unvacuumed) main relation, we can get into trouble.

OK, if it's true that we don't perform that check on the TOAST table,
then I agree there's a potential problem there.  I don't immediately
know where in the code to look to check that.

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

On 2016-06-16 13:44:34 -0400, Robert Haas wrote:
> On Thu, Jun 16, 2016 at 12:54 PM, Andres Freund <andres@anarazel.de> wrote:
> >> The root of my confusion is: if we prune a tuple, we'll bump the page
> >> LSN, so any session that is still referencing that tuple will error
> >> out as soon as it touches the page on which that tuple used to exist.
> >
> > Right. On the main table. But we don't peform that check on the toast
> > table/pages. So if we prune toast tuples, which are still referenced by
> > (unvacuumed) main relation, we can get into trouble.
>
> OK, if it's true that we don't perform that check on the TOAST table,
> then I agree there's a potential problem there.  I don't immediately
> know where in the code to look to check that.

static inline void
TestForOldSnapshot(Snapshot snapshot, Relation relation, Page page)
{
    Assert(relation != NULL);

    if (old_snapshot_threshold >= 0
        && (snapshot) != NULL
        && (snapshot)->satisfies == HeapTupleSatisfiesMVCC
        && !XLogRecPtrIsInvalid((snapshot)->lsn)
        && PageGetLSN(page) > (snapshot)->lsn)
        TestForOldSnapshot_impl(snapshot, relation);
}

The relevant part is the HeapTupleSatisfiesMVCC check, we're using
SatisfiesToast for toast lookups.


FWIW, I just tried to reproduce this with old_snapshot_threshold = 0 -
but ran into the problem that I couldn't get it to vacuum anything away
(neither main nor toast rel).   That appears to be
        if (old_snapshot_threshold == 0)
        {
            if (TransactionIdPrecedes(latest_xmin, MyPgXact->xmin)
                && TransactionIdFollows(latest_xmin, xlimit))
                xlimit = latest_xmin;
because latest_xmin always is equal to MyPgXact->xmin, which is actually
kinda unsurprising?

Regards,

Andres

On Thu, Jun 16, 2016 at 1:01 PM, Andres Freund <andres@anarazel.de> wrote:

> The relevant part is the HeapTupleSatisfiesMVCC check, we're using
> SatisfiesToast for toast lookups.
>
> FWIW, I just tried to reproduce this with old_snapshot_threshold = 0 -
> but ran into the problem that I couldn't get it to vacuum anything away
> (neither main nor toast rel).   That appears to be
>                 if (old_snapshot_threshold == 0)
>                 {
>                         if (TransactionIdPrecedes(latest_xmin, MyPgXact->xmin)
>                                 && TransactionIdFollows(latest_xmin, xlimit))
>                                 xlimit = latest_xmin;
> because latest_xmin always is equal to MyPgXact->xmin, which is actually
> kinda unsurprising?

Sure -- the STO feature *never* advances the point for early
pruning past the earliest still-active transaction ID.  If it did
we would have all sorts of weird problems.

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

On Thu, Jun 16, 2016 at 11:54 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-06-16 12:43:34 -0400, Robert Haas wrote:

>> The root of my confusion is: if we prune a tuple, we'll bump the page
>> LSN, so any session that is still referencing that tuple will error
>> out as soon as it touches the page on which that tuple used to exist.
>
> Right. On the main table. But we don't peform that check on the toast
> table/pages. So if we prune toast tuples, which are still referenced by
> (unvacuumed) main relation, we can get into trouble.

I thought that we should never be using visibility information from
the toast table; that the visibility information in the heap should
control.  If that's the case, how would we prune toast rows without
pruning the heap?  You pointed out that the *reverse* case has an
option bit -- if that is ever set there could be toasted values
which would not have a row.  Do they still have a line pointer in
the heap, like "dead" index entries?  How are they cleaned up in
current production versions?  (Note the question mark -- I'm not
big on using that with assertions and rarely fall back on
rhetorical questions.)

>> It won't even survive long enough to care that the tuple isn't there
>> any more.
>>
>> Maybe it would help if you lay out the whole sequence of events, like:
>>
>> S1: Does this.
>> S2: Does that.
>> S1: Now does something else.
>
> I presume it'd be something like:
>
> Assuming a 'toasted' table, which contains one row, with a 1GB field.
>
> S1: BEGIN REPEATABLE READ;
> S1: SELECT SUM(length(one_gb_record)) FROM toasted;
> S2: DELETE FROM toasted;
> AUTOVAC: vacuum toasted's toast table, it's large. skip toasted, it's small
> S1: SELECT SUM(length(one_gb_record)) FROM toasted;
> <missing chunk error>

I'll put together a test like that and post in a bit.

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

On 2016-06-16 13:16:35 -0500, Kevin Grittner wrote:
> On Thu, Jun 16, 2016 at 1:01 PM, Andres Freund <andres@anarazel.de> wrote:
>
> > The relevant part is the HeapTupleSatisfiesMVCC check, we're using
> > SatisfiesToast for toast lookups.
> >
> > FWIW, I just tried to reproduce this with old_snapshot_threshold = 0 -
> > but ran into the problem that I couldn't get it to vacuum anything away
> > (neither main nor toast rel).   That appears to be
> >                 if (old_snapshot_threshold == 0)
> >                 {
> >                         if (TransactionIdPrecedes(latest_xmin, MyPgXact->xmin)
> >                                 && TransactionIdFollows(latest_xmin, xlimit))
> >                                 xlimit = latest_xmin;
> > because latest_xmin always is equal to MyPgXact->xmin, which is actually
> > kinda unsurprising?
>
> Sure -- the STO feature *never* advances the point for early
> pruning past the earliest still-active transaction ID.  If it did
> we would have all sorts of weird problems.

Both latest_xmin, MyPgXact->xmin are equivalent to txid_current() here.
Note that a threshold of 1 actually vacuums in this case (after waiting
obviously), but 0 never does.  Afaics that's because before
TransactionIdLimitedForOldSnapshots() is reached,
MaintainOldSnapshotTimeMapping will have updated latest_xmin to the
current value.


With old_snapshot_threshold=1 I indeed can reproduce the issue. I
disabled autovacuum, to make the scheduling more predictable. But it
should "work" just as well with autovacuum.

S1: CREATE TABLE toasted(largecol text);
    INSERT INTO toasted SELECT string_agg(random()::text, '-') FROM generate_series(1, 10000000);
    BEGIN;
    DELETE FROM toasted;
S2: BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> ...
S1: COMMIT;
S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> ...
S1: /* wait for snapshot threshold to be passed */
S1: VACUUM pg_toast.pg_toast_16437;
> INFO:  00000: "pg_toast_16437": found 61942 removable, 0 nonremovable row versions in 15486 out of 15486 pages
> DETAIL:  0 dead row versions cannot be removed yet.
S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
ERROR:  XX000: missing chunk number 0 for toast value 16455 in pg_toast_16437
LOCATION:  toast_fetch_datum, tuptoaster.c:1945

Andres

Hi,

On 2016-06-16 13:19:13 -0500, Kevin Grittner wrote:
> On Thu, Jun 16, 2016 at 11:54 AM, Andres Freund <andres@anarazel.de> wrote:
> > On 2016-06-16 12:43:34 -0400, Robert Haas wrote:
>
> >> The root of my confusion is: if we prune a tuple, we'll bump the page
> >> LSN, so any session that is still referencing that tuple will error
> >> out as soon as it touches the page on which that tuple used to exist.
> >
> > Right. On the main table. But we don't peform that check on the toast
> > table/pages. So if we prune toast tuples, which are still referenced by
> > (unvacuumed) main relation, we can get into trouble.
>
> I thought that we should never be using visibility information from
> the toast table; that the visibility information in the heap should
> control.

We use visibility information for vacuuming, toast vacuum puts toast
chunk tuples through HeapTupleSatisfiesVacuum(), just like for normal
tuples.  Otherwise we'd have to collect dead toast tuples during the
normal vacuum, and then do explicit vacuums for those. That'd end up
being pretty expensive.


> If that's the case, how would we prune toast rows without
> pruning the heap?

I'm not sure what you mean? We prune toast tuples by checking xmin/xmax,
and then comparing with OldestXmin. Without STO that's safe, because we
know nobody could lookup up those toast tuples.


> You pointed out that the *reverse* case has an
> option bit -- if that is ever set there could be toasted values
> which would not have a row.

We vacuum toast tables without the main table, by simply calling
vacuum() on the toast relation.  So you can get the case that only the
normal relation is vacuumed *or* that only the toast relation is vacuumed.


> Do they still have a line pointer in the heap, like "dead" index
> entries?

You can have non-pruned toast tuples, where any evidence of the
referencing main-heap tuple is gone.


> How are they cleaned up in current production versions?

There's simply no interlock except OldestXmin preveting referenced toast
tuples to be vacuumed, as long as any alive snapshot can read them.


Greetings,

Andres Freund

On Thu, Jun 16, 2016 at 1:19 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Thu, Jun 16, 2016 at 11:54 AM, Andres Freund <andres@anarazel.de> wrote:
>> On 2016-06-16 12:43:34 -0400, Robert Haas wrote:

>>> Maybe it would help if you lay out the whole sequence of events, like:
>>>
>>> S1: Does this.
>>> S2: Does that.
>>> S1: Now does something else.
>>
>> I presume it'd be something like:
>>
>> Assuming a 'toasted' table, which contains one row, with a 1GB field.
>>
>> S1: BEGIN REPEATABLE READ;
>> S1: SELECT SUM(length(one_gb_record)) FROM toasted;
>> S2: DELETE FROM toasted;
>> AUTOVAC: vacuum toasted's toast table, it's large. skip toasted, it's small
>> S1: SELECT SUM(length(one_gb_record)) FROM toasted;
>> <missing chunk error>
>
> I'll put together a test like that and post in a bit.

old_snapshot_threshold = '1min'
autovacuum_vacuum_threshold = 0\
autovacuum_vacuum_scale_factor = 0.0000000001

test=# CREATE TABLE gb (rec bytea not null);
CREATE TABLE
test=# ALTER TABLE gb ALTER COLUMN rec SET STORAGE external;
ALTER TABLE
test=# INSERT INTO gb SELECT t FROM (SELECT repeat('x',
1000000000)::bytea) x(t);
INSERT 0 1
test=# BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
BEGIN
test=# SELECT SUM(length(rec)) FROM gb;
    sum
------------
 1000000000
(1 row)

[wait for autovacuum to run]

test=# SELECT SUM(length(rec)) FROM gb;
ERROR:  snapshot too old

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

On Thu, Jun 16, 2016 at 1:32 PM, Andres Freund <andres@anarazel.de> wrote:

> With old_snapshot_threshold=1 I indeed can reproduce the issue. I
> disabled autovacuum, to make the scheduling more predictable. But it
> should "work" just as well with autovacuum.
>
> S1: CREATE TABLE toasted(largecol text);
>     INSERT INTO toasted SELECT string_agg(random()::text, '-') FROM generate_series(1, 10000000);
>     BEGIN;
>     DELETE FROM toasted;
> S2: BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
> S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
>> ...
> S1: COMMIT;
> S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
>> ...
> S1: /* wait for snapshot threshold to be passed */
> S1: VACUUM pg_toast.pg_toast_16437;
>> INFO:  00000: "pg_toast_16437": found 61942 removable, 0 nonremovable row versions in 15486 out of 15486 pages
>> DETAIL:  0 dead row versions cannot be removed yet.
> S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> ERROR:  XX000: missing chunk number 0 for toast value 16455 in pg_toast_16437
> LOCATION:  toast_fetch_datum, tuptoaster.c:1945

Thanks!  That's something I should be able to work with.
Unfortunately, I am going to be on vacation, so I won't have any
results until sometime after 28 June.

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

On 2016-06-16 13:53:01 -0500, Kevin Grittner wrote:
> On Thu, Jun 16, 2016 at 1:19 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
> > On Thu, Jun 16, 2016 at 11:54 AM, Andres Freund <andres@anarazel.de> wrote:
> >> On 2016-06-16 12:43:34 -0400, Robert Haas wrote:
>
> >>> Maybe it would help if you lay out the whole sequence of events, like:
> >>>
> >>> S1: Does this.
> >>> S2: Does that.
> >>> S1: Now does something else.
> >>
> >> I presume it'd be something like:
> >>
> >> Assuming a 'toasted' table, which contains one row, with a 1GB field.
> >>
> >> S1: BEGIN REPEATABLE READ;
> >> S1: SELECT SUM(length(one_gb_record)) FROM toasted;
> >> S2: DELETE FROM toasted;
> >> AUTOVAC: vacuum toasted's toast table, it's large. skip toasted, it's small
> >> S1: SELECT SUM(length(one_gb_record)) FROM toasted;
> >> <missing chunk error>
> >
> > I'll put together a test like that and post in a bit.
>
> old_snapshot_threshold = '1min'
> autovacuum_vacuum_threshold = 0\
> autovacuum_vacuum_scale_factor = 0.0000000001
>
> test=# CREATE TABLE gb (rec bytea not null);
> CREATE TABLE
> test=# ALTER TABLE gb ALTER COLUMN rec SET STORAGE external;
> ALTER TABLE
> test=# INSERT INTO gb SELECT t FROM (SELECT repeat('x',
> 1000000000)::bytea) x(t);
> INSERT 0 1
> test=# BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
> BEGIN
> test=# SELECT SUM(length(rec)) FROM gb;
>     sum
> ------------
>  1000000000
> (1 row)
>
> [wait for autovacuum to run]
>
> test=# SELECT SUM(length(rec)) FROM gb;
> ERROR:  snapshot too old

See https://www.postgresql.org/message-id/20160616183207.wygoktoplycdzav7@alap3.anar
for a recipe that reproduce the issue. I presume your example also
vacuums the main table due to the threshold and scale factor you set
(which will pretty much alwasy vacuum a table, no?).

Andres Freund

On 2016-06-16 11:57:48 -0700, Andres Freund wrote:
> See https://www.postgresql.org/message-id/20160616183207.wygoktoplycdzav7@alap3.anar

For posterity's sake, that was supposed to be
https://www.postgresql.org/message-id/20160616183207.wygoktoplycdzav7@alap3.anarazel.de

On Thu, Jun 16, 2016 at 01:56:44PM -0500, Kevin Grittner wrote:
> On Thu, Jun 16, 2016 at 1:32 PM, Andres Freund <andres@anarazel.de> wrote:
>
> > With old_snapshot_threshold=1 I indeed can reproduce the issue. I
> > disabled autovacuum, to make the scheduling more predictable. But it
> > should "work" just as well with autovacuum.
> >
> > S1: CREATE TABLE toasted(largecol text);
> >     INSERT INTO toasted SELECT string_agg(random()::text, '-') FROM generate_series(1, 10000000);
> >     BEGIN;
> >     DELETE FROM toasted;
> > S2: BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
> > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> >> ...
> > S1: COMMIT;
> > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> >> ...
> > S1: /* wait for snapshot threshold to be passed */
> > S1: VACUUM pg_toast.pg_toast_16437;
> >> INFO:  00000: "pg_toast_16437": found 61942 removable, 0 nonremovable row versions in 15486 out of 15486 pages
> >> DETAIL:  0 dead row versions cannot be removed yet.
> > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> > ERROR:  XX000: missing chunk number 0 for toast value 16455 in pg_toast_16437
> > LOCATION:  toast_fetch_datum, tuptoaster.c:1945
>
> Thanks!  That's something I should be able to work with.
> Unfortunately, I am going to be on vacation, so I won't have any
> results until sometime after 28 June.

This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com

On 2016-06-30 23:51:18 -0400, Noah Misch wrote:
> On Thu, Jun 16, 2016 at 01:56:44PM -0500, Kevin Grittner wrote:
> > On Thu, Jun 16, 2016 at 1:32 PM, Andres Freund <andres@anarazel.de> wrote:
> >
> > > With old_snapshot_threshold=1 I indeed can reproduce the issue. I
> > > disabled autovacuum, to make the scheduling more predictable. But it
> > > should "work" just as well with autovacuum.
> > >
> > > S1: CREATE TABLE toasted(largecol text);
> > >     INSERT INTO toasted SELECT string_agg(random()::text, '-') FROM generate_series(1, 10000000);
> > >     BEGIN;
> > >     DELETE FROM toasted;
> > > S2: BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
> > > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> > >> ...
> > > S1: COMMIT;
> > > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> > >> ...
> > > S1: /* wait for snapshot threshold to be passed */
> > > S1: VACUUM pg_toast.pg_toast_16437;
> > >> INFO:  00000: "pg_toast_16437": found 61942 removable, 0 nonremovable row versions in 15486 out of 15486 pages
> > >> DETAIL:  0 dead row versions cannot be removed yet.
> > > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> > > ERROR:  XX000: missing chunk number 0 for toast value 16455 in pg_toast_16437
> > > LOCATION:  toast_fetch_datum, tuptoaster.c:1945
> >
> > Thanks!  That's something I should be able to work with.
> > Unfortunately, I am going to be on vacation, so I won't have any
> > results until sometime after 28 June.
>
> This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com

IIRC Kevin is out of the office this week, so this'll have to wait till
next week.

Andres