Re: [HACKERS] Lazy hash table for XidInMVCCSnapshot (helps Zipfian a bit)

Greetings,

* Sokolov Yura (funny(dot)falcon(at)postgrespro(dot)ru) wrote:
> diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
> index 08a08c8e8f..7c3fe7563e 100644
> --- a/src/backend/utils/time/snapmgr.c
> +++ b/src/backend/utils/time/snapmgr.c
> @@ -662,13 +662,16 @@ CopySnapshot(Snapshot snapshot)
> Snapshot newsnap;
> Size subxipoff;
> Size size;
> + int xcnt, subxcnt;
> + uint8 xhlog, subxhlog;
>
> Assert(snapshot != InvalidSnapshot);
>
> + xcnt = ExtendXipSizeForHash(snapshot->xcnt, &xhlog);
> + subxcnt = ExtendXipSizeForHash(snapshot->subxcnt, &subxhlog);
> /* We allocate any XID arrays needed in the same palloc block. */
> - size = subxipoff = sizeof(SnapshotData) +
> - snapshot->xcnt * sizeof(TransactionId);
> - if (snapshot->subxcnt > 0)
> + size = subxipoff = sizeof(SnapshotData) + xcnt * sizeof(TransactionId);
> + if (subxcnt > 0)
> size += snapshot->subxcnt * sizeof(TransactionId);

Here you've changed to use xcnt instead of snapshot->xcnt for
calculating size, but when it comes to adding in the subxcnt, you
calculate a subxcnt but still use snapshot->subxcnt...? That doesn't
seem like what you intended to do here.

> diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
> index f9da9e17f5..a5e7d427b4 100644
> --- a/src/backend/utils/time/tqual.c
> +++ b/src/backend/utils/time/tqual.c
> @@ -1451,6 +1451,69 @@ HeapTupleIsSurelyDead(HeapTuple htup, TransactionId OldestXmin)
> }
>
> /*
> + * XidInXip searches xid in xip array.
> + *
> + * If xcnt is smaller than SnapshotMinHash (60), or *xhlog is zero, then simple
> + * linear scan of xip array used. Looks like there is no benifit in hash table
> + * for small xip array.

I wouldn't write '60' in there, anyone who is interested could go look
up whatever it ends up being set to.

I tend to agree with Robert that it'd be nice if simplehash could be
used here instead. The insertion is definitely more expensive but
that's specifically to try and improve lookup performance, so it might
end up not being so bad. I do understand that it would end up using
more memory, so that's a fair concern.

I do think this could use with more comments and perhaps having some
Assert()'s thrown in (and it looks like you're removing one..?).

I haven't got a whole lot else to say on this patch, would be good if
someone could spend some more time reviewing it more carefully and
testing it to see what kind of performance they get. The improvements
that you've posted definitely look nice, especially with the lwlock
performance work.

Thanks!

Stephen