On Mon, Jan 31, 2022 at 04:40:09PM -0500, Greg Stark wrote: > 4) This isn't really an issue with your patch at all but why on earth > do we have a bitvector for WAL compression methods?! Like, what does > it mean to have multiple compression methods set? That should just be > a separate field with values for each type of compression surely?
I don't have an answer to your question, but the discussion was here. In the versions of the patches I sent on Mar 15, Mar 21, May 18, May 24, Jun 13, I avoided "one bit per compression method", but Michael thought this was simpler. https://www.postgresql.org/message-id/20210622031358.gf29...@telsasoft.com On Mon, Jun 21, 2021 at 10:13:58PM -0500, Justin Pryzby wrote: > +/* compression methods supported */ > +#define BKPIMAGE_COMPRESS_PGLZ 0x04 > +#define BKPIMAGE_COMPRESS_ZLIB 0x08 > +#define BKPIMAGE_COMPRESS_LZ4 0x10 > +#define BKPIMAGE_COMPRESS_ZSTD 0x20 > +#define BKPIMAGE_IS_COMPRESSED(info) \ > + ((info & (BKPIMAGE_COMPRESS_PGLZ | BKPIMAGE_COMPRESS_ZLIB | \ > + BKPIMAGE_COMPRESS_LZ4 | BKPIMAGE_COMPRESS_ZSTD)) != > 0) > > You encouraged saving bits here, so I'm surprised to see that your patches > use one bit per compression method: 2 bits to support no/pglz/lz4, 3 to add > zstd, and the previous patch used 4 bits to also support zlib. > > There are spare bits available for that, but now there can be an inconsistency > if two bits are set. Also, 2 bits could support 4 methods (including "no"). On Tue, Jun 22, 2021 at 12:53:46PM +0900, Michael Paquier wrote: > Yeah, I know. I have just finished with that to get something > readable for the sake of the tests. As you say, the point is moot > just we add one new method, anyway, as we need just one new bit. > And that's what I would like to do for v15 with LZ4 as the resulting > patch is simple. It would be an idea to discuss more compression > methods here once we hear more from users when this is released in the > field, re-considering at this point if more is necessary or not.
