Date: Mon, 16 Nov 2020 21:08:32 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>,
	Dmitry Dolgov <9erthalion6@gmail.com>,
	James Coleman <jtc331@gmail.com>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: remove spurious CREATE INDEX CONCURRENTLY wait
Message-ID: <20201117000832.GA19824@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2087667.1604971933@sss.pgh.pa.us>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2020-Nov-09, Tom Lane wrote:

> Michael Paquier <michael@paquier.xyz> writes:
> >> +		LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
> >> +		MyProc->vacuumFlags |= PROC_IN_SAFE_IC;
> >> +		ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
> >> +		LWLockRelease(ProcArrayLock);
> 
> > I can't help noticing that you are repeating the same code pattern
> > eight times.  I think that this should be in its own routine, and that
> > we had better document that this should be called just after starting
> > a transaction, with an assertion enforcing that.
> 
> Do we really need exclusive lock on the ProcArray to make this flag
> change?  That seems pretty bad from a concurrency standpoint.

BTW I now know that the reason for taking ProcArrayLock is not the
vacuumFlags itself, but rather MyProc->pgxactoff, which can move.

On the other hand, if we stopped mirroring the flags in ProcGlobal, it
would mean we would have to access all procs' PGPROC entries in
GetSnapshotData, which is undesirable for performance reason (per commit
5788e258bb26).