Re: Avoiding deadlock errors in CREATE INDEX CONCURRENTLY
On Fri, Nov 08, 2019 at 10:30:39AM +0900, Michael Paquier wrote: > Per the arguments of upthread, storing a 64-bit XID would require a > catalog change and you cannot backpatch that. I would suggest to keep > this patch focused on HEAD, and keep it as an improvement of the > existing features. Concurrent deadlock risks caused by CCI exist > since the feature came to life. Marked as returned with feedback per lack of activity and the patch was waiting on author for a bit more than two weeks. -- Michael signature.asc Description: PGP signature
Re: Avoiding deadlock errors in CREATE INDEX CONCURRENTLY
On Mon, Oct 28, 2019 at 05:17:52AM +, imai.yoshik...@fujitsu.com wrote: > 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. That's actually this part of the patch: - /* Assert that current xact hasn't done any transactional updates */ - Assert(GetTopTransactionIdIfAny() == InvalidTransactionId); And this thread (for commit 3c84046): https://www.postgresql.org/message-id/19082.1349481...@sss.pgh.pa.us And while looking at this patch, I have doubts that what you are doing is actually safe either. > 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? Per the arguments of upthread, storing a 64-bit XID would require a catalog change and you cannot backpatch that. I would suggest to keep this patch focused on HEAD, and keep it as an improvement of the existing features. Concurrent deadlock risks caused by CCI exist since the feature came to life. > Also, if we apply this patch in this way, there are several comments > to be fixed which state the method of CREATE INDEX CONCURRENTLY. Are we sure as well that all the cache lookup failures are addressed? The CF robot does not complain per its latest status, but are we sure to be out of the ground here? The indentation of your patch is wrong in some places by the way. -- Michael signature.asc Description: PGP signature
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 wrote:
> >> On Jun 9, 2019, at 5:33 PM, Tom Lane wrote:
> >> Andres Freund writes:
> >>> On June 9, 2019 8:36:37 AM PDT, Tom Lane 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
Re: Avoiding deadlock errors in CREATE INDEX CONCURRENTLY
On Tue, Jul 9, 2019 at 10:33 AM Goel, Dhruv wrote: > I have attached the revised patch. I ran check-world multiple times on my > machine and it seems to succeed now. Do you mind kicking-off the CI build > with the latest patch? Thanks. It's triggered automatically when you post patches to the thread and also once a day, though it took ~35 minutes to get around to noticing your new version due to other activity in other threads, and general lack of horsepower. I'm planning to fix that with more horses. It passed on both OSes. See here: http://cfbot.cputube.org/dhruv-goel.html -- Thomas Munro https://enterprisedb.com
Re: Avoiding deadlock errors in CREATE INDEX CONCURRENTLY
On Mon, Jul 8, 2019 at 9:51 AM Thomas Munro wrote: > I noticed that check-world passed several times with this patch > applied, but the most recent CI run failed in multiple-cic: > > +error in steps s2i s1i: ERROR: cache lookup failed for index 26303 > > https://travis-ci.org/postgresql-cfbot/postgresql/builds/555472214 And in another run, this time on Windows, create_index failed: https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.46455 -- Thomas Munro https://enterprisedb.com
Re: Avoiding deadlock errors in CREATE INDEX CONCURRENTLY
On Sun, Jun 30, 2019 at 7:30 PM Goel, Dhruv wrote: > > On Jun 10, 2019, at 1:20 PM, Goel, Dhruv wrote: > >> On Jun 9, 2019, at 5:33 PM, Tom Lane wrote: > >> Andres Freund writes: > >>> On June 9, 2019 8:36:37 AM PDT, Tom Lane 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? > > 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? I noticed that check-world passed several times with this patch applied, but the most recent CI run failed in multiple-cic: +error in steps s2i s1i: ERROR: cache lookup failed for index 26303 https://travis-ci.org/postgresql-cfbot/postgresql/builds/555472214 -- Thomas Munro https://enterprisedb.com
Re: Avoiding deadlock errors in CREATE INDEX CONCURRENTLY
Andres Freund writes: > On June 9, 2019 8:36:37 AM PDT, Tom Lane 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. > We could replace that with storing a 64 xid in a normal column nowadays. Perhaps, but that's a nontrivial change that'd be prerequisite to doing what's suggested in this thread. regards, tom lane
Re: Avoiding deadlock errors in CREATE INDEX CONCURRENTLY
Hi, On June 9, 2019 8:36:37 AM PDT, Tom Lane wrote: >"Goel, Dhruv" writes: >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. We could replace that with storing a 64 xid in a normal column nowadays. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: Avoiding deadlock errors in CREATE INDEX CONCURRENTLY
"Goel, Dhruv" writes: > Yes, you are correct. The test case here was that if a tuple is inserted > after the reference snapshot is taken in Phase 2 and before the index is > marked ready. If this tuple is deleted before the reference snapshot of Phase > 3, it will never make it to the index. I have fixed this problem by making > pg_index tuple updates transactional (I believe there is no reason why it has > to be in place now) so that the xmin of the pg_index tuple is same the xmin > of the snapshot in Phase 3. 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. So a transactional update would restart that clock and result in temporary denial of service. regards, tom lane
Re: Avoiding deadlock errors in CREATE INDEX CONCURRENTLY
Hello,
On Wed, May 15, 2019 at 1:45 PM Goel, Dhruv wrote:
>
>
> Proposed Solution:
>
> We remove the third wait state completely from the concurrent index build.
> When we mark the index as ready, we also mark “indcheckxmin” to true which
> essentially enforces Postgres to not use this index for older snapshots.
>
>
>
I think there is a problem in the proposed solution. When phase 3 is
reached, the index is valid. But it might not contain tuples deleted
just before the reference snapshot was taken. Hence, we wait for those
transactions that might have older snapshot. The TransactionXmin of these
transactions can be greater than the xmin of the pg_index entry for this
index.
Instead of waiting in the third phase, if we just set indcheckxmin as true,
the above transactions will be able to use the index which is wrong.
(because they won't find the recently deleted tuples from the index that
are still live according to their snapshots)
The respective code from get_relation_info:
if (index->indcheckxmin &&
!TransactionIdPrecedes(HeapTupleHeaderGetXmin(indexRelation->rd_indextuple->t_data),
TransactionXmin))
{ /* don't use this index */ }
Please let me know if I'm missing something.
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
