Testing the async-commit patch

So I was testing my fix for the problem noted here:
http://archives.postgresql.org/pgsql-hackers/2007-08/msg00196.php
and promptly found *another* bug. To wit, that repair_frag calls
HeapTupleSatisfiesVacuum without bothering to acquire any buffer
content lock. This results in an Assert failure inside
SetBufferCommitInfoNeedsSave, if HeapTupleSatisfiesVacuum tries to
update any hint bits for the tuple. I think that is impossible in
current releases, because the tuple's logical status was fully
determined by the prior call in scan_heap. But it's possible as of
8.3 because the walwriter or other backends could have moved the WAL
flush point, allowing a previously unhintable XMAX to become hintable.

I think the best solution for this is to acquire the buffer content lock
before calling HeapTupleSatisfiesVacuum --- it's really a pretty ugly
shortcut that the code didn't do that already. We could alternatively
refuse to do shrinking unless both XMIN and XMAX are correctly hinted
at scan_heap time; but there is not anything else in vacuum.c that seems
to require XMAX_COMMITTED to be set, so I'd rather not make that
restriction.

But to get to the point: the urgency of testing the patch more
extensively has just moved up a full order of magnitude, IMHO anyway.
I muttered something in the other thread about providing a buildfarm
option to run the regression tests with synchronous_commit off. That
would still be a good idea in the long run, but I want to take some more
drastic measures now. I propose that we actually set synchronous_commit
off by default for the next little while --- at least up to 8.3beta1,
maybe until we approach the RC point. That will ensure that every
buildfarm machine is exercising the async-commit behavior, as well as
every developer who's testing HEAD.

Of course the risk is that we might forget to turn it back on before
release :-(

Comments?

regards, tom lane

Tom Lane wrote:
> But to get to the point: the urgency of testing the patch more
> extensively has just moved up a full order of magnitude, IMHO anyway.
> I muttered something in the other thread about providing a buildfarm
> option to run the regression tests with synchronous_commit off. That
> would still be a good idea in the long run, but I want to take some more
> drastic measures now. I propose that we actually set synchronous_commit
> off by default for the next little while --- at least up to 8.3beta1,
> maybe until we approach the RC point. That will ensure that every
> buildfarm machine is exercising the async-commit behavior, as well as
> every developer who's testing HEAD.
>
> Of course the risk is that we might forget to turn it back on before
> release :-(
>
>
>

Turn it off, I doubt we'll forget.

I have some ideas about testing configuration items. Doing all our tests
with the default config is not ideal, I think. Essentially we'd put up a
server that would have sets of <branch, list-of-config-lines>. The
client would connect to the server if it could and get the set(s) of
lines for the branch on question, and for each set it would try another
run of installcheck (I'm also wondering if we should switch to doing
installcheck-parallel). Anyway, this would be a config option on the
buildfarm, so we wouldn't overburden hosts with limited run windows
(e.g. the Solaris boxes Sun has on the farm) or slow run times (e.g.
some of the old and/or tiny hardware we have).

If this seems worth it I'll put it on my TODO.

cheers

andrew

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> But to get to the point: the urgency of testing the patch more
> extensively has just moved up a full order of magnitude, IMHO anyway.
> I muttered something in the other thread about providing a buildfarm
> option to run the regression tests with synchronous_commit off. That
> would still be a good idea in the long run, but I want to take some more
> drastic measures now. I propose that we actually set synchronous_commit
> off by default for the next little while --- at least up to 8.3beta1,
> maybe until we approach the RC point. That will ensure that every
> buildfarm machine is exercising the async-commit behavior, as well as
> every developer who's testing HEAD.
>
> Of course the risk is that we might forget to turn it back on before
> release :-(

I'll set a cron job to remind us. What date should I set it for? :)

Seems like a fine plan to me. It's supposed to be 100% reliable and have
indistinguishable behaviour barring a system crash and nobody should be
running production data on a beta or pre-beta build, so they should never see
a difference.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I have some ideas about testing configuration items. Doing all our tests
> with the default config is not ideal, I think. Essentially we'd put up a
> server that would have sets of <branch, list-of-config-lines>. The
> client would connect to the server if it could and get the set(s) of
> lines for the branch on question, and for each set it would try another
> run of installcheck (I'm also wondering if we should switch to doing
> installcheck-parallel). Anyway, this would be a config option on the
> buildfarm, so we wouldn't overburden hosts with limited run windows
> (e.g. the Solaris boxes Sun has on the farm) or slow run times (e.g.
> some of the old and/or tiny hardware we have).

> If this seems worth it I'll put it on my TODO.

Sounds like a good plan, except that an extra server seems unnecessary
mechanism (and perhaps an unnecessary security risk). We can just put
a file into CVS src/test/regress showing what we'd like tested.

regards, tom lane

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> I propose that we actually set synchronous_commit
>> off by default for the next little while --- at least up to 8.3beta1,
>> maybe until we approach the RC point. That will ensure that every
>> buildfarm machine is exercising the async-commit behavior, as well as
>> every developer who's testing HEAD.
>>
>> Of course the risk is that we might forget to turn it back on before
>> release :-(

> I'll set a cron job to remind us. What date should I set it for? :)

I thought of a low-tech solution for that: put a note in
src/tools/RELEASE_CHANGES about it. We invariably look at that file
while preparing releases.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> I have some ideas about testing configuration items. Doing all our tests
>> with the default config is not ideal, I think. Essentially we'd put up a
>> server that would have sets of <branch, list-of-config-lines>. The
>> client would connect to the server if it could and get the set(s) of
>> lines for the branch on question, and for each set it would try another
>> run of installcheck (I'm also wondering if we should switch to doing
>> installcheck-parallel). Anyway, this would be a config option on the
>> buildfarm, so we wouldn't overburden hosts with limited run windows
>> (e.g. the Solaris boxes Sun has on the farm) or slow run times (e.g.
>> some of the old and/or tiny hardware we have).
>>
>
>
>> If this seems worth it I'll put it on my TODO.
>>
>
> Sounds like a good plan, except that an extra server seems unnecessary
> mechanism (and perhaps an unnecessary security risk). We can just put
> a file into CVS src/test/regress showing what we'd like tested.
>
>
>

That could work.

Let's say that this file looks just like a postgresql.conf file, except
that any line beginning with '[<identifier]>' is a config set name for
the lines that follow. So we might have:

[asynch_commit]
synchronous_commit = off

[no_fsync]
fsync = off

[csvlogs]
start_log_collector = true
log_destination = 'stderr, csvlog'

Then there would be an extra installcheck-parallel run for each set. If
the file isn't there we do nothing.

cheers

andrew

On Mon, 2007-08-13 at 16:27 -0400, Andrew Dunstan wrote:

> Let's say that this file looks just like a postgresql.conf file, except
> that any line beginning with '[<identifier]>' is a config set name for
> the lines that follow. So we might have:
>
> [asynch_commit]
> synchronous_commit = off
>
> [no_fsync]
> fsync = off
>
> [csvlogs]
> start_log_collector = true
> log_destination = 'stderr, csvlog'
>
> Then there would be an extra installcheck-parallel run for each set. If
> the file isn't there we do nothing.

Sounds fine, though I'd prefer this in XML to allow further
extensibility in the future.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> But to get to the point: the urgency of testing the patch more
> extensively has just moved up a full order of magnitude,

The problem testing this patch is that the window for a committed transaction
to not be synced is quite narrow, especially for the regression tests. For
testing purposes I wonder if there are ways we can widen this window. Some
ideas, some wackier than others, are:

. Raise the default wal_writer_delay to 5s or so -- also temporary until
release

. Add an ifdef USE_ASSERT_CHECKING which randomly omits setting hint bits even
when it could.

. add an ifdef USE_ASSERT_CHECKING which randomly fails to update the LSN when
syncing WAL so that even after a buffer flush we still can't set hint bits.

Only the first one isn't really wacky, but perhaps there's something there.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

Simon Riggs wrote:
> On Mon, 2007-08-13 at 16:27 -0400, Andrew Dunstan wrote:
>
> > Let's say that this file looks just like a postgresql.conf file, except
> > that any line beginning with '[<identifier]>' is a config set name for
> > the lines that follow. So we might have:
> >
> > [asynch_commit]
> > synchronous_commit = off
> >
> > [no_fsync]
> > fsync = off
> >
> > [csvlogs]
> > start_log_collector = true
> > log_destination = 'stderr, csvlog'
> >
> > Then there would be an extra installcheck-parallel run for each set. If
> > the file isn't there we do nothing.
>
> Sounds fine, though I'd prefer this in XML to allow further
> extensibility in the future.

YAML?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Simon Riggs wrote:
>> Sounds fine, though I'd prefer this in XML to allow further
>> extensibility in the future.

> YAML?

That would seem to require making pg_regress depend on some XML library
or other, which is a dependency I'd rather not add.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>
>> Simon Riggs wrote:
>>
>>> Sounds fine, though I'd prefer this in XML to allow further
>>> extensibility in the future.
>>>
>
>
>> YAML?
>>
>
> That would seem to require making pg_regress depend on some XML library
> or other, which is a dependency I'd rather not add.
>
>
>

Yeah, I think the way I set it out would work just fine for the intended
purpose. XML, YAML, JSON et al are all well suited to tree structured
data. But what we're describing here isn't tree structured. It is simply
some named sets of postgresql.conf directives. As such, it would be best
if it were as close as possible to actual postgresql.conf syntax.

And I am also reluctant to add an additional dependency onto the
buildfarm script. (I wasn't actually thinking of doing this throught
pg_regress, but I'm open to persuasion).

cheers

andrew

Tom,

We're getting some additional test infrastructre at Sun; I'll throw this on
the pile of stuff to test.

However, the current tests we're doing are regression tests and benchmark
runs. If there's some other kind of testing we need to do, I'll need
specifics.

--
Josh Berkus
PostgreSQL @ Sun
San Francisco

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> The problem testing this patch is that the window for a committed transaction
> to not be synced is quite narrow, especially for the regression tests. For
> testing purposes I wonder if there are ways we can widen this window. Some
> ideas, some wackier than others, are:

> . Raise the default wal_writer_delay to 5s or so -- also temporary until
> release

I ran 100+ cycles of the parallel regression tests with this setting,
and didn't see any failures.

> . Add an ifdef USE_ASSERT_CHECKING which randomly omits setting hint
> bits even when it could.

I think this is better done by code inspection, ie, look for places that
assume HEAP_XMIN/XMAX_COMMITTED is or can be set.

I made a pass over CVS HEAD and found some apparent trouble spots:
heapam.c lines 1843-1852 presume previous xmax can be hinted
immediately, ditto lines 2167-2176, ditto lines 2716-2725.
I think probably we should just remove those lines --- they are only
trying to save work for future tqual.c calls.

regards, tom lane

On Tue, 2007-08-14 at 12:09 -0400, Tom Lane wrote:

> I think this is better done by code inspection, ie, look for places that
> assume HEAP_XMIN/XMAX_COMMITTED is or can be set.
>
> I made a pass over CVS HEAD and found some apparent trouble spots:
> heapam.c lines 1843-1852 presume previous xmax can be hinted
> immediately, ditto lines 2167-2176, ditto lines 2716-2725.
> I think probably we should just remove those lines --- they are only
> trying to save work for future tqual.c calls.

I'll check those out later tonight.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> On Tue, 2007-08-14 at 12:09 -0400, Tom Lane wrote:
>> heapam.c lines 1843-1852 presume previous xmax can be hinted
>> immediately, ditto lines 2167-2176, ditto lines 2716-2725.
>> I think probably we should just remove those lines --- they are only
>> trying to save work for future tqual.c calls.

> I'll check those out later tonight.

[ looks closer ] Actually, we can't just dike out those code sections,
because the immediately following code assumes that XMAX_INVALID is
correct. So I guess we need to export HeapTupleSetHintBits from tqual.c
and do the full pushup in these places.

regards, tom lane

On Tue, 2007-08-14 at 12:29 -0400, Tom Lane wrote:
> "Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> > On Tue, 2007-08-14 at 12:09 -0400, Tom Lane wrote:
> >> heapam.c lines 1843-1852 presume previous xmax can be hinted
> >> immediately, ditto lines 2167-2176, ditto lines 2716-2725.
> >> I think probably we should just remove those lines --- they are only
> >> trying to save work for future tqual.c calls.
>
> > I'll check those out later tonight.
>
> [ looks closer ] Actually, we can't just dike out those code sections,
> because the immediately following code assumes that XMAX_INVALID is
> correct. So I guess we need to export HeapTupleSetHintBits from tqual.c
> and do the full pushup in these places.

[ without looking ] XMAX_INVALID is always set if we know it, so that
wouldn't be a reason to do that.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

On Mon, 2007-08-13 at 16:27 -0400, Andrew Dunstan wrote:
> [asynch_commit]
> synchronous_commit = off
> [no_fsync]
> fsync = off

This is the Windows INI file format. As such, it's easy to find code
samples in almost any language that parse this format for you. For
example, Python has a core library called ConfigParser that will read
these in, and somewhere around here I even have some UNIX shell code that
parses it. There's already a PostgreSQL-related project using this
format--even on UNIX systems the odbc.ini config files look like this.

I already have a program that generates multiple postgresql.conf files
using this format around here for exactly this sort of test (compare
benchmark results with two different configurations), just never had a
reason to package it up for anybody else to use. It's trivial code if you
use the Python parser.

On Tue, 14 Aug 2007, Simon Riggs wrote:

> Sounds fine, though I'd prefer this in XML to allow further
> extensibility in the future.

Putting this in XML significantly raises the bar for how complicated tools
that work on these files must be, with the implicit dependencies that go
with that. And as Andrew already pointed out, there is very little
tree-structure to this data that justifies the extra complexity.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

On Tue, 2007-08-14 at 12:29 -0400, Tom Lane wrote:
> "Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> > On Tue, 2007-08-14 at 12:09 -0400, Tom Lane wrote:
> >> heapam.c lines 1843-1852 presume previous xmax can be hinted
> >> immediately, ditto lines 2167-2176, ditto lines 2716-2725.
> >> I think probably we should just remove those lines --- they are only
> >> trying to save work for future tqual.c calls.
>
> > I'll check those out later tonight.
>
> [ looks closer ] Actually, we can't just dike out those code sections,
> because the immediately following code assumes that XMAX_INVALID is
> correct. So I guess we need to export HeapTupleSetHintBits from tqual.c
> and do the full pushup in these places.

Looks good: the code is neater and better commented than before as well
as being fully accurate with async commit.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com