Re: logical changeset generation v6.7 - Mailing list pgsql-hackers
From | Kyotaro HORIGUCHI |
---|---|
Subject | Re: logical changeset generation v6.7 |
Date | |
Msg-id | 20131203.191553.144706248.horiguchi.kyotaro@lab.ntt.co.jp Whole thread Raw |
In response to | Re: logical changeset generation v6.7 (Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>) |
Responses |
Re: logical changeset generation v6.7
Re: logical changeset generation v6.7 |
List | pgsql-hackers |
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
pgsql-hackers by date: