Re: refactoring basebackup.c
От | Jeevan Ladhe |
---|---|
Тема | Re: refactoring basebackup.c |
Дата | |
Msg-id | CANm22Ch_hU-86CCC-HkUAOY+6jYfGp5jJkde97fTu3u7u=PW9g@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: refactoring basebackup.c (Dipesh Pandit <dipesh.pandit@gmail.com>) |
Ответы |
Re: refactoring basebackup.c
|
Список | pgsql-hackers |
Thanks for the patch, Dipesh.
I had a look at the patch and also tried to take the backup. I have
following suggestions and observations:
I get following error at my end:
$ pg_basebackup -D /tmp/zstd_bk -Ft -Xfetch --compress=server-zstd:7@4
pg_basebackup: error: could not initiate base backup: ERROR: could not compress data: Unsupported parameter
pg_basebackup: removing data directory "/tmp/zstd_bk"
This is mostly because I have the zstd library version v1.4.4, which
does not have default support for parallel workers. Maybe we should
have a better error, something that is hinting that the parallelism is
not supported by the particular build.
The regression for pg_verifybackup test 008_untar.pl also fails with a
similar error. Here, I think we should have some logic in regression to
skip the test if the parameter is not supported?
+ if (ZSTD_isError(ret))
+ elog(ERROR,
+ "could not compress data: %s",
+ ZSTD_getErrorName(ret));
I think all of this can go on one line, but anyhow we have to improve
the error message here.
Also, just a thought, for the versions where parallelism is not
supported, should we instead just throw a warning and fall back to
non-parallel behavior?
Regards,
Jeevan Ladhe
I had a look at the patch and also tried to take the backup. I have
following suggestions and observations:
I get following error at my end:
$ pg_basebackup -D /tmp/zstd_bk -Ft -Xfetch --compress=server-zstd:7@4
pg_basebackup: error: could not initiate base backup: ERROR: could not compress data: Unsupported parameter
pg_basebackup: removing data directory "/tmp/zstd_bk"
This is mostly because I have the zstd library version v1.4.4, which
does not have default support for parallel workers. Maybe we should
have a better error, something that is hinting that the parallelism is
not supported by the particular build.
The regression for pg_verifybackup test 008_untar.pl also fails with a
similar error. Here, I think we should have some logic in regression to
skip the test if the parameter is not supported?
+ if (ZSTD_isError(ret))
+ elog(ERROR,
+ "could not compress data: %s",
+ ZSTD_getErrorName(ret));
I think all of this can go on one line, but anyhow we have to improve
the error message here.
Also, just a thought, for the versions where parallelism is not
supported, should we instead just throw a warning and fall back to
non-parallel behavior?
Regards,
Jeevan Ladhe
On Mon, 14 Mar 2022 at 21:41, Dipesh Pandit <dipesh.pandit@gmail.com> wrote:
Hi,I tried to implement support for parallel ZSTD compression. Thelibrary provides an option (ZSTD_c_nbWorkers) to specify thenumber of compression workers. The number of parallelworkers can be set as part of compression parameter and if thisoption is specified then the library performs parallel compressionbased on the specified number of workers.User can specify the number of parallel worker as part of--compress option by appending an integer value after at sign (@).(-Z, --compress=[{client|server}-]{gzip|lz4|zstd}[:LEVEL][@WORKERS])Please find the attached patch v1 with the above changes.Note: ZSTD library version 1.5.x supports parallel compressionby default and if the library version is lower than 1.5.x thenparallel compression is enabled only the source is compiled with buildmacro ZSTD_MULTITHREAD. If the linked library version doesn'tsupport parallel compression then setting the value of parameterZSTD_c_nbWorkers to a value other than 0 will be no-op andreturns an error.Thanks,Dipesh
В списке pgsql-hackers по дате отправления: