Re: Issues with ON CONFLICT UPDATE and REINDEX CONCURRENTLY

On 2024-Aug-24, Michail Nikolaev wrote:

Hello, I'm working on getting the 0002 fix committed, with the tests it
fixes.  I ran across this comment:

> @@ -813,7 +814,13 @@ infer_arbiter_indexes(PlannerInfo *root)
>  		idxRel = index_open(indexoid, rte->rellockmode);
>  		idxForm = idxRel->rd_index;
>  
> -		if (!idxForm->indisvalid)
> +		/*
> +		 * We need to consider both indisvalid and indisready indexes because
> +		 * them may become indisvalid before execution phase. It is required
> +		 * to keep set of indexes used as arbiter to be the same for all
> +		 * concurrent transactions.
> +		 */
> +		if (!idxForm->indisready)
>  			goto next;
>  
>  		/*

I think this comment is wrong, or at least confusing.  It says "we need
to consider both indisvalid and indisready indexes", which is somewhat
dubious: perhaps it wants to say "we need to consider indexes that have
indisvalid and indisready".  But that is also wrong: I mean, if we
wanted to consider indexes that are marked indisready=false, then we
wouldn't "next" here (which essentially ignores such indexes).  So,
really, I think what this wants to say is "we need to consider indexes
that are indisready regardless of whether or not they are indisvalid,
because they may become valid later".  

Also, I think this comment lacks an explanation of _why_ indexes with
indisvalid=false are required.  You wrote earlier[1] in the thread the
following:

> The reason is that these indexes are required for correct processing during
> the execution phase.
> If "ready" indexes are skipped as arbiters by one transaction, they may
> already have become "valid" for another concurrent transaction during its
> planning phase.
> As a result, both transactions could concurrently process the UPSERT
> command with different sets of arbiters (while using the same set of
> indexes for tuple insertion later).
> This can lead to unexpected "duplicate key value violates unique
> constraint" errors and deadlocks.

I think this text is also confusing or wrong.  I think you meant 'If
"not valid" indexes are skipped as arbiters, they may have become
"valid" for another concurrent transaction'.  The text in catalogs.sgml
for these columns is:

: indisvalid bool
:
: If true, the index is currently valid for queries. False means the
: index is possibly incomplete: it must still be modified by
: INSERT/UPDATE operations, but  it cannot safely be used for queries.
: If it is unique, the uniqueness property is not guaranteed true
: either.

: indisready bool
:
: If true, the index is currently ready for inserts. False means the
: index must be ignored by INSERT/UPDATE operations.

It makes sense that indisready indexes must be ignored, because
basically the catalog state is not ready yet.  So that part seems
correct.

The other critical point is that the uniqueness must hold, and an index
with indisvalid=false would not necessarily detect that because the tree
might not be built completely yet, so the heap tuple that would be a
duplicate of the one we're writing might not have been scanned yet.  But
that's not a problem, because we require that a valid index exists, even
if we don't "infer" that one -- which means the uniqueness clause is
being enforced by that other index.

Given all of that, I have rewritten the comment thusly:

    /*
     * Ignore indexes that aren't indisready, because we cannot trust their
     * catalog structure yet.  However, if any indexes are marked
     * indisready but not yet indisvalid, we still consider them, because
     * they might turn valid while we're running.  Doing it this way
     * allows a concurrent transaction with a slightly later catalog
     * snapshot infer the same set of indexes, which is critical to
     * prevent spurious 'duplicate key' errors.
     *
     * However, another critical aspect is that a unique index that isn't
     * yet marked indisvalid=true might not be complete yet, meaning it
     * wouldn't detect possible duplicate rows.  In order to prevent false
     * negatives, we require that we include in the set of inferred indexes
     * at least one index that is marked valid.
     */
    if (!idxForm->indisready)
        goto next;

Am I understanding this correctly?

Thanks,

[1] https://postgr.es/m/CANtu0ogv+6wqRzPK241jik4U95s1pW3MCZ3rX5ZqbFdUysz7Qw@mail.gmail.com

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Learn about compilers. Then everything looks like either a compiler or
a database, and now you have two problems but one of them is fun."
            https://twitter.com/thingskatedid/status/1456027786158776329