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                     http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to