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(at)2ndquadrant(dot)com>
wrote:

> At 2014-07-04 21:02:33 +0530, ams(at)2ndQuadrant(dot)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
>