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
Attachments:
GetCurrVXIDs.v1.patchtext/x-patch; charset=UTF-8; name=GetCurrVXIDs.v1.patchDownload
Index: src/backend/storage/ipc/procarray.c
===================================================================
RCS file: /home/sriggs/pg/REPOSITORY/pgsql/src/backend/storage/ipc/procarray.c,v
retrieving revision 1.47
diff -c -r1.47 procarray.c
*** src/backend/storage/ipc/procarray.c 1 Jan 2009 17:23:47 -0000 1.47
--- src/backend/storage/ipc/procarray.c 16 Jan 2009 16:39:23 -0000
***************
*** 1023,1030 ****
*
* The array is palloc'd and is terminated with an invalid VXID.
*
! * If limitXmin is not InvalidTransactionId, we skip any backends
! * with xmin >= limitXmin. If allDbs is false, we skip backends attached
* to other databases. If excludeVacuum isn't zero, we skip processes for
* which (excludeVacuum & vacuumFlags) is not zero. Also, our own process
* is always skipped.
--- 1023,1030 ----
*
* The array is palloc'd and is terminated with an invalid VXID.
*
! * If limitXmin is not InvalidTransactionId, we skip any backends with a
! * valid xmin >= limitXmin. If allDbs is false, we skip backends attached
* to other databases. If excludeVacuum isn't zero, we skip processes for
* which (excludeVacuum & vacuumFlags) is not zero. Also, our own process
* is always skipped.
***************
*** 1059,1069 ****
TransactionId pxmin = proc->xmin;
/*
! * Note that InvalidTransactionId precedes all other XIDs, so a
! * proc that hasn't set xmin yet will always be included.
*/
if (!TransactionIdIsValid(limitXmin) ||
! TransactionIdPrecedes(pxmin, limitXmin))
{
VirtualTransactionId vxid;
--- 1059,1070 ----
TransactionId pxmin = proc->xmin;
/*
! * If we have a specific limitXmin then we need not include
! * any procs with an unset xmin, since they have no snapshots
! * older than limitXmin
*/
if (!TransactionIdIsValid(limitXmin) ||
! (TransactionIdIsValid(pxmin) && TransactionIdPrecedes(pxmin, limitXmin)))
{
VirtualTransactionId vxid;
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.com
PostgreSQL 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.com
PostgreSQL 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! There might still be snapshots in
* use that treat some transaction as in-progress that our reference
* snapshot treats as committed. If such a recently-committed transaction
* deleted tuples in the table, we will not include them in the index; yet
* those transactions which see the deleting one as still-in-progress will
* expect them to be there once we 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 contain tuples deleted just
* before the reference snap was taken, we have to wait out any
* transactions that might have older 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.com
PostgreSQL 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