RE: Avoiding deadlock errors in CREATE INDEX CONCURRENTLY

Hi Dhruv,

On Sun, June 30, 2019 at 7:30 AM, Goel, Dhruv wrote:
> > On Jun 10, 2019, at 1:20 PM, Goel, Dhruv <goeldhru(at)amazon(dot)com> wrote:
> >> On Jun 9, 2019, at 5:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Andres Freund <andres(at)anarazel(dot)de> writes:
> >>> On June 9, 2019 8:36:37 AM PDT, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>>> I think you are mistaken that doing transactional updates in
> >>>> pg_index is OK. If memory serves, we rely on xmin of the pg_index
> >>>> row for purposes such as detecting whether a concurrently-created
> >>>> index is safe to use yet.
> >
> > I took a deeper look regarding this use case but was unable to find more evidence. As part of this patch, we essentially
> make concurrently-created index safe to use only if transaction started after the xmin of Phase 3. Even today concurrent
> indexes can not be used for transactions before this xmin because of the wait (which I am trying to get rid of in this
> patch), is there any other denial of service you are talking about? Both the other states indislive, indisready can
> be transactional updates as far as I understand. Is there anything more I am missing here?
>
>
> Hi,
>
> I did some more concurrency testing here through some python scripts which compare the end state of the concurrently
> created indexes. I also back-ported this patch to PG 9.6 and ran some custom concurrency tests (Inserts, Deletes, and
> Create Index Concurrently) which seem to succeed. The intermediate states unfortunately are not easy to test in an
> automated manner, but to be fair concurrent indexes could never be used for older transactions. Do you have more
> inputs/ideas on this patch?

According to the commit 3c8404649 [1], transactional update in pg_index is not safe in non-MVCC catalog scans before PG9.4.
But it seems to me that we can use transactional update in pg_index after the commit 813fb03155 [2] which got rid of SnapshotNow.

If we apply this patch back to 9.3 or earlier, we might need to consider another way or take the Andres suggestion (which I don't understand the way fully though), but which version do you want/do we need to apply this patch?

Also, if we apply this patch in this way, there are several comments to be fixed which state the method of CREATE INDEX CONCURRENTLY.

ex.
[index.c]
/*
* validate_index - support code for concurrent index builds
...
* After completing validate_index(), we wait until all transactions that
* were alive at the time of the reference snapshot are gone; this is
* necessary to be sure there are none left with a transaction snapshot
* older than the reference (and hence possibly able to see tuples we did
* not index). Then we mark the index "indisvalid" and commit. Subsequent
* transactions will be able to use it for queries.
...
valiate_index()
{
}

[1] https://github.com/postgres/postgres/commit/3c84046490bed3c22e0873dc6ba492e02b8b9051#diff-b279fc6d56760ed80ce4178de1401d2c
[2] https://github.com/postgres/postgres/commit/813fb0315587d32e3b77af1051a0ef517d187763#diff-b279fc6d56760ed80ce4178de1401d2c

--
Yoshikazu Imai