Greetings, * Sokolov Yura (funny.fal...@postgrespro.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
signature.asc
Description: PGP signature
