Adapt pg_pathman for PG 11. #166

arssher · 2018-07-09T13:06:52Z

I kinda lost interest to exorcise a couple of tests further in attempts to make
them pass on all supported versions and just added copies. These are

pathman_expressions now differs because planner converts ROW(Const, Const) to
just Const of record type.
Same with pathman_rebuild_updates.

I have removed inclusion of partition_filter.h in pg_compat.h in 9.5 as it
created circular dependency hell. I think it is not worthwhile to fight with it
since the only thing actually needed was error message, which is used in this
single place.

Small typo fix in partitioning_test.py: con2.begin instead of con1.begin.

Finally, run python tests with --failfast and --verbose options.

codecov-io · 2018-07-09T13:17:56Z

Codecov Report

Merging #166 into next will decrease coverage by 0.02%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             next     #166      +/-   ##
==========================================
- Coverage   90.07%   90.04%   -0.03%     
==========================================
  Files          40       40              
  Lines        6460     6462       +2     
==========================================
  Hits         5819     5819              
- Misses        641      643       +2

Impacted Files	Coverage Δ
src/pl_funcs.c	`95.44% <ø> (ø)`	⬆️
src/pl_range_funcs.c	`96.67% <ø> (ø)`	⬆️
src/include/relation_info.h	`89.65% <ø> (ø)`	⬆️
src/init.c	`86.62% <ø> (ø)`	⬆️
src/pg_pathman.c	`94.43% <ø> (ø)`	⬆️
src/include/compat/pg_compat.h	`0% <0%> (ø)`	⬆️
src/pathman_workers.c	`84.26% <100%> (-0.06%)`	⬇️
src/hooks.c	`92.23% <100%> (ø)`	⬆️
src/compat/pg_compat.c	`69.01% <100%> (ø)`	⬆️
src/partition_filter.c	`95.32% <100%> (ø)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adfaa72...44a3f67. Read the comment docs.

funbringer · 2018-07-09T13:26:10Z

src/compat/pg_compat.c

@@ -48,7 +48,12 @@ create_plain_partial_paths(PlannerInfo *root, RelOptInfo *rel)
 {
 	int			parallel_workers;

+#if PG_VERSION_NUM >= 110000
+	parallel_workers = compute_parallel_worker(rel, rel->pages, -1,


Could you extract this one as compute_parallel_worker macro, please?

funbringer · 2018-07-09T13:27:57Z

src/hooks.c

-	generate_gather_paths(root, rel);
+		/* consider gathering partial paths for the parent appendrel */
+#if PG_VERSION_NUM >= 110000
+		generate_gather_paths(root, rel, false);


And also this one.

funbringer · 2018-07-09T13:28:51Z

src/init.c

@@ -25,7 +25,9 @@
 #include "catalog/indexing.h"
 #include "catalog/pg_extension.h"
 #include "catalog/pg_inherits.h"
+#if PG_VERSION_NUM < 110000


Bikeshedding: usually, I place conditional includes at the bottom.

funbringer · 2018-07-09T13:32:29Z

src/hooks.c

@@ -514,13 +514,20 @@ pathman_rel_pathlist_hook(PlannerInfo *root,
 	rel->partial_pathlist = NIL;
 #endif

+/* Convert list to array for faster lookups */
+#if PG_VERSION_NUM >= 110000
+	setup_append_rel_array(root);


Are you sure this is ok? We might add new AppendRelInfos each time pathman_rel_pathlist_hook is called.

Yeah, this is not good. Fixed.

funbringer · 2018-07-09T13:35:05Z

src/relation_info.c

@@ -411,7 +414,7 @@ build_pathman_relation_info(Oid relid, Datum *values)

 	/* Create a new memory context to store expression tree etc */
 	prel_mcxt = AllocSetContextCreate(PathmanParentsCacheContext,
-									  __FUNCTION__,


Because since 9fa6f00b1308dd10da4eca2f31ccbfc7b35bb461 AllocSetContextCreate ensures that this arg is __builtin_constant_p, and __FUNCTION__ stores func name in global buffer.

Ok, thanks!

funbringer · 2018-07-09T13:44:14Z

src/include/partition_filter.h

@@ -29,7 +29,7 @@


 #define ERR_PART_ATTR_NULL				"partitioning expression's value should not be NULL"
-#define ERR_PART_ATTR_MULTIPLE_RESULTS	"partitioning expression should return single value"


This one looks odd.

See below changes in pg_compat.h

funbringer · 2018-07-09T13:45:15Z

src/nodes_common.c

-			RelOptInfo 	   *child_rel = ((Path *) lfirst(lc1))->parent;
+			RelOptInfo	   *child_rel = ((Path *) lfirst(lc1))->parent;
+#if PG_VERSION_NUM >= 110000
+			AppendRelInfo *appinfo = root->append_rel_array[child_rel->relid];


NOTE: this is closely related to setup_append_rel_array.
UPD: maybe we should reconsider each usage of root->append_rel_list.

Well, except for access in nodes_common.c all others are just iterations over the whole list, so not sure we need to change something here.

funbringer · 2018-07-09T13:47:57Z

src/pathman_workers.c

@@ -483,7 +483,7 @@ bgw_main_concurrent_part(Datum main_arg)

 		Oid		types[2]	= { OIDOID,				INT4OID };
 		Datum	vals[2]		= { part_slot->relid,	part_slot->batch_size };
-		bool	nulls[2]	= { false,				false };
+		char	nulls[2]	= { false,				false };


Nice catch, but this is also wrong. SPI requires nulls to be equal to 'n'. I guess we should get rid of nulls array and just pass NULL instead.

Indeed, fixed.

funbringer · 2018-07-09T13:48:31Z

tests/python/partitioning_test.py

@@ -651,7 +651,7 @@ def test_conc_part_drop_runtime_append(self):

                # Thread for connection #2 (it has to wait)
                def con2_thread():
-                    con1.begin()
+                    con2.begin()


funbringer · 2018-07-09T13:50:24Z

src/include/compat/pg_compat.h

-static inline void mult_result_handler() { elog(ERROR, ERR_PART_ATTR_MULTIPLE_RESULTS); }
+static inline void mult_result_handler()
+{
+	elog(ERROR, "partitioning expression should return single value");


I guess ERR_PART_ATTR_MULTIPLE_RESULTS is useless now, right?

Yes. As commit message says, inclusion of partition_filter.h created dependency hell here.

funbringer · 2018-07-10T12:19:10Z

expected/pathman_permissions.out

+BEGIN
+    SELECT create_range_partitions('permissions.user1_table', 'id', 1, 10, 2);
+EXCEPTION
+    WHEN insufficient_privilege THEN


Shouldn't we raise a notice in case of insufficient_privilege? Otherwise we won't know if this call failed.

Well, if it is indeed insufficient_privilige, then that's what we are after here, so silently eating it is fine. Actually, an attempt to hide the error is the only reason for this change -- one word changed in the error message from relation to table in 10 and 11 PG. If this is any other exception, it will fall through shouting along the way.

(We still can add some unifying NOTICE in exception handler or just a comment explaining this, if you like)

funbringer · 2018-07-10T12:27:22Z

src/relation_info.c

@@ -45,7 +48,7 @@
 #include "optimizer/planmain.h"
 #endif

-#if PG_VERSION_NUM >= 90600
+#if PG_VERSION_NUM >= 90600 && PG_VERSION_NUM < 11000


Probably should replace 11000 with 110000.

I kinda lost interest to exorcise a couple of tests further in attempts to make them pass on all supported versions and just added copies. These are * pathman_expressions now differs because planner converts ROW(Const, Const) to just Const of record type. * Same with pathman_rebuild_updates. I have removed inclusion of partition_filter.h in pg_compat.h in 9.5 as it created circular dependency hell. I think it is not worthwhile to fight with it since the only thing actually needed was error message, which is used in this single place. Small typo fix in partitioning_test.py: con2.begin instead of con1.begin. Finally, run python tests with --failfast and --verbose options.

@funbringer

Also a bunch of other stuff noted by @funbringer.

funbringer · 2018-07-12T14:41:02Z

Merged, thanks a lot!

arssher force-pushed the postgres_11 branch from fc73125 to bbf5a65 Compare July 9, 2018 13:17

funbringer reviewed Jul 9, 2018

View reviewed changes

funbringer reviewed Jul 10, 2018

View reviewed changes

arssher force-pushed the postgres_11 branch from a7e7a62 to 26e9f96 Compare July 11, 2018 16:20

arssher added 6 commits July 12, 2018 14:21

Fix handling of append_rel_array.

6a60886

Also a bunch of other stuff noted by @funbringer.

REL10 typo fix

0137d19

Raise notice 'insufficient privileges', fix pg_constraint_fn inclusion.

4e84256

Fix pathman_rebuild_deletes test for 11.

6418761

Fix pathman_rebuild_updates test.

50bfb92

arssher force-pushed the postgres_11 branch from 442d54c to 50bfb92 Compare July 12, 2018 11:21

Purge spurious whitespace

44a3f67

funbringer merged commit 44a3f67 into postgrespro:next Jul 12, 2018

		@@ -29,7 +29,7 @@


		#define ERR_PART_ATTR_NULL "partitioning expression's value should not be NULL"
		#define ERR_PART_ATTR_MULTIPLE_RESULTS "partitioning expression should return single value"

Adapt pg_pathman for PG 11. #166

Adapt pg_pathman for PG 11. #166

Uh oh!

Conversation

arssher commented Jul 9, 2018

Uh oh!

codecov-io commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

funbringer Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

funbringer commented Jul 12, 2018

Uh oh!

Uh oh!

codecov-io commented Jul 9, 2018 •

edited

Loading

funbringer Jul 9, 2018 •

edited

Loading