Prev: [HACKERS] keepalive in libpq using
Next: [HACKERS] Does mbutils.c really need to use L'\0' ?
From: Marc Cousin on 6 Jul 2010 12:35
2010/2/10 Takahiro Itagaki <itagaki.takahiro(a)oss.ntt.co.jp>

>
> Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
>
> > Takahiro Itagaki <itagaki.takahiro(a)oss.ntt.co.jp> writes:
> > > We have an optimization to bulkload date in pg_restore, but the code
> > > only works in parallel restore (--jobs >= 2). Why don't we do the
> > > same optimization in the serial restore (--jobs = 1) ?
> >
> > The code is only trying to substitute for something you can't have
> > in parallel restore, ie --single-transaction.
>
> Yeah, the comment says so. But it does not necessarily mean that
> we cannot optimize the copy also in non-single-transaction restore.
>
> The attached patch improve the judgment condition,
> I'll add it to the next commit-fest.
>
>
Hi

This is my first review, so I hope I won't do too many things wrong. Please
tell me how to improve.

I've taken all the points from the 'reviewing a patch' wiki page. But to
sum up what is below, I did see a large performance improvement (in a very
simple test case) and no problems with the patch.

Submission review
Is the patch in context diff format?
Yes

Does it apply cleanly to the current CVS HEAD?
Yes

Does it include reasonable tests, necessary doc patches, etc?
Not applicable: two lines updated
Usability review

Not applicable, doesn't change the use of pg_restore

Read what the patch is supposed to do, and consider:
Does the patch actually implement that?
Yes: it wraps the truncate table + copy in a single transaction when
doing pg_restore -c

Do we want that?
I think so, see the improvements below. And it makes the performance
consistent between -j, -1 and 'simple' uses of pg_restore.

Do we already have it?
No

Does it follow SQL spec, or the community-agreed behavior?
Yes: if this is the way to do restore with -j, there is no point in not
doing it with simple restores

Does it include pg_dump support (if applicable)?
Not applicable

Are there dangers?
I dont think so. The patch replaces the (is_parallel && te->created)
with (!ropt->single_txn && te->created) to wrap every copy during restore in
a transaction, preceding it with a truncate. This can only be done if the
restore isn't already 'single transaction' (and there is no point in doing
it in that case), and if we successfully created the table in our restore
work, so the table is empty. The purpose being not doing unnecessary wal
logging if possible.

Feature test
Apply the patch, compile it and test:
Does the feature work as advertised?
Yes. COPY is preceded by BEGIN and TRUNCATE when doing restore, and
followed by COMMIT. This happens if and only if the table has been created
during the restore. If the table was already there and restore appends data
in it, only COPY is run. This was checked when explicitely restoring only
data (-a, no truncate), and when restoring structure AND data (truncate only
if creating is really done, not in the case of error because the table was
already there). No WAL was generated.

Are there corner cases the author has failed to consider?
I don't think so

Are there any assertion failures or crashes?
Not during my tests

Performance review
Does the patch slow down simple tests?
No

If it claims to improve performance, does it?
Yes. Test case : one 10 million rows table, single int column, no
indexes. One single SATA 5400rpm disk, laptop. Dump was generated with
pg_dump -Fc -Z0. A checkpoint was triggered between each run.
wal_sync_method to fdatasync.
First test (default configuration), wal_buffers=64kB, checkpoint_segments=3,
shared_buffers=32MB. With patch, restore in 15s, without, 38s.

Second test, wal_buffers=512kB, checkpoint_segments=32,
shared_buffers=512MB. With patch, restore in 15s, without, 36s (this is a
laptop, it is IO-bound during this test).

Does it slow down other things?
It didn't seem to, and there is no reason why it should.

Coding review
Read the changes to the code in detail and consider:
Does it follow the project coding guidelines?
Yes, to my knowledge

Are there portability issues?
No

Will it work on Windows/BSD etc?
Yes

Are the comments sufficient and accurate?
Yes, comments were modified to explain the new behaviour

Does it do what it says, correctly?
Yes

Does it produce compiler warnings?
No

Can you make it crash?
No

Architecture review
Consider the changes to the code in the context of the project as a whole:
Is everything done in a way that fits together coherently with other
features/modules?
Yes, it's a 2 line-patch for the code, and 5 lnes of doc.

Are there interdependencies that can cause problems?
No

Again, this is my first. Please tell me how to improve.
From: Robert Haas on 14 Jul 2010 22:49
On Wed, Feb 10, 2010 at 12:19 AM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
> Andrew Dunstan <andrew(a)dunslane.net> writes:
>> Tom Lane wrote:
>>> The code is only trying to substitute for something you can't have
>>> in parallel restore, ie --single-transaction.
>
>> Exactly. IIRC that's why --single-transaction was introduced in the
>> first place.
>
> To me, --single-transaction is mostly there for people who want to be
> sure they have all-or-nothing restore behavior. �Optimizations are
> secondary.
>
>> Takahiro-san is suggesting there is a case for doing the optimisation in
>> non-parallel mode. But if we do that, is there still a case for
>> --single-transaction?
>
> Yeah, per above. �The main problem I have with doing it in non-parallel
> restore mode is that we couldn't safely do it when outputting to a
> script (since we don't know if the user will try to put begin/end
> around the script), and I really do not want to allow any differences
> between script output and direct-to-database output. �Once that camel's
> nose gets under the tent, debuggability will go down the tubes...

Is this a fatal defect or is there a way to salvage this idea?

Another possible issue is that this changes the behavior. Suppose the
table wasn't empty before we truncated it...

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

 | 
Pages: 1
Prev: [HACKERS] keepalive in libpq using
Next: [HACKERS] Does mbutils.c really need to use L'\0' ?