Date: Sat, 7 Aug 2021 15:19:57 -0700
From: Noah Misch <noah@leadboat.com>
To: Andrey Borodin <x4mmm@yandex-team.ru>
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>
Subject: Re: CREATE INDEX CONCURRENTLY does not index prepared xact's data
Message-ID: <20210807221957.GA169359@rfd.leadboat.com>
References: <B8B5720D-B325-42B1-A5EF-ED01725503FA@yandex-team.ru>
 <ABABD793-1438-445B-879E-457499C2FC11@yandex-team.ru>
 <20210730022548.GA1940096@gust.leadboat.com>
 <A345A2B4-5041-4C95-9084-DC1BA83E3328@yandex-team.ru>
 <20210730184126.GA2018234@gust.leadboat.com>
 <D22DEA09-80DA-4350-839B-0FC0BD0668A4@yandex-team.ru>
 <CAH2-Wz=Et8WLvS6d7-BH_yT9b5=cV89fXBmY0T-KgsT7R0=R_A@mail.gmail.com>
 <85D32B53-FB25-4F2F-A253-0D7B26F72881@yandex-team.ru>
 <DBD361BB-4327-4143-A849-36A262B22CEE@yandex-team.ru>
 <341252AE-B633-471E-9FF4-B09FC5D036E2@yandex-team.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <341252AE-B633-471E-9FF4-B09FC5D036E2@yandex-team.ru>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: 
 <https://www.postgresql.org/message-id/20210807221957.GA169359%40rfd.leadboat.com>
Precedence: bulk

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").

> 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.

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.

> --- /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.

> +# 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.

> +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.

> +			\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.


[1] https://postgr.es/m/20160215171129.GA347322@tornado.leadboat.com