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: 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>
Date: 2021-10-19T03:02:12Z
Lists: pgsql-bugs 
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.

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.