Thread: Re: refactoring basebackup.c (zstd workers)
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Mar 21, 2022 at 2:22 PM Justin Pryzby <pryzby@telsasoft.com> 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/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: > <varlistentry> > - <term><literal>COMPRESSION_LEVEL</literal> <replaceable>level</replaceable></term> > + <term><literal>COMPRESSION_DETAIL</literal> <replaceable>detail</replaceable></term> > <listitem> > <para> > Specifies the compression level to be used. This is no longer the accurate. How about something like like "Specifies details of the chosen compression method"? - ilmari
On Mon, Mar 21, 2022 at 2:41 PM Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> 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
Attachment
On Tue, Mar 22, 2022 at 11:37 AM Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Mar 21, 2022 at 2:41 PM Dagfinn Ilmari Mannsåker > <ilmari@ilmari.org> 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. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> 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. */
>>> CID 1503251: Null pointer dereferences (REVERSE_INULL)
>>> Null-checking "vend" suggests that it may be null, but it has already been dereferenced on all paths leading to
thecheck.
194 specification = vend == NULL ? kwend + 1 : vend + 1;
195 }
196 }
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.
regards, tom lane
On Sun, Mar 27, 2022 at 1:47 PM Tom Lane <tgl@sss.pgh.pa.us> 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 pointer dereferences (REVERSE_INULL) > >>> Null-checking "vend" suggests that it may be null, but it has already been dereferenced on all paths leading tothe check. > 194 specification = vend == NULL ? kwend + 1 : vend + 1; > 195 } > 196 } > > 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. Yes, I think this is buggy. I think there's only a theoretical bug right now, because the only keyword we have is "level" and that requires a value. But if I add an example keyword that does not require an associated value (as demonstrated in the attached patch) and do something like pg_basebackup -cfast -D whatever --compress lz4:example, then the present code will dereference "vend" even though it's NULL, which is not good. The attached patch also shows how I think that should be fixed. As I hope is apparent, the first hunk of this patch is not for commit, and the second hunk is for commit. -- Robert Haas EDB: http://www.enterprisedb.com
Attachment
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Mar 27, 2022 at 1:47 PM Tom Lane <tgl@sss.pgh.pa.us> 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 commit.
Looks plausible to me.
regards, tom lane
On Mon, Mar 28, 2022 at 03:50:50PM -0400, Robert Haas wrote: > On Sun, Mar 27, 2022 at 1:47 PM Tom Lane <tgl@sss.pgh.pa.us> 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 pointer dereferences (REVERSE_INULL) > > >>> Null-checking "vend" suggests that it may be null, but it has already been dereferenced on all paths leadingto the check. > > 194 specification = vend == NULL ? kwend + 1 : vend + 1; > > 195 } > > 196 } > > > > 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. > > Yes, I think this is buggy. I think there's only a theoretical bug > right now, because the only keyword we have is "level" and that > requires a value. But if I add an example keyword that does not > require an associated value (as demonstrated in the attached patch) > and do something like pg_basebackup -cfast -D whatever --compress > lz4:example, then the present code will dereference "vend" even though > it's NULL, which is not good. The attached patch also shows how I > think that should be fixed. > > As I hope is apparent, the first hunk of this patch is not for commit, > and the second hunk is for commit. Confirmed that it's a real issue with my patch for zstd long match mode. But you need to specify another option after the value-less flag option for it to crash. I suggest to write it differently, as in 0002. This also fixes some rebase-induced errors with my previous patches, and adds expect_boolean().