Re: logical changeset generation v6.7

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
HeapTupleGetOidnot 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
changedfor 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
ofthe  additional information. XLOG_HEAP_CONTAINS_NEW_TUPLE only  seems to be needed. Also is in heap_multi_insert and
heap_upate.
- 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]
makingnew  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
constructionon 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
shouldbe  removed.
 
- log_heap_new_cid() is called at several part just before other  xlogs is being inserted. I suppose this should be
builtin 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
becomestrue 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,
seemstoo  long. Isn't MarkCur(r/rent)XidLoggedIfAny sufficient?  Anyway,  the work involving this function seems would
bebetter 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  INDEX_ATTR_BITMAP_FKEY. And INDEX_ATTR_BITMAP_IDENTITY_KEY  should be
INDEX_ATTR_BITMAP_REPLID_KEYor 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