Re: [HACKERS] Custom compression methods

On Mon, Mar 22, 2021 at 01:38:36PM -0400, Robert Haas wrote:
> On Mon, Mar 22, 2021 at 12:16 PM Robert Haas <robertmhaas(at)gmail(dot)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