Hello, This is rather trivial and superficial comments as not
fully gripping functions of this patchset.

- Some patches have line offset to master. Rebase needed.

Other random comments follows,

===== 0001:

 - You assined HeapTupleGetOid(tuple) into relid to read in
   several points but no modification. Nevertheless, you left
   HeapTupleGetOid not replaced there. I think 'relid' just for
   repeated reading has far small merit compared to demerit of
   lowering readability. You'd be better to make them uniform in
   either way.

===== 0002:

 - You are identifying the wal_level with the expr 'wal_level >=
   WAL_LEVEL_LOGICAL' but it seems somewhat improper.

 - In heap_insert, you added following comment and code,

   >     * Also, if this is a catalog, we need to transmit combocids to
   >     * properly decode, so log that as well.
   >     */
   >    need_tuple_data = RelationIsLogicallyLogged(relation);
   >    if (RelationIsAccessibleInLogicalDecoding(relation))
   >            log_heap_new_cid(relation, heaptup);

   Actually 'is a catalog' is checkied in
   RelationIsAcc...Decodeing() but this either of naming or
   commnet should be changed for consistent look. (I this the
   name of the macro is rather long but gives a vague
   illustration of the function..)

 - RelationIsAccessibleInLogicalDecoding and
   RelationIsLogicallyLogged are identical in code. Together with
   the name ambiguity, this is quite confising and cause of
   future misuse between these macros, I suppose. Plus the names
   seem too long.

 - In heap_insert, the information conveyed on rdata[3] seems to
   be better to be in rdata[2] because of the scarecity of the
   additional information. XLOG_HEAP_CONTAINS_NEW_TUPLE only
   seems to be needed. Also is in heap_multi_insert and

 - In heap_multi_insert, need_cids referred only once so might be
   better removed.

 - In heap_delete, at the point adding replica identity, same to
   the aboves, rdata[3] could be moved into rdata[2] making new
   type like 'xl_heap_replident'.

 - In heapam_xlog.h, the new type xl_heap_header_len is
   inadequate in both of naming which is confising and
   construction on which the header in xl_heap_header is no
   longer be a header and indecisive member name 't_len'..

 - In heapam_xlog.h, XLOG_HEAP_CONTAINS_OLD looks incomplete. And
   it seems to be used in nowhere in this patchset. It should be

 - log_heap_new_cid() is called at several part just before other
   xlogs is being inserted. I suppose this should be built in the
   target xlog structures.

 - in RecovoerPreparedTransactions(), any commend needed for the
   reason calling XLogLogicalInfoActive()..

 - In xact.c, the comment for the member 'didLogXid' in
   TransactionStateData seems differ from it's meaning. It
   becomes true when any WAL record for the current transaction
   id just has been written to WAL buffer. So the comment,

   > /* has xid been included in WAL record? */

   would be better be something like (Should need corrected as
   I'm not native speaker.)

    /* Any WAL record for this transaction has been emitted ? */

   and also the member name should be something like
   XidIsLogged. (Not so chaned?)

 - The name of the function MarkCurrentTransactionIdLoggedIfAny,
   although irregular abbreviations are discouraged, seems too
   long. Isn't MarkCur(r/rent)XidLoggedIfAny sufficient?  Anyway,
   the work involving this function seems would be better to be
   done in some other way..

 - The comment for RelationGetIndexAttrBitmap() should be edited
   for attrKind.

 - The macro name INDEX_ATTR_BITMAP_KEY should be
   should be INDEX_ATTR_BITMAP_REPLID_KEY or something.

 - In rel.h the member name 'rd_idattr' would be better being
   'rd_replidattr' or something like that.

===== 0004:

 - Could the macro name 'RelationIsUsedAsCatalogTable' be as
   simple as IsUserCatalogRelation or something? It's from the
   viewpoint of not only simplicity but also similarity to other
   macro and/or functions having closer functionality. You
   already call the table 'user_catalog_table' in rel.h.

To be continued to next mail.

Kyotaro Horiguchi
NTT Open Source Software Center

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to