Re: [REVIEW] Re: Compression of full-page-writes

On Thu, Dec 18, 2014 at 5:27 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> Thanks!
Thanks for your input.

> + else
> + memcpy(compression_scratch, page, page_len);
>
> I don't think the block image needs to be copied to scratch buffer here.
> We can try to compress the "page" directly.
Check.

> +#include "utils/pg_lzcompress.h"
> #include "utils/memutils.h"
>
> pg_lzcompress.h should be after meutils.h.
Oops.

> +/* Scratch buffer used to store block image to-be-compressed */
> +static char compression_scratch[PGLZ_MAX_BLCKSZ];
>
> Isn't it better to allocate the memory for compression_scratch in
> InitXLogInsert()
> like hdr_scratch?
Because the OS would not touch it if wal_compression is never used,
but now that you mention it, it may be better to get that in the
context of xlog_insert..

> + uncompressed_page = (char *) palloc(PGLZ_RAW_SIZE(header));
>
> Why don't we allocate the buffer for uncompressed page only once and
> keep reusing it like XLogReaderState->readBuf? The size of uncompressed
> page is at most BLCKSZ, so we can allocate the memory for it even before
> knowing the real size of each block image.
OK, this would save some cycles. I was trying to make process allocate
a minimum of memory only when necessary.

> - printf(" (FPW); hole: offset: %u, length: %u\n",
> - record->blocks[block_id].hole_offset,
> - record->blocks[block_id].hole_length);
> + if (record->blocks[block_id].is_compressed)
> + printf(" (FPW); hole offset: %u, compressed length %u\n",
> + record->blocks[block_id].hole_offset,
> + record->blocks[block_id].bkp_len);
> + else
> + printf(" (FPW); hole offset: %u, length: %u\n",
> + record->blocks[block_id].hole_offset,
> + record->blocks[block_id].bkp_len);
>
> We need to consider what info about FPW we want pg_xlogdump to report.
> I'd like to calculate how much bytes FPW was compressed, from the report
> of pg_xlogdump. So I'd like to see also the both length of uncompressed FPW
> and that of compressed one in the report.
OK, so let's add a parameter in the decoder for the uncompressed
length. Sounds fine?

> In pg_config.h, the comment of BLCKSZ needs to be updated? Because
> the maximum size of BLCKSZ can be affected by not only itemid but also
> XLogRecordBlockImageHeader.
Check.

> bool has_image;
> + bool is_compressed;
>
> Doesn't ResetDecoder need to reset is_compressed?
Check.

> +#wal_compression = off # enable compression of full-page writes
> Currently wal_compression compresses only FPW, so isn't it better to place
> it after full_page_writes in postgresql.conf.sample?
Check.

> + uint16 extra_data; /* used to store offset of bytes in
> "hole", with
> + * last free bit used to check if block is
> + * compressed */
> At least to me, defining something like the following seems more easy to
> read.
> uint16 hole_offset:15,
> is_compressed:1
Check++.

Updated patches addressing all those things are attached.
Regards,
--
Michael