On Sat, Jan 14, 2023 at 03:43:09PM -0600, Justin Pryzby wrote: > On Sun, Jan 08, 2023 at 01:45:25PM -0600, Justin Pryzby wrote: >> pg_compress_specification is being passed by value, but I think it >> should be passed as a pointer, as is done everywhere else. > > ISTM that was an issue with 5e73a6048, affecting a few public and > private functions. I wrote a pre-preparatory patch which changes to > pass by reference.
The functions changed by 0001 are cfopen[_write](), AllocateCompressor() and ReadDataFromArchive(). Why is it a good idea to change these interfaces which basically exist to handle inputs? Is there some benefit in changing compression_spec within the internals of these routines before going back one layer down to their callers? Changing the compression_spec on-the-fly in these internal paths could be risky, actually, no? > And addressed a handful of other issues I reported as separate fixup > commits. And changed to use LZ4 by default for CI. Are your slight changes shaped as of 0003-f.patch, 0005-f.patch and 0007-f.patch on top of the original patches sent by Georgios? > I also rebased my 2 year old patch to support zstd in pg_dump. I hope > it can finally added for v16. I'll send it for the next CF if these > patches progress. Good idea to see if what you have done for zstd fits with what's presented here. >> pg_compress_algorithm is being writen directly into the pg_dump header. Do you mean that this is what happens once the patch series 0001~0008 sent upthread is applied on HEAD? >> Currently, I think that's not an externally-visible value (it could be >> renumbered, theoretically even in a minor release). Maybe there should >> be a "private" enum for encoding the pg_dump header, similar to >> WAL_COMPRESSION_LZ4 vs BKPIMAGE_COMPRESS_LZ4 ? Or else a comment there >> should warn that the values are encoded in pg_dump, and must never be >> changed. > > Michael, WDYT ? Changing the order of the members in an enum would cause an ABI breakage, so that would not happen, and we tend to be very careful about that. Appending new members would be fine, though. FWIW, I'd rather avoid adding more enums that would just be exact maps to pg_compress_algorithm. - /* - * For now the compression type is implied by the level. This will need - * to change once support for more compression algorithms is added, - * requiring a format bump. - */ - WriteInt(AH, AH->compression_spec.level); + AH->WriteBytePtr(AH, AH->compression_spec.algorithm); I may be missing something here, but it seems to me that you ought to store as well the level in the dump header, or it would not be possible to report in the dump's description what was used? Hence, K_VERS_1_15 should imply that we have both the method compression and the compression level. -- Michael
signature.asc
Description: PGP signature