Why is RegisterPredicateLockingXid called while holding XidGenLock?

On Thu, May 05, 2011 at 11:12:40PM -0400, Tom Lane wrote:

Even if it's actually necessary to set up that data structure while
holding XidGenLock, I would *really* like the call to not be exactly
where it is.

Good question.

I don't believe it needs to be while XidGenLock is being held at all;
certainly not in this particular place. All that really matters is that
it happens before any backend starts seeing said xid in tuple headers.

I believe the call can be moved over to AssignTransactionId. I'd
probably put it at the end of that function, but it can go anywhere
between there and where it is now. Do you have any preference?

Dan

--
Dan R. K. Ports MIT CSAIL http://drkp.net/

Tom Lane wrote:

This seems like a pretty lousy place to do it, first because of the
contention hit from holding that high-traffic lock any longer than
necessary, and second because every added chance for error between
ExtendCLOG() and TransactionIdAdvance(ShmemVariableCache->nextXid)
gives us another way to fail in the way recently mentioned by Joe
Conway:

http://archives.postgresql.org/message-id/4DBE4E7D.80501@joeconway.com

Even if it's actually necessary to set up that data structure while
holding XidGenLock, I would *really* like the call to not be
exactly where it is.

On a quick scan, I can't see any reason this couldn't be moved down
to just above the return. I think the reason I dropped it where I
did was simply that it was where we seemed to be letting other parts
of the system know about the new xid, so I was going for consistency.

I want to double-check this when I'm a little more awake, but my I
don't think that anything will be doing anything in the predicate
locking area where xid would matter until after the return from this
function.

-Kevin

Dan Ports <drkp@csail.mit.edu> writes:

On Thu, May 05, 2011 at 11:12:40PM -0400, Tom Lane wrote:

Even if it's actually necessary to set up that data structure while
holding XidGenLock, I would *really* like the call to not be exactly
where it is.

Good question.

I don't believe it needs to be while XidGenLock is being held at all;
certainly not in this particular place. All that really matters is that
it happens before any backend starts seeing said xid in tuple headers.

I believe the call can be moved over to AssignTransactionId. I'd
probably put it at the end of that function, but it can go anywhere
between there and where it is now. Do you have any preference?

Yeah, I was thinking that it'd be better to pull it out of
GetNewTransactionId and put it in a higher level. No strong preference
about where in AssignTransactionId to put it. Is there any chance that
it would be significant whether we do it before or after taking the lock
on the XID (XactLockTableInsert)?

regards, tom lane

Tom Lane wrote:

Yeah, I was thinking that it'd be better to pull it out of
GetNewTransactionId and put it in a higher level.

As long as it is always called when an xid is assigned. Since this
function appears to be on the only path to that, it should be fine.

No strong preference about where in AssignTransactionId to put it.
Is there any chance that it would be significant whether we do it
before or after taking the lock on the XID (XactLockTableInsert)?

No, but since we need to do it only on a top level assignment, we
could save a couple cycles by putting it on an else on line 456.

-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane wrote:

Yeah, I was thinking that it'd be better to pull it out of
GetNewTransactionId and put it in a higher level.

As long as it is always called when an xid is assigned. Since this
function appears to be on the only path to that, it should be fine.

Well, the division of labor between GetNewTransactionId and
AssignTransactionId isn't terribly well-defined, but to the extent that
there is any bright line it is that the former does what needs to be
done while holding XidGenLock. So I'd prefer to see the call out of
there entirely. The fact that varsup.c has no other connection to the
SSI code is an additional argument that it doesn't belong there.

No strong preference about where in AssignTransactionId to put it.
Is there any chance that it would be significant whether we do it
before or after taking the lock on the XID (XactLockTableInsert)?

No, but since we need to do it only on a top level assignment, we
could save a couple cycles by putting it on an else on line 456.

Didn't particularly care for that, since this action is not the inverse
of, nor in any other way related to, pushing the XID into pg_subtrans.
After some thought I did this instead:

if (isSubXact)
SubTransSetParent(s->transactionId, s->parent->transactionId, false);

/*
* If it's a top-level transaction, the predicate locking system needs to
* be told about it too.
*/
if (!isSubXact)
RegisterPredicateLockingXid(s->transactionId);

A reasonably bright compiler will optimize that into the same thing, and
if the compiler doesn't catch it, it's an insignificant cost anyway.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:

After some thought I did this instead:

Thanks! I can see why that's better.

-Kevin