Re: pgsql: Fix a couple of bugs in MultiXactId freezing

On 2013-12-13 13:39:20 -0300, Alvaro Herrera wrote:
> Here's cache code with LRU superpowers (ahem.)

Heh.

> I settled on 256 as number of entries because it's in the same ballpark
> as MaxHeapTuplesPerPage which seems a reasonable guideline to follow.

Sounds ok.

> I considered the idea of avoiding palloc/pfree for cache entries
> entirely, instead storing them in a static array which is referenced
> from the dlist; unfortunately that doesn't work because each cache entry
> is variable size, depending on number of members.  We could try to work
> around that and allocate a large shared array for members, but that
> starts to smell of over-engineering, so I punted.

Good plan imo.

> *** 1326,1331 **** mXactCacheGetBySet(int nmembers, MultiXactMember *members)
> --- 1331,1337 ----
>           if (memcmp(members, entry->members, nmembers * sizeof(MultiXactMember)) == 0)
>           {
>               debug_elog3(DEBUG2, "CacheGet: found %u", entry->multi);
> +             dlist_move_head(&MXactCache, iter.cur);
>               return entry->multi;
>           }
>       }

That's only possible because we immediately abort the loop, otherwise
we'd corrupt the iterator. Maybe that deserves a comment.

> + 
> +             dlist_move_head(&MXactCache, iter.cur);
> + 

Heh. I forgot that we already had that bit; I was wondering whether you
had to forgot to include it in the patch ;)

>   static char *
> --- 1420,1435 ----
>       /* mXactCacheGetBySet assumes the entries are sorted, so sort them */
>       qsort(entry->members, nmembers, sizeof(MultiXactMember), mxactMemberComparator);
>   
> !     dlist_push_head(&MXactCache, &entry->node);
> !     if (MXactCacheMembers++ >= MAX_CACHE_ENTRIES)
> !     {
> !         dlist_node *node;
> ! 
> !         node = dlist_tail_node(&MXactCache);
> !         dlist_delete(dlist_tail_node(&MXactCache));
> !         MXactCacheMembers--;
> !         pfree(dlist_container(mXactCacheEnt, node, node));
> !     }
>   }

Duplicate dlist_tail_node(). Maybe add a debug_elog3(.. "CacheGet:
pruning %u from cache")?

I wondered before if we shouldn't introduce a layer above dlists, that
support keeping track of the number of elements, and maybe also have
support for LRU behaviour. Not as a part this patch, just generally.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services