On Mon, Apr 11, 2022 at 12:46:02PM +0000, gkokolatos@pm.me wrote:
> On Monday, April 11th, 2022 at 8:52 AM, Michael Paquier <michael@paquier.xyz>
> wrote:
>> - 0001 is a simple rename of backup_compression.{c,h} to
>> compression.{c,h}, removing anything related to base backups from
>> that. One extra reason behind this renaming is that I would like to
>> use this infra for pg_dump, but that's material for 16~.
>
> I agree with the design. If you permit me a couple of nitpicks regarding naming.
>
> +typedef enum pg_compress_algorithm
> +{
> + PG_COMPRESSION_NONE,
> + PG_COMPRESSION_GZIP,
> + PG_COMPRESSION_LZ4,
> + PG_COMPRESSION_ZSTD
> +} pg_compress_algorithm;
>
> Elsewhere in the codebase, (e.g. create_table.sgml, alter_table.sgml,
> brin_tuple.c, detoast.c, toast_compression.c, tupdesc.c, gist.c to mention a
> few) variations of of the nomenclature "compression method" are used, like
> 'VARDATA_COMPRESSED_GET_COMPRESS_METHOD' or 'InvalidCompressionMethod' etc. I
> feel that it would be nicer if we followed one naming rule for this and I
> recommend to substitute algorithm for method throughout.
Technically and as far as I know, both are correct and hold more or
less the same meaning. pg_basebackup's code exposes algorithm in a
more extended way, so I have just stuck to it for the internal
variables and such. Perhaps we could rename the whole, but I see no
strong reason to do that.
> Last, even though it is not needed now, it will be helpful to have a
> PG_COMPRESSION_INVALID in some scenarios. Though we can add it when we come to
> it.
Perhaps. There is no need for it yet, though. pg_dump would not need
that, as well.
>> - 0002 removes WalCompressionMethod, replacing it by
>> pg_compress_algorithm as these are the same enums. Robert complained
>> about the confusion that WalCompressionMethod could lead to as this
>> could be used for the compression of data, and not only WAL. I have
>> renamed some variables to be more consistent, while on it.
>
> It looks good. If you choose to discard the comment regarding the use of
> 'method' over 'algorithm' from above, can you please use the full word in the
> variable, e.g. 'wal_compress_algorithm' instead of 'wal_compress_algo'. I can
> not really explain it, the later reads a bit rude. Then again that may be just
> me.
Thanks. I have been able to do an extra pass on 0001 and 0002, fixing
those naming inconsistencies with "algo" vs "algorithm" that you and
Robert have reported, and applied them. For 0003, I'll look at it
later. Attached is a rebase with improvements about the variable
names.
--
Michael
