Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data

On Fri, Jan 22, 2021 at 10:44:35AM +0500, Andrey Borodin wrote:
> > 17 янв. 2021 г., в 12:24, Noah Misch <noah(at)leadboat(dot)com> написал(а):
> >> @@ -4461,9 +4462,21 @@ bool
> >> VirtualXactLock(VirtualTransactionId vxid, bool wait)
> >> {
> >> LOCKTAG tag;
> >> - PGPROC *proc;
> >> + PGPROC *proc = NULL;
> >>
> >> - Assert(VirtualTransactionIdIsValid(vxid));
> >> + Assert(VirtualTransactionIdIsValid(vxid) ||
> >> + VirtualTransactionIdIsPreparedXact(vxid));
> >> +
> >> + if (VirtualTransactionIdIsPreparedXact(vxid))
> >> + {
> >> + LockAcquireResult lar;
> >> + /* If it's prepared xact - just wait for it */
> >> + SET_LOCKTAG_TRANSACTION(tag, vxid.localTransactionId);
> >> + lar = LockAcquire(&tag, ShareLock, false, !wait);
> >> + if (lar == LOCKACQUIRE_OK)
> >
> > This should instead test "!= LOCKACQUIRE_NOT_AVAIL", lest
> > LOCKACQUIRE_ALREADY_HELD happen. (It perhaps can't happen, but skipping the
> > LockRelease() would be wrong if it did.)
> I think that code that successfully acquired lock should release it. Other way we risk to release someone's else lock held for a reason. We only acquire lock to release it instantly anyway.

LockAcquire() increments the reference count before returning
LOCKACQUIRE_ALREADY_HELD, so it is an acquire. If this caller doesn't
LockRelease(), the lock will persist until end of transaction.

I changed that, updated comments, and fixed pgindent damage. I plan to push
the attached version.

> > The array can contain many of these new "invalid"
> > VXIDs, and an all-zeroes VXID terminates the array. Rather than change the

Correction: the terminator contains (InvalidBackendId,0).

> By the way maybe rename "check-prepared-txns" to "check-prepared-xacts" for consistency?

The "xact" term is about three times as frequent "txn". I favor "xact" in new
usage. It's not dominant enough to change old usage unless done pervasively.