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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers