RE: Add LZ4 compression in pg_dump

On Fri, Jan 27, 2023 2:04 AM gkokolatos(at)pm(dot)me <gkokolatos(at)pm(dot)me> wrote:
>
> ------- Original Message -------
> On Thursday, January 26th, 2023 at 12:53 PM, Michael Paquier
> <michael(at)paquier(dot)xyz> wrote:
>
>
> >
> >
> > On Thu, Jan 26, 2023 at 11:24:47AM +0000, gkokolatos(at)pm(dot)me wrote:
> >
> > > I gave this a little bit of thought. I think that ReadHead should not
> > > emit a warning, or at least not this warning as it is slightly misleading.
> > > It implies that it will automatically turn off data restoration, which is
> > > false. Further ahead, the code will fail with a conflicting error message
> > > stating that the compression is not available.
> > >
> > > Instead, it would be cleaner both for the user and the maintainer to
> > > move the check in RestoreArchive and make it the sole responsible for
> > > this logic.
> >
> >
> > - pg_fatal("cannot restore from compressed archive (compression not
> supported in this installation)");
> > + pg_fatal("cannot restore data from compressed archive (compression not
> supported in this installation)");
> > Hmm. I don't mind changing this part as you suggest.
> >
> > -#ifndef HAVE_LIBZ
> > - if (AH->compression_spec.algorithm == PG_COMPRESSION_GZIP)
> >
> > - pg_fatal("archive is compressed, but this installation does not support
> compression");
> > -#endif
> > However I think that we'd better keep the warning, as it can offer a
> > hint when using pg_restore -l not built with compression support if
> > looking at a dump that has been compressed.
>
> Fair enough. Please find v27 attached.
>

Hi,

I am interested in this feature and tried the patch. While reading the comments,
I noticed some minor things that could possibly be improved (in v27-0003 patch).

1.
+ /*
+ * Open a file for writing.
+ *
+ * 'mode' can be one of ''w', 'wb', 'a', and 'ab'. Requrires an already
+ * initialized CompressFileHandle.
+ */
+ int (*open_write_func) (const char *path, const char *mode,
+ CompressFileHandle *CFH);

There is a redundant single quote in front of 'w'.

2.
/*
* Callback function for WriteDataToArchive. Writes one block of (compressed)
* data to the archive.
*/
/*
* Callback function for ReadDataFromArchive. To keep things simple, we
* always read one compressed block at a time.
*/

Should the function names in the comments be updated?

WriteDataToArchive
->
writeData

ReadDataFromArchive
->
readData

3.
+ Assert(strcmp(mode, "r") == 0 || strcmp(mode, "rb") == 0);

Could we use PG_BINARY_R instead of "r" and "rb" here?

Regards,
Shi Yu