Re: TRUNCATE+COPY optimization and --jobs=1 in pg_restore
| От | Marc Cousin |
|---|---|
| Тема | Re: TRUNCATE+COPY optimization and --jobs=1 in pg_restore |
| Дата | |
| Msg-id | AANLkTim7sUpnVMS3VXYJ6JK5n9HOYb9BMi-d0gYd8GDt@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: TRUNCATE+COPY optimization and --jobs=1 in pg_restore (Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp>) |
| Список | pgsql-hackers |
2010/2/10 Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp>
Yeah, the comment says so. But it does not necessarily mean that
Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Takahiro Itagaki <itagaki.takahiro@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.
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.
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Robert HaasДата:
Сообщение: Re: Re: [COMMITTERS] pgsql: Fix log_temp_files docs and comments to say bytes not kilobytes.