On Mon, Mar 22, 2021 at 01:38:36PM -0400, Robert Haas wrote: > On Mon, Mar 22, 2021 at 12:16 PM Robert Haas <robertmh...@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.
+1 guc.c should not longer define this as extern: default_toast_compression_options 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 Shouldn't varlena.c pg_column_compression() call GetCompressionMethodName () ? I guess it should already have done that. Maybe pg_dump.c can't use those constants, though (?) -- Justin
