On Tue, Jul 16, 2019 at 2:20 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
Few comments on the new patch:
1.
Additionally,
+there is a mechanism for multi-insert, wherein multiple records are prepared
+and inserted at a time.
Which mechanism are you talking about here? By any chance is this
related to some old code?
2.
+Fetching and undo record
+------------------------
+To fetch an undo record, a caller must provide a valid undo record pointer.
+Optionally, the caller can provide a callback function with the information of
+the block and offset, which will help in faster retrieval of undo record,
+otherwise, it has to traverse the undo-chain.
I think this is out-dated information. You seem to forget updating
README after latest changes in API.
3.
+ * The cid/xid/reloid/rmid information will be added in the undo record header
+ * in the following cases:
+ * a) The first undo record of the transaction.
+ * b) First undo record of the page.
+ * c) All subsequent record for the transaction which is not the first
+ * transaction on the page.
+ * Except above cases, If the rmid/reloid/xid/cid is same in the subsequent
+ * records this information will not be stored in the record, these information
+ * will be retrieved from the first undo record of that page.
+ * If any of the member rmid/reloid/xid/cid has changed, the changed
information
+ * will be stored in the undo record and the remaining information will be
+ * retrieved from the first complete undo record of the page
+ */
+UndoCompressionInfo undo_compression_info[UndoLogCategories];
a. Do we want to compress fork_number also? It is an optional field
and is only include when undo record is for not MAIN_FORKNUM. For
zheap, this means it will never be included, but in future, it could
be included for some other AM or some other use case. So, not sure if
there is any benefit in compressing the same.
b. cid/xid/reloid/rmid - I think it is better to write it as rmid,
reloid, xid, cid in the same order as you declare them in
UndoPackStage.
c. Some minor corrections. /Except above/Except for above/; /, If
the/, if the/; /is same/is the same/; /record, these
information/record rather this information/
d. I think there is no need to start the line "If any of the..." from
a new line, it can be continued where the previous line ends. Also,
at the end of that line, add a full stop.
4.
/*
+ * Copy the compression global compression info to our context before
+ * starting prepare because this value might get updated multiple time in
+ * case of multi-prepare but the global value should be updated only after
+ * we have successfully inserted the undo record.
+ */
In the above comment, the first 'compression' is not required. /time/times/
5.
+/*
+ * The below common information will be stored in the first undo
record of the page.
+ * Every subsequent undo record will not store this information, if
required this information
+ * will be retrieved from the first undo record of the page.
+ */
+typedef struct UndoCompressionInfo
The line length in the above comments exceeds the 80-char limit. You
might want to run pgindent to avoid such problems.
6.
+/*
+ * Exclude the common info in undo record flag and also set the compression
+ * info in the context.
+ *
'flag' seems to be a redundant word here?
7.
+UndoSetCommonInfo(UndoCompressionInfo *compressioninfo,
+ UnpackedUndoRecord *urec, UndoRecPtr urp,
+ Buffer buffer)
+{
+
+ /*
+ * If we have valid compression info and the for the same transaction and
+ * the current undo record is on the same block as the last undo record
+ * then exclude the common information which are same as first complete
+ * record on the page.
+ */
+ if (compressioninfo->valid &&
+ FullTransactionIdEquals(compressioninfo->fxid, urec->uur_fxid) &&
+ UndoRecPtrGetBlockNum(urp) == UndoRecPtrGetBlockNum(lasturp))
Here the comment is just a verbal for of if-check. How about writing
it as: "Exclude the common information from the record which is same
as the first record on the page."
8.
UndoSetCommonInfo()
{
..
if (compressioninfo->valid &&
+ FullTransactionIdEquals(compressioninfo->fxid, urec->uur_fxid) &&
+ UndoRecPtrGetBlockNum(urp) == UndoRecPtrGetBlockNum(lasturp))
+ {
+ urec->uur_info &= ~UREC_INFO_XID;
+
+ /* Don't include rmid if it's same. */
+ if (urec->uur_rmid == compressioninfo->rmid)
+ urec->uur_info &= ~UREC_INFO_RMID;
+
+ /* Don't include reloid if it's same. */
+ if (urec->uur_reloid == compressioninfo->reloid)
+ urec->uur_info &= ~UREC_INFO_RELOID;
In all the checks except for transaction id, urec's info is on the
left side. I think all the checks can be consistent.
These are some of the things I noticed while skimming through this
patch. I will do some more detailed review later.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
