Horton
Menu

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

Noah Misch <noah@leadboat.com>

View thread
From: Noah Misch <noah@leadboat.com>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Michael Paquier <michael@paquier.xyz>, Tom Lane <tgl@sss.pgh.pa.us>, pgsql-bugs@lists.postgresql.org
Date: 2021-01-24T02:27:43Z
Lists: pgsql-bugs

Attachments

On Fri, Jan 22, 2021 at 10:44:35AM +0500, Andrey Borodin wrote:
> > 17 янв. 2021 г., в 12:24, Noah Misch <noah@leadboat.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.

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Improve contrib/amcheck's tests for CREATE INDEX CONCURRENTLY.

  2. Fix minor memory leaks in pg_dump.

  3. Avoid race in RelationBuildDesc() affecting CREATE INDEX CONCURRENTLY.

  4. Fix CREATE INDEX CONCURRENTLY for the newest prepared transactions.

  5. Add -w back to the flags for pg_ctl (re)start in PostgresNode

  6. Fix CREATE INDEX CONCURRENTLY for simultaneous prepared transactions.