refactoring & bugfixes in scan_result_parts_storage() & select_partition_for_insert(), more tests

Author: funbringer

hash.sql

@@ -75,15 +75,15 @@ BEGIN
 
 	IF lock_parent THEN
 		/* Acquire data modification lock (prevent further modifications) */
-		PERFORM @extschema@.prevent_relation_modification(parent_relid);
+		PERFORM @extschema@.prevent_data_modification(parent_relid);
 	ELSE
 		/* Acquire lock on parent */
-		PERFORM @extschema@.lock_partitioned_relation(parent_relid);
+		PERFORM @extschema@.prevent_part_modification(parent_relid);
 	END IF;
 
 	/* Acquire data modification lock (prevent further modifications) */
-	PERFORM @extschema@.prevent_relation_modification(old_partition);
-	PERFORM @extschema@.prevent_relation_modification(new_partition);
+	PERFORM @extschema@.prevent_data_modification(old_partition);
+	PERFORM @extschema@.prevent_data_modification(new_partition);
 
 	/* Ignore temporary tables */
 	SELECT relpersistence FROM pg_catalog.pg_class

init.sql

@@ -438,10 +438,10 @@ BEGIN
 
 	IF partition_data = true THEN
 		/* Acquire data modification lock */
-		PERFORM @extschema@.prevent_relation_modification(parent_relid);
+		PERFORM @extschema@.prevent_data_modification(parent_relid);
 	ELSE
 		/* Acquire lock on parent */
-		PERFORM @extschema@.lock_partitioned_relation(parent_relid);
+		PERFORM @extschema@.prevent_part_modification(parent_relid);
 	END IF;
 
 	/* Ignore temporary tables */
@@ -601,7 +601,7 @@ BEGIN
 	PERFORM @extschema@.validate_relname(parent_relid);
 
 	/* Acquire data modification lock */
-	PERFORM @extschema@.prevent_relation_modification(parent_relid);
+	PERFORM @extschema@.prevent_data_modification(parent_relid);
 
 	IF NOT EXISTS (SELECT FROM @extschema@.pathman_config
 				   WHERE partrel = parent_relid) THEN
@@ -916,17 +916,17 @@ LANGUAGE C;
  * Lock partitioned relation to restrict concurrent
  * modification of partitioning scheme.
  */
-CREATE OR REPLACE FUNCTION @extschema@.lock_partitioned_relation(
+CREATE OR REPLACE FUNCTION @extschema@.prevent_part_modification(
 	parent_relid	REGCLASS)
-RETURNS VOID AS 'pg_pathman', 'lock_partitioned_relation'
+RETURNS VOID AS 'pg_pathman', 'prevent_part_modification'
 LANGUAGE C STRICT;
 
 /*
  * Lock relation to restrict concurrent modification of data.
  */
-CREATE OR REPLACE FUNCTION @extschema@.prevent_relation_modification(
+CREATE OR REPLACE FUNCTION @extschema@.prevent_data_modification(
 	parent_relid	REGCLASS)
-RETURNS VOID AS 'pg_pathman', 'prevent_relation_modification'
+RETURNS VOID AS 'pg_pathman', 'prevent_data_modification'
 LANGUAGE C STRICT;
 
 

range.sql

@@ -443,10 +443,10 @@ BEGIN
 	PERFORM @extschema@.validate_relname(partition_relid);
 
 	/* Acquire lock on parent */
-	PERFORM @extschema@.lock_partitioned_relation(parent_relid);
+	PERFORM @extschema@.prevent_part_modification(parent_relid);
 
 	/* Acquire data modification lock (prevent further modifications) */
-	PERFORM @extschema@.prevent_relation_modification(partition_relid);
+	PERFORM @extschema@.prevent_data_modification(partition_relid);
 
 	part_expr_type = @extschema@.get_partition_key_type(parent_relid);
 	part_expr := @extschema@.get_partition_key(parent_relid);
@@ -536,7 +536,7 @@ BEGIN
 	PERFORM @extschema@.validate_relname(parent_relid);
 
 	/* Acquire lock on parent */
-	PERFORM @extschema@.lock_partitioned_relation(parent_relid);
+	PERFORM @extschema@.prevent_part_modification(parent_relid);
 
 	part_expr_type := @extschema@.get_partition_key_type(parent_relid);
 
@@ -640,7 +640,7 @@ BEGIN
 	PERFORM @extschema@.validate_relname(parent_relid);
 
 	/* Acquire lock on parent */
-	PERFORM @extschema@.lock_partitioned_relation(parent_relid);
+	PERFORM @extschema@.prevent_part_modification(parent_relid);
 
 	part_expr_type := @extschema@.get_partition_key_type(parent_relid);
 
@@ -744,7 +744,7 @@ BEGIN
 	PERFORM @extschema@.validate_relname(parent_relid);
 
 	/* Acquire lock on parent */
-	PERFORM @extschema@.lock_partitioned_relation(parent_relid);
+	PERFORM @extschema@.prevent_part_modification(parent_relid);
 
 	IF start_value >= end_value THEN
 		RAISE EXCEPTION 'failed to create partition: start_value is greater than end_value';
@@ -798,7 +798,7 @@ BEGIN
 	END IF;
 
 	/* Acquire lock on parent */
-	PERFORM @extschema@.lock_partitioned_relation(parent_relid);
+	PERFORM @extschema@.prevent_part_modification(parent_relid);
 
 	IF NOT delete_data THEN
 		EXECUTE format('INSERT INTO %s SELECT * FROM %s',
@@ -849,7 +849,7 @@ BEGIN
 	PERFORM @extschema@.validate_relname(partition_relid);
 
 	/* Acquire lock on parent */
-	PERFORM @extschema@.lock_partitioned_relation(parent_relid);
+	PERFORM @extschema@.prevent_part_modification(parent_relid);
 
 	/* Ignore temporary tables */
 	SELECT relpersistence FROM pg_catalog.pg_class
@@ -926,7 +926,7 @@ BEGIN
 	PERFORM @extschema@.validate_relname(partition_relid);
 
 	/* Acquire lock on parent */
-	PERFORM @extschema@.prevent_relation_modification(parent_relid);
+	PERFORM @extschema@.prevent_data_modification(parent_relid);
 
 	part_type := @extschema@.get_partition_type(parent_relid);
 

src/include/partition_creation.h

@@ -76,7 +76,9 @@ Node * build_raw_hash_check_tree(Node *raw_expression,
 								 Oid relid,
 								 Oid value_type);
 
-void drop_check_constraint(Oid relid);
+/* Add & drop pg_pathman's check constraint */
+void drop_pathman_check_constraint(Oid relid);
+void add_pathman_check_constraint(Oid relid, Constraint *constraint);
 
 
 /* Update triggers */

src/include/xact_handling.h

@@ -20,11 +20,7 @@
 /*
  * Transaction locks.
  */
-LockAcquireResult xact_lock_partitioned_rel(Oid relid, bool nowait);
-void xact_unlock_partitioned_rel(Oid relid);
-
-LockAcquireResult xact_lock_rel_exclusive(Oid relid, bool nowait);
-void xact_unlock_rel_exclusive(Oid relid);
+LockAcquireResult xact_lock_rel(Oid relid, LOCKMODE lockmode, bool nowait);
 
 /*
  * Utility checks.
@@ -35,7 +31,7 @@ bool xact_is_transaction_stmt(Node *stmt);
 bool xact_is_set_transaction_stmt(Node *stmt);
 bool xact_object_is_visible(TransactionId obj_xmin);
 
-void prevent_relation_modification_internal(Oid relid);
+void prevent_data_modification_internal(Oid relid);
 
 
 #endif /* XACT_HANDLING_H */

src/partition_creation.c

@@ -375,7 +375,7 @@ create_partitions_for_value_internal(Oid relid, Datum value, Oid value_type,
 				else if (nparts == 1)
 				{
 					/* Unlock the parent (we're not going to spawn) */
-					xact_unlock_partitioned_rel(relid);
+					UnlockRelationOid(relid, ShareUpdateExclusiveLock);
 
 					/* Simply return the suitable partition */
 					partid = parts[0];
@@ -1125,7 +1125,7 @@ copy_foreign_keys(Oid parent_relid, Oid partition_oid)
 
 /* Drop pg_pathman's check constraint by 'relid' */
 void
-drop_check_constraint(Oid relid)
+drop_pathman_check_constraint(Oid relid)
 {
 	char		   *constr_name;
 	AlterTableStmt *stmt;
@@ -1146,9 +1146,25 @@ drop_check_constraint(Oid relid)
 
 	stmt->cmds = list_make1(cmd);
 
-	AlterTable(relid, ShareUpdateExclusiveLock, stmt);
+	/* See function AlterTableGetLockLevel() */
+	AlterTable(relid, AccessExclusiveLock, stmt);
 }
 
+/* Add pg_pathman's check constraint using 'relid' */
+void
+add_pathman_check_constraint(Oid relid, Constraint *constraint)
+{
+	Relation part_rel = heap_open(relid, AccessExclusiveLock);
+
+	AddRelationNewConstraints(part_rel, NIL,
+							  list_make1(constraint),
+							  false, true, true);
+
+	heap_close(part_rel, NoLock);
+}
+
+
+
 /* Build RANGE check constraint expression tree */
 Node *
 build_raw_range_check_tree(Node *raw_expression,

src/partition_filter.c

@@ -253,6 +253,11 @@ scan_result_parts_storage(Oid partid, ResultPartsStorage *parts_storage)
 		LockRelationOid(partid, parts_storage->head_open_lock_mode);
 		if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(partid)))
 		{
+			/* Don't forget to drop invalid hash table entry */
+			hash_search(parts_storage->result_rels_table,
+						(const void *) &partid,
+						HASH_REMOVE, NULL);
+
 			UnlockRelationOid(partid, parts_storage->head_open_lock_mode);
 			return NULL;
 		}
@@ -419,34 +424,50 @@ select_partition_for_insert(Datum value, Oid value_type,
 {
 	MemoryContext			old_mcxt;
 	ResultRelInfoHolder	   *rri_holder;
+	Oid						parent_relid = PrelParentRelid(prel);
 	Oid						selected_partid = InvalidOid;
 	Oid					   *parts;
 	int						nparts;
 
-	/* Search for matching partitions */
-	parts = find_partitions_for_value(value, value_type, prel, &nparts);
-
-	if (nparts > 1)
-		elog(ERROR, ERR_PART_ATTR_MULTIPLE);
-	else if (nparts == 0)
+	do
 	{
-		 selected_partid = create_partitions_for_value(PrelParentRelid(prel),
-													   value, prel->ev_type);
+		/* Search for matching partitions */
+		parts = find_partitions_for_value(value, value_type, prel, &nparts);
 
-		 /* get_pathman_relation_info() will refresh this entry */
-		 invalidate_pathman_relation_info(PrelParentRelid(prel), NULL);
-	}
-	else selected_partid = parts[0];
+		if (nparts > 1)
+			elog(ERROR, ERR_PART_ATTR_MULTIPLE);
+		else if (nparts == 0)
+		{
+			 selected_partid = create_partitions_for_value(parent_relid,
+														   value, prel->ev_type);
 
-	/* Replace parent table with a suitable partition */
-	old_mcxt = MemoryContextSwitchTo(estate->es_query_cxt);
-	rri_holder = scan_result_parts_storage(selected_partid, parts_storage);
-	MemoryContextSwitchTo(old_mcxt);
+			 /* get_pathman_relation_info() will refresh this entry */
+			 invalidate_pathman_relation_info(parent_relid, NULL);
+		}
+		else selected_partid = parts[0];
 
-	/* Could not find suitable partition */
-	if (rri_holder == NULL)
-		elog(ERROR, ERR_PART_ATTR_NO_PART,
-			 datum_to_cstring(value, prel->ev_type));
+		/* Replace parent table with a suitable partition */
+		old_mcxt = MemoryContextSwitchTo(estate->es_query_cxt);
+		rri_holder = scan_result_parts_storage(selected_partid, parts_storage);
+		MemoryContextSwitchTo(old_mcxt);
+
+		/* This partition has been dropped, repeat with a new 'prel' */
+		if (rri_holder == NULL)
+		{
+			/* get_pathman_relation_info() will refresh this entry */
+			invalidate_pathman_relation_info(parent_relid, NULL);
+
+			/* Get a fresh PartRelationInfo */
+			prel = get_pathman_relation_info(parent_relid);
+
+			/* Paranoid check (all partitions have vanished) */
+			if (!prel)
+				elog(ERROR, "table \"%s\" is not partitioned",
+					 get_rel_name_or_relid(parent_relid));
+		}
+	}
+	/* Loop until we get some result */
+	while (rri_holder == NULL);
 
 	return rri_holder;
 }
@@ -629,7 +650,10 @@ partition_filter_exec(CustomScanState *node)
 		if (itemIsDone != ExprSingleResult)
 			elog(ERROR, ERR_PART_ATTR_MULTIPLE_RESULTS);
 
-		/* Search for a matching partition */
+		/*
+		 * Search for a matching partition.
+		 * WARNING: 'prel' might change after this call!
+		 */
 		rri_holder = select_partition_for_insert(value, prel->ev_type, prel,
 												 &state->result_parts, estate);
 

src/pl_funcs.c

@@ -64,8 +64,8 @@ PG_FUNCTION_INFO_V1( is_tuple_convertible );
 PG_FUNCTION_INFO_V1( add_to_pathman_config );
 PG_FUNCTION_INFO_V1( pathman_config_params_trigger_func );
 
-PG_FUNCTION_INFO_V1( lock_partitioned_relation );
-PG_FUNCTION_INFO_V1( prevent_relation_modification );
+PG_FUNCTION_INFO_V1( prevent_part_modification );
+PG_FUNCTION_INFO_V1( prevent_data_modification );
 
 PG_FUNCTION_INFO_V1( validate_part_callback_pl );
 PG_FUNCTION_INFO_V1( invoke_on_partition_created_callback );
@@ -774,8 +774,8 @@ add_to_pathman_config(PG_FUNCTION_ARGS)
 	else ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 						 errmsg("'parent_relid' should not be NULL")));
 
-	/* Protect relation from concurrent modification */
-	xact_lock_rel_exclusive(relid, true);
+	/* Protect data + definition from concurrent modification */
+	LockRelationOid(relid, AccessExclusiveLock);
 
 	/* Check that relation exists */
 	if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(relid)))
@@ -1000,12 +1000,12 @@ pathman_config_params_trigger_func(PG_FUNCTION_ARGS)
  * Acquire appropriate lock on a partitioned relation.
  */
 Datum
-lock_partitioned_relation(PG_FUNCTION_ARGS)
+prevent_part_modification(PG_FUNCTION_ARGS)
 {
 	Oid			relid = PG_GETARG_OID(0);
 
 	/* Lock partitioned relation till transaction's end */
-	xact_lock_partitioned_rel(relid, false);
+	LockRelationOid(relid, ShareUpdateExclusiveLock);
 
 	PG_RETURN_VOID();
 }
@@ -1014,9 +1014,9 @@ lock_partitioned_relation(PG_FUNCTION_ARGS)
  * Lock relation exclusively & check for current isolation level.
  */
 Datum
-prevent_relation_modification(PG_FUNCTION_ARGS)
+prevent_data_modification(PG_FUNCTION_ARGS)
 {
-	prevent_relation_modification_internal(PG_GETARG_OID(0));
+	prevent_data_modification_internal(PG_GETARG_OID(0));
 
 	PG_RETURN_VOID();
 }

src/pl_range_funcs.c

@@ -670,15 +670,15 @@ merge_range_partitions_internal(Oid parent, Oid *parts, uint32 nparts)
 	ranges = PrelGetRangesArray(prel);
 
 	/* Lock parent till transaction's end */
-	xact_lock_partitioned_rel(parent, false);
+	LockRelationOid(parent, ShareUpdateExclusiveLock);
 
 	/* Process partitions */
 	for (i = 0; i < nparts; i++)
 	{
 		int j;
 
-		/* Lock partition in ACCESS EXCLUSIVE mode */
-		prevent_relation_modification_internal(parts[0]);
+		/* Prevent modification of partitions */
+		LockRelationOid(parts[0], AccessExclusiveLock);
 
 		/* Look for the specified partition */
 		for (j = 0; j < PrelChildrenCount(prel); j++)
@@ -1072,10 +1072,9 @@ modify_range_constraint(Oid partition_relid,
 {
 	Node		   *expr;
 	Constraint	   *constraint;
-	Relation		partition_rel;
 
 	/* Drop old constraint */
-	drop_check_constraint(partition_relid);
+	drop_pathman_check_constraint(partition_relid);
 
 	/* Parse expression */
 	expr = parse_partitioning_expression(partition_relid, expression, NULL, NULL);
@@ -1087,12 +1086,8 @@ modify_range_constraint(Oid partition_relid,
 											  upper,
 											  expression_type);
 
-	/* Open the relation and add new check constraint */
-	partition_rel = heap_open(partition_relid, AccessExclusiveLock);
-	AddRelationNewConstraints(partition_rel, NIL,
-							  list_make1(constraint),
-							  false, true, true);
-	heap_close(partition_rel, NoLock);
+	/* Add new constraint */
+	add_pathman_check_constraint(partition_relid, constraint);
 }
 
 /*