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

Thank you for review comments.

>There are still numerous formatting changes required, e.g. spaces around
>"=" and correct formatting of comments. And "git diff --check" still has
>a few whitespace problems. I won't point these out one by one, but maybe
>you should run pgindent

I will do this.

>Could you look into his suggestions of other places to do the

>allocation, please?

I will get back to you on this

>Wouldn't it be better to set

>    bkpb->block_compression = compress_backup_block;

>once earlier instead of setting it that way once and setting it to
>BACKUP_BLOCK_COMPRESSION_OFF in two other places

Yes.

If you're using VARATT_IS_COMPRESSED() to detect compression, don't you

need SET_VARSIZE_COMPRESSED() in CompressBackupBlock? pglz_compress()
does it for you, but the other two algorithms don't.

Yes we need SET_VARSIZE_COMPRESSED. It is present in wrappers around snappy and LZ4 namely pg_snappy_compress and pg_LZ4_compress.

>But now that you've added bkpb.block_compression, you should be able to
>avoid VARATT_IS_COMPRESSED() altogether, unless I'm missing something.
>What do you think?

You are right. It can be removed.

Thank you,

On Fri, Jul 4, 2014 at 9:35 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:

At 2014-07-04 21:02:33 +0530, ams@2ndQuadrant.com wrote:
>
> > +/*
> > + */
> > +static const struct config_enum_entry backup_block_compression_options[] = {

Oh, I forgot to mention that the configuration setting changes are also
pending. I think we had a working consensus to use full_page_compression
as the name of the GUC. As I understand it, that'll accept an algorithm
name as an argument while we're still experimenting, but eventually once
we select an algorithm, it'll become just a boolean (and then we don't
need to put algorithm information into BkpBlock any more either).

-- Abhijit