Re: POC: Cleaning up orphaned files using undo logs

On Thu, Jul 11, 2019 at 12:38 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Tue, Jul 9, 2019 at 6:28 AM Dilip Kumar <dilipbalaut(at)gmail(dot)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.
>
Thanks for the review, I will work on this.

> You (still) need to rename blkprev to something more generic, as
> mentioned in previous rounds of review.

I will change this.
>
> 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.
ok
>
> 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),

Actually, this will get modified otherwise across undo record
insertion how we will know what was the values of the common fields in
the first record of the page. Another option could be that every time
we insert the record, read the value from the first complete undo
record on the page but that will be costly because for every new
insertion we need to read the first undo record of the page.

Currently, we are doing like this

a) BeginUndoRecordInsert
- Copy the global "undo_compression_info" to our local context for
handling multi-prepare because for multi-prepare we don't want to
update the global value until we have successfully inserted the undo
record.

b) PrepareUndoInsert
-Operate on the context and update the context->undo_compression_info
if required (page changed)

c)InsertPrepareUndo
- After we have inserted successfully overwrite
context->undo_compression_info to the global "undo_compression_info".
So that next undo insertion can get the right information.

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).

Initially, I was doing that but later I thought that InvalidUndoRecPtr
is macro (although the value is 0) shouldn't we initialize all
UndoRecPtr variables with value InvalidUndoRecPtr instead of directly
using 0 so I changed like this.

>
> 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."

I will fix.
>
> This is by no means a complete review, for which I unfortunately lack
> the time at present. Just some initial observations.
>
ok

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com