Skip to content

Commit 72d88ab

Browse files
funbringerfunbringer
committed
refactoring & bugfixes, fix memory consumption and error handling in cook_partitioning_expression()
1 parent 0e650dd commit 72d88ab

File tree

10 files changed

+208
-116
lines changed

10 files changed

+208
-116
lines changed

‎expected/pathman_expressions.out

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,18 @@
22
SET search_path = 'public';
33
CREATE EXTENSION pg_pathman;
44
CREATE SCHEMA test_exprs;
5-
/* hash */
5+
/* We use this rel to check 'pathman_hooks_enabled' */
6+
CREATE TABLE test_exprs.canary(val INT4 NOT NULL);
7+
CREATE TABLE test_exprs.canary_copy (LIKE test_exprs.canary);
8+
SELECT create_hash_partitions('test_exprs.canary', 'val', 5);
9+
create_hash_partitions
10+
------------------------
11+
5
12+
(1 row)
13+
14+
/*
15+
* Test HASH
16+
*/
617
CREATE TABLE test_exprs.hash_rel (
718
id SERIAL PRIMARY KEY,
819
value INTEGER,
@@ -18,9 +29,20 @@ SELECT COUNT(*) FROM test_exprs.hash_rel;
1829

1930
SELECT create_hash_partitions('test_exprs.hash_rel', 'random()', 4);
2031
ERROR: functions in partitioning expression must be marked IMMUTABLE
32+
/* Check that 'pathman_hooks_enabled' is true (1 partition in plan) */
33+
EXPLAIN (COSTS OFF) INSERT INTO test_exprs.canary_copy
34+
SELECT * FROM test_exprs.canary WHERE val = 1;
35+
QUERY PLAN
36+
----------------------------------
37+
Insert on canary_copy
38+
-> Append
39+
-> Seq Scan on canary_0
40+
Filter: (val = 1)
41+
(4 rows)
42+
2143
\set VERBOSITY default
2244
SELECT create_hash_partitions('test_exprs.hash_rel', 'value * value2))', 4);
23-
ERROR: failed to parse partitioning expression
45+
ERROR: failed to parse partitioning expression (value * value2)))
2446
DETAIL: syntax error at or near ")"
2547
QUERY: SELECT public.validate_expression(parent_relid, expression)
2648
CONTEXT: PL/pgSQL function prepare_for_partitioning(regclass,text,boolean) line 9 at PERFORM
@@ -29,7 +51,7 @@ SQL statement "SELECT public.prepare_for_partitioning(parent_relid,
2951
partition_data)"
3052
PL/pgSQL function create_hash_partitions(regclass,text,integer,boolean,text[],text[]) line 4 at PERFORM
3153
SELECT create_hash_partitions('test_exprs.hash_rel', 'value * value3', 4);
32-
ERROR: failed to analyze partitioning expression
54+
ERROR: failed to analyze partitioning expression (value * value3)
3355
DETAIL: column "value3" does not exist
3456
HINT: Perhaps you meant to reference the column "hash_rel.value" or the column "hash_rel.value2".
3557
QUERY: SELECT public.validate_expression(parent_relid, expression)
@@ -38,6 +60,17 @@ SQL statement "SELECT public.prepare_for_partitioning(parent_relid,
3860
expression,
3961
partition_data)"
4062
PL/pgSQL function create_hash_partitions(regclass,text,integer,boolean,text[],text[]) line 4 at PERFORM
63+
/* Check that 'pathman_hooks_enabled' is true (1 partition in plan) */
64+
EXPLAIN (COSTS OFF) INSERT INTO test_exprs.canary_copy
65+
SELECT * FROM test_exprs.canary WHERE val = 1;
66+
QUERY PLAN
67+
----------------------------------
68+
Insert on canary_copy
69+
-> Append
70+
-> Seq Scan on canary_0
71+
Filter: (val = 1)
72+
(4 rows)
73+
4174
\set VERBOSITY terse
4275
SELECT create_hash_partitions('test_exprs.hash_rel', 'value * value2', 4);
4376
create_hash_partitions
@@ -93,7 +126,9 @@ EXPLAIN (COSTS OFF) SELECT * FROM test_exprs.hash_rel WHERE (value * value2) = 5
93126
Filter: ((value * value2) = 5)
94127
(3 rows)
95128

96-
/* range */
129+
/*
130+
* Test RANGE
131+
*/
97132
CREATE TABLE test_exprs.range_rel (id SERIAL PRIMARY KEY, dt TIMESTAMP, txt TEXT);
98133
INSERT INTO test_exprs.range_rel (dt, txt)
99134
SELECT g, md5(g::TEXT) FROM generate_series('2015-01-01', '2020-04-30', '1 month'::interval) as g;
@@ -174,5 +209,5 @@ SELECT COUNT(*) FROM test_exprs.range_rel_2;
174209
(1 row)
175210

176211
DROP SCHEMA test_exprs CASCADE;
177-
NOTICE: drop cascades to 17 other objects
212+
NOTICE: drop cascades to 24 other objects
178213
DROP EXTENSION pg_pathman;

‎sql/pathman_expressions.sql

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,17 @@ CREATE EXTENSION pg_pathman;
55
CREATE SCHEMA test_exprs;
66

77

8-
/* hash */
8+
/* We use this rel to check 'pathman_hooks_enabled' */
9+
CREATE TABLE test_exprs.canary(val INT4 NOT NULL);
10+
CREATE TABLE test_exprs.canary_copy (LIKE test_exprs.canary);
11+
SELECT create_hash_partitions('test_exprs.canary', 'val', 5);
12+
13+
14+
15+
/*
16+
* Test HASH
17+
*/
18+
919
CREATE TABLE test_exprs.hash_rel (
1020
id SERIAL PRIMARY KEY,
1121
value INTEGER,
@@ -16,11 +26,24 @@ INSERT INTO test_exprs.hash_rel (value, value2)
1626

1727
SELECT COUNT(*) FROM test_exprs.hash_rel;
1828
SELECT create_hash_partitions('test_exprs.hash_rel', 'random()', 4);
29+
30+
/* Check that 'pathman_hooks_enabled' is true (1 partition in plan) */
31+
EXPLAIN (COSTS OFF) INSERT INTO test_exprs.canary_copy
32+
SELECT * FROM test_exprs.canary WHERE val = 1;
33+
34+
1935
\set VERBOSITY default
36+
2037
SELECT create_hash_partitions('test_exprs.hash_rel', 'value * value2))', 4);
2138
SELECT create_hash_partitions('test_exprs.hash_rel', 'value * value3', 4);
2239

40+
/* Check that 'pathman_hooks_enabled' is true (1 partition in plan) */
41+
EXPLAIN (COSTS OFF) INSERT INTO test_exprs.canary_copy
42+
SELECT * FROM test_exprs.canary WHERE val = 1;
43+
2344
\set VERBOSITY terse
45+
46+
2447
SELECT create_hash_partitions('test_exprs.hash_rel', 'value * value2', 4);
2548
SELECT COUNT(*) FROM ONLY test_exprs.hash_rel;
2649
SELECT COUNT(*) FROM test_exprs.hash_rel;
@@ -33,7 +56,12 @@ SELECT COUNT(*) FROM test_exprs.hash_rel;
3356
EXPLAIN (COSTS OFF) SELECT * FROM test_exprs.hash_rel WHERE value = 5;
3457
EXPLAIN (COSTS OFF) SELECT * FROM test_exprs.hash_rel WHERE (value * value2) = 5;
3558

36-
/* range */
59+
60+
61+
/*
62+
* Test RANGE
63+
*/
64+
3765
CREATE TABLE test_exprs.range_rel (id SERIAL PRIMARY KEY, dt TIMESTAMP, txt TEXT);
3866

3967
INSERT INTO test_exprs.range_rel (dt, txt)

‎src/hooks.c

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -506,23 +506,23 @@ pathman_rel_pathlist_hook(PlannerInfo *root,
506506
* Intercept 'pg_pathman.enable' GUC assignments.
507507
*/
508508
void
509-
pg_pathman_enable_assign_hook(bool newval, void *extra)
509+
pathman_enable_assign_hook(bool newval, void *extra)
510510
{
511511
elog(DEBUG2, "pg_pathman_enable_assign_hook() [newval = %s] triggered",
512512
newval ? "true" : "false");
513513

514514
/* Return quickly if nothing has changed */
515-
if (newval == (pg_pathman_init_state.pg_pathman_enable &&
516-
pg_pathman_init_state.auto_partition &&
517-
pg_pathman_init_state.override_copy &&
515+
if (newval == (pathman_init_state.pg_pathman_enable &&
516+
pathman_init_state.auto_partition &&
517+
pathman_init_state.override_copy &&
518518
pg_pathman_enable_runtimeappend &&
519519
pg_pathman_enable_runtime_merge_append &&
520520
pg_pathman_enable_partition_filter &&
521521
pg_pathman_enable_bounds_cache))
522522
return;
523523

524-
pg_pathman_init_state.auto_partition = newval;
525-
pg_pathman_init_state.override_copy = newval;
524+
pathman_init_state.auto_partition = newval;
525+
pathman_init_state.override_copy = newval;
526526
pg_pathman_enable_runtimeappend = newval;
527527
pg_pathman_enable_runtime_merge_append = newval;
528528
pg_pathman_enable_partition_filter = newval;
@@ -552,11 +552,13 @@ pathman_planner_hook(Query *parse, int cursorOptions, ParamListInfo boundParams)
552552

553553
PlannedStmt *result;
554554
uint32 query_id = parse->queryId;
555-
bool pathman_ready = IsPathmanReady(); /* in case it changes */
555+
556+
/* Save the result in case it changes */
557+
bool pathman_ready = pathman_hooks_enabled && IsPathmanReady();
556558

557559
PG_TRY();
558560
{
559-
if (pathman_ready && pathman_hooks_enabled)
561+
if (pathman_ready)
560562
{
561563
/* Increment relation tags refcount */
562564
incr_refcount_relation_tags();
@@ -571,7 +573,7 @@ pathman_planner_hook(Query *parse, int cursorOptions, ParamListInfo boundParams)
571573
else
572574
result = standard_planner(parse, cursorOptions, boundParams);
573575

574-
if (pathman_ready && pathman_hooks_enabled)
576+
if (pathman_ready)
575577
{
576578
/* Give rowmark-related attributes correct names */
577579
ExecuteForPlanTree(result, postprocess_lock_rows);
@@ -711,13 +713,13 @@ pathman_relcache_hook(Datum arg, Oid relid)
711713
PartParentSearch search;
712714
Oid partitioned_table;
713715

714-
if (!IsPathmanReady())
715-
return;
716-
717716
/* Hooks can be disabled */
718717
if (!pathman_hooks_enabled)
719718
return;
720719

720+
if (!IsPathmanReady())
721+
return;
722+
721723
/* We shouldn't even consider special OIDs */
722724
if (relid < FirstNormalObjectId)
723725
return;

‎src/include/hooks.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ void pathman_rel_pathlist_hook(PlannerInfo *root,
4040
Index rti,
4141
RangeTblEntry *rte);
4242

43-
void pg_pathman_enable_assign_hook(char newval, void *extra);
43+
void pathman_enable_assign_hook(char newval, void *extra);
4444

4545
PlannedStmt * pathman_planner_hook(Query *parse,
4646
int cursorOptions,

‎src/include/init.h

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,10 @@ extern HTAB *parent_cache;
6363
extern HTAB *bound_cache;
6464

6565
/* pg_pathman's initialization state */
66-
extern PathmanInitState pg_pathman_init_state;
66+
extern PathmanInitState pathman_init_state;
67+
68+
/* pg_pathman's hooks state */
69+
extern bool pathman_hooks_enabled;
6770

6871

6972
/* Transform pg_pathman's memory context into simple name */
@@ -94,12 +97,12 @@ simpify_mcxt_name(MemoryContext mcxt)
9497
/*
9598
* Check if pg_pathman is initialized.
9699
*/
97-
#define IsPathmanInitialized() ( !pg_pathman_init_state.initialization_needed )
100+
#define IsPathmanInitialized() ( !pathman_init_state.initialization_needed )
98101

99102
/*
100103
* Check if pg_pathman is enabled.
101104
*/
102-
#define IsPathmanEnabled() ( pg_pathman_init_state.pg_pathman_enable )
105+
#define IsPathmanEnabled() ( pathman_init_state.pg_pathman_enable )
103106

104107
/*
105108
* Check if pg_pathman is initialized & enabled.
@@ -109,12 +112,12 @@ simpify_mcxt_name(MemoryContext mcxt)
109112
/*
110113
* Should we override COPY stmt handling?
111114
*/
112-
#define IsOverrideCopyEnabled() ( pg_pathman_init_state.override_copy )
115+
#define IsOverrideCopyEnabled() ( pathman_init_state.override_copy )
113116

114117
/*
115118
* Check if auto partition creation is enabled.
116119
*/
117-
#define IsAutoPartitionEnabled() ( pg_pathman_init_state.auto_partition )
120+
#define IsAutoPartitionEnabled() ( pathman_init_state.auto_partition )
118121

119122
/*
120123
* Enable/disable auto partition propagation. Note that this only works if
@@ -124,18 +127,18 @@ simpify_mcxt_name(MemoryContext mcxt)
124127
#define SetAutoPartitionEnabled(value) \
125128
do { \
126129
Assert((value) == true || (value) == false); \
127-
pg_pathman_init_state.auto_partition = (value); \
130+
pathman_init_state.auto_partition = (value); \
128131
} while (0)
129132

130133
/*
131134
* Emergency disable mechanism.
132135
*/
133136
#define DisablePathman() \
134137
do { \
135-
pg_pathman_init_state.pg_pathman_enable = false; \
136-
pg_pathman_init_state.auto_partition = false; \
137-
pg_pathman_init_state.override_copy = false; \
138-
pg_pathman_init_state.initialization_needed = true; \
138+
pathman_init_state.pg_pathman_enable = false; \
139+
pathman_init_state.auto_partition = false; \
140+
pathman_init_state.override_copy = false; \
141+
pathman_init_state.initialization_needed = true; \
139142
} while (0)
140143

141144

‎src/include/pathman.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,6 @@
9494
extern Oid pathman_config_relid;
9595
extern Oid pathman_config_params_relid;
9696

97-
/* Hooks enable state */
98-
extern bool pathman_hooks_enabled;
99-
10097
/*
10198
* Just to clarify our intentions (return the corresponding relid).
10299
*/

‎src/include/relation_info.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,7 @@ const PartRelationInfo *get_pathman_relation_info_after_lock(Oid relid,
284284

285285
/* Partitioning expression routines */
286286
Node *parse_partitioning_expression(const Oid relid,
287-
const char *expression,
287+
const char *expr_cstr,
288288
char **query_string_out,
289289
Node **parsetree_out);
290290

‎src/init.c

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,16 @@ HTAB *parent_cache = NULL;
5555
HTAB *bound_cache = NULL;
5656

5757
/* pg_pathman's init status */
58-
PathmanInitState pg_pathman_init_state;
58+
PathmanInitState pathman_init_state;
59+
60+
/* pg_pathman's hooks state */
61+
bool pathman_hooks_enabled = true;
62+
5963

6064
/* Shall we install new relcache callback? */
6165
static bool relcache_callback_needed = true;
6266

67+
6368
/* Functions for various local caches */
6469
static bool init_pathman_relation_oids(void);
6570
static void fini_pathman_relation_oids(void);
@@ -123,13 +128,13 @@ pathman_cache_search_relid(HTAB *cache_table,
123128
void
124129
save_pathman_init_state(PathmanInitState *temp_init_state)
125130
{
126-
*temp_init_state = pg_pathman_init_state;
131+
*temp_init_state = pathman_init_state;
127132
}
128133

129134
void
130135
restore_pathman_init_state(const PathmanInitState *temp_init_state)
131136
{
132-
pg_pathman_init_state = *temp_init_state;
137+
pathman_init_state = *temp_init_state;
133138
}
134139

135140
/*
@@ -142,19 +147,19 @@ init_main_pathman_toggles(void)
142147
DefineCustomBoolVariable("pg_pathman.enable",
143148
"Enables pg_pathman's optimizations during the planner stage",
144149
NULL,
145-
&pg_pathman_init_state.pg_pathman_enable,
150+
&pathman_init_state.pg_pathman_enable,
146151
DEFAULT_PATHMAN_ENABLE,
147152
PGC_SUSET,
148153
0,
149154
NULL,
150-
pg_pathman_enable_assign_hook,
155+
pathman_enable_assign_hook,
151156
NULL);
152157

153158
/* Global toggle for automatic partition creation */
154159
DefineCustomBoolVariable("pg_pathman.enable_auto_partition",
155160
"Enables automatic partition creation",
156161
NULL,
157-
&pg_pathman_init_state.auto_partition,
162+
&pathman_init_state.auto_partition,
158163
DEFAULT_AUTO,
159164
PGC_SUSET,
160165
0,
@@ -166,7 +171,7 @@ init_main_pathman_toggles(void)
166171
DefineCustomBoolVariable("pg_pathman.override_copy",
167172
"Override COPY statement handling",
168173
NULL,
169-
&pg_pathman_init_state.override_copy,
174+
&pathman_init_state.override_copy,
170175
DEFAULT_OVERRIDE_COPY,
171176
PGC_SUSET,
172177
0,
@@ -208,7 +213,7 @@ load_config(void)
208213
}
209214

210215
/* Mark pg_pathman as initialized */
211-
pg_pathman_init_state.initialization_needed = false;
216+
pathman_init_state.initialization_needed = false;
212217

213218
elog(DEBUG2, "pg_pathman's config has been loaded successfully [%u]", MyProcPid);
214219

@@ -228,7 +233,7 @@ unload_config(void)
228233
fini_local_cache();
229234

230235
/* Mark pg_pathman as uninitialized */
231-
pg_pathman_init_state.initialization_needed = true;
236+
pathman_init_state.initialization_needed = true;
232237

233238
elog(DEBUG2, "pg_pathman's config has been unloaded successfully [%u]", MyProcPid);
234239
}

0 commit comments

Comments
 (0)