On Mon, Mar 22, 2021 at 1:58 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> guc.c should not longer define this as extern:
> default_toast_compression_options
Fixed.
> I think you should comment that default_toast_compression is an int as far as
> guc.c is concerned, but storing one of the char value of TOAST_*_COMPRESSION
Done.
> Shouldn't varlena.c pg_column_compression() call GetCompressionMethodName () ?
> I guess it should already have done that.
It has a 0-3 integer, not a char value.
> Maybe pg_dump.c can't use those constants, though (?)
Hmm, toast_compression.h might actually be safe for frontend code now,
or if necessary we could add #ifdef FRONTEND stanzas to make it so. I
don't know if that is really this patch's job, but I guess it could
be.
A couple of other things:
- Upon further reflection, I think the NO_LZ4_SUPPORT() message is
kinda not great. I'm thinking we should change it to say "LZ4 is not
supported by this build" instead of "unsupported LZ4 compression
method" and drop the hint and detail. That seems more like how we've
handled other such cases.
- It is not very nice that the three possible values of attcompression
are TOAST_PGLZ_COMPRESSION, TOAST_LZ4_COMPRESSION, and
InvalidCompressionMethod. One of those three identifiers looks very
little like the other two, and there's no real good reason for that. I
think we should try to standardize on something, but I'm not sure what
it should be. It would also be nice if these names were more visually
distinct from the related but very different enum values
TOAST_PGLZ_COMPRESSION_ID and TOAST_LZ4_COMPRESSION_ID. Really, as the
comments I added explain, we want to minimize the amount of code that
knows about the 0-3 "ID" values, and use the char values whenever we
can.
--
Robert Haas
EDB: http://www.enterprisedb.com
