fix update trigger column list collecting function; refactoring

Author: zilder

src/partition_filter.c

@@ -66,7 +66,9 @@ int					pg_pathman_insert_into_fdw = PF_FDW_INSERT_POSTGRES;
 CustomScanMethods	partition_filter_plan_methods;
 CustomExecMethods	partition_filter_exec_methods;
 
-
+static ExprState *prepare_expr_state(Node *expr,
+									 Oid relid,
+									 EState *estate);
 static void prepare_rri_for_insert(EState *estate,
 								   ResultRelInfoHolder *rri_holder,
 								   const ResultPartsStorage *rps_storage,
@@ -455,8 +457,6 @@ select_partition_for_insert(ExprContext *econtext, ExprState *expr_state,
 	}
 	else selected_partid = parts[0];
 
-	/* Replace parent table with a suitable partition */
-	/* TODO: write a correct comment */
 	old_mcxt = MemoryContextSwitchTo(estate->es_query_cxt);
 	rri_holder = scan_result_parts_storage(selected_partid, parts_storage);
 
@@ -471,35 +471,13 @@ select_partition_for_insert(ExprContext *econtext, ExprState *expr_state,
 
 		/* Build an expression state if not yet */
 		if (!rri_holder->expr_state)
-		{
-			MemoryContext			tmp_mcxt;
-			Node				   *expr;
-			Index					varno = 1;
-			ListCell *lc;
-
-			/* Change varno in Vars according to range table */
-			expr = copyObject(subprel->expr);
-			foreach(lc, estate->es_range_table)
-			{
-				RangeTblEntry *entry = lfirst(lc);
-				if (entry->relid == selected_partid)
-				{
-					if (varno > 1)
-						ChangeVarNodes(expr, 1, varno, 0);
-					break;
-				}
-				varno += 1;
-			}
-
-			/* Prepare state for expression execution */
-			tmp_mcxt = MemoryContextSwitchTo(estate->es_query_cxt);
-			rri_holder->expr_state = ExecInitExpr((Expr *) expr, NULL);
-			MemoryContextSwitchTo(tmp_mcxt);
-		}
+			rri_holder->expr_state = prepare_expr_state(subprel->expr,
+														selected_partid,
+														estate);
 
 		Assert(rri_holder->expr_state != NULL);
 
-		/* Dive in */
+		/* Recursively search for subpartitions */
 		rri_holder = select_partition_for_insert(econtext, rri_holder->expr_state,
 												 subprel,
 												 parts_storage,
@@ -516,6 +494,38 @@ select_partition_for_insert(ExprContext *econtext, ExprState *expr_state,
 	return rri_holder;
 }
 
+static ExprState *
+prepare_expr_state(Node *expr,
+				   Oid relid,
+				   EState *estate)
+{
+		ExprState				   *expr_state;
+		MemoryContext				old_mcxt;
+		Index						varno = 1;
+		Node					   *expr_copy;
+		ListCell				   *lc;
+
+		/* Change varno in Vars according to range table */
+		expr_copy = copyObject(expr);
+		foreach(lc, estate->es_range_table)
+		{
+			RangeTblEntry *entry = lfirst(lc);
+			if (entry->relid == relid)
+			{
+				if (varno > 1)
+					ChangeVarNodes(expr_copy, 1, varno, 0);
+				break;
+			}
+			varno += 1;
+		}
+
+		/* Prepare state for expression execution */
+		old_mcxt = MemoryContextSwitchTo(estate->es_query_cxt);
+		expr_state = ExecInitExpr((Expr *) expr_copy, NULL);
+		MemoryContextSwitchTo(old_mcxt);
+
+		return expr_state;
+}
 
 /*
  * --------------------------------
@@ -596,40 +606,22 @@ partition_filter_create_scan_state(CustomScan *node)
 void
 partition_filter_begin(CustomScanState *node, EState *estate, int eflags)
 {
-	Index						varno = 1;
-	Node					   *expr;
-	MemoryContext				old_mcxt;
 	PartitionFilterState	   *state = (PartitionFilterState *) node;
-	const PartRelationInfo	   *prel;
-	ListCell				   *lc;
 
 	/* It's convenient to store PlanState in 'custom_ps' */
 	node->custom_ps = list_make1(ExecInitNode(state->subplan, estate, eflags));
 
 	if (state->expr_state == NULL)
 	{
+		const PartRelationInfo	   *prel;
+
 		/* Fetch PartRelationInfo for this partitioned relation */
 		prel = get_pathman_relation_info(state->partitioned_table);
 		Assert(prel != NULL);
 
-		/* Change varno in Vars according to range table */
-		expr = copyObject(prel->expr);
-		foreach(lc, estate->es_range_table)
-		{
-			RangeTblEntry *entry = lfirst(lc);
-			if (entry->relid == state->partitioned_table)
-			{
-				if (varno > 1)
-					ChangeVarNodes(expr, 1, varno, 0);
-				break;
-			}
-			varno += 1;
-		}
-
-		/* Prepare state for expression execution */
-		old_mcxt = MemoryContextSwitchTo(estate->es_query_cxt);
-		state->expr_state = ExecInitExpr((Expr *) expr, NULL);
-		MemoryContextSwitchTo(old_mcxt);
+		state->expr_state = prepare_expr_state(prel->expr,
+											   state->partitioned_table,
+											   estate);
 	}
 
 	/* Init ResultRelInfo cache */

src/pl_funcs.c

@@ -1118,8 +1118,6 @@ pathman_update_trigger_func(PG_FUNCTION_ARGS)
 	/* Find (or create) target partition */
 	target_relid = find_target_partition(source_rel, new_tuple);
 
-	/* TODO: check for InvalidOid */
-
 	/* Convert tuple if target partition has changed */
 	if (target_relid != source_relid)
 	{
@@ -1129,9 +1127,7 @@ pathman_update_trigger_func(PG_FUNCTION_ARGS)
 		/* Lock partition and check if it exists */
 		LockRelationOid(target_relid, lockmode);
 		if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(target_relid)))
-			/* TODO: !!! */
-			elog(ERROR, ERR_PART_ATTR_NO_PART, "()");
-			// elog(ERROR, ERR_PART_ATTR_NO_PART, datum_to_cstring(value, value_type));
+			elog(ERROR, "no suitable target partition");
 
 		/* Open partition */
 		target_rel = heap_open(target_relid, lockmode);
@@ -1152,7 +1148,7 @@ pathman_update_trigger_func(PG_FUNCTION_ARGS)
 }
 
 /*
- * Find partition satisfying values of the tuple
+ * Find partition satisfying values of the tuple or return InvalidOid
  */
 static Oid
 find_target_partition(Relation source_rel, HeapTuple tuple)
@@ -1247,20 +1243,24 @@ find_deepest_partition(Oid parent, Relation source_rel, HeapTuple tuple)
 		elog(ERROR, ERR_PART_ATTR_MULTIPLE);
 	else if (nparts == 0)
 	{
-		 target_relid = create_partitions_for_value(PrelParentRelid(prel),
+		/* No partition found, create a new one */
+		target_relid = create_partitions_for_value(PrelParentRelid(prel),
 													value, value_type);
 
-		 /* get_pathman_relation_info() will refresh this entry */
-		 invalidate_pathman_relation_info(PrelParentRelid(prel), NULL);
+		/* Get_pathman_relation_info() will refresh this entry */
+		invalidate_pathman_relation_info(PrelParentRelid(prel), NULL);
 	}
 	else
+	{
+		/* Found partition */
 		target_relid = parts[0];
-	pfree(parts);
 
-	/* Try to go deeper recursively and see if there is subpartition */
-	subpartition = find_deepest_partition(target_relid, source_rel, tuple);
-	if (OidIsValid(subpartition))
-		return subpartition;
+		/* Try to go deeper recursively and see if there is subpartition */
+		subpartition = find_deepest_partition(target_relid, source_rel, tuple);
+		if (OidIsValid(subpartition))
+			return subpartition;
+	}
+	pfree(parts);
 
 	return target_relid;
 }
@@ -1499,7 +1499,6 @@ create_update_triggers(PG_FUNCTION_ARGS)
 	trigname = build_update_trigger_name_internal(parent);
 
 	/* Create trigger for parent */
-	// columns = PrelExpressionColumnNames(prel);
 	collect_update_trigger_columns(parent, &columns);
 	create_single_update_trigger_internal(parent, trigname, columns);
 
@@ -1525,7 +1524,7 @@ collect_update_trigger_columns(Oid relid, List **columns)
 		return;
 
 	/* Collect columns from current level */
-	*columns = list_concat(*columns, PrelExpressionColumnNames(prel));
+	*columns = list_union(*columns, PrelExpressionColumnNames(prel));
 
 	/* Collect columns from parent */
 	parent = get_parent_of_partition(relid, &parent_search);