Re: Review: Fix snapshot taking inconsistencies

On 2010-10-13 2:10 AM +0300, Tom Lane wrote:
> Marko Tiikkaja<marko.tiikkaja@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