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

Hello,

Please find the updated patch attached.

>1) I don't think it's a good idea to put the full page write compression
into struct XLogRecord.

1. The compressed blocks is of varlena type. Hence, VARATT_IS_COMPRESSED
can be used to detect if the datum is compressed. But, it can give false
positive when blocks are not compressed because uncompressed blocks in WAL
record are not of type varlena. If I understand correctly,
VARATT_IS_COMPRESSED looks for particular bit pattern in the datum which
when found it returns true irrespective of type of datum.

2. BkpBlock header of the first block in a WAL record can be copied as it
is followed by compressed data including block corresponding to first
header and remaining headers and blocks. This header can then be used to
store flag indicating if the blocks are compressed or not. This seems to be
a feasible option but will increase few bytes equivalent to
sizeof(BkpBlock) in record when compared to the method of compressing all
blocks and headers.
Also , the full page write compression currently stored in WAL record
occupies 1 byte of padding hence does not increase the overall size. But at
the same timecompression attribute is related to backup up blocks hence it
makes more sense to have it in BkpBlock header. Although, the attached
patch does not include this yet as it will be better to get consensus
first.
Thoughts?

>2) You've essentially removed a lot of checks about the validity of bkp
blocks in xlogreader. I don't think that's acceptable

Check to see if size of compressed blocks agrees with the total size stored
on WAL record header is added in the patch attached. This serves as a check
to validate length of record.

>3) You have both FullPageWritesStr() and full_page_writes_str().

This has not changed for now reason being full_page_writes_str() is
true/false version of FullPageWritesStr macro. It
is implemented for backward compatibility with pg_xlogdump.

>4)I don't like FullPageWritesIsNeeded(). For one it, at least to me,
>7) Unless I miss something CompressBackupBlock should be plural, right?
ATM it compresses all the blocks?
>8) I don't tests like "if (fpw <= FULL_PAGE_WRITES_COMPRESS)". That
relies on the, less than intuitive, ordering of
FULL_PAGE_WRITES_COMPRESS (=1) before FULL_PAGE_WRITES_ON (=2).
>9) I think you've broken the case where we first think 1 block needs to
be backed up, and another doesn't. If we then detect, after the
START_CRIT_SECTION(), that we need to "goto begin;" orig_len will
still have it's old content.
I have corrected these in the patch attached.

>5) CompressBackupBlockPagesAlloc is declared static but not defined as
such.
Have made it global now in order to be able to access it from PostgresMain.

>6) You call CompressBackupBlockPagesAlloc() from two places. Neither is
IIRC within a critical section. So you imo should remove the outOfMem
handling and revert to palloc() instead of using malloc directly.
This has not been changed in the current patch reason being outOfMem
handling is done in order to
proceed without compression of FPW in case sufficient memory is not
available for compression.

>One
> thing worthy of note is that I don't think you currently can
> "legally" check fullPageWrites == FULL_PAGE_WRITES_ON when calling it
> only during startup as fullPageWrites can be changed at runtime

In the attached patch, this check is also added in PostgresMain on SIGHUP
after processing postgresql.conf file.

Thank you,

Rahila Syed

On Mon, Sep 29, 2014 at 6:06 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> Hi,
>
> On 2014-09-22 10:39:32 +0000, Syed, Rahila wrote:
> > >Please find attached the patch to compress FPW.
>
> I've given this a quick look and noticed some things:
> 1) I don't think it's a good idea to put the full page write compression
> into struct XLogRecord.
>
> 2) You've essentially removed a lot of checks about the validity of bkp
> blocks in xlogreader. I don't think that's acceptable.
>
> 3) You have both FullPageWritesStr() and full_page_writes_str().
>
> 4) I don't like FullPageWritesIsNeeded(). For one it, at least to me,
> sounds grammatically wrong. More importantly when reading it I'm
> thinking of it being about the LSN check. How about instead directly
> checking whatever != FULL_PAGE_WRITES_OFF?
>
> 5) CompressBackupBlockPagesAlloc is declared static but not defined as
> such.
>
> 6) You call CompressBackupBlockPagesAlloc() from two places. Neither is
> IIRC within a critical section. So you imo should remove the outOfMem
> handling and revert to palloc() instead of using malloc directly. One
> thing worthy of note is that I don't think you currently can
> "legally" check fullPageWrites == FULL_PAGE_WRITES_ON when calling it
> only during startup as fullPageWrites can be changed at runtime.
>
> 7) Unless I miss something CompressBackupBlock should be plural, right?
> ATM it compresses all the blocks?
>
> 8) I don't tests like "if (fpw <= FULL_PAGE_WRITES_COMPRESS)". That
> relies on the, less than intuitive, ordering of
> FULL_PAGE_WRITES_COMPRESS (=1) before FULL_PAGE_WRITES_ON (=2).
>
> 9) I think you've broken the case where we first think 1 block needs to
> be backed up, and another doesn't. If we then detect, after the
> START_CRIT_SECTION(), that we need to "goto begin;" orig_len will
> still have it's old content.
>
>
> I think that's it for now. Imo it'd be ok to mark this patch as returned
> with feedback and deal with it during the next fest.
>
> Greetings,
>
> Andres Freund
>
> --
> Andres Freund http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>