On Fri, Jan 26, 2018 at 4:24 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> I took a look at this today and thought it might be OK to commit,
Thank you for looking at this!
> modulo a few minor issues: (1) you didn't document the new tranche and
Fixed.
> (2) I prefer to avoid if (blah) { Assert(thing) } in favor of
> Assert(!blah || thing).
Done.
> But then I got a little bit concerned about ReleasePredicateLocks().
> Suppose that in the middle of a read-only transaction,
> SXACT_FLAG_RO_SAFE becomes true. The next call to
> SerializationNeededForRead in each process will call
> ReleasePredicateLocks. In the workers, this won't do anything, so
> we'll just keep coming back there. But in the leader, we'll go ahead
> do all that stuff, including MySerializableXact =
> InvalidSerializableXact. But in the workers, it's still set. Maybe
> that's OK, but I'm not sure that it's OK...
Ouch. Yeah. It's not OK. If the leader gives up its
SERIALIZABLEXACT object early due to that safe-read-only optimisation,
the workers are left with a dangling pointer to a SERIALIZABLEXACT
object that has been pushed onto FinishedSerializableTransactions.
From there it will move to PredXact->availableTransactions and might
be handed out to some other transaction, so it is not safe to retain a
pointer to that object.
The best solution I have come up with so far is to add a reference
count to SERIALIZABLEXACT. I toyed with putting the refcount into the
DSM instead, but then I ran into problems making that work when you
have a query with multiple Gather nodes. Since the refcount is in
SERIALIZABLEXACT I also had to add a generation counter so that I
could detect the case where you try to attach too late (the leader has
already errored out, the refcount has reached 0 and the
SERIALIZABLEXACT object has been recycled).
The attached is a draft patch only, needing some testing and polish.
Brickbats, better ideas?
FWIW I also considered a couple of other ideas: (1) Keeping the
object alive on the FinishedSerializableTransactions list until the
leader's transaction is finished seems broken because we need to be
able to spill that list to the SLRU at any time, and if we somehow
made them sticky we could run out of memory. (2) Anything involving
the leader having sole control of the object lifetime seems
problematic... well, it might work if you disabled the
SXACT_FLAG_RO_SAFE optimisation so that ReleasePredicateLocks() always
happens after all workers have finished, but that seems like cheating.
PS I noticed that for BecomeLockGroupMember() we say "If we can't
join the lock group, the leader has gone away, so just exit quietly"
but for various other similar things we spew errors (most commonly
seen one being "ERROR: could not map dynamic shared memory segment").
Intentional?
--
Thomas Munro
http://www.enterprisedb.com