Date: Mon, 18 Oct 2021 20:02:12 -0700
From: Noah Misch <noah@leadboat.com>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>,
	Michael Paquier <michael@paquier.xyz>, Tom Lane <tgl@sss.pgh.pa.us>,
	Peter Geoghegan <pg@bowt.ie>, Andres Freund <andres@anarazel.de>
Subject: Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data
Message-ID: <20211019030212.GB3403374@rfd.leadboat.com>
References: <20210815134547.3okuz6ktvu4qf7ig@alap3.anarazel.de>
 <15936F8A-0436-40C8-9D1A-4A4623681554@yandex-team.ru>
 <20210829180913.ghojzzsgkqgm7vki@alap3.anarazel.de>
 <364BAA98-1A3F-4C39-B0D9-ED8C40EA902D@yandex-team.ru>
 <20210908064515.GA2496820@rfd.leadboat.com>
 <20210920044143.GA3414844@rfd.leadboat.com>
 <593C6C66-F19E-412A-9896-F1F5B272424A@yandex-team.ru>
 <20210925201012.GC4134968@rfd.leadboat.com>
 <20211017151205.GA2384251@rfd.leadboat.com>
 <9BA43784-E4A7-4638-900C-FC9868C388FA@yandex-team.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9BA43784-E4A7-4638-900C-FC9868C388FA@yandex-team.ru>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: 
 <https://www.postgresql.org/message-id/20211019030212.GB3403374%40rfd.leadboat.com>
Precedence: bulk

On Mon, Oct 18, 2021 at 06:23:05PM +0500, Andrey Borodin wrote:
> > 17 окт. 2021 г., в 20:12, Noah Misch <noah@leadboat.com> написал(а):
> > I think the attached version is ready for commit.  Notable differences
> > vs. v14:
> Few notes:
> 
> 1. Maybe an Assert(in_progress_list_maxlen) when in_progress_list_maxlen is used?

RelationCacheInitialize() initializes both in_progress_list_maxlen and the
RelationIdCache hash table, and any ERROR there is promoted to FATAL.  Without
the hash table, nothing good can happen in relcache.  Hence, I think such an
assert is excessive.

> 2. 
> -#define VirtualTransactionIdIsPreparedXact(vxid) \
> +#define VirtualTransactionIdIsRecoveredPreparedXact(vxid) \
> 
> This is a very neat transition. Yes, the function argument will always be a xid only for recovered transactions. Maybe add a comment here that this function is expected to be used only for results of GetLockConflicts()?

One can use it on any VirtualTransactionId, though some sources only return
values for which this returns false.  It can return true for the result of
GET_VXID_FROM_PGPROC().  I think it can return true for the result of
GetCurrentVirtualXIDs(limitXmin = InvalidTransactionId), but core code doesn't
make such a call.

> > One thing not done here is to change the tests to use CREATE INDEX
> > CONCURRENTLY instead of REINDEX CONCURRENTLY, so they're back-patchable to v11
> > and earlier.  I may do that before pushing, or I may just omit the tests from
> > older branches.
> 
> The tests refactors PostgresNode.pm and some tests. Back-patching this would be quite invasive.

That's fine with me.  Back-patching a fix without its tests is riskier than
back-patching test infrastructure changes.

> But swapping every "REINDEX INDEX CONCURRENTLY idx;" with
>             DROP INDEX CONCURRENTLY idx;
>             CREATE INDEX CONCURRENTLY idx on tbl(i);
> works.
> 
> > <inval-build-race-v1.patch><prepared-transactions-cic-series202107-v15nm.patch>
> I've checked that this patches work for some time on my machines. I do not observe failures.

Good.  Thanks for checking.