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
whenreading 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
shouldremove the outOfMem handling and revert to palloc() instead of using malloc directly. One thing worthy of note
isthat I don't think you currently can "legally" check fullPageWrites == FULL_PAGE_WRITES_ON when calling it only
duringstartup 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
