From: | "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | "gkokolatos(at)pm(dot)me" <gkokolatos(at)pm(dot)me>, Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Justin Pryzby <pryzby(at)telsasoft(dot)com>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Rachel Heaton <rachelmheaton(at)gmail(dot)com> |
Subject: | RE: Add LZ4 compression in pg_dump |
Date: | 2023-02-15 13:51:21 |
Message-ID: | OSZPR01MB631087FC3B53AD8B1E3439F7FDA39@OSZPR01MB6310.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Drouvot, Bertrand | 2023-02-15 14:21:13 | Re: Minimal logical decoding on standbys |
Previous Message | Jim Jones | 2023-02-15 13:49:30 | Re: [PATCH] Add pretty-printed XML output option |