On Mon, Mar 22, 2021 at 1:58 PM Justin Pryzby <pry...@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
v2-0001-Tidy-up-more-loose-ends-related-to-configurable-T.patch
Description: Binary data