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

From: andres(at)anarazel(dot)de (Andres Freund)
To: "Syed, Rahila" <Rahila(dot)Syed(at)nttdata(dot)com>
Cc: Rahila Syed <rahilasyed90(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Rahila Syed <rahilasyed(dot)90(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: [REVIEW] Re: Compression of full-page-writes
Date: 2014-09-29 12:36:02
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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

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

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.


Andres Freund

Andres Freund
PostgreSQL Development, 24x7 Support, Training & Services

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2014-09-29 12:42:01 Re: pg_receivexlog and replication slots
Previous Message Andres Freund 2014-09-29 11:42:16 Re: Patch to support SEMI and ANTI join removal