Re: refactoring basebackup.c

2023-04-12 Thread Robert Haas
On Wed, Apr 12, 2023 at 10:57 AM Justin Pryzby wrote: > I think these maybe got forgotten ? Committed. -- Robert Haas EDB: http://www.enterprisedb.com

Re: refactoring basebackup.c

2023-04-12 Thread Justin Pryzby
On Fri, Mar 24, 2023 at 10:46:37AM -0400, Robert Haas wrote: > On Thu, Mar 23, 2023 at 4:11 PM Robert Haas wrote: > > On Wed, Mar 22, 2023 at 10:09 PM Thomas Munro > > wrote: > > > > BaseBackupSync is not documented > > > > BaseBackupWrite is not documented > > > > > > [Resending with trimmed CC

Re: refactoring basebackup.c

2023-03-24 Thread Robert Haas
On Thu, Mar 23, 2023 at 4:11 PM Robert Haas wrote: > On Wed, Mar 22, 2023 at 10:09 PM Thomas Munro wrote: > > > BaseBackupSync is not documented > > > BaseBackupWrite is not documented > > > > [Resending with trimmed CC: list, because the mailing list told me to > > due to a blocked account, sorr

Re: refactoring basebackup.c

2023-03-23 Thread Robert Haas
On Wed, Mar 22, 2023 at 10:09 PM Thomas Munro wrote: > > BaseBackupSync is not documented > > BaseBackupWrite is not documented > > [Resending with trimmed CC: list, because the mailing list told me to > due to a blocked account, sorry if you already got the above.] Bummer. I'll write a patch to

Re: refactoring basebackup.c

2023-03-22 Thread Thomas Munro
On Thu, Mar 23, 2023 at 2:50 PM Thomas Munro wrote: > In rem: commit 3500ccc3, > > for X in ` grep -E '^[^*]+event_name = "' > src/backend/utils/activity/wait_event.c | >sed 's/^.* = "//;s/";$//;/unknown/d' ` > do > if ! git grep "$X" doc/src/sgml/monitoring.sgml > /dev/null > then

Re: refactoring basebackup.c (zstd workers)

2022-03-30 Thread Justin Pryzby
On Wed, Mar 30, 2022 at 04:14:47PM -0400, Tom Lane wrote: > Robert Haas writes: > >> Maybe if I just put that last sentence into the comment it's clear enough? > > > Done that way, since I thought it was better to fix the bug than wait > > for more feedback on the wording. We can still adjust the

Re: refactoring basebackup.c (zstd workers)

2022-03-30 Thread Tom Lane
Robert Haas writes: >> Maybe if I just put that last sentence into the comment it's clear enough? > Done that way, since I thought it was better to fix the bug than wait > for more feedback on the wording. We can still adjust the wording, or > the coding, if it's not clear enough. FWIW, I though

Re: refactoring basebackup.c (zstd workers)

2022-03-30 Thread Robert Haas
On Tue, Mar 29, 2022 at 8:51 AM Robert Haas wrote: > On Mon, Mar 28, 2022 at 8:11 PM Tom Lane wrote: > > I suspect Robert wrote it that way intentionally --- but if so, > > I agree it could do with more than zero commentary. > > Well, the point is, we stop advancing kwend when we get to the end o

Re: refactoring basebackup.c (zstd workers)

2022-03-29 Thread Robert Haas
On Mon, Mar 28, 2022 at 8:11 PM Tom Lane wrote: > I suspect Robert wrote it that way intentionally --- but if so, > I agree it could do with more than zero commentary. Well, the point is, we stop advancing kwend when we get to the end of the keyword, and *vend when we get to the end of the value.

Re: refactoring basebackup.c (zstd workers)

2022-03-28 Thread Tom Lane
Justin Pryzby writes: > Also, why wouldn't *kwend be checked in any case ? I suspect Robert wrote it that way intentionally --- but if so, I agree it could do with more than zero commentary. regards, tom lane

Re: refactoring basebackup.c (zstd workers)

2022-03-28 Thread Justin Pryzby
On Mon, Mar 28, 2022 at 05:39:31PM -0400, Robert Haas wrote: > On Mon, Mar 28, 2022 at 4:53 PM Justin Pryzby wrote: > > I suggest to write it differently, as in 0002. > > That doesn't seem better to me. What's the argument for it? I find this much easier to understand: /* If we

Re: refactoring basebackup.c (zstd workers)

2022-03-28 Thread Robert Haas
On Mon, Mar 28, 2022 at 4:53 PM Justin Pryzby wrote: > I suggest to write it differently, as in 0002. That doesn't seem better to me. What's the argument for it? -- Robert Haas EDB: http://www.enterprisedb.com

Re: refactoring basebackup.c (zstd workers)

2022-03-28 Thread Justin Pryzby
On Mon, Mar 28, 2022 at 03:50:50PM -0400, Robert Haas wrote: > On Sun, Mar 27, 2022 at 1:47 PM Tom Lane wrote: > > Coverity has a nitpick about this: > > > > /srv/coverity/git/pgsql-git/postgresql/src/common/backup_compression.c: 194 > > in parse_bc_specification() > > 193 /*

Re: refactoring basebackup.c (zstd workers)

2022-03-28 Thread Tom Lane
Robert Haas writes: > On Sun, Mar 27, 2022 at 1:47 PM Tom Lane wrote: >> Not sure if you should remove this null-check or add some other ones, >> but I think you ought to do one or the other. > As I hope is apparent, the first hunk of this patch is not for commit, > and the second hunk is for co

Re: refactoring basebackup.c (zstd workers)

2022-03-28 Thread Robert Haas
On Sun, Mar 27, 2022 at 1:47 PM Tom Lane wrote: > Coverity has a nitpick about this: > > /srv/coverity/git/pgsql-git/postgresql/src/common/backup_compression.c: 194 > in parse_bc_specification() > 193 /* Advance to next entry and loop around. */ > >>> CID 1503251: Null po

Re: refactoring basebackup.c (zstd workers)

2022-03-27 Thread Tom Lane
Robert Haas writes: > [ v5-0001-Replace-BASE_BACKUP-COMPRESSION_LEVEL-option-with.patch ] Coverity has a nitpick about this: /srv/coverity/git/pgsql-git/postgresql/src/common/backup_compression.c: 194 in parse_bc_specification() 193 /* Advance to next entry and loop around.

Re: refactoring basebackup.c (zstd workers)

2022-03-23 Thread Robert Haas
On Tue, Mar 22, 2022 at 11:37 AM Robert Haas wrote: > On Mon, Mar 21, 2022 at 2:41 PM Dagfinn Ilmari Mannsåker > wrote: > > This is no longer the accurate. How about something like like "Specifies > > details of the chosen compression method"? > > Good catch. v5 attached. And committed. -- Rob

Re: refactoring basebackup.c (zstd workers)

2022-03-22 Thread Robert Haas
On Mon, Mar 21, 2022 at 2:41 PM Dagfinn Ilmari Mannsåker wrote: > This is no longer the accurate. How about something like like "Specifies > details of the chosen compression method"? Good catch. v5 attached. -- Robert Haas EDB: http://www.enterprisedb.com v5-0001-Replace-BASE_BACKUP-COMPRESS

Re: refactoring basebackup.c (zstd workers)

2022-03-21 Thread Dagfinn Ilmari Mannsåker
Robert Haas writes: > On Mon, Mar 21, 2022 at 2:22 PM Justin Pryzby wrote: >> + * during parsing, and will otherwise contain a an appropriate error >> message. > > OK, thanks. v4 attached. I haven't read the whole patch, but I noticed an omission in the documentation changes: > diff --git a/d

Re: refactoring basebackup.c (zstd workers)

2022-03-21 Thread Robert Haas
On Mon, Mar 21, 2022 at 2:22 PM Justin Pryzby wrote: > + * during parsing, and will otherwise contain a an appropriate error message. OK, thanks. v4 attached. > I think "algorithm" could be much more nuanced than "lz4", but I also think > we've spent more than enough time on it now :) Oh dear.

Re: refactoring basebackup.c (zstd workers)

2022-03-21 Thread Justin Pryzby
On Mon, Mar 21, 2022 at 12:57:36PM -0400, Robert Haas wrote: > > typo: contain a an > I searched for the "contain a an" typo that you mentioned but was not able to > find it. Can you give me a more specific pointer? Here: + * during parsing, and will otherwise contain a an appropriate error messa

Re: refactoring basebackup.c (zstd workers)

2022-03-21 Thread Robert Haas
On Mon, Mar 21, 2022 at 9:18 AM Justin Pryzby wrote: > On Sun, Mar 20, 2022 at 09:38:44PM -0400, Robert Haas wrote: > > > This patch also needs to update the other user-facing docs. > > > > Which ones exactly? > > I mean pg_basebackup -Z > > -Z level > -Z [{client|server}-]method[:level] > --compr

Re: refactoring basebackup.c (zstd workers)

2022-03-21 Thread Justin Pryzby
On Sun, Mar 20, 2022 at 09:38:44PM -0400, Robert Haas wrote: > > This patch also needs to update the other user-facing docs. > > Which ones exactly? I mean pg_basebackup -Z -Z level -Z [{client|server}-]method[:level] --compress=level --compress=[{client|server}-]method[:level]

Re: refactoring basebackup.c (zstd workers)

2022-03-20 Thread Robert Haas
On Sun, Mar 20, 2022 at 9:32 PM Tom Lane wrote: > Robert Haas writes: > > I think I'm guilty of verbal inexactitude here but not bad coding. > > Checking for *endptr != '\0', as I did, is not sufficient to detect > > "whether an error occurred," as I alleged. But, in the part of my > > response y

Re: refactoring basebackup.c (zstd workers)

2022-03-20 Thread Robert Haas
On Sun, Mar 20, 2022 at 3:40 PM Justin Pryzby wrote: > The user-facing docs are already standardized using "compression method", with > 2 exceptions, of which one is contrib/ and the other is what I'm suggesting to > make consistent here. > > $ git grep 'compression algorithm' doc > doc/src/sgml/p

Re: refactoring basebackup.c (zstd workers)

2022-03-20 Thread Tom Lane
Robert Haas writes: > I think I'm guilty of verbal inexactitude here but not bad coding. > Checking for *endptr != '\0', as I did, is not sufficient to detect > "whether an error occurred," as I alleged. But, in the part of my > response you didn't quote, I believe I made it clear that I only need

Re: refactoring basebackup.c (zstd workers)

2022-03-20 Thread Robert Haas
On Sun, Mar 20, 2022 at 3:11 PM Tom Lane wrote: > > Even after reading the man page for strtol, it's not clear to me that > > this is needed. That page represents checking *endptr != '\0' as > > sufficient to tell whether an error occurred. > > I'm not sure whose man page you looked at, but the PO

Re: refactoring basebackup.c (zstd workers)

2022-03-20 Thread Justin Pryzby
On Sun, Mar 20, 2022 at 03:05:28PM -0400, Robert Haas wrote: > On Thu, Mar 17, 2022 at 3:41 PM Justin Pryzby wrote: > > -errmsg("unrecognized > > compression algorithm: \"%s\"", > > +errmsg("unrecogniz

Re: refactoring basebackup.c (zstd workers)

2022-03-20 Thread Tom Lane
Robert Haas writes: >> Should this also set/check errno ? >> And check if value != ivalue_endp ? >> See strtol(3) > Even after reading the man page for strtol, it's not clear to me that > this is needed. That page represents checking *endptr != '\0' as > sufficient to tell whether an error occurr

Re: refactoring basebackup.c (zstd workers)

2022-03-20 Thread Robert Haas
On Thu, Mar 17, 2022 at 3:41 PM Justin Pryzby wrote: > gzip comma I think it's fine the way it's written. If we made that change, then we'd have a comma for gzip and not for the other two algorithms. Also, I'm just moving that sentence, so any change that there is to be made here is a job for som

Re: refactoring basebackup.c (zstd workers)

2022-03-17 Thread Robert Haas
Thanks for the review! I'll address most of these comments later, but quickly for right now... On Thu, Mar 17, 2022 at 3:41 PM Justin Pryzby wrote: > It'd be great if this were re-usable for wal_compression, which I hope in > pg16 will > support at least level=N. And eventually pg_dump. But t

Re: refactoring basebackup.c (zstd workers)

2022-03-17 Thread Justin Pryzby
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 9178c779ba..00c593f1af 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -2731,14 +2731,24 @@ The commands accepted in replication mode are: + + For gzip the compression level shoul

Re: refactoring basebackup.c (zstd workers)

2022-03-17 Thread Robert Haas
On Mon, Mar 14, 2022 at 1:21 PM Robert Haas wrote: > There's some appeal to that, but one downside is that it means that > the client can't be used to fetch data that is compressed in a way > that the server knows about and the client doesn't. I don't think > that's great. Why should, for example,

Re: refactoring basebackup.c (zstd negative compression)

2022-03-16 Thread Justin Pryzby
Should zstd's negative compression levels be supported here ? Here's a POC patch which is enough to play with it. $ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --compress=zstd |wc -c 12305659 $ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D -

Re: refactoring basebackup.c

2022-03-15 Thread Robert Haas
On Tue, Mar 15, 2022 at 6:33 AM Jeevan Ladhe wrote: > 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: remov

Re: refactoring basebackup.c

2022-03-15 Thread Jeevan Ladhe
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: E

Re: refactoring basebackup.c (zstd workers)

2022-03-14 Thread Robert Haas
On Mon, Mar 14, 2022 at 1:11 PM Justin Pryzby wrote: > Internally, I was thinking they'd all be handled as first-class options, with > separate struct fields and separate replication protocol options. If an > option > isn't known, it'd be rejected on the client side, rather than causing an error

Re: refactoring basebackup.c (zstd workers)

2022-03-14 Thread Justin Pryzby
On Mon, Mar 14, 2022 at 01:02:20PM -0400, Robert Haas wrote: > On Mon, Mar 14, 2022 at 12:35 PM Justin Pryzby wrote: > > I suggest to use a syntax that's more general than that, maybe something > > like > > > > :[level=]N,parallel=N,flag,flag,... > > > > For example, someone may want to use zstd

Re: refactoring basebackup.c (zstd workers)

2022-03-14 Thread Robert Haas
On Mon, Mar 14, 2022 at 12:35 PM Justin Pryzby wrote: > I suggest to use a syntax that's more general than that, maybe something like > > :[level=]N,parallel=N,flag,flag,... > > For example, someone may want to use zstd "long" mode or (when it's released) > rsyncable mode, or specify fine-grained

Re: refactoring basebackup.c (zstd workers)

2022-03-14 Thread Justin Pryzby
On Mon, Mar 14, 2022 at 09:41:35PM +0530, Dipesh Pandit wrote: > I tried to implement support for parallel ZSTD compression. The > library provides an option (ZSTD_c_nbWorkers) to specify the > number of compression workers. The number of parallel > workers can be set as part of compression paramet

Re: refactoring basebackup.c

2022-03-14 Thread Dipesh Pandit
Hi, I tried to implement support for parallel ZSTD compression. The library provides an option (ZSTD_c_nbWorkers) to specify the number of compression workers. The number of parallel workers can be set as part of compression parameter and if this option is specified then the library performs paral

Re: refactoring basebackup.c

2022-03-14 Thread Robert Haas
On Fri, Mar 11, 2022 at 8:52 PM Andres Freund wrote: > You could also just append a manifest as a compresed tar to the compressed tar > stream. Unfortunately GNU tar requires -i to read concated compressed > archives, so perhaps that's not quite an alternative. s/Unfortunately/Fortunately/ :-p I

Re: refactoring basebackup.c

2022-03-11 Thread Andres Freund
Hi, On 2022-03-11 10:19:29 -0500, Robert Haas wrote: > Thanks for the report. The problem here is that, when the output is > standard output (-D -), pg_basebackup can only produce a single output > file, so the manifest gets injected into the tar file on the client > side rather than being written

Re: refactoring basebackup.c

2022-03-11 Thread Robert Haas
On Tue, Feb 15, 2022 at 11:26 AM Robert Haas wrote: > On Wed, Feb 9, 2022 at 8:41 AM Abhijit Menon-Sen wrote: > > It took me a while to assimilate these patches, including the backup > > targets one, which I hadn't looked at before. Now that I've wrapped my > > head around how to put the pieces t

Re: refactoring basebackup.c

2022-03-11 Thread Robert Haas
On Fri, Mar 11, 2022 at 11:29 AM Justin Pryzby wrote: > Sounds right. OK, committed. > Also, I think the magic 8 for .gz should actually be a 7. > > I'm not sure why it tests for ".gz" but not ".tar.gz", which would help to > make > them all less magic. > > commit 1fb1e21ba7a500bb2b85ec3e65f591

Re: refactoring basebackup.c

2022-03-11 Thread Justin Pryzby
On Fri, Mar 11, 2022 at 10:19:29AM -0500, Robert Haas wrote: > So I think we should just refuse this command. Patch for that attached. Sounds right. Also, I think the magic 8 for .gz should actually be a 7. I'm not sure why it tests for ".gz" but not ".tar.gz", which would help to make them all

Re: refactoring basebackup.c

2022-03-11 Thread Robert Haas
On Thu, Mar 10, 2022 at 8:02 PM Justin Pryzby wrote: > I'm getting errors from pg_basebackup when using both -D- and > --compress=server-* > The issue seems to go away if I use --no-manifest. > > $ ./src/bin/pg_basebackup/pg_basebackup -h /tmp -Ft -D- --wal-method none > --compress=server-gzip >

Re: refactoring basebackup.c

2022-03-10 Thread Justin Pryzby
I'm getting errors from pg_basebackup when using both -D- and --compress=server-* The issue seems to go away if I use --no-manifest. $ ./src/bin/pg_basebackup/pg_basebackup -h /tmp -Ft -D- --wal-method none --compress=server-gzip >/dev/null ; echo $? pg_basebackup: error: tar member has empty na

Re: refactoring basebackup.c

2022-03-08 Thread Jeevan Ladhe
ok got it. Thanks for your insights. Regards, Jeevan Ladhe On Tue, 8 Mar 2022 at 22:23, Robert Haas wrote: > On Tue, Mar 8, 2022 at 11:32 AM Jeevan Ladhe > wrote: > > I reviewed the patch, and it seems to be capturing and replacing all the > > places of HAVE_LIB* with USE_* correctly. > > Just

Re: refactoring basebackup.c

2022-03-08 Thread Robert Haas
On Tue, Mar 8, 2022 at 11:32 AM Jeevan Ladhe wrote: > I reviewed the patch, and it seems to be capturing and replacing all the > places of HAVE_LIB* with USE_* correctly. > Just curious, apart from consistency, do you see other problems as well > when testing one vs the other? So, the kind of pro

Re: refactoring basebackup.c

2022-03-08 Thread Jeevan Ladhe
> > OK, committed all that stuff. > Thanks for the commit Robert. > I think we also need to fix one other thing. Right now, for LZ4 > support we test HAVE_LIBLZ4, but TOAST and XLOG compression are > testing USE_LZ4, so I think we should be doing the same here. And > similarly I think we should

Re: refactoring basebackup.c

2022-03-08 Thread Robert Haas
On Tue, Mar 8, 2022 at 4:49 AM Jeevan Ladhe wrote: > I agree with your patch. The patch looks good to me. > Yes, the LZ4 flush check should also be fixed. Please find the attached > patch to fix the LZ4 code. OK, committed all that stuff. I think we also need to fix one other thing. Right now, f

Re: refactoring basebackup.c

2022-03-08 Thread Jeevan Ladhe
Hi Robert, My proposed changes are largely cosmetic, but one thing that isn't is > revising the size - pos <= bound tests to instead check size - pos < > bound. My reasoning for that change is: if the number of bytes > remaining in the buffer is exactly equal to the maximum number we can > write,

Re: refactoring basebackup.c

2022-03-07 Thread Robert Haas
On Wed, Feb 16, 2022 at 8:46 PM Jeevan Ladhe wrote: > Thanks for the comments Robert. I have addressed your comments in the > attached patch v13-0002-ZSTD-add-server-side-compression-support.patch. > Rest of the patches are similar to v12, but just bumped the version number. OK, here's a consolid

walmethods.c is kind of a mess (was Re: refactoring basebackup.c)

2022-03-04 Thread Robert Haas
On Fri, Mar 4, 2022 at 3:32 AM Dipesh Pandit wrote: > GZIP manages to overcome this problem as it provides an option to turn on/off > compression on the fly while writing a compressed archive with the help of > zlib > library function deflateParams(). The current gzip implementation for > CreateW

Re: refactoring basebackup.c

2022-03-04 Thread Dipesh Pandit
Hi, > > It will be good if we can also fix > > CreateWalTarMethod to support LZ4 and ZSTD. > Ok we will see, either Dipesh or I will take care of it. I took a look at the CreateWalTarMethod to support LZ4 compression for WAL files. The current implementation involves a 3 step to backup a WAL file

Re: refactoring basebackup.c

2022-02-16 Thread Jeevan Ladhe
Thanks for the comments Robert. I have addressed your comments in the attached patch v13-0002-ZSTD-add-server-side-compression-support.patch. Rest of the patches are similar to v12, but just bumped the version number. > It will be good if we can also fix > CreateWalTarMethod to support LZ4 and ZST

Re: refactoring basebackup.c

2022-02-16 Thread Robert Haas
On Wed, Feb 16, 2022 at 12:46 PM Jeevan Ladhe wrote: > So, I went ahead and have now also implemented client side decompression > for zstd. > > Robert separated[1] the ZSTD configure switch from my original patch > of server side compression and also added documentation related to > the switch. I

Re: refactoring basebackup.c

2022-02-16 Thread Jeevan Ladhe
Hi Everyone, So, I went ahead and have now also implemented client side decompression for zstd. Robert separated[1] the ZSTD configure switch from my original patch of server side compression and also added documentation related to the switch. I have included that patch here in the patch series f

Re: refactoring basebackup.c

2022-02-16 Thread Robert Haas
On Wed, Feb 16, 2022 at 11:11 AM Alvaro Herrera wrote: > This is hard to interpret for humans though because of the nested > brackets and braces. It gets considerably easier if you split it in > separate variants: > >-Z, --compress=[{client|server}-]{gzip|lz4}[:LEVEL] >-Z, --compress=LEVE

Re: refactoring basebackup.c

2022-02-16 Thread Alvaro Herrera
On 2022-Feb-14, Robert Haas wrote: > A more consistent way of writing the supported syntax would be like this: > > -Z, --compress={[{client|server}-]{gzip|lz4}}[:LEVEL]|LEVEL|none} > > I would be somewhat inclined to leave the level-only variant > undocumented and instead write it like this: >

Re: refactoring basebackup.c (zstd)

2022-02-16 Thread Robert Haas
On Tue, Feb 15, 2022 at 12:59 PM Justin Pryzby wrote: > There's superfluous changes to ./configure unrelated to the changes in > configure.ac. Probably because you're using a different version of autotools, > or a vendor's patched copy. You can remove the changes with git checkout -p > or > sim

Re: refactoring basebackup.c

2022-02-15 Thread Jeevan Ladhe
Thanks Tushar for the testing. I further worked on ZSTD and now have implemented client side compression as well. Attached are the patches for both server-side and client-side compression. The patch 0001 is a server-side patch, and has not changed since the last patch version - v10, but, just bum

Re: refactoring basebackup.c (zstd)

2022-02-15 Thread Justin Pryzby
+++ b/configure @@ -801,6 +805,7 @@ infodir docdir oldincludedir includedir +runstatedir There's superfluous changes to ./configure unrelated to the changes in configure.ac. Probably because you're using a different version of autotools, or a vendor's patched copy. You can remove the changes

Re: refactoring basebackup.c

2022-02-15 Thread tushar
On 2/15/22 6:48 PM, Jeevan Ladhe wrote: Please find the attached updated version of patch for ZSTD server side Thanks, Jeevan, I again tested with the attached patch, and as mentioned the crash is fixed now. also, I tested with different labels with gzip V/s zstd against data directory size w

Re: refactoring basebackup.c

2022-02-15 Thread Robert Haas
On Wed, Feb 9, 2022 at 8:41 AM Abhijit Menon-Sen wrote: > It took me a while to assimilate these patches, including the backup > targets one, which I hadn't looked at before. Now that I've wrapped my > head around how to put the pieces together, I really like the idea. As > you say, writing non-tr

Re: refactoring basebackup.c

2022-02-15 Thread Jeevan Ladhe
Hi, Please find the attached updated version of patch for ZSTD server side compression. This patch has following changes: - Fixes the issue Tushar reported[1]. - Adds a tap test. - Makes document changes related to zstd. - Updates pg_basebackup help for pg_basebackup. Here I have chosen the sugg

Re: refactoring basebackup.c

2022-02-14 Thread Robert Haas
On Sat, Feb 12, 2022 at 1:01 AM Shinoda, Noriyoshi (PN Japan FSIP) wrote: > Thank you for developing a great feature. > The current help message shown below does not seem to be able to specify the > 'client-' or 'server-' for lz4 compression. > --compress = {[{client, server}-]gzip, lz4, none}[:

Re: refactoring basebackup.c

2022-02-12 Thread Andres Freund
Hi, On 2022-02-12 15:12:21 -0600, Justin Pryzby wrote: > I think they would've been visible in the CI environment, too. Yea, but only if you looked carefully enough. The postgres github repo has CI enabled, and it's green. But the windows build step does show the warnings: https://cirrus-ci.com/

Re: refactoring basebackup.c

2022-02-12 Thread Justin Pryzby
The LZ4 patches caused new compiler warnings. It's the same issue that was fixed at 71ce8 for gzip. I think they would've been visible in the CI environment, too. https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=wrasse&dt=2022-02-12%2005%3A08%3A48&stg=make "/export/home/nm/farm/st

RE: refactoring basebackup.c

2022-02-11 Thread Shinoda, Noriyoshi (PN Japan FSIP)
i...@gmail.com>; Jeevan Ladhe ; Mark Dilger ; pgsql-hack...@postgresql.org; tushar Subject: Re: refactoring basebackup.c On Fri, Feb 11, 2022 at 10:29 AM Justin Pryzby wrote: > FYI: there's a couple typos in the last 2 patches. Hmm. OK. But I don't consider "can be optiona

Re: refactoring basebackup.c

2022-02-11 Thread Robert Haas
On Fri, Feb 11, 2022 at 10:29 AM Justin Pryzby wrote: > FYI: there's a couple typos in the last 2 patches. Hmm. OK. But I don't consider "can be optionally specified" incorrect or worse than "can optionally be specified". I do agree that spelling words correctly is a good idea. -- Robert Haas

Re: refactoring basebackup.c

2022-02-11 Thread Justin Pryzby
On Fri, Feb 11, 2022 at 08:35:25PM +0530, Jeevan Ladhe wrote: > Thanks Robert for the bravity :-) FYI: there's a couple typos in the last 2 patches. I added them to my typos branch; feel free to wait until April if you'd prefer to see them fixed in bulk. diff --git a/doc/src/sgml/ref/pg_baseback

Re: refactoring basebackup.c

2022-02-11 Thread Jeevan Ladhe
Thanks Robert for the bravity :-) Regards, Jeevan Ladhe On Fri, 11 Feb 2022, 20:31 Robert Haas, wrote: > On Fri, Feb 11, 2022 at 7:20 AM Dipesh Pandit > wrote: > > > Sure, please find the rebased patch attached. > > > > Thanks, I have validated v2 patch on top of rebased patch. > > I'm still

Re: refactoring basebackup.c

2022-02-11 Thread Robert Haas
On Fri, Feb 11, 2022 at 7:20 AM Dipesh Pandit wrote: > > Sure, please find the rebased patch attached. > > Thanks, I have validated v2 patch on top of rebased patch. I'm still feeling brave, so I committed this too after fixing a few things. In the process I noticed that we don't have support for

Re: refactoring basebackup.c

2022-02-11 Thread Robert Haas
On Fri, Feb 11, 2022 at 5:58 AM Jeevan Ladhe wrote: > >Jeevan, Your v12 patch does not apply on HEAD, it requires a > rebase. > > Sure, please find the rebased patch attached. It's Friday today, but I'm feeling brave, and it's still morning here, so ... committed. -- Robert Haas EDB: http://www

Re: refactoring basebackup.c

2022-02-11 Thread Dipesh Pandit
> Sure, please find the rebased patch attached. Thanks, I have validated v2 patch on top of rebased patch. Thanks, Dipesh

Re: refactoring basebackup.c

2022-02-11 Thread Jeevan Ladhe
>Jeevan, Your v12 patch does not apply on HEAD, it requires a rebase. Sure, please find the rebased patch attached. Regards, Jeevan On Fri, 11 Feb 2022 at 14:13, Dipesh Pandit wrote: > Hi, > > Thanks for the feedback, I have incorporated the suggestions > and updated a new patch. PFA v2 patch.

Re: refactoring basebackup.c

2022-02-11 Thread Dipesh Pandit
Hi, Thanks for the feedback, I have incorporated the suggestions and updated a new patch. PFA v2 patch. > I think similar to bbstreamer_lz4_compressor_content() in > bbstreamer_lz4_decompressor_content() we can change len to avail_in. In bbstreamer_lz4_decompressor_content(), we are modifying av

Re: refactoring basebackup.c

2022-02-10 Thread Jeevan Ladhe
Thanks for the patch, Dipesh. With a quick look at the patch I have following observations: -- In bbstreamer_lz4_compressor_new(), I think this alignment is not needed on client side: /* Align the output buffer length. */ compressed_

Re: refactoring basebackup.c

2022-02-10 Thread Dipesh Pandit
Hi, > On Mon, Jan 31, 2022 at 4:41 PM Jeevan Ladhe < jeevan.la...@enterprisedb.com> wrote: > Hi Robert, > > I had an offline discussion with Dipesh, and he will be working on the > lz4 client side decompression part. > Please find the attached patch to support client side compression and decompr

Re: refactoring basebackup.c

2022-02-09 Thread Abhijit Menon-Sen
At 2022-02-02 10:55:53 -0500, robertmh...@gmail.com wrote: > > On Tue, Jan 18, 2022 at 1:55 PM Robert Haas wrote: > > 0001 adds "server" and "blackhole" as backup targets. It now has some > > tests. This might be more or less ready to ship, unless somebody else > > sees a problem, or I find one. >

Re: refactoring basebackup.c

2022-02-02 Thread Robert Haas
On Tue, Jan 18, 2022 at 1:55 PM Robert Haas wrote: > 0001 adds "server" and "blackhole" as backup targets. It now has some > tests. This might be more or less ready to ship, unless somebody else > sees a problem, or I find one. I played around with this a bit and it seems quite easy to extend thi

Re: refactoring basebackup.c

2022-01-31 Thread Jeevan Ladhe
> I think you are right, I have removed the message and again introduced > the Assert() back. > In my previous version of patch, this was a problem, basically, there should not be an assert as the code is still reachable be it server-lz4 or client-lz4. I removed the assert and added the level range

Re: refactoring basebackup.c

2022-01-31 Thread Robert Haas
On Mon, Jan 31, 2022 at 6:11 AM Jeevan Ladhe wrote: > I had an offline discussion with Dipesh, and he will be working on the > lz4 client side decompression part. OK. I guess we should also be thinking about client-side LZ4 compression. It's probably best to focus on that before worrying about ZS

Re: refactoring basebackup.c

2022-01-31 Thread Jeevan Ladhe
Hi Robert, I had an offline discussion with Dipesh, and he will be working on the lz4 client side decompression part. Please find the attached patch with the following changes: - Even if we were going to support LZ4 only on the server side, surely > it's not right to refuse --compress lz4 and --

Re: refactoring basebackup.c

2022-01-28 Thread Jeevan Ladhe
On Sat, Jan 29, 2022 at 1:20 AM Robert Haas wrote: > On Fri, Jan 28, 2022 at 12:48 PM Jeevan Ladhe > wrote: > > I have attached the latest rebased version of the LZ4 server-side > compression > > patch on the recent commits. This patch also introduces the compression > level > > and adds a tap t

Re: refactoring basebackup.c

2022-01-28 Thread Robert Haas
On Fri, Jan 28, 2022 at 12:48 PM Jeevan Ladhe wrote: > I have attached the latest rebased version of the LZ4 server-side compression > patch on the recent commits. This patch also introduces the compression level > and adds a tap test. In view of this morning's commit of d45099425eb19e420433c9d81

Re: refactoring basebackup.c

2022-01-28 Thread Jeevan Ladhe
Hi Robert, I have attached the latest rebased version of the LZ4 server-side compression patch on the recent commits. This patch also introduces the compression level and adds a tap test. Also, while adding the lz4 case in the pg_verifybackup/t/008_untar.pl, I found an unused variable {have_zlib}

Re: refactoring basebackup.c

2022-01-28 Thread Robert Haas
On Fri, Jan 28, 2022 at 3:54 AM Dipesh Pandit wrote: > Thanks. This makes sense. > > +#ifdef HAVE_LIBZ > + /* > +* If the user has requested a server compressed archive along with > archive > +* extraction at client then we need to decompress it. > +*/ > + if (format == 'p' && com

Re: refactoring basebackup.c

2022-01-28 Thread tushar
On 1/27/22 11:12 PM, Robert Haas wrote: Well what's weird here is that you are using both --gzip and also --compress. Those both control the same behavior, so it's a surprising idea to specify both. But I guess if someone does, we should make the second one fully override the first one. Here's a

Re: refactoring basebackup.c

2022-01-28 Thread Dipesh Pandit
Hi, > I made a pass over these patches today and made a bunch of minor > corrections. New version attached. The two biggest things I changed > are (1) s/gzip_extractor/gzip_compressor/, because I feel like you > extract an archive like a tarfile, but that is not what is happening > here, this is n

Re: refactoring basebackup.c

2022-01-27 Thread Robert Haas
On Thu, Jan 27, 2022 at 2:37 AM Dipesh Pandit wrote: > I have updated the patches to support server compression (gzip) for > plain format backup. Please find attached v4 patches. I made a pass over these patches today and made a bunch of minor corrections. New version attached. The two biggest th

Re: refactoring basebackup.c

2022-01-27 Thread Robert Haas
On Thu, Jan 27, 2022 at 12:08 PM tushar wrote: > On 1/27/22 10:17 PM, Robert Haas wrote: > > Cool. I committed that patch. > Thanks , Please refer to this scenario where the label is set to 0 for > server-gzip but the directory is still compressed > > [edb@centos7tushar bin]$ ./pg_basebackup -t

Re: refactoring basebackup.c

2022-01-27 Thread tushar
On 1/27/22 10:17 PM, Robert Haas wrote: Cool. I committed that patch. Thanks , Please refer to this scenario  where the label is set to  0 for server-gzip but the directory is still  compressed [edb@centos7tushar bin]$ ./pg_basebackup -t server:/tmp/11 --gzip --compress=0 -Xnone NOTICE:  all

Re: refactoring basebackup.c

2022-01-27 Thread Robert Haas
On Thu, Jan 27, 2022 at 7:15 AM tushar wrote: > On 1/27/22 2:15 AM, Robert Haas wrote: > > The attached patch should fix this, too. > Thanks, the issues seem to be fixed now. Cool. I committed that patch. -- Robert Haas EDB: http://www.enterprisedb.com

Re: refactoring basebackup.c

2022-01-27 Thread tushar
On 1/27/22 2:15 AM, Robert Haas wrote: The attached patch should fix this, too. Thanks, the issues seem to be fixed now. -- regards,tushar EnterpriseDB https://www.enterprisedb.com/ The Enterprise PostgreSQL Company

Re: refactoring basebackup.c

2022-01-26 Thread Dipesh Pandit
Hi, > It only needed trivial rebasing; I have committed it after doing that. I have updated the patches to support server compression (gzip) for plain format backup. Please find attached v4 patches. Thanks, Dipesh From 4d0c84d6fac841aafb757535cc0e48334a251581 Mon Sep 17 00:00:00 2001 From: Dipes

Re: refactoring basebackup.c

2022-01-26 Thread Robert Haas
On Tue, Jan 25, 2022 at 11:22 AM tushar wrote: > A)Getting syntax error if -z is used along with -t > > [edb@centos7tushar bin]$ ./pg_basebackup -t server:/tmp/data902 -z -Xfetch > pg_basebackup: error: could not initiate base backup: ERROR: syntax error Oops. The attached patch should fix this.

Re: refactoring basebackup.c

2022-01-26 Thread Robert Haas
On Tue, Jan 25, 2022 at 8:23 PM Michael Paquier wrote: > On Tue, Jan 25, 2022 at 03:54:52AM +, Shinoda, Noriyoshi (PN Japan FSIP) > wrote: > > Michael, I am proposing to that we remove this message as part of > > this commit: > > > > -pg_log_info("no value specified for compre

  1   2   >