Revert "Revert "Avoids deadlocks for concurrent index creation on AO tables.""

This reverts commit c3726b09.

The original commit, which we're re-introducing with this Revert, was failing
because some isolation2 tests had this situation:

Session one successfully reindexes with a blocking call without committing,
continuing to hold the lock. Session two attempts to create an index on the
same table, which is locked by session one.

We introduced backgrounding of session two, which allows session one to return
and commit, releasing the lock. Our change enables the test runner to behave in
a realistic concurrent way, after the introduction of the
ShareRowExclusiveLock.
Co-authored-by: NJesse Zhang <sbjesse@gmail.com>

src/backend/catalog/aoblkdir.c

@@ -21,6 +21,7 @@
 #include "catalog/aocatalog.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
+#include "utils/faultinjector.h"
 
 void
 AlterTableCreateAoBlkdirTable(Oid relOid, bool is_part_child, bool is_part_parent)
@@ -32,6 +33,8 @@ AlterTableCreateAoBlkdirTable(Oid relOid, bool is_part_child, bool is_part_paren
 	int16		coloptions[3];
 	List	   *indexColNames;
 
+	SIMPLE_FAULT_INJECTOR(BeforeAcquireLockDuringCreateAoBlkdirTable);
+
 	/*
 	 * Grab an exclusive lock on the target table, which we will NOT release
 	 * until end of transaction.  (This is probably redundant in all present

src/backend/catalog/namespace.c

@@ -26,6 +26,7 @@
 #include "catalog/namespace.h"
 #include "catalog/oid_dispatch.h"
 #include "catalog/pg_authid.h"
+#include "catalog/pg_class.h"
 #include "catalog/pg_collation.h"
 #include "catalog/pg_conversion.h"
 #include "catalog/pg_conversion_fn.h"
@@ -657,6 +658,38 @@ RangeVarAdjustRelationPersistence(RangeVar *newRelation, Oid nspid)
 	}
 }
 
+static char
+get_relation_storage_method(RangeVar *heapRelation)
+{
+	Oid       relOid;
+	HeapTuple tuple;
+	char      relstorage;
+
+	relOid = RangeVarGetRelid(heapRelation, NoLock, true);
+
+	if (!OidIsValid(relOid))
+		elog(ERROR, "Oid %u is invalid", relOid);
+
+	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relOid));
+
+	if (!HeapTupleIsValid(tuple))
+		elog(ERROR, "cache lookup failed for relation %u", relOid);
+
+	relstorage = ((Form_pg_class) GETSTRUCT(tuple))->relstorage;
+	ReleaseSysCache(tuple);
+	return relstorage;
+}
+
+/*
+ * Determine from a RangeVar if the table it refers to is AppendOptimized
+ */
+bool
+RangeVarIsAppendOptimizedTable(RangeVar *relation)
+{
+	return relstorage_is_ao(
+		get_relation_storage_method(relation));
+}
+
 /*
  * RelnameGetRelid
  *		Try to resolve an unqualified relation name.

src/backend/commands/indexcmds.c

@@ -368,6 +368,7 @@ CheckIndexCompatible(Oid oldId,
 	return ret;
 }
 
+
 /*
  * DefineIndex
  *		Creates a new index.
@@ -487,7 +488,11 @@ DefineIndex(RangeVar *heapRelation,
 	 * index build; but for concurrent builds we allow INSERT/UPDATE/DELETE
 	 * (but not VACUUM).
 	 */
-	heap_lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock;
+	if (RangeVarIsAppendOptimizedTable(heapRelation))
+		heap_lockmode = ShareRowExclusiveLock;
+	else
+		heap_lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock;
+
 	rel = heap_openrv(heapRelation, heap_lockmode);
 
 	relationId = RelationGetRelid(rel);

src/include/catalog/namespace.h

@@ -52,6 +52,7 @@ typedef void (*RangeVarGetRelidCallback) (const RangeVar *relation, Oid relId,
 #define RangeVarGetRelid(relation, lockmode, missing_ok) \
 	RangeVarGetRelidExtended(relation, lockmode, missing_ok, false, NULL, NULL)
 
+
 extern Oid RangeVarGetRelidExtended(const RangeVar *relation,
 						 LOCKMODE lockmode, bool missing_ok, bool nowait,
 						 RangeVarGetRelidCallback callback,
@@ -61,6 +62,7 @@ extern Oid RangeVarGetAndCheckCreationNamespace(RangeVar *newRelation,
 									 LOCKMODE lockmode,
 									 Oid *existing_relation_id);
 extern void RangeVarAdjustRelationPersistence(RangeVar *newRelation, Oid nspid);
+extern bool RangeVarIsAppendOptimizedTable(RangeVar *relation);
 extern Oid	RelnameGetRelid(const char *relname);
 extern bool RelationIsVisible(Oid relid);
 

src/include/utils/faultinjector_lists.h

@@ -247,6 +247,8 @@ FI_IDENT(AutoVacWorkerBeforeDoAutovacuum, "auto_vac_worker_before_do_autovacuum"
 FI_IDENT(GetDnsCachedAddress, "get_dns_cached_address")
 /* inject fault before notify fts probe */
 FI_IDENT(BeforeFtsNotify, "before_fts_notify")
+/* inject fault before aquiring lock during AlterTableCreateAoBlkdirTable */
+FI_IDENT(BeforeAcquireLockDuringCreateAoBlkdirTable, "before_acquire_lock_during_create_ao_blkdir_table")
 /* inject fault during gang creation, before check for interrupts */
 FI_IDENT(CreateGangInProgress, "create_gang_in_progress")
 #endif

src/test/isolation2/expected/concurrent_index_creation_should_not_deadlock.out

+CREATE extension if NOT EXISTS gp_inject_fault;
+CREATE
+
+-- Create an append only table, popluated with data
+CREATE TABLE index_deadlocking_test_table (value int) WITH (appendonly=true);
+CREATE
+
+-- Setup a fault to ensure that the first session pauses while creating an index,
+-- ensuring a concurrent index creation.
+SELECT gp_inject_fault('before_acquire_lock_during_create_ao_blkdir_table', 'suspend', 1);
+gp_inject_fault
+---------------
+t              
+(1 row)
+
+-- Attempt to concurrently create an index
+1>: CREATE INDEX index_deadlocking_test_table_idx1 ON index_deadlocking_test_table (value);  <waiting ...>
+SELECT gp_wait_until_triggered_fault('before_acquire_lock_during_create_ao_blkdir_table', 1, 1);
+gp_wait_until_triggered_fault
+-----------------------------
+t                            
+(1 row)
+2>: CREATE INDEX index_deadlocking_test_table_idx2 ON index_deadlocking_test_table (value);  <waiting ...>
+SELECT gp_inject_fault('before_acquire_lock_during_create_ao_blkdir_table', 'reset', 1);
+gp_inject_fault
+---------------
+t              
+(1 row)
+
+-- Both index creation attempts should succeed
+1<:  <... completed>
+CREATE
+2<:  <... completed>
+CREATE

src/test/isolation2/expected/reindex/createidx_while_reindex_ao_gist.out

@@ -46,10 +46,11 @@ DELETE 254
 BEGIN
 1: REINDEX index idx_gist_reindex_ao;
 REINDEX
-2: create index idx_gist_reindex_ao2 on reindex_ao_gist USING Gist(target);
-CREATE
+2&: create index idx_gist_reindex_ao2 on reindex_ao_gist USING Gist(target);  <waiting ...>
 1: COMMIT;
 COMMIT
+2<:  <... completed>
+CREATE
 2: COMMIT;
 COMMIT
 3: SELECT COUNT(*) FROM reindex_ao_gist WHERE id = 1500;

src/test/isolation2/expected/reindex/createidx_while_reindex_ao_partition.out

@@ -45,10 +45,11 @@ DELETE 254
 BEGIN
 1: REINDEX index idx_gist_reindex_ao;
 REINDEX
-2: create index idx_gist_reindex_ao2 on reindex_ao_gist USING Gist(target);
-CREATE
+2&: create index idx_gist_reindex_ao2 on reindex_ao_gist USING Gist(target);  <waiting ...>
 1: COMMIT;
 COMMIT
+2<:  <... completed>
+CREATE
 2: COMMIT;
 COMMIT
 3: SELECT COUNT(*) FROM reindex_ao_gist WHERE id = 1500;

src/test/isolation2/expected/reindex/createidx_while_reindex_aoco_gist.out

@@ -27,10 +27,11 @@ DELETE 254
 BEGIN
 1: REINDEX index idx_gist_reindex_aoco;
 REINDEX
-2: create index idx_gist_reindex_aoco2 on reindex_aoco_gist USING Gist(target);
-CREATE
+2&: create index idx_gist_reindex_aoco2 on reindex_aoco_gist USING Gist(target);  <waiting ...>
 1: COMMIT;
 COMMIT
+2<:  <... completed>
+CREATE
 2: COMMIT;
 COMMIT
 3: SELECT COUNT(*) FROM reindex_aoco_gist WHERE id = 1500;

src/test/isolation2/expected/reindex/createidx_while_reindex_idx_ao_bitmap.out

@@ -26,10 +26,11 @@ BEGIN
 BEGIN
 1: REINDEX index idx_reindex_crtab_ao_bitmap;
 REINDEX
-2: create index idx_reindex_crtab_ao_bitmap2 on reindex_crtab_ao_bitmap USING bitmap(a);
-CREATE
+2&: create index idx_reindex_crtab_ao_bitmap2 on reindex_crtab_ao_bitmap USING bitmap(a);  <waiting ...>
 1: COMMIT;
 COMMIT
+2<:  <... completed>
+CREATE
 2: COMMIT;
 COMMIT
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_ao_bitmap' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);

src/test/isolation2/expected/reindex/createidx_while_reindex_idx_ao_btree.out

@@ -26,10 +26,11 @@ BEGIN
 BEGIN
 1: REINDEX index idx_reindex_crtab_ao_btree;
 REINDEX
-2: create index idx_reindex_crtab_ao_btree2 on reindex_crtab_ao_btree(a);
-CREATE
+2&: create index idx_reindex_crtab_ao_btree2 on reindex_crtab_ao_btree(a);  <waiting ...>
 1: COMMIT;
 COMMIT
+2<:  <... completed>
+CREATE
 2: COMMIT;
 COMMIT
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_ao_btree' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);

src/test/isolation2/expected/reindex/createidx_while_reindex_idx_aoco_bitmap.out

@@ -26,10 +26,11 @@ BEGIN
 BEGIN
 1: REINDEX index idx_reindex_crtab_aoco_bitmap;
 REINDEX
-2: create index idx_reindex_crtab_aoco_bitmap2 on reindex_crtab_aoco_bitmap USING bitmap(a);
-CREATE
+2&: create index idx_reindex_crtab_aoco_bitmap2 on reindex_crtab_aoco_bitmap USING bitmap(a);  <waiting ...>
 1: COMMIT;
 COMMIT
+2<:  <... completed>
+CREATE
 2: COMMIT;
 COMMIT
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_aoco_bitmap' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);

src/test/isolation2/expected/reindex/createidx_while_reindex_idx_aoco_btree.out

@@ -26,10 +26,11 @@ BEGIN
 BEGIN
 1: REINDEX index idx_reindex_crtab_aoco_btree;
 REINDEX
-2: create index idx_reindex_crtab_aoco_btree2 on reindex_crtab_aoco_btree(a);
-CREATE
+2&: create index idx_reindex_crtab_aoco_btree2 on reindex_crtab_aoco_btree(a);  <waiting ...>
 1: COMMIT;
 COMMIT
+2<:  <... completed>
+CREATE
 2: COMMIT;
 COMMIT
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_aoco_btree' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);

src/test/isolation2/isolation2_schedule

@@ -24,6 +24,7 @@ test: gp_collation
 
 test: setup
 # Tests on Append-Optimized tables (row-oriented).
+test: concurrent_index_creation_should_not_deadlock
 test: uao/alter_while_vacuum_row uao/alter_while_vacuum2_row
 test: uao/compaction_full_stats_row
 test: uao/compaction_utility_row

src/test/isolation2/sql/concurrent_index_creation_should_not_deadlock.sql

+CREATE extension if NOT EXISTS gp_inject_fault;
+
+-- Create an append only table, popluated with data
+CREATE TABLE index_deadlocking_test_table (value int) WITH (appendonly=true);
+
+-- Setup a fault to ensure that the first session pauses while creating an index,
+-- ensuring a concurrent index creation.
+SELECT gp_inject_fault('before_acquire_lock_during_create_ao_blkdir_table', 'suspend', 1);
+
+-- Attempt to concurrently create an index
+1>: CREATE INDEX index_deadlocking_test_table_idx1 ON index_deadlocking_test_table (value);
+SELECT gp_wait_until_triggered_fault('before_acquire_lock_during_create_ao_blkdir_table', 1, 1);
+2>: CREATE INDEX index_deadlocking_test_table_idx2 ON index_deadlocking_test_table (value);
+SELECT gp_inject_fault('before_acquire_lock_during_create_ao_blkdir_table', 'reset', 1);
+
+-- Both index creation attempts should succeed
+1<:
+2<:

src/test/isolation2/sql/reindex/createidx_while_reindex_ao_gist.sql

@@ -41,8 +41,9 @@ SELECT 1 AS table_oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE r
 DELETE FROM reindex_ao_gist  WHERE id < 128;
 1: BEGIN;
 1: REINDEX index idx_gist_reindex_ao;
-2: create index idx_gist_reindex_ao2 on reindex_ao_gist USING Gist(target);
+2&: create index idx_gist_reindex_ao2 on reindex_ao_gist USING Gist(target);
 1: COMMIT;
+2<:
 2: COMMIT;
 3: SELECT COUNT(*) FROM reindex_ao_gist WHERE id = 1500;
 3: insert into reindex_ao_gist (id, owner, description, property, poli, target) values(1500, 'gpadmin', 'Reindex Concurrency test', '((1500, 1500), (1560, 1580))', '( (111, 112), (114, 115), (110, 110) )', '( (96, 86), 96)' );

src/test/isolation2/sql/reindex/createidx_while_reindex_ao_partition.sql

@@ -40,8 +40,9 @@ SELECT 1 AS default_partition_oid_same_on_all_segs from gp_dist_random('pg_class
 DELETE FROM reindex_ao_gist  WHERE id < 128;
 1: BEGIN;
 1: REINDEX index idx_gist_reindex_ao;
-2: create index idx_gist_reindex_ao2 on reindex_ao_gist USING Gist(target);
+2&: create index idx_gist_reindex_ao2 on reindex_ao_gist USING Gist(target);
 1: COMMIT;
+2<:
 2: COMMIT;
 3: SELECT COUNT(*) FROM reindex_ao_gist WHERE id = 1500;
 3: insert into reindex_ao_gist (id, owner, description, property, poli, target) values(1500, 'gpadmin', 'Reindex Concurrency test', '((1500, 1500), (1560, 1580))', '( (111, 112), (114, 115), (110, 110) )', '( (96, 86), 96)' );

src/test/isolation2/sql/reindex/createidx_while_reindex_aoco_gist.sql

@@ -34,8 +34,9 @@ SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname
 DELETE FROM reindex_aoco_gist  WHERE id < 128;
 1: BEGIN;
 1: REINDEX index idx_gist_reindex_aoco;
-2: create index idx_gist_reindex_aoco2 on reindex_aoco_gist USING Gist(target);
+2&: create index idx_gist_reindex_aoco2 on reindex_aoco_gist USING Gist(target);
 1: COMMIT;
+2<:
 2: COMMIT;
 3: SELECT COUNT(*) FROM reindex_aoco_gist WHERE id = 1500;
 3: select count(*) from reindex_aoco_gist;

src/test/isolation2/sql/reindex/createidx_while_reindex_idx_ao_bitmap.sql

@@ -13,8 +13,9 @@ DELETE FROM reindex_crtab_ao_bitmap WHERE a < 128;
 1: BEGIN;
 2: BEGIN;
 1: REINDEX index idx_reindex_crtab_ao_bitmap;
-2: create index idx_reindex_crtab_ao_bitmap2 on reindex_crtab_ao_bitmap USING bitmap(a);
+2&: create index idx_reindex_crtab_ao_bitmap2 on reindex_crtab_ao_bitmap USING bitmap(a);
 1: COMMIT;
+2<:
 2: COMMIT;
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_ao_bitmap' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_ao_bitmap2' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);

src/test/isolation2/sql/reindex/createidx_while_reindex_idx_ao_btree.sql

@@ -13,8 +13,9 @@ DELETE FROM reindex_crtab_ao_btree WHERE a < 128;
 1: BEGIN;
 2: BEGIN;
 1: REINDEX index idx_reindex_crtab_ao_btree;
-2: create index idx_reindex_crtab_ao_btree2 on reindex_crtab_ao_btree(a);
+2&: create index idx_reindex_crtab_ao_btree2 on reindex_crtab_ao_btree(a);
 1: COMMIT;
+2<:
 2: COMMIT;
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_ao_btree' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_ao_btree2' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);

src/test/isolation2/sql/reindex/createidx_while_reindex_idx_aoco_bitmap.sql

@@ -13,8 +13,9 @@ DELETE FROM reindex_crtab_aoco_bitmap WHERE a < 128;
 1: BEGIN;
 2: BEGIN;
 1: REINDEX index idx_reindex_crtab_aoco_bitmap;
-2: create index idx_reindex_crtab_aoco_bitmap2 on reindex_crtab_aoco_bitmap USING bitmap(a);
+2&: create index idx_reindex_crtab_aoco_bitmap2 on reindex_crtab_aoco_bitmap USING bitmap(a);
 1: COMMIT;
+2<:
 2: COMMIT;
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_aoco_bitmap' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_aoco_bitmap2' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);

src/test/isolation2/sql/reindex/createidx_while_reindex_idx_aoco_btree.sql

@@ -13,8 +13,9 @@ DELETE FROM reindex_crtab_aoco_btree WHERE a < 128;
 1: BEGIN;
 2: BEGIN;
 1: REINDEX index idx_reindex_crtab_aoco_btree;
-2: create index idx_reindex_crtab_aoco_btree2 on reindex_crtab_aoco_btree(a);
+2&: create index idx_reindex_crtab_aoco_btree2 on reindex_crtab_aoco_btree(a);
 1: COMMIT;
+2<:
 2: COMMIT;
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_aoco_btree' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);
 3: SELECT 1 AS oid_same_on_all_segs from gp_dist_random('pg_class')   WHERE relname = 'idx_reindex_crtab_aoco_btree2' GROUP BY oid having count(*) = (SELECT count(*) FROM gp_segment_configuration WHERE role='p' AND content > -1);