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

On Sat, Sep 25, 2021 at 01:10:12PM -0700, Noah Misch wrote:
> On Sat, Sep 25, 2021 at 10:25:05PM +0500, Andrey Borodin wrote:
> > > 20 сент. 2021 г., в 09:41, Noah Misch <noah@leadboat.com> написал(а):

> I'll queue a task to review the rest of the patch.

I think the attached version is ready for commit.  Notable differences
vs. v14:

- Made TwoPhaseGetXidByVXid() stop leaking TwoPhaseStateLock when it found a
  second match.

- Instead of moving the ProcArrayClearTransaction() call within
  PrepareTransaction(), move the PostPrepare_Locks() call.  I didn't find any
  bug in the other way, but it's a good principle to maximize similarity with
  CommitTransaction().  PostPrepare_Locks() has no counterpart in
  CommitTransaction(), so that principle is indifferent to moving it.

- inval-build-race-v0.1.patch was incompatible with debug_discard_caches.  The
  being-built relation would always get invalidated, making
  RelationBuildDesc() an infinite loop.  I've fixed this by making
  RelationCacheInvalidate() invalidate in-progress rels only when called for
  sinval overflow, not when called for debug_discard_caches.  This adds some
  function arguments that only matter in assert builds.  That's not great, but
  InvalidateSystemCaches() is expensive anyway.  I considered instead adding
  functions HoldDebugInval() and ResumeDebugInval(), which RelationBuildDesc()
  would use to suppress debug_discard_caches during any iteration after the
  first.  That didn't seem better.

- Discard the in-progress array after an ERROR during RelationBuildDesc().

- Made the relcache.c changes repalloc the list of in-progress rels as needed.

- Changed the background_pgbench args from ($dbname, $stdin, \$stdout, $timer,
  $files, $opts) to ($opts, $files, \$stdout, $timer).  $dbname was unused.
  pgbench doesn't make interesting use of its stdin, so I dropped that arg
  until we have a use case.  $opts and $files seem akin to the $dbname arg of
  background_psql, so I moved them to the front.  I'm not sure that last
  change was an improvement.

- Made 001_pgbench_with_server.pl use PostgresNode::pgbench(), rather than
  duplicate code.  Factored out a subroutine of PostgresNode::pgbench() and
  PostgresNode::background_pgbench().

- Lots of comment and naming changes.

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.

> > > - v13 WaitPreparedXact() experiences starvation when a steady stream of
> > >  prepared transactions have the same VXID.  Since VXID reuse entails
> > >  reconnecting, starvation will be unnoticeable in systems that follow best
> > >  practices around connection lifespan.  The 2021-08-23 patch version didn't
> > >  have that hazard.
> > I think the probability of such a stream is abysmal. You not only need a stream of 2PC with the same vxid, but a stream of overlapping 2PC with the same vxid. And the most critical thing that can happen - CIC waiting for the stream to became one-2PC-at-a-time for a moment.
> 
> You're probably right about that.

I didn't know of the "stateP->nextLXID = nextLocalTransactionId;" in
CleanupInvalidationState(), which indeed makes this all but impossible.

On Sat, Aug 07, 2021 at 03:19:57PM -0700, Noah Misch wrote:
> On Sun, Aug 08, 2021 at 12:00:55AM +0500, Andrey Borodin wrote:
> > Changes:
> > 1. Added assert in step 2 (fix for missed invalidation message). I wonder how deep possibly could be RelationBuildDesc() inside RelationBuildDesc() inside RelationBuildDesc() ... ? If the depth is unlimited we, probably, need a better data structure.
> 
> I don't know either, hence that quick data structure to delay the question.
> debug_discard_caches=3 may help answer the question.  RelationBuildDesc()
> reads pg_constraint, which is !rd_isnailed.  Hence, I expect one can at least
> get RelationBuildDesc("pg_constraint") inside RelationBuildDesc("user_table").

debug_discard_caches=5 yields a depth of eight when opening a relation having
a CHECK constraint:

my_rel_having_check_constraint
pg_constraint_conrelid_contypid_conname_index
pg_index
pg_constraint
pg_constraint
pg_constraint
pg_constraint
pg_constraint

While debug_discard_caches doesn't permit higher values, I think one could
reach depths greater than eight by, for example, having a number of sessions
invalidate pg_constraint as often as possible.  Hence, I'm glad the code no
longer relies on a depth limit.