Thread: add --no-sync to pg_upgrade's calls to pg_dump and pg_dumpall
This is likely small potatoes compared to some of the other pg_upgrade-related improvements I've proposed [0] [1] or plan to propose, but this is easy enough, and I already wrote the patch, so here it is. AFAICT there's no reason to bother syncing these dump files to disk. If someone pulls the plug during pg_upgrade, it's not like you can resume pg_upgrade from where it left off. Also, I think we skipped syncing before v10, anyway, as the --no-sync flag was only added in commit 96a7128, which added the code to sync dump files, too. [0] https://postgr.es/m/20240418041712.GA3441570%40nathanxps13 [1] https://postgr.es/m/20240503025140.GA1227404%40nathanxps13 -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On 03.05.24 19:13, Nathan Bossart wrote: > This is likely small potatoes compared to some of the other > pg_upgrade-related improvements I've proposed [0] [1] or plan to propose, > but this is easy enough, and I already wrote the patch, so here it is. > AFAICT there's no reason to bother syncing these dump files to disk. If > someone pulls the plug during pg_upgrade, it's not like you can resume > pg_upgrade from where it left off. Also, I think we skipped syncing before > v10, anyway, as the --no-sync flag was only added in commit 96a7128, which > added the code to sync dump files, too. Looks good to me.
On Wed, May 08, 2024 at 10:09:46AM +0200, Peter Eisentraut wrote: > On 03.05.24 19:13, Nathan Bossart wrote: >> This is likely small potatoes compared to some of the other >> pg_upgrade-related improvements I've proposed [0] [1] or plan to propose, >> but this is easy enough, and I already wrote the patch, so here it is. >> AFAICT there's no reason to bother syncing these dump files to disk. If >> someone pulls the plug during pg_upgrade, it's not like you can resume >> pg_upgrade from where it left off. Also, I think we skipped syncing before >> v10, anyway, as the --no-sync flag was only added in commit 96a7128, which >> added the code to sync dump files, too. > > Looks good to me. Thanks for looking. I noticed that the version check is unnecessary since we always use the new binary's pg_dump[all], so I removed that in v2. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
Nathan Bossart <nathandbossart@gmail.com> writes:
> Thanks for looking. I noticed that the version check is unnecessary since
> we always use the new binary's pg_dump[all], so I removed that in v2.
+1
regards, tom lane
On Wed, May 08, 2024 at 02:49:58PM -0400, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> Thanks for looking. I noticed that the version check is unnecessary since >> we always use the new binary's pg_dump[all], so I removed that in v2. > > +1 +1. Could there be an argument in favor of a backpatch? This is a performance improvement, but one could also side that the addition of sync support in pg_dump[all] has made that a regression that we'd better fix because the flushes don't matter in this context. They also bring costs for no gain. -- Michael
Attachment
On Thu, May 09, 2024 at 09:03:56AM +0900, Michael Paquier wrote: > +1. Could there be an argument in favor of a backpatch? This is a > performance improvement, but one could also side that the addition of > sync support in pg_dump[all] has made that a regression that we'd > better fix because the flushes don't matter in this context. They > also bring costs for no gain. I don't see a strong need to back-patch this, if for no other reason than it seems to have gone unnoticed for 7 major versions. Plus, based on my admittedly limited testing, this is unlikely to provide significant improvements. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
> On 9 May 2024, at 21:34, Nathan Bossart <nathandbossart@gmail.com> wrote: > > On Thu, May 09, 2024 at 09:03:56AM +0900, Michael Paquier wrote: >> +1. Could there be an argument in favor of a backpatch? This is a >> performance improvement, but one could also side that the addition of >> sync support in pg_dump[all] has made that a regression that we'd >> better fix because the flushes don't matter in this context. They >> also bring costs for no gain. > > I don't see a strong need to back-patch this, if for no other reason than > it seems to have gone unnoticed for 7 major versions. Plus, based on my > admittedly limited testing, this is unlikely to provide significant > improvements. Agreed, this is a nice little improvement but it's unlikely to be enough of a speedup to warrant changing the backbranches. -- Daniel Gustafsson
Committed. -- nathan