On 2013-12-03 19:15:53 +0900, Kyotaro HORIGUCHI wrote:
> - In heapam.c, it seems to be better replacing t_self only
> during logical decoding.
I don't see what'd be gained by that except make the test matrix bigger
at no gain.
> - 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.
The comment above the part explains the reason:
/*
* Acquire the current global xmin value and directly set the logical
xmin
* before releasing the lock if necessary. We do this so wal decoding is
* guaranteed to have all catalog rows produced by xacts with an xid >
* walsnd->xmin available.
*
* We can't use ComputeLogicalXmin here as that acquires ProcArrayLock
* separately which would open a short window for the global xmin to
* advance above walsnd->xmin.
*/
> - 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.
What if LogicalDecodingCountDBSlots() is needed in other places? That
seems like a layering violation to me.
> - 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,
I don't see an advantage in that.
> - In tqual.c, in Setup/RevertFrom DecodingSnapshots, the name
> CatalogSnapshotData looks lacking unity with other
> Snapshot*Data's.
That part needs a bit of work, agreed.
> ===== 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..
Hm. It's a mighty long name... but it indeed is clearer.
> - 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);
Well, it requires introducing a new variable (which better not be named
xmin, but OldestXmin or similar). But I don't really care.
> index_fetch_heap in indexam.c has similar part to this, and
> you coded in latter style in IndexBuildHeapScan in index.c.
It's different there, because we do an explicit GetOldestXmin() call
there which we surely don't want to do twice.
> 0008 and after to come later..
Thanks for your review!
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
