Re: GetCurrentVirtualXIDs()

Simon Riggs <simon(at)2ndQuadrant(dot)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