Re: Changeset Extraction v7.7

On Wed, Feb 26, 2014 at 12:29 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-02-24 17:06:53 -0500, Robert Haas wrote:
>> - heap_page_prune_opt(scan->rs_rd, buffer, RecentGlobalXmin);
>> + 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);
>>
>> Instead of changing the callers of heap_page_prune_opt() in this way,
>> I think it might be better to change heap_page_prune_opt() to take
>> only the first two of its current three parameters; everybody's just
>> passing RecentGlobalXmin right now anyway.
>
> I've changed stuff this way, and it indeed looks better.
>
> I am wondering about the related situation of GetOldestXmin()
> callers. There's a fair bit of duplicated logic in the callers, before
> but especially after this patchset. What about adding 'Relation rel'
> parameter instead of `allDbs' and `systable'? That keeps the logic
> centralized and there's been a fair amount of talk about vacuum
> optimizations that could also use it.
> It's a bit sad that that requires including rel.h from procarray.h...
>
> What do you think? Isolated patch attached.

Seems reasonable to me.

+ * considered, but for non-shared non-shared relations that's not required,

Duplicate word.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company