Thread: GetCurrentVirtualXIDs()
No need to wait for idle-in-transaction sessions during index builds. GetCurrentVirtualXIDs() specifically *includes* backends that have proc->xmin == InvalidTransactionId (0), but I'm not sure why. $SUBJECT is currently used by DefineIndex() to wait for all backends that might be able to see index changes in phase 2 of concurrent index build. The code comments say "we have to wait out any transactions that might have older snapshots". If proc->xmin == 0 it is because they haven't got any snapshots at all and therefore the index build does *not* need to wait for them. I'm using this routine for Hot Standby also, so patching this will allow me to ignore idle-in-transaction sessions unless they are from serializable transactions or have open cursors. But there's no need for me to include this in the patch if its a general fix. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and Support
Attachment
Simon Riggs <simon@2ndQuadrant.com> writes:
> No need to wait for idle-in-transaction sessions during index builds.
> GetCurrentVirtualXIDs() specifically *includes* backends that have
> proc->xmin == InvalidTransactionId (0), but I'm not sure why.
Because they might be about to change xmin to something real?
regards, tom lane
On Fri, 2009-01-16 at 12:15 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: > > No need to wait for idle-in-transaction sessions during index builds. > > GetCurrentVirtualXIDs() specifically *includes* backends that have > > proc->xmin == InvalidTransactionId (0), but I'm not sure why. > > Because they might be about to change xmin to something real? True, they might. GetSnapshotData() involuntarily sets xmin in their proc, so they would have to be doing some strange footwork: they would have to have started GetSnapshotData() *before* the index build and not finished it until *after* we check GetCurrentVirtualXIDs() (after the index build). If you think that is a serious possibility, then grabbing ProcArrayLock in exclusive mode should close that gap. I can't see a reason for idle-in-transaction sessions to block index builds. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
> No need to wait for idle-in-transaction sessions during index builds.
> GetCurrentVirtualXIDs() specifically *includes* backends that have
> proc->xmin == InvalidTransactionId (0), but I'm not sure why.
On further consideration, this patch is simply *wrong*, and would still
be wrong even if we changed GetCurrentVirtualXIDs to take ProcArrayLock
exclusive instead of shared.
If a backend currently has no snapshot, then if it takes a snapshot
immediately after we finish running GetCurrentVirtualXIDs, it will
set its proc->xmin (and that of the snapshot) to the oldest currently
running XID. There is no reason to assume that that value is >=
limitXmin, which is what you propose we do.
A safe modification of the patch would be to determine the oldest
running XID and exclude xmin-less VXIDs when that number is greater
than limitXmin. However, I think that that would be pretty useless:
for the one current use of GetCurrentVirtualXIDs, limitXmin is the
xmax of the snapshot we used for the index build, and we can assume
that our *own* XID is less than that, never mind anyone else's.
So I don't think this works...
regards, tom lane
On Fri, 2009-04-03 at 15:46 -0400, Tom Lane wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: > > No need to wait for idle-in-transaction sessions during index builds. > > GetCurrentVirtualXIDs() specifically *includes* backends that have > > proc->xmin == InvalidTransactionId (0), but I'm not sure why. > > On further consideration, this patch is simply *wrong*, and would still > be wrong even if we changed GetCurrentVirtualXIDs to take ProcArrayLock > exclusive instead of shared. > > If a backend currently has no snapshot, then if it takes a snapshot > immediately after we finish running GetCurrentVirtualXIDs, it will > set its proc->xmin (and that of the snapshot) to the oldest currently > running XID. There is no reason to assume that that value is >= > limitXmin, which is what you propose we do. > > A safe modification of the patch would be to determine the oldest > running XID and exclude xmin-less VXIDs when that number is greater > than limitXmin. However, I think that that would be pretty useless: > for the one current use of GetCurrentVirtualXIDs, limitXmin is the > xmax of the snapshot we used for the index build, and we can assume > that our *own* XID is less than that, never mind anyone else's. > > So I don't think this works... I see your logic and agree with it. However, the basic premise is that idle-in-transaction sessions do not need to block index builds. I think that's still true in many but not all cases, so we just need to modify it slightly to include the cases you mention. I propose: 1. GetCurrentVirtualXIDs() ignoring procs with proc->xmin == 0 2. Wait for all of those to disappear 3. GetCurrentVirtualXIDs() again with same limitXmin, again ignoring xmin==0 procs. 4. Wait again. This then catches any idle-in-transactions that took a snapshot after the first step and before the second. It doesn't guarantee that there are no backends that have an xmin less than limitXmin, but it does mean, I think, that they can't see any non-indexed row versions. Not sure if that helps my original thought for Hot Standby, but that's a different issue. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
> However, the basic premise is that idle-in-transaction sessions do not
> need to block index builds.
[ thinks for awhile... ] Actually, I believe that your premise is
correct; the problem is with your proof ;-). Considering only the
xmins is insufficient to prove that this is safe; but as we know,
xmin comparisons are an oversimplification of snapshot relationships.
What the indexcmds.c comments say are
* Now take the "reference snapshot" that will be used by validate_index() * to filter candidate tuples. Beware!
Theremight still be snapshots in * use that treat some transaction as in-progress that our reference * snapshot
treatsas committed. If such a recently-committed transaction * deleted tuples in the table, we will not include them
inthe index; yet * those transactions which see the deleting one as still-in-progress will * expect them to be there
oncewe mark the index as valid.
and then (after validate_index)
* The index is now valid in the sense that it contains all currently * interesting tuples. But since it might not
containtuples deleted just * before the reference snap was taken, we have to wait out any * transactions that might
haveolder snapshots. Obtain a list of VXIDs * of such transactions, and wait for them individually.
The important point here is that we only have to wait for transactions
that might have snapshots older than our reference snapshot. A
transaction with no snapshots (evidenced by its xmin = 0), a fortiori,
has no older snapshots. And even if it's in process of taking one when
GetCurrentVirtualXIDs examines it, it cannot conclude that any
transaction that our reference snap saw as committed is still running.
(This is true even if the other guy started his GetSnapshotData before
we did and has somehow managed to not finish yet. In that case, he's
been holding ProcArrayLock shared the whole time, and no transaction
could have exited "running" state at all.)
So on third thought I think the patch logic is sound; but I think that
as documentation we had better add another bool parameter to
GetCurrentVirtualXIDs indicating whether it's okay to ignore procs
with xmin = 0. It seems at least possible that some future usage of
GetCurrentVirtualXIDs might need that done differently.
(This logic also shows that we need not actually wait for a transaction
to *complete*; as soon as it's gone idle and has no snapshots, we could
disregard it. But that would take a much more invasive patch to
implement, so it's definitely too late for 8.4.)
Comments?
regards, tom lane
On Fri, 2009-04-03 at 18:28 -0400, Tom Lane wrote: > So on third thought I think the patch logic is sound; but I think that > as documentation we had better add another bool parameter to > GetCurrentVirtualXIDs indicating whether it's okay to ignore procs > with xmin = 0. That sounds better through being more explicit. I didn't consider whether the patched function was true in all cases, only that it looked correct in the current usage. Another lesson in future-proofing code. Thanks also for the clear proof. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
> On Fri, 2009-04-03 at 18:28 -0400, Tom Lane wrote:
>> So on third thought I think the patch logic is sound; but I think that
>> as documentation we had better add another bool parameter to
>> GetCurrentVirtualXIDs indicating whether it's okay to ignore procs
>> with xmin = 0.
> That sounds better through being more explicit. I didn't consider
> whether the patched function was true in all cases, only that it looked
> correct in the current usage. Another lesson in future-proofing code.
I had another thought about this. The point of the limitXmin filtering
is that we may exclude transactions whose oldest snapshot is provably
no older than our reference snapshot. Wouldn't it be sufficient to
exclude those with xmin > reference snapshot xmin? Currently we exclude
those with xmin >= reference snapshot xmax, which is obviously a weaker
condition.
regards, tom lane
Simon Riggs <simon@2ndQuadrant.com> writes:
> No need to wait for idle-in-transaction sessions during index builds.
> GetCurrentVirtualXIDs() specifically *includes* backends that have
> proc->xmin == InvalidTransactionId (0), but I'm not sure why.
Applied with the discussed tweaks. I also added some logic to make
DefineIndex recheck the GetCurrentVirtualXIDs output each time it is
about to wait for another session. If we observe some other session
to have xmin = 0 at any one of those instants, we don't have to
wait for it. Not sure how much that will really help in practice,
but it can't hurt.
regards, tom lane