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 seems to
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 other than 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 better to
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
somewhat paranoic but it is safer being like follows,

| XLogRecPtr recptr = InvalidXLogRecPtr;
| ....
|
| /* LogCurrentRunningXacts shoud be done before unlock when logical 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 looks lacking 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