Avoid query deparse and planning of shard query in local execution. by colm-mchugh · Pull Request #8035 · citusdata/citus

Colm (colm-mchugh) · 2025-06-24T17:53:39Z

DESCRIPTION: Adds citus.enable_local_fast_path_query_optimization (enabled by default) GUC to avoid unnecessary query deparsing to improve performance of fast-path queries targeting local shards

Avoid query deparse and planning of shard query in local execution.

If a fast path query resolves to a shard that is local to the node planning the query, a shortcut can be taken so that the OID of the shard is plugged into the parse tree, which is then planned by Postgres. In local_executor.c the task uses that plan instead of parsing and planning a shard query. How this is done: The fast path planner identifies if the shortcut is possible, and then the distributed planner checks, using CheckAndBuildDelayedFastPathPlan(), if a local plan can be generated or if the shard query should be generated.

This optimization is controlled by a GUC citus.enable_local_execution_local_plan which is on by default. A new regress test local_execution_local_plan tests both row-sharding and schema sharding. Negative tests are added to local_shard_execution_dropped_column to verify that the optimization is not taken when the shard is local but there is a difference between the shard and distributed table because of a dropped column.

codecov · 2025-06-26T09:25:18Z

Codecov Report

Attention: Patch coverage is 90.86294% with 18 lines in your changes missing coverage. Please review.

Project coverage is 89.04%. Comparing base (c978de4) to head (a37cc1a).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8035      +/-   ##
==========================================
+ Coverage   88.45%   89.04%   +0.59%     
==========================================
  Files         284      284              
  Lines       61524    61689     +165     
  Branches     7698     7717      +19     
==========================================
+ Hits        54423    54934     +511     
+ Misses       4850     4509     -341     
+ Partials     2251     2246       -5

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Naisila Puka (naisila)

Thanks, LGTM.
Would be nice to provide the pgbench results in the PR description, that show the improvement when using this approach.

Onur Tirtir (onurctirtir)

Thanks for the PR, it looks good overall.

And just started reviewing this - probably just looked into a small portion of the files changed.

I'll continue reviewing this once I'm back on Wednesday but still sharing my quick and minor comments / questions.

Onur Tirtir (onurctirtir) · 2025-07-14T13:07:41Z



+static bool
+WarnIfLocalExecutionDisabled(bool *newval, void **extra, GucSource source)


Should we also add a similar warning check for citus.enable_local_execution?

Also, it can be better if we add a comment to the function as in:

/* * WarnIfLocalExecutionDisabled is used to emit a warning message when * enabling <new-GUC-name> if citus.enable_local_execution was disabled. */

In general, we try to add comments (that start with the function name) on top of functions as much as possible.

Should we also add a similar warning check for citus.enable_local_execution?

this was the only remaining part of above comment

#8035 (comment)
Seems this is yet addressed, do you prefer to skip adding a similar check function for citus.enable_local_execution?

#8035 (comment) Seems this is yet addressed, do you prefer to skip adding a similar check function for citus.enable_local_execution?

citus.enable_local_execution is explicitly used 50+ times in existing regress tests, and all associated goldfiles (.out) are impacted. Also, citus.enable_local_execution is also changed somewhere in EXPLAIN logic. So given this suggestion is having a broad impact on regress test output I'm omitting this.

Onur Tirtir (onurctirtir)

Done with code review, will need to test this a bit today.

Onur Tirtir (onurctirtir)

LGTM, leaving just a few more suggestions if you see them helpful.

Also, not necessarily to be done in this PR but it might worth to add a few lines about this change in technical readme, maybe somewhere after the place where we mention fast path queries.

Onur Tirtir (onurctirtir) · 2025-07-21T09:25:03Z



+static bool
+WarnIfLocalExecutionDisabled(bool *newval, void **extra, GucSource source)


#8035 (comment)
Seems this is yet addressed, do you prefer to skip adding a similar check function for citus.enable_local_execution?

Onur Tirtir (onurctirtir) · 2025-07-21T09:32:29Z

+												 !FindNodeMatchingCheckFunction(
+			(Node *) query,
+			CitusIsVolatileFunction);
+	}


maybe we can inform the user why we cannot use local fast path query optimizations when it's the case

Suggested change

}

}

else if (!EnableLocalFastPathQueryOptimization)

{

// using debug2, log that we cannot use local fast path query optimizations because

// citus.enable_local_fast_path_query_optimization was disabled

}

else if (!EnableLocalExecution)

{

// using debug2, log that we cannot use local fast path query optimizations because

// citus.ebable_local_execution was disabled

}

else

{

// using debug2, log that we cannot use local fast path query optimizations because earlier in the

// transaction a query / command accessed local shard placements via a remote connection

// so local execution was implicitly disabled for the rest of the transaction

}

I like this idea but deciding not to add because we don't have all the context here (InitializeFastPathContext()) and may give a misleading message, for example if its a reference table (which are unsupported) and either GUC is disabled, the user may think that enabling the GUC would make it work for reference tables and be puzzled why it does not. So I'm opting to pass on this rather than extend the function with all the needed context. Thanks for the suggestion though.

Onur Tirtir (onurctirtir) · 2025-07-21T09:41:01Z

+WHERE a = 8 AND b IN (1,3,5,8,13,21)
+GROUP BY b
+ORDER BY b;
+


it might be nice if we can add a test for the case where we cannot apply the optimizations because earlier in the transaction local execution state was switched to "disabled" by Citus

Suggested change

BEGIN;

-- force accessing local placements via remote connections first

SET citus.enable_local_execution TO false;

SELECT * FROM test_tbl;

-- Now, even if we enable local execution back before the query that

-- could normally benefit from fast path local query optimizations,

-- this time it won't be the case because local execution was implicitly

-- disabled by Citus as we accessed local shard placements via remote

-- connections.

SET citus.enable_local_execution TO true;

SELECT b, AVG(data_f), MIN(data_f), MAX(data_f), COUNT(1)

FROM test_tbl

WHERE a = 8 AND b IN (1,3,5,8,13,21)

GROUP BY b

ORDER BY b;

COMMIT;

Thanks for the suggestion - have added it to local_execution_local_plan.sql

If a fast past query resolves to a shard that is local to the node planning the query, a shortcut can be taken so that the OID of the shard is plugged into the parse tree, which is then put through the Postgres planner. The task query uses that plan instead of deparsing and compiling a shard query.

Colm (colm-mchugh) marked this pull request as draft June 24, 2025 17:53

Marco Slot (marcoslot) reviewed Jun 24, 2025

View reviewed changes

Comment thread src/backend/distributed/planner/fast_path_router_planner.c

Colm (colm-mchugh) force-pushed the colm/single_shard_local_exec branch 2 times, most recently from c8d3325 to 711873a Compare June 26, 2025 09:22

Colm (colm-mchugh) force-pushed the colm/single_shard_local_exec branch 9 times, most recently from dc7f94d to 3cbcf40 Compare July 3, 2025 12:18

Colm (colm-mchugh) marked this pull request as ready for review July 3, 2025 12:31

Colm (colm-mchugh) requested review from Naisila Puka (naisila), Onur Tirtir (onurctirtir) and Teja Mupparti (tejeswarm) July 4, 2025 09:10

Naisila Puka (naisila) approved these changes Jul 14, 2025

View reviewed changes

Comment thread src/backend/distributed/planner/multi_router_planner.c Outdated

Onur Tirtir (onurctirtir) reviewed Jul 14, 2025

View reviewed changes

Onur Tirtir (onurctirtir) reviewed Jul 17, 2025

View reviewed changes

Comment thread src/backend/distributed/planner/multi_router_planner.c

Onur Tirtir (onurctirtir) reviewed Jul 17, 2025

View reviewed changes

Colm (colm-mchugh) force-pushed the colm/single_shard_local_exec branch from 6194621 to 377c7af Compare July 18, 2025 09:43

Onur Tirtir (onurctirtir) approved these changes Jul 21, 2025

View reviewed changes

Colm (colm-mchugh) force-pushed the colm/single_shard_local_exec branch 2 times, most recently from 865d9d2 to ef8cfdb Compare July 21, 2025 15:38

Onur Tirtir (onurctirtir) self-requested a review July 22, 2025 10:37

Onur Tirtir (onurctirtir) approved these changes Jul 22, 2025

View reviewed changes

Colm (colm-mchugh) added 2 commits July 22, 2025 16:52

Round 1 of review comments.

b8c0a60

Review comments #2

a37cc1a

Colm (colm-mchugh) force-pushed the colm/single_shard_local_exec branch from ef8cfdb to a37cc1a Compare July 22, 2025 15:52

Colm (colm-mchugh) self-assigned this Jul 22, 2025

Colm (colm-mchugh) merged commit 245a62d into main Jul 22, 2025
148 of 149 checks passed

Colm (colm-mchugh) deleted the colm/single_shard_local_exec branch July 22, 2025 16:16



		static bool
		WarnIfLocalExecutionDisabled(bool newval, void *extra, GucSource source)

-	}
+	}
+	else if (!EnableLocalFastPathQueryOptimization)
+	{
+	// using debug2, log that we cannot use local fast path query optimizations because
+	// citus.enable_local_fast_path_query_optimization was disabled
+	}
+	else if (!EnableLocalExecution)
+	{
+	// using debug2, log that we cannot use local fast path query optimizations because
+	// citus.ebable_local_execution was disabled
+	}
+	else
+	{
+	// using debug2, log that we cannot use local fast path query optimizations because earlier in the
+	// transaction a query / command accessed local shard placements via a remote connection
+	// so local execution was implicitly disabled for the rest of the transaction
+	}

+BEGIN;
+  -- force accessing local placements via remote connections first
+  SET citus.enable_local_execution TO false;
+  SELECT * FROM test_tbl;
+  -- Now, even if we enable local execution back before the query that
+  -- could normally benefit from fast path local query optimizations,
+  -- this time it won't be the case because local execution was implicitly
+  -- disabled by Citus as we accessed local shard placements via remote
+  -- connections.
+  SET citus.enable_local_execution TO true;
+  SELECT b, AVG(data_f), MIN(data_f), MAX(data_f), COUNT(1)
+  FROM test_tbl
+  WHERE a = 8 AND b IN (1,3,5,8,13,21)
+  GROUP BY b
+  ORDER BY b;
+COMMIT;

Uh oh!

Conversation

Colm (colm-mchugh) commented Jun 24, 2025 • edited by onurctirtir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Naisila Puka (naisila) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Onur Tirtir (onurctirtir) left a comment • 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!

Onur Tirtir (onurctirtir) Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

Onur Tirtir (onurctirtir) Jul 17, 2025 • 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!

Onur Tirtir (onurctirtir) Jul 21, 2025 • 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!

Colm (colm-mchugh) Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Onur Tirtir (onurctirtir) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Onur Tirtir (onurctirtir) left a comment

Choose a reason for hiding this comment

Uh oh!

Onur Tirtir (onurctirtir) Jul 21, 2025 • 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!

Onur Tirtir (onurctirtir) Jul 21, 2025 • 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!

Colm (colm-mchugh) Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Onur Tirtir (onurctirtir) Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Colm (colm-mchugh) Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Colm (colm-mchugh) commented Jun 24, 2025 •

edited by onurctirtir

Loading

codecov Bot commented Jun 26, 2025 •

edited

Loading

Onur Tirtir (onurctirtir) left a comment •

edited

Loading

Onur Tirtir (onurctirtir) Jul 17, 2025 •

edited

Loading

Onur Tirtir (onurctirtir) Jul 21, 2025 •

edited

Loading

Onur Tirtir (onurctirtir) Jul 21, 2025 •

edited

Loading

Onur Tirtir (onurctirtir) Jul 21, 2025 •

edited

Loading