Marko Tiikkaja <marko.tiikk...@cs.helsinki.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers