Re: Compression of full-page-writes

Hi,

I did a partial review of this patch, wherein I focused on the patch and
the code itself, as I saw other contributors already did some testing on
it, so that we know it applies cleanly and work to some good extend.

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> In this patch, full_page_writes accepts three values: on, compress, and off.
> When it's set to compress, the full page image is compressed before it's
> inserted into the WAL buffers.

Code review :

In full_page_writes_str() why are you returning "unrecognized" rather
than doing an ELOG(ERROR, …) for this unexpected situation?

The code switches to compression (or trying to) when the following
condition is met:

+ if (fpw <= FULL_PAGE_WRITES_COMPRESS)
+ {
+ rdt->data = CompressBackupBlock(page, BLCKSZ - bkpb->hole_length, &(rdt->len));

We have

+ typedef enum FullPageWritesLevel
+ {
+ FULL_PAGE_WRITES_OFF = 0,
+ FULL_PAGE_WRITES_COMPRESS,
+ FULL_PAGE_WRITES_ON
+ } FullPageWritesLevel;

+ #define FullPageWritesIsNeeded(fpw) (fpw >= FULL_PAGE_WRITES_COMPRESS)

I don't much like using the <= test against and ENUM and I'm not sure I
understand the intention you have here. It somehow looks like a typo and
disagrees with the macro. What about using the FullPageWritesIsNeeded
macro, and maybe rewriting the macro as

#define FullPageWritesIsNeeded(fpw) \
(fpw == FULL_PAGE_WRITES_COMPRESS || fpw == FULL_PAGE_WRITES_ON)

Also, having "on" imply "compress" is a little funny to me. Maybe we
should just finish our testing and be happy to always compress the full
page writes. What would the downside be exactly (on buzy IO system
writing less data even if needing more CPU will be the right trade-off).

I like that you're checking the savings of the compressed data with
respect to the uncompressed data and cancel the compression if there's
no gain. I wonder if your test accounts for enough padding and headers
though given the results we saw in other tests made in this thread.

Why do we have both the static function full_page_writes_str() and the
macro FullPageWritesStr, with two different implementations issuing
either "true" and "false" or "on" and "off"?

! unsigned hole_offset:15, /* number of bytes before "hole" */
! flags:2, /* state of a backup block, see below */
! hole_length:15; /* number of bytes in "hole" */

I don't understand that. I wanted to use that patch as a leverage to
smoothly discover the internals of our WAL system but won't have the
time to do that here. That said, I don't even know that C syntax.

+ #define BKPBLOCK_UNCOMPRESSED 0 /* uncompressed */
+ #define BKPBLOCK_COMPRESSED 1 /* comperssed */

There's a typo in the comment above.

> [time required to replay WAL generated during running pgbench]
> 61s (on) .... 1209911 transactions were replayed,
> recovery speed: 19834.6 transactions/sec
> 39s (compress) .... 1445446 transactions were replayed,
> recovery speed: 37062.7 transactions/sec
> 37s (off) .... 1629235 transactions were replayed,
> recovery speed: 44033.3 transactions/sec

How did you get those numbers ? pg_basebackup before the test and
archiving, then a PITR maybe? Is it possible to do the same test with
the same number of transactions to replay, I guess using the -t
parameter rather than the -T one for this testing.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support