Re: Snapshot management, final - Mailing list pgsql-patches

From Tom Lane
Subject Re: Snapshot management, final
Date
Msg-id 25109.1210547423@sss.pgh.pa.us
Whole thread Raw
In response to Re: Snapshot management, final  (Alvaro Herrera <alvherre@commandprompt.com>)
Responses Re: Snapshot management, final
Re: Snapshot management, final
List pgsql-patches
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> Also, I think that the whole snapshot-sharing mechanism is not working
>> as intended except for the serializable case; otherwise sequences
>> like
>> x = RegisterSnapshot(GetTransactionSnapshot());
>> y = RegisterSnapshot(GetTransactionSnapshot());
>> will result in x and y being separate copies.  Or are you assuming
>> that this just isn't worth optimizing?

> It's not that I don't think it's worth optimizing, but I think it's a
> bit away from the scope of this patch.  The problem here is how to
> notice that two consecutive GetTransactionSnapshot calls should really
> return different snapshots, considering that shared state may change in
> between.  Perhaps there's an easy way to optimize that; I don't know.

Yeah, in general you could only optimize it if no other backend had
changed state, and there doesn't seem any real simple way to know that.
Maybe we could teach GetSnapshotData to test for it but it's a bit
doubtful that it's worth the cycles.  I'm fine with leaving this as-is.

I have a few other gripes though:

The UnregisterSnapshot call at line 631 of indexcmds.c is definitely too
early: the snapshot is touched in the very next statement.  I'd be
inclined to move it down to right after the Pop at line 696; there's
no point in unregistering till you pop anyway.

In FreeQueryDesc, the unregister calls should be after the Assert.

Drop this comment fragment in plancache.c, it's not relevant anymore:

                         * Having to
!              * replan is an unusual case, and it seems a really bad idea for
!              * RevalidateCachedPlan to affect the snapshot only in unusual
!              * cases.
!              */

s_level and as_level fields must be int not uint32, because they are
being compared to nesting levels that are declared as int.  You risk
getting the wrong comparison semantics.  (Not that it should ever matter
in this code, but mixing signed and unsigned arithmetic is just bad
form.)

Why Assert(snap->satisfies == HeapTupleSatisfiesMVCC) in PushActiveSnapshot
and RegisterSnapshot?  AFAICT this code will work fine on non-MVCC
snapshots.

Seems a bit odd that RegisterSnapshot and PushActiveSnapshot do their
palloc's in opposite orders.  I think making the list elt first is
probably better; in either case, if the second palloc fails then you're
gonna leak the first one, so it's better to make the smaller allocation
first.

Shouldn't UnregisterSnapshot insist that s_level be equal to current
xact nest level?

AtSubCommit_Snapshot can leave us with multiple RegdSnapshotElt's for
the same snap and s_level, which seems a bit bogus.  I don't think the
unregister code will go wrong in its current form, but you at least need
some comments about the invariants that are expected to hold for the
list data structure.  Example: the code depends on the assumption that
elements are in nonincreasing s_level order, but that's nowhere stated.

AtSubAbort_Snapshot has Assert(tofree->s_snap->active_count == 0)
which seems wrong: couldn't the snap be active in an outer subxact?

            regards, tom lane

pgsql-patches by date:

Previous
From: Tom Lane
Date:
Subject: Re: [badalex@gmail.com: Re: [BUGS] Problem identifying constraints which should not be inherited]
Next
From: Tom Lane
Date:
Subject: Re: options for RAISE statement