Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

On Tuesday, September 15, 2015 12:07 PM, Alvaro Herrera  wrote:
> Kevin Grittner wrote:
>> Alvaro Herrera  wrote:

>>> I would place it inside src/test/modules, so that buildfarm
>>> runs it automatically; I'm not sure it will pick up things in
>>> src/test.
>>
>> As it stands now, the test is getting run as part of `make
>> check-world`, and it seems like src/test/modules is about testing
>> separate executables, so I don't think it makes sense to move the
>> tests -- but I could be convinced that I'm missing something.
>
> It's not conceived just as a way to test separate executables; maybe it
> is at the moment (though I don't think it is?) but if so that's just an
> accident.  The intention is to have modules that get tested without them
> being installed, which wasn't the case when they were in contrib.
>
> The problem with check-world is that buildfarm doesn't run it.  We don't
> want to set up separate buildfarm modules for each subdir in src/test;
> that would be pretty tedious.

OK, moved.

All other issues raised by Álvaro and Steve have been addressed,
except for this one, which I will argue against:

> So if I understand correctly, every call to BufferGetPage needs to have
> a TestForOldSnapshot immediately afterwards?  It seems easy to later
> introduce places that fail to test for old snapshots.  What happens if
> they do?  Does vacuum remove tuples anyway and then the query returns
> wrong results?  That seems pretty dangerous.  Maybe the snapshot could
> be an argument of BufferGetPage?

There are 486 occurences of BufferGetPage in the source code, and
this patch follows 36 of them with TestForOldSnapshot.  This only
needs to be done when a page is going to be used for a scan which
produces user-visible results.  That is, it is *not* needed for
positioning within indexes to add or vacuum away entries, for heap
inserts or deletions (assuming the row to be deleted has already
been found).  It seems wrong to modify about 450 BufferGetPage
references to add a NULL parameter; and if we do want to make that
noop change as insurance, it seems like it should be a separate
patch, since the substance of this patch would be buried under the
volume of that.

I will add this to the November CF.

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

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

snapshot too old, configured by time

As discussed when the "proof of concept" patch was submitted during
9.5 development, here is a version intended to be considered for
commit to 9.6, with the following changes:

1. It is configured using time rather than number of transactions.
Not only was there unanimous agreement here that this was better,
but the EDB customer who had requested this feature and who had
been testing it independently made the same request.

2. The "proof of concept" patch only supported heap and btree
checking; this supports all index types.

3. Documentation has been added.

4. Tests have been added.  They are currently somewhat minimal,
since this is using a whole new technique for testing from any
existing committed tests -- I wanted to make sure that this
approach to testing was OK with everyone before expanding it.  If
it is, I assume we will want to move some of the more generic
portions to a .pm file to make it available for other tests.

Basically, this patch aims to limit bloat when there are snapshots
that are kept registered for prolonged periods.  The immediate
reason for this is a customer application that keeps read-only
cursors against fairly static data open for prolonged periods, and
automatically fields SQLSTATE 72000 to re-open them if necessary.
When used, it should also provide some protections against extreme
bloat from forgotten "idle in transaction" connections which are
left holding a snapshot.

Once a snapshot reaches the age threshold, it can be terminated if
reads data modified after the snapshot was built.  It is expected
that useful ranges will normally be somewhere from a few hours to a
few days.

By default old_snapshot_threshold is set to -1, which disables the
new behavior.

The customer has been testing a preliminary version of this
time-based patch for several weeks, and is happy with the results
-- it is preventing bloat for them and not generating "snapshot too
old" errors at unexpected times.

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

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

On 08/31/2015 10:07 AM, Kevin Grittner wrote:


Kevin,

I've started to do a review on this patch but I am a bit confused with 
some of what I am seeing.

The attached testcase fails I  replace the cursor in your test case with 
direct selects from the table.
I would have expected this to generate the snapshot too old error as 
well but it doesn't.



#   Failed test 'expect "snapshot too old" error'
#   at t/002_snapshot_too_old_select.pl line 64.
#          got: ''
#     expected: '72000'
# Looks like you failed 1 test of 9.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/9 subtests

Am I misunderstanding something or is the patch not working as expected?




> As discussed when the "proof of concept" patch was submitted during
> 9.5 development, here is a version intended to be considered for
> commit to 9.6, with the following changes:
>
> 1. It is configured using time rather than number of transactions.
> Not only was there unanimous agreement here that this was better,
> but the EDB customer who had requested this feature and who had
> been testing it independently made the same request.
>
> 2. The "proof of concept" patch only supported heap and btree
> checking; this supports all index types.
>
> 3. Documentation has been added.
>
> 4. Tests have been added.  They are currently somewhat minimal,
> since this is using a whole new technique for testing from any
> existing committed tests -- I wanted to make sure that this
> approach to testing was OK with everyone before expanding it.  If
> it is, I assume we will want to move some of the more generic
> portions to a .pm file to make it available for other tests.
>
> Basically, this patch aims to limit bloat when there are snapshots
> that are kept registered for prolonged periods.  The immediate
> reason for this is a customer application that keeps read-only
> cursors against fairly static data open for prolonged periods, and
> automatically fields SQLSTATE 72000 to re-open them if necessary.
> When used, it should also provide some protections against extreme
> bloat from forgotten "idle in transaction" connections which are
> left holding a snapshot.
>
> Once a snapshot reaches the age threshold, it can be terminated if
> reads data modified after the snapshot was built.  It is expected
> that useful ranges will normally be somewhere from a few hours to a
> few days.
>
> By default old_snapshot_threshold is set to -1, which disables the
> new behavior.
>
> The customer has been testing a preliminary version of this
> time-based patch for several weeks, and is happy with the results
> -- it is preventing bloat for them and not generating "snapshot too
> old" errors at unexpected times.
>
> --
> Kevin Grittner
> EDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

New patch just to merge in recent commits -- it was starting to
show some bit-rot.  Tests folded in with main patch.

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

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

On Fri, Jan 8, 2016 at 5:22 PM, Alvaro Herrera  wrote:

>> People have said that issuing SQL commands directly from a TAP test
>> via DBD::Pg is not acceptable for a core feature, and (despite
>> assertions to the contrary) I see no way to test this feature with
>> existing testing mechanisms.  The bigger set of work here, if we
>> don't want this feature to go in without any testing scripts (which
>> is not acceptable IMO), is to enhance the isolation tester or
>> hybridize TAP testing with the isolation tester.
>
> Is it possible to use the PostgresNode stuff to test this?  If not,
> perhaps if you restate what additional capabilities you need we could
> look into adding them there.  I suspect that what you need is the
> ability to keep more than one session open and feed them commands;
> perhaps we could have the framework have a function that opens a psql
> process and returns a FD to which the test program can write, using the
> IPC::Run stuff (start / pump / finish).

Resubmitting for the March CF.

The main thing that changed is that I can now run all the
regression and isolation tests using installcheck with
old_snapshot_threshold = 0 and get a clean run.  That probably gets
better overall coverage than specific tests to demonstrate the
"snapshot too old" error, but of course we need those, too.  While
I can do that with hand-run psql sessions or through connectors
from different languages, I have not been able to wrangle the
testing tools we support through the build system into working for
this purpose.  (I had been hoping that the recent improvements to
the TAP testing libraries would give me the traction to get there,
but either it's still not there or my perl-fu is just too weak to
figure out how to use those features -- suggestions welcome.)

Basically, a connection needs to remain open and interleave
commands with other connections, which the isolation tester does
just fine; but it needs to do that using a custom postgresql.conf
file, which TAP does just fine.  I haven't been able to see the
right way to get a TAP test to set up a customized installation to
run isolation tests against.  If I can get that working, I have
additional tests I can drop into that.

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

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

On Tue, Mar 1, 2016 at 12:58 AM, Michael Paquier
 wrote:
> On Tue, Mar 1, 2016 at 9:35 AM, Andres Freund  wrote:
>> On 2016-02-29 18:30:27 -0600, Kevin Grittner wrote:
>>> Basically, a connection needs to remain open and interleave
>>> commands with other connections, which the isolation tester does
>>> just fine; but it needs to do that using a custom postgresql.conf
>>> file, which TAP does just fine.  I haven't been able to see the
>>> right way to get a TAP test to set up a customized installation to
>>> run isolation tests against.  If I can get that working, I have
>>> additional tests I can drop into that.

>> Check contrib/test_decoding's makefile. It does just that with
>> isolationtester.
>
> pg_isolation_regress --temp-config is the key item here, you can
> enforce a test to run on a server with a wanted configuration set.

Thanks for the tips.  Attached is a minimal set of isolation tests.
I can expand on it if needed, but wanted:

(1) to confirm that this is the right way to do this, and

(2) how long people were willing to tolerate these tests running.

Since we're making this time-based (by popular demand), there must
be delays to see the new behavior.  This very minimal pair of tests
runs in just under one minute on my i7.  Decent coverage of all the
index AMs would probably require tests which run for at least 10
minutes, and probably double that.  I don't recall any satisfactory
resolution to prior discussions about long-running tests.

This is a follow-on patch, just to add isolation testing; the prior
patch must be applied, too.

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

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

On Wed, Mar 30, 2016 at 3:26 PM, Alvaro Herrera
 wrote:
> Kevin Grittner wrote:
>> On Wed, Mar 30, 2016 at 2:22 PM, Alvaro Herrera  wrote:
>
>> > I said that we should change BufferGetPage into having the snapshot
>> > check built-in, except in the cases where a flag is passed; and the flag
>> > would be passed in all cases except those 30-something you identified.
>> > In other words, the behavior in all the current callsites would be
>> > identical to what's there today; we could have a macro do the first
>> > check so that we don't introduce the overhead of a function call in the
>> > 450 cases where it's not needed.
>>
>> In many of the places that BufferGetPage is called there is not a
>> snapshot available.  I assume that you would be OK with an Assert
>> that the flag was passed if the snapshot is NULL?
>
> Sure, that's fine.
>
> BTW I said "a macro" but I was forgetting that we have static inline
> functions in headers now, which means you can avoid the horrors of
> actually writing a macro.

Attached is what I think you're talking about for the first patch.
AFAICS this should generate identical executable code to unpatched.
Then the patch to actually implement the feature would, instead
of adding 30-some lines with TestForOldSnapshot() would implement
that as the behavior for the other enum value, and alter those
30-some BufferGetPage() calls.

Álvaro and Michael, is this what you were looking for?

Is everyone else OK with this approach?

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

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

On Sun, Apr 3, 2016 at 4:09 PM, Jeff Janes  wrote:
> On Wed, Mar 30, 2016 at 12:34 PM, Kevin Grittner  wrote:
>> On Sat, Mar 19, 2016 at 1:27 AM, Jeff Janes  wrote:

>>> I set the value to 1min.
>>>
>>> I set up a test like this:
>>>
>>> pgbench -i
>>>
>>> pgbench -c4 -j4 -T 3600 &
>>>
>>> ### watch the size of branches table
>>> while (true) ; do psql -c "\dt+" | fgrep _branches; sleep 10; done &
>>>
>>> ### set up a long lived snapshot.
>>> psql -c 'begin; set transaction isolation level repeatable read;
>>> select sum(bbalance) from pgbench_branches; select pg_sleep(300);
>>> select sum(bbalance) from pgbench_branches;'
>>>
>>> As this runs, I can see the size of the pgbench_branches bloating once
>>> the snapshot is taken, and continues bloating at a linear rate for the
>>> full 300 seconds.

I'm not seeing that on my i7 box.

>>> Once the 300 second pg_sleep is up, the long-lived snapshot holder
>>> receives an error once it tries to access the table again, and then
>>> the bloat stops increasing.  But shouldn't the bloat have stopped
>>> increasing as soon as the snapshot became doomed, which would be after
>>> a minute or so?

It will, limited by how well your autovacuum can keep up with the
workload on your system.  See attached graph.  I ran 5 times each
in 3 configurations and graphed the median and average of be each.

master: development checkout from today, no config changes

patch: patch with no config changes except:
  old_snapshot_threshold = '1min'

patch + av: patch with these config changes:
  old_snapshot_threshold = '1min'
  autovacuum_max_workers = 8
  autovacuum_vacuum_cost_limit = 2000
  autovacuum_naptime = '10s'
  autovacuum_work_mem = '1GB'

As expected, differences are minimal at first, then the patch
starts to win, and wins even better with more aggressive
autovacuum.

> I can verify that a manual vacuum does stop the bloat from continuing
> to increase.  But I don't see why autovacuum is not already stopping
> the bloat.  It is running often enough that it really ought to do so
> (as verified by setting log_autovacuum_min_duration = 0 and looking in
> the log files to see that it is vacuuming the table once per nap-time,
> although it is not accomplishing much by doing so as no tuples can be
> removed.)

Perhaps the CPUs I have or the way I have my machine tuned allows
autovacuum to be more effective in the face of the pgbench load
than on yours?

> Also, HOT-cleanup should stop the bloat increase once the snapshot
> crosses the old_snapshot_threshold without even needing to wait until
> the next autovac runs.

It should help some, but you really need a vacuum in there to take
care things thoroughly.

> Does the code intentionally only work for manual vacuums?  If so, that
> seems quite surprising.  Or perhaps I am missing something else here.

Perhaps it is that VACUUM tries harder to get the work done, while
autovacuum steps out of the way when it detects that it is blocking
something.  This is a pretty small table (it never gets to 1MB even
when bloating) with multiple clients pounding on it.  It might just
be that your system doesn't allow much autovacuum activity before
it find itself blocking a pgbench process.

FWIW, our customer's 30-day test runs were on databases of hundreds
of GB and showed similar benefits -- linear growth indefinitely
without the patch, settling in to a pretty steady state after a few
hours with the patch.

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

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time

Re: snapshot too old, configured by time