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
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] 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
removed.
- 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
INDEX_ATTR_BITMAP_FKEY. And INDEX_ATTR_BITMAP_IDENTITY_KEY
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:
http://www.postgresql.org/mailpref/pgsql-hackers
