Re: Add LZ4 compression in pg_dump - Mailing list pgsql-hackers
From | Tomas Vondra |
---|---|
Subject | Re: Add LZ4 compression in pg_dump |
Date | |
Msg-id | d8cbc586-a8e3-1662-c158-9022d5c8de83@enterprisedb.com Whole thread Raw |
In response to | Re: Add LZ4 compression in pg_dump (gkokolatos@pm.me) |
Responses |
Re: Add LZ4 compression in pg_dump
|
List | pgsql-hackers |
Hi, On 1/19/23 17:42, gkokolatos@pm.me wrote: > > ------- Original Message ------- > On Thursday, January 19th, 2023 at 4:45 PM, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote: >> >> On 1/18/23 20:05, gkokolatos@pm.me wrote: >> >>> ------- Original Message ------- >>> On Wednesday, January 18th, 2023 at 3:00 PM, Tomas Vondra tomas.vondra@enterprisedb.com wrote: >> >> I'm not sure I understand why leave the lz4/zstd in this place? > > You are right, it is not obvious. Those were added in 5e73a60488 which is > already committed in master and I didn't want to backtrack. Of course, I am > not opposing in doing so if you wish. > Ah, I didn't realize it was already added by earlier commit. In that case let's not worry about it. >> >>>> 2) I wouldn't reorder the cases in WriteDataToArchive, i.e. I'd keep >>>> "none" at the end. It might make backpatches harder. >>> >>> Agreed. However a 'default' is needed in order to avoid compilation warnings. >>> Also note that 0002 completely does away with cases within WriteDataToArchive. >> >> >> OK, although that's also a consequence of using a "switch" instead of >> plan "if" branches. >> >> Furthermore, I'm not sure we really need the pg_fatal() about invalid >> compression method in these default blocks. I mean, how could we even >> get to these places when the build does not support the algorithm? All >> of this (ReadDataFromArchive, WriteDataToArchive, EndCompressor, ...) >> happens looong after the compressor was initialized and the method >> checked, no? So maybe either this should simply do Assert(false) or use >> a different error message. > > I like Assert(false). > OK, good. Do you agree we should never actually get there, if the earlier checks work correctly? >> >>>> 4) "cfp" struct no longer wraps gzFile, but the comment was not updated. >>>> FWIW I'm not sure switching to "void *" is an improvement, maybe it'd be >>>> better to have a "union" of correct types? >>> >>> Please find and updated comment and a union in place of the void *. Also >>> note that 0002 completely does away with cfp in favour of a new struct >>> CompressFileHandle. I maintained the void * there because it is used by >>> private methods of the compressors. 0003 contains such an example with >>> LZ4CompressorState. >> >> >> I wonder if this (and also the previous item) makes sense to keep 0001 >> and 0002 or to combine them. The "intermediate" state is a bit annoying. > > Agreed. It was initially submitted as one patch. Then it was requested to be > split up in two parts, one to expand the use of the existing API and one to > replace with the new interface. Unfortunately the expansion of usage of the > existing API requires some tweaking, but that is not a very good reason for > the current patch set. I should have done a better job there. > > Please find v22 attach which combines back 0001 and 0002. It is missing the > documentation that was discussed above as I wanted to give a quick feedback. > Let me know if you think that the combined version is the one to move forward > with. > Thanks, I'll take a look. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
pgsql-hackers by date: