Re: POC: Cleaning up orphaned files using undo logs - Mailing list pgsql-hackers

I have stated review of
0008-Provide-interfaces-to-store-and-fetch-undo-records.patch, here are few

quick comments.

1) README.undointerface should provide more information like API details or
the sequence in which API should get called.

2) Information about the API's in the undoaccess.c file header block would
good.  For reference please look at heapam.c.

3) typo

+ * Later, during insert phase we will write actual records into thse buffers.
+ */

%s/thse/these

4) UndoRecordUpdateTransInfo() comments says that this must be called under
the critical section, but seems like undo_xlog_apply_progress() do call it
outside of critical section?  Is there exception, then should add comments?
or Am I missing anything?


5) In function UndoBlockGetFirstUndoRecord() below code:

    /* Calculate the size of the partial record. */
    partial_rec_size = UndoRecordHeaderSize(phdr->uur_info) +
                       phdr->tuple_len + phdr->payload_len -
                       phdr->record_offset;

can directly use UndoPagePartialRecSize().

6)

+static int
+UndoGetBufferSlot(UndoRecordInsertContext *context,
+                  RelFileNode rnode,
+                  BlockNumber blk,
+                  ReadBufferMode rbm)
+{
+    int            i;

In the above code variable "i" is mean "block index".  It would be good
to give some valuable name to the variable, maybe "blockIndex" ?

7)

* We will also keep a previous undo record pointer to the first and last undo
 * record of the transaction in the previous log.  The last undo record
 * location is used find the previous undo record pointer during rollback.


%s/used fine/used to find

8)

/*
 * Defines the number of times we try to wait for rollback hash table to get
 * initialized.  After these many attempts it will return error and the user
 * can retry the operation.
 */
#define ROLLBACK_HT_INIT_WAIT_TRY      60

%s/error/an error

9)

 * we can get the exact size of partial record in this page.
 */

%s/of partial/of the partial"

10)

* urecptr - current transaction's undo record pointer which need to be set in
*             the previous transaction's header.

%s/need/needs

11)

    /*
     * If we are writing first undo record for the page the we can set the
     * compression so that subsequent records from the same transaction can
     * avoid including common information in the undo records.
     */


%s/the page the/the page then

12)

    /*
     * If the transaction's undo records are split across the undo logs.  So
     * we need to  update our own transaction header in the previous log.
     */

double space between "to" and "update"

13)

* The undo record should be freed by the caller by calling ReleaseUndoRecord.
 * This function will old the pin on the buffer where we read the previous undo
 * record so that when this function is called repeatedly with the same context

%s/old/hold

I will continue further review for the same patch.

Regards,