Re: logical changeset generation v6.7

Hello, this is continued comments.

> ===== 0004:
....
> To be continued to next mail.


===== 0005:
- In heapam.c, it seems to be better replacing t_self only  during logical decoding.
- In GetOldestXmin(), the parameter name 'alreadyLocked' would  be better being simplly 'nolock' since alreadyLocked
seemsto  me suggesting that it will unlock the lock acquired beforehand.
 
- Before that, In LogicalDecodingAcquireFreeSlot, lock window  for procarray is extended after GetOldestXmin, but
procarray does not seem to be accessed during the additional period. If  you are holding this lock for the purpose
otherthan accessing  procArray, it'd be better to provide its own lock object.
 
   > LWLockAcquire(ProcArrayLock, LW_SHARED);   > slot->effective_xmin = GetOldestXmin(true, true, true);   >
slot->xmin= slot->effective_xmin;   >    > if (!TransactionIdIsValid(LogicalDecodingCtl->xmin) ||   >
NormalTransactionIdPrecedes(slot->effective_xmin,LogicalDecodingCtl->xmin))   >     LogicalDecodingCtl->xmin =
slot->effective_xmin;  > LWLockRelease(ProcArrayLock);
 
- In dropdb in dbcommands.c, ereport is invoked referring the  result of LogicalDecodingCountDBSlots. But it seems
betterto  me issueing this exception within LogicalDecodingCountDBSlots  even if goto is required.
 
- In LogStandbySnapshot in standby.c, two complementary  conditions are imposed on two same unlocks. It might be
somewhatparanoic but it is safer being like follows,
 
   | XLogRecPtr  recptr = InvalidXLogRecPtr;   | ....   |   | /* LogCurrentRunningXacts shoud be done before unlock
whenlogical decoding*/    | if (wal_level >= WAL_LEVEL_LOGICAL)   |    recptr = LogCurrentRunningXacts(running);   |
|LWLockRelease(ProcArrayLock);   |    | if (recptr == InvalidXLogRecPtr)   |    recptr =
LogCurrentRunningXacts(running);    - In tqual.c, in Setup/RevertFrom DecodingSnapshots, the name  CatalogSnapshotData
lookslacking unity with other  Snapshot*Data's.
 

===== 0007:
- In heapam.c, the new global variable 'RecentGlobalDataXmin' is  quite similar to 'RecentGlobalXmin' and does not
represents what it is. The name should be  changed. RecentGlobalNonCatalogXmin would be more preferable..
 
- Althgough simplly from my teste, the following part in  heapam.c
   > if (IsSystemRelation(scan->rs_rd)   >     || RelationIsAccessibleInLogicalDecoding(scan->rs_rd))   >
heap_page_prune_opt(scan->rs_rd,buffer, RecentGlobalXmin);   > else   >     heap_page_prune_opt(scan->rs_rd, buffer,
RecentGlobalDataXmin);
  would be readable to be like,
   > if (IsSystemRelation(scan->rs_rd)   >     || RelationIsAccessibleInLogicalDecoding(scan->rs_rd))   >   xmin =
RecentGlobalXmin  > else   >   xmin = RecentGlobalDataXmin   >     heap_page_prune_opt(scan->rs_rd, buffer, xmin);
 
   index_fetch_heap in indexam.c has similar part to this, and   you coded in latter style in IndexBuildHeapScan in
index.c.
- In procarray.c, you should add documentation for new parameter  'systable' for GetOldestXmin. And the name 'systable'
seems somewhat confusing, since its full-splled meaning is  'including systables'. This name should be changed to
'include_systable'or 'only_usertable' with inverting or  something..
 

0008 and after to come later..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center