Re: [REVIEW] Re: Compression of full-page-writes
| От | Rahila Syed |
|---|---|
| Тема | Re: [REVIEW] Re: Compression of full-page-writes |
| Дата | |
| Msg-id | CAH2L28sf60i36fvoN_xNLEnOU+=AXMv-h9GqxfYoJE0h91yyZA@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: [REVIEW] Re: Compression of full-page-writes (andres@anarazel.de (Andres Freund)) |
| Ответы |
Re: [REVIEW] Re: Compression of full-page-writes
|
| Список | pgsql-hackers |
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.
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
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.
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.
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.
>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 notavailable 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
> 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@anarazel.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
Вложения
В списке pgsql-hackers по дате отправления: