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