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

At 2014-07-04 19:27:10 +0530, rahilasyed90(at)gmail(dot)com wrote:
>
> Please find attached patches with no whitespace error and improved
> formatting.

Thanks.

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.

> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> index 3f92482..39635de 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -60,6 +60,9 @@
> #include "storage/spin.h"
> #include "utils/builtins.h"
> #include "utils/guc.h"
> +#include "utils/pg_lzcompress.h"
> +#include "utils/pg_snappy.h"
> +#include "utils/pg_lz4.h"
> #include "utils/ps_status.h"
> #include "utils/relmapper.h"
> #include "utils/snapmgr.h"

This hunk still fails to apply to master (due to the subsequent
inclusion of memutils.h), but I just added it in by hand.

> +int compress_backup_block = false;

Should be initialised to BACKUP_BLOCK_COMPRESSION_OFF as noted earlier.

> + /* Allocates memory for compressed backup blocks according to the compression
> + * algorithm used.Once per session at the time of insertion of first XLOG
> + * record.
> + * This memory stays till the end of session. OOM is handled by making the
> + * code proceed without FPW compression*/

I suggest something like this:

/*
* Allocates pages to store compressed backup blocks, with the page
* size depending on the compression algorithm selected. These pages
* persist throughout the life of the backend. If the allocation
* fails, we disable backup block compression entirely.
*/

But though the code looks better locally than before, the larger problem
is that this is still unsafe. As Pavan pointed out, XLogInsert is called
from inside critical sections, so we can't allocate memory here.

Could you look into his suggestions of other places to do the
allocation, please?

> + static char *compressed_pages[XLR_MAX_BKP_BLOCKS];
> + static bool compressed_pages_allocated = false;

These declarations can't just be in the middle of the function, they'll
have to move up to near the top of the closest enclosing scope (wherever
you end up doing the allocation).

> + if (compress_backup_block != BACKUP_BLOCK_COMPRESSION_OFF &&
> + compressed_pages_allocated!= true)

No need for "!= true" with a boolean.

> + if (compress_backup_block == BACKUP_BLOCK_COMPRESSION_SNAPPY)
> + buffer_size += snappy_max_compressed_length(BLCKSZ);
> + else if (compress_backup_block == BACKUP_BLOCK_COMPRESSION_LZ4)
> + buffer_size += LZ4_compressBound(BLCKSZ);
> + else if (compress_backup_block == BACKUP_BLOCK_COMPRESSION_PGLZ)
> + buffer_size += PGLZ_MAX_OUTPUT(BLCKSZ);

There's nothing wrong with this, but given that XLR_MAX_BKP_BLOCKS is 4,
I would just allocate pages of size BLCKSZ. But maybe that's just me.

> + bkpb->block_compression=BACKUP_BLOCK_COMPRESSION_OFF;

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?

> + if(VARSIZE(buf) < orig_len-2)
> + /* successful compression */
> + {
> + *len = VARSIZE(buf);
> + return (char *) buf;
> + }
> + else
> + return NULL;
> +}

That comment after the "if" just has to go. It's redundant given the
detailed explanation above anyway. Also, I'd strongly prefer checking
for failure rather than success here, i.e.

if (VARSIZE(buf) >= orig_len - 2)
return NULL;

*len = VARSIZE(buf); /* Doesn't this need + VARHDRSIZE? */

return (char *) buf;

I don't quite remember what I suggested last time, but if it was what's
in the patch now, I apologise.

> + /* Decompress if backup block is compressed*/
> + else if (VARATT_IS_COMPRESSED((struct varlena *) blk)
> + && bkpb.block_compression!=BACKUP_BLOCK_COMPRESSION_OFF)

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.

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?

> +/*
> + */
> +static const struct config_enum_entry backup_block_compression_options[] = {
> + {"off", BACKUP_BLOCK_COMPRESSION_OFF, false},
> + {"false", BACKUP_BLOCK_COMPRESSION_OFF, true},
> + {"no", BACKUP_BLOCK_COMPRESSION_OFF, true},
> + {"0", BACKUP_BLOCK_COMPRESSION_OFF, true},
> + {"pglz", BACKUP_BLOCK_COMPRESSION_PGLZ, true},
> + {"snappy", BACKUP_BLOCK_COMPRESSION_SNAPPY, true},
> + {"lz4", BACKUP_BLOCK_COMPRESSION_LZ4, true},
> + {NULL, 0, false}
> +};

An empty comment probably isn't the best idea. ;-)

Thanks for all your work on this patch. I'll set it back to waiting on
author for now, but let me know if you need more time to resubmit, and
I'll move it to the next CF.

-- Abhijit