Horton
Menu

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

Andrey Borodin <x4mmm@yandex-team.ru>

View thread
From: Andrey Borodin <x4mmm@yandex-team.ru>
To: Noah Misch <noah@leadboat.com>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>, Michael Paquier <michael@paquier.xyz>, Tom Lane <tgl@sss.pgh.pa.us>, Peter Geoghegan <pg@bowt.ie>
Date: 2021-08-08T11:31:07Z
Lists: pgsql-bugs

Attachments


> 8 авг. 2021 г., в 03:19, Noah Misch <noah@leadboat.com> написал(а):
> 
> 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").
I've toyed around with
$node->append_conf('postgresql.conf', 'debug_invalidate_system_caches_always = 3'); in test.
I had failures only at most with
Assert(in_progress_offset < 4);
See [0] for stack trace. But I do not think that it proves that deeper calls are impossible with other DB schemas.

>> Tomorrow I'll try to cleanup step 1 (tap tests).
> 
> For the final commits, I would like to have two commits.  The first will fix
> the non-2PC bug and add the test for that fix.  The second will fix the 2PC
> bug and add the additional test.  Feel free to structure your changes into
> more patch files, so long as it's straightforward to squash them into two
> commits that way.
Done so.
Step 1. Test for CIC with regular transactions.
Step 2. Fix
Step 3. Test for CIC with 2PC
Step 4. Part of the fix that I'm sure about
Step 5. Dubious part of fix...

I had to refactor subs pgbench and pgbench_background to be reused among tests. Though I did not refactor pgbench tests to use this functions: they have two different versions for sub pgbench.

> A few things I noticed earlier:
> 
>> --- a/contrib/amcheck/Makefile
>> +++ b/contrib/amcheck/Makefile
>> @@ -12,7 +12,7 @@ PGFILEDESC = "amcheck - function for verifying relation integrity"
>> 
>> REGRESS = check check_btree check_heap
>> 
>> -TAP_TESTS = 1
>> +TAP_TESTS = 2
> 
> TAP_TESTS is a Boolean flag, not a count.  Leave it set to 1.
Fixed.

>> --- /dev/null
>> +++ b/contrib/amcheck/t/002_cic_2pc.pl
> 
>> +$node->append_conf('postgresql.conf', 'lock_timeout = 5000');
> 
>> +my $main_timer = IPC::Run::timeout(5);
> 
>> +my $reindex_timer = IPC::Run::timeout(5);
> 
> Values this short cause spurious buildfarm failures.  Generally, use 180s for
> timeouts that do not expire in a successful run.
Fixed.

>> +# Run background pgbench with CIC. We cannot mix-in this script into single pgbench:
>> +# CIC will deadlock with itself occasionally.
> 
> Consider fixing that by taking an advisory lock before running CIC.  However,
> if use of separate pgbench executions works better, feel free to keep that.
I've tried this approach. Advisory lock owns vxid, thus deadlocking with CIC.

>> +pgbench(
>> +	'--no-vacuum --client=5 --time=1',
> 
> Consider using a transaction count instead of --time.  That way, slow machines
> still catch the bug, and fast machines waste less time.  For the "concurrent
> OID generation" test, I tuned the transaction count so the test failed about
> half the time[1] on a buggy build.
I've tuned tests for my laptop to observe probabilistic test to pass sometimes.

>> +			\set txid random(1, 1000000000)
>> +			BEGIN;
>> +			INSERT INTO tbl VALUES(0);
>> +			PREPARE TRANSACTION 'tx:txid';
> 
> Try "PREPARE TRANSACTION 'c:client_id'" (or c:client_id:txid) to eliminate the
> chance of collision.
Fixed.

How to you think, do we have a chance to fix things before next release on August 12th?

Thanks!

Best regards, Andrey Borodin.

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Improve contrib/amcheck's tests for CREATE INDEX CONCURRENTLY.

  2. Fix minor memory leaks in pg_dump.

  3. Avoid race in RelationBuildDesc() affecting CREATE INDEX CONCURRENTLY.

  4. Fix CREATE INDEX CONCURRENTLY for the newest prepared transactions.

  5. Add -w back to the flags for pg_ctl (re)start in PostgresNode

  6. Fix CREATE INDEX CONCURRENTLY for simultaneous prepared transactions.