Re: Review: Fix snapshot taking inconsistencies

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
> On 2010-10-13 1:21 AM +0300, Tom Lane wrote:
>> I started looking at this patch, and I'm wondering why you inserted all
>> the Register/UnregisterSnapshot calls that weren't there before

> That's actually just my ignorance I forgot to mention. As I understand
> it, our code currently first pushes one snapshot and then does multiple
> PushActiveSnapshot (or PushUpdatedSnapshot)/PopActiveSnapshot rounds
> before popping the oldest snapshot off the stack (and releasing it). So
> in the patch, I would've had to push the snapshot twice the first time
> to avoid it being released.

It looks to me like you've added quite a lot of management overhead that
wasn't there before. Wouldn't it be better to just not pop the snapshot
till you're done with it?

> Thinking about it now, that might be a better option. Or maybe we
> should change the snapshot API to make this more convenient?

Well, I'm not in love with the current snapshot API by any means,
particularly not PushUpdatedSnapshot which seems to be the only
API-sanctioned way to put a new CID into a snapshot without taking
a whole new snapshot. It'd be better if the logic was something
along the lines of:

* at start of a query, PushActiveSnapshot(GetTransactionSnapshot()).

* between commands of a query, CommandCounterIncrement and
then directly modify the curcid of the active snapshot;
AFAICS there's no reason to make another copy of it at this
point. Especially not if we can see it has refcount 1.

* at end of query, PopActiveSnapshot().

where a "query" is whatever we think the unit of noticing commits by
other backends ought to be.

> The spi.c change also changes the logic; the SPI code currently takes a
> new snapshot for every query if the caller doesn't provide a snapshot.

[ squint... ] Oh. I see now, but that is horribly ugly and
underdocumented. The code was previously treating the snapshot argument
as a constant and relying on that constant value to tell it what to do
each time through the loop. Now you've got it changing the flag and
then changing it back sometime later. Ick.

I think what you need to do to make this understandable is to move the
snapshot push/pop logic outside the per-command loop, instead of hacking
things around to keep it exactly where it was before. We may well need
to adjust the API of snapmgr.c to make that sane.

BTW, this patch seems to be also the time to remove the AtStart_Cache()
call in CommandCounterIncrement, as foreseen in the comment there.

regards, tom lane