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
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
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
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.
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
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
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
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 /*
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
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
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.
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
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
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
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.
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
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
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]
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
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
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
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
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
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
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
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
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
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,
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 -
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
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
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
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
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
+++ 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
35 matches
Mail list logo
