On Tue, Jul 9, 2019 at 6:28 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > PFA, updated patch version which includes > - One defect fix in undo interface related to undo page compression > for handling persistence level > - Implemented pending TODO optimization in undo page compression. > - One defect fix in undo processing related to the prepared transaction
Looking at 0002 a bit, it seems to me that you really need to spend some energy getting things into a consistent order all across the patch. For example, UndoPackStage uses the ordering: HEADER, TRANSACTION, RMID, RELOID, XID, CID... But the declarations of the UREC_INFO constants go in a different order: TRANSACTION, FORK, BLOCK, BLKPREV... The comments defining those go in a different order and some of them are missing. The definition of the UndoRecordBlah structures go in a different order still: Transaction, Block, LogSwitch, Payload. UndoRecordHeaderSize goes with FORK, BLOCK, BLPREV, TRANSACTION, LOGSWITCH, .... That really needs to be straightened out and made consistent. You (still) need to rename blkprev to something more generic, as mentioned in previous rounds of review. I think it would be a good idea to avoid complex macros in favor of functions where possible, e.g. UNDO_PAGE_PARTIAL_REC_SIZE. If performance is a concern, it could be declared static inline, which should be as good as a macro. I don't like the fact that undoaccess.c has a new global, undo_compression_info. I haven't read the code thoroughly, but do we really need that? I think it's never modified (so it could just be declared const), and I also think it's just all zeroes (so initializing it isn't really necessary), and I also think that it's just used for initializing other UndoCompressionInfos (so we could just initialize them directly, either by setting the members individually or jus zeroing them). It seems like UndoRecordPrepareTransInfo ought to have an Assert(index < some_limit) in the loop. A comment in PrepareUndoInsert refers to "low switch" where it means "log switch." This is by no means a complete review, for which I unfortunately lack the time at present. Just some initial observations. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
