On Mon, Mar 22, 2021 at 12:16 PM Robert Haas <robertmhaas@gmail.com> wrote:
> But, what about giving the default_toast_compression_method GUC an
> assign hook that sets a global variable of type "char" to the
> appropriate value? Then GetDefaultToastCompression() goes away
> entirely. That might be worth exploring.
Actually, we can do even better. We should just make the values
actually assigned to the GUC be TOAST_PGLZ_COMPRESSION etc. rather
than TOAST_PGLZ_COMPRESSION_ID etc. Then a whole lot of complexity
just goes away. I added some comments explaining why using
TOAST_PGLZ_COMPRESSION is the wrong thing anyway. Then I got hacking
and rearranged a few other things. So the attached patch does these
thing:
- Changes default_toast_compression to an enum, as in your patch, but
now with values that are the same as what ultimately gets stored in
attcompression.
- Adds a comment warning against incautious use of
TOAST_PGLZ_COMPRESSION_ID, etc.
- Moves default_toast_compression_options to guc.c.
- After doing the above two things, we can remove the #include of
utils/guc.h into access/toast_compression.h, so the patch does that.
- Moves NO_LZ4_SUPPORT, GetCompressionMethodName, and
CompressionNameToMethod to guc.c. Making these inline functions
doesn't save anything meaningful; it's more important not to export a
bunch of random identifiers.
- Removes an unnecessary cast to bool from the definition of
CompressionMethodIsValid.
I think this is significantly cleaner than what we have now, and I
also prefer it to your proposal.
Comments?
--
Robert Haas
EDB: http://www.enterprisedb.com
