On Thu, Dec 18, 2014 at 5:27 PM, Fujii Masao <masao.fu...@gmail.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
20141219_fpw_compression_v9.tar.gz
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers