Re: logical changeset generation v6.7

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