On 2010-10-13 2:10 AM +0300, Tom Lane wrote:
Marko Tiikkaja<marko.tiikk...@cs.helsinki.fi> writes:
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?
Yes, you're absolutely right.
It'd be better if the logic was something
along the lines of:
That's exactly what I had in mind, so +1.
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.
*blushes*
Yeah, that makes a lot more sense.
BTW, this patch seems to be also the time to remove the AtStart_Cache()
call in CommandCounterIncrement, as foreseen in the comment there.
Frankly, I have no idea what to do about this.
Regards,
Marko Tiikkaja
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
