Skip to content
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

Store the total number of plans in PlannedStmt. #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yrashk
Copy link

@yrashk yrashk commented Jul 16, 2024

Extensions that use planner/executor hooks can't precisely pre-allocate memory for data structures mirroring the plan as the number of plan nodes is unknown.

This information is effectively collected in the standard planner in the node ID counter but was discarded at the end. This change preserves this information.

@yrashk yrashk force-pushed the planned-stmt-total_plans branch from 336fb9c to 9d21e62 Compare July 16, 2024 17:03
@einhverfr
Copy link

I am assuming that the performance implications of this are minor and plan-time only.

Nonetheless if we want to submit this upstream we will definitely need to benchmark it to prove that is the case.

@@ -97,6 +97,8 @@ typedef struct PlannedStmt
/* statement location in source string (copied from Query) */
ParseLoc stmt_location; /* start location, or -1 if unknown */
ParseLoc stmt_len; /* length in bytes; 0 means "rest of string" */

int total_plans; /* indicates the number of plan nodes in the statement */
Copy link

Choose a reason for hiding this comment

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

why not unsigned int ?

also, let's name it plan_nodes_count or similar as total_plans could indicate that there are potentially multiple plans.

Copy link
Author

Choose a reason for hiding this comment

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

Makes a lot of sense, will address shortly!

Copy link
Author

Choose a reason for hiding this comment

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

src/backend/nodes/gen_node_support.pl doesn't support unsigned int:

could not handle type "unsigned int" in struct "PlannedStmt" field "plan_nodes_count"

Extensions that use planner/executor hooks can't precisely pre-allocate
memory for data structures mirroring the plan as the number of plan
nodes is unknown.

This information is effectively collected in the standard planner in the
node ID counter but was discarded at the end. This change preserves this
information.
@yrashk yrashk force-pushed the planned-stmt-total_plans branch from 9d21e62 to 68d38d2 Compare July 21, 2024 15:54
@danolivo
Copy link

Extensions that use planner/executor hooks can't precisely pre-allocate memory for data structures mirroring the plan as the number of plan nodes is unknown.

This information is effectively collected in the standard planner in the node ID counter but was discarded at the end. This change preserves this information.

For me, it may be resolved by just walking through the plan nodes: Some people want to take into account only nodes from the plan, while others want to count CTEs and Subplans nodes, too. It depends on the purpose.
I think, more general solution could be to add a 'List *extended_data' field into PlannedStmt where any extension could store information and reuse it during execution phase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants