On 1/4/21 3:53 AM, Justin Pryzby wrote:
About 89% smaller.

Did a quick code review of the patch. I have not yet taken it for a spin yet and there are parts of the code I have not read yet.

## Is there any reason for this diff?

-    cfp        *fp = pg_malloc(sizeof(cfp));
+    cfp        *fp = pg_malloc0(sizeof(cfp));

## Since we know have multiple returns in cfopen() I am not sure that setting fp to NULL is still clearer than just returning NULL.

## I do not like that this pretends to support r+, w+ and a+ but does not actually do so since it does not create an input stream in those cases.

else if (mode[0] == 'w' || mode[0] == 'a' ||
        strchr(mode, '+') != NULL)
[...]
else if (strchr(mode, 'r'))

## Wouldn't cfread(), cfwrite(), cfgetc(), cfgets(), cfclose() and cfeof() be cleaner with sitch statments similar to cfopen()?

## "/* Should be called "method" or "library" ? */"

Maybe, but personally I think algorithm is fine too.

## "Is a nondefault level set ?"

The PostgreSQL project does not use space before question mark (at least not in English).

## Why isn't level_set just a local variable in parse_compression()? It does not seem to be used elsewhere.

## Shouldn't we call the Compression variable in OpenArchive() nocompress to match with the naming convention in other places.

And in general I wonder if we should not write "nocompression = {COMPR_ALG_NONE}" rather than "nocompression = {0}".

## Why not use const on the pointers to Compression for functions like cfopen()? As far as I can see several of them could be const.

## Shouldn't "AH->compression.alg = Z_DEFAULT_COMPRESSION" in ReadHead() be "AH->compression.alg = COMPR_ALG_DEFAULT"?

Additionally I am not convinced that returning COMPR_ALG_DEFAULT will even work but I have not had the time to test that theory yet. And in general I am quite sceptical of that we really need of COMPR_ALG_DEFAULT.

## Some white space issues

Add spaces around plus in "atoi(1+eq)" and "pg_log_error("unknown compression algorithm: %s", 1+eq)".

Add spaces around plus in parse_compression(), e.g. in "strlen(1+eq)".

## Shouldn't hasSuffix() take the current compression algorithm as a parameter? Or alternatively look up which compression algorithm to use from the suffix?

## Why support multiple ways to write zlib on the command line? I do not see any advatange of being able to write it as libz.

## I feel renaming SaveOutput() to GetOutput() would make it more clear what it does now that you have changed the return type.

## You have accidentally committed "-runstatedir" in configure. I have no idea why we do not have it (maybe it is something Debian specific) but even if we are going to add it it should not be in this patch. Same with the parenthesis changes to LARGE_OFF_T.

## This is probably out of scope of your patch but I am not a fan of the fallback logic in cfopen_read(). I feel ideally we should always know if there is a suffix or not and not try to guess file names and do pointless syscalls.

## COMPR_ALG_DEFAULT looks like it would error out for archive and directory if someone has neither zlib nor zstandard. It feels like it should default to uncompressed if we have neither. Or at least give a better error message.

Note, there's currently several "compression" patches in CF app.  This patch
seems to be independent of the others, but probably shouldn't be totally
uncoordinated (like adding lz4 in one and ztsd in another might be poor
execution).

A thought here is that maybe we want to use the same values for the enums in all patches. Especially if we write the numeric value to pg dump files.

Andreas



Reply via email to