On Wed, Jul 24, 2019 at 11:28 AM Rushabh Lathia
<rushabh.lat...@gmail.com> wrote:
>
> Hi,
>
> 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.
I have improved the readme where I am describing the more user
specific details based on Robert's suggestions offlist. I think I
need further improvement which can describe the order of api's to be
called. Unfortunately that is not yet included in this patch set.
>
> 2) Information about the API's in the undoaccess.c file header block would
> good. For reference please look at heapam.c.
Done
>
> 3) typo
>
> + * Later, during insert phase we will write actual records into thse buffers.
> + */
>
> %s/thse/these
Fixed
>
> 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?
During recovery, there is an exception but we can add comments for the same.
I think I missed this in the latest patch, I will keep a note of it
and will do this in the next version.
>
>
> 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().
This function is part of another patch in undoprocessing patch set
>
> 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" ?
>
Fixed
> 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
Fixed
>
> 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
This is part of different patch in undoprocessing patch set
>
> 9)
>
> * we can get the exact size of partial record in this page.
> */
>
> %s/of partial/of the partial"
This comment is removed in the latest code
>
> 10)
>
> * urecptr - current transaction's undo record pointer which need to be set in
> * the previous transaction's header.
>
> %s/need/needs
Done
>
> 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"
Fixed
>
> 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
Fixed
>
> I will continue further review for the same patch.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
