Thread: TRUNCATE+COPY optimization and --jobs=1 in pg_restore
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) ?
We checks is_parallel to decide to use BEGIN-TRUNCATE-COPY: if (is_parallel && te->created)
but we can always do it unless --single-transaction, right? if (!ropt->single_txn && te->created)
[ in restore_toc_entry() ]
/** In parallel restore, if we created the table earlier in* the run then we wrap the COPY in a transaction and*
precedeit with a TRUNCATE. If archiving is not on* this prevents WAL-logging the COPY. This obtains a* speedup similar
tothat from using single_txn mode in* non-parallel restores.*/
if (is_parallel && te->created) <==== HERE
{ /* * Parallel restore is always talking directly to a * server, so no need to see if we should issue BEGIN.
*/ StartTransaction(AH);
/* * If the server version is >= 8.4, make sure we issue * TRUNCATE with ONLY so that child tables are not
*wiped. */ ahprintf(AH, "TRUNCATE TABLE %s%s;\n\n", (PQserverVersion(AH->connection) >= 80400 ?
"ONLY " : ""), fmtId(te->tag));
}
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
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.
regards, tom lane
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. 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. Regards, --- Takahiro Itagaki NTT Open Source Software Center
Attachment
Tom Lane 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. > > > Exactly. IIRC that's why --single-transaction was introduced in the first place. 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? cheers andrew
Andrew Dunstan <andrew@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...
regards, tom lane
Andrew Dunstan <andrew@dunslane.net> wrote: > 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? I think --single-transaction is useful to restore data into non-empty databases. A normal restore ignores errors, but it might make database inconsistent state. So, we'd better keep --single-transaction option to support all-or-nothing restore. Regards, --- Takahiro Itagaki NTT Open Source Software Center
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.
On Wed, Feb 10, 2010 at 12:19 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andrew Dunstan <andrew@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