Adapt pg_pathman for PG 11. #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

funbringer merged 7 commits into postgrespro:next from arssher:postgres_11

Jul 12, 2018

Merged

Adapt pg_pathman for PG 11. #166

funbringer merged 7 commits into postgrespro:next from arssher:postgres_11

Jul 12, 2018

Conversation

arssher

Copy link

Contributor

@arssher arssher commented Jul 9, 2018

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.

@arssher arssher force-pushed the postgres_11 branch from fc73125 to bbf5a65 Compare

July 9, 2018 13:17

@codecov-io

Copy link

codecov-io commented Jul 9, 2018 •

edited

Loading

Codecov Report

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

Impacted file tree graph

@@ 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

funbringer reviewed

Jul 9, 2018

View reviewed changes

src/compat/pg_compat.c Outdated

int parallel_workers;

#if PG_VERSION_NUM >= 110000

parallel_workers = compute_parallel_worker(rel, rel->pages, -1,

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you extract this one as compute_parallel_worker macro, please?

Copy link

Contributor Author

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/hooks.c Outdated

generate_gather_paths(root, rel);

/* consider gathering partial paths for the parent appendrel */

#if PG_VERSION_NUM >= 110000

generate_gather_paths(root, rel, false);

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also this one.

Copy link

Contributor Author

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/init.c Outdated

#include "catalog/indexing.h"

#include "catalog/pg_extension.h"

#include "catalog/pg_inherits.h"

#if PG_VERSION_NUM < 110000

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Contributor Author

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/hooks.c Outdated

/* Convert list to array for faster lookups */

#if PG_VERSION_NUM >= 110000

setup_append_rel_array(root);

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Contributor Author

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is not good. Fixed.

src/relation_info.c

/* Create a new memory context to store expression tree etc */

prel_mcxt = AllocSetContextCreate(PathmanParentsCacheContext,

__FUNCTION__,

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link

Contributor Author

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks!

funbringer

funbringer reviewed

Jul 9, 2018

View reviewed changes

src/include/partition_filter.h

#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"

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one looks odd.

Copy link

Contributor Author

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below changes in pg_compat.h

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm.

funbringer

funbringer reviewed

Jul 9, 2018

View reviewed changes

src/nodes_common.c Outdated

RelOptInfo *child_rel = ((Path *) lfirst(lc1))->parent;

#if PG_VERSION_NUM >= 110000

AppendRelInfo *appinfo = root->append_rel_array[child_rel->relid];

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Contributor Author

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

funbringer reviewed

Jul 9, 2018

View reviewed changes

src/pathman_workers.c Outdated

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 };

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor Author

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, fixed.

funbringer

funbringer reviewed

Jul 9, 2018

View reviewed changes

tests/python/partitioning_test.py

# Thread for connection #2 (it has to wait)

def con2_thread():

con1.begin()

con2.begin()

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one.

funbringer

funbringer reviewed

Jul 9, 2018

View reviewed changes

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");

Copy link

Collaborator

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess ERR_PART_ATTR_MULTIPLE_RESULTS is useless now, right?

Copy link

Contributor Author

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

funbringer

funbringer reviewed

Jul 10, 2018

View reviewed changes

expected/pathman_permissions.out

BEGIN

SELECT create_range_partitions('permissions.user1_table', 'id', 1, 10, 2);

EXCEPTION

WHEN insufficient_privilege THEN

Copy link

Collaborator

@funbringer funbringer Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Contributor Author

@arssher arssher Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor Author

@arssher arssher Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Contributor Author

@arssher arssher Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

funbringer

funbringer reviewed

Jul 10, 2018

View reviewed changes

src/relation_info.c Outdated

#endif

#if PG_VERSION_NUM >= 90600

#if PG_VERSION_NUM >= 90600&&PG_VERSION_NUM<11000

Copy link

Collaborator

@funbringer funbringer Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should replace 11000 with 110000.

Copy link

Contributor Author

@arssher arssher Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@arssher 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

@arssher


 Adapt pg_pathman for PG 11.

389c807

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.

@arssher


 Fix handling of append_rel_array.

6a60886

Also a bunch of other stuff noted by @funbringer.

@arssher


 REL10 typo fix

0137d19

@arssher


 Raise notice 'insufficient privileges', fix pg_constraint_fn inclusion.

4e84256

@arssher


 Fix pathman_rebuild_deletes test for 11.

@arssher


 Fix pathman_rebuild_updates test.

50bfb92

@arssher arssher force-pushed the postgres_11 branch from 442d54c to 50bfb92 Compare

July 12, 2018 11:21

@arssher


 Purge spurious whitespace

44a3f67

@funbringer funbringer merged commit 44a3f67 into postgrespro:next

Jul 12, 2018

@funbringer

Copy link

Collaborator

funbringer commented Jul 12, 2018

Merged, thanks a lot!

Labels

None yet

3 participants

@arssher @codecov-io @funbringer

Adapt pg_pathman for PG 11. #166

Adapt pg_pathman for PG 11. #166

Uh oh!

Conversation

@arssher 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!

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer 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!

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer funbringer Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@arssher arssher Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

@funbringer funbringer Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

@arssher arssher Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 9, 2018 •

edited

Loading

@funbringer funbringer Jul 9, 2018 •

edited

Loading