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(at)leadboat(dot)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.