Re: Review: Fix snapshot taking inconsistencies

On 2010-10-13 2:10 AM +0300, Tom Lane wrote:
> Marko Tiikkaja<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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