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.
Wedo 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
windowfor 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