Add support for router INSERT .. SELECT commands #7077

onderkalaci · 2023-07-20T13:36:06Z

Tradionally our planner works in the following order:
router - > pushdown -> repartition -> pull to coordinator

However, for INSERT .. SELECT commands, we did not support "router".

In practice, that is not a big issue, because pushdown planning can handle router case as well.

However, with PG 16, certain outer joins are converted to JOIN without any conditions (e.g., JOIN .. ON (true)) and the filters are pushed down to the tables.

When the filters are pushed down to the tables, router planner can detect. However, pushdown planner relies on JOIN conditions.

An example query:

INSERT INTO agg_events (user_id)
        SELECT raw_events_first.user_id
        FROM raw_events_first LEFT JOIN raw_events_second
        	ON raw_events_first.user_id = raw_events_second.user_id
        WHERE raw_events_first.user_id = 10;

As a side effect of this change, now we can also relax certain limitation that "pushdown" planner emposes, but not "router". So, with this PR, we also allow those.

Closes #6772
DESCRIPTION: Prevents unnecessarily pulling the data into coordinator for some INSERT .. SELECT queries that target a single-shard group

tejeswarm · 2023-07-20T17:55:21Z

@onderkalaci "The absence of JOIN restriction information in PG16 results in the Citus planner's inability to recognize the potential of pushdown operations for single shard queries, leading to alternative planning. All multi-shard queries we are covered and Citus does recognize the pushdown opportunity?", is that a fair statement?

src/backend/distributed/planner/insert_select_planner.c

onderkalaci · 2023-07-21T06:27:28Z

@onderkalaci "The absence of JOIN restriction information in PG16 results in the Citus planner's inability to recognize the potential of pushdown operations for single shard queries, leading to alternative planning. All multi-shard queries we are covered and Citus does recognize the pushdown opportunity?", is that a fair statement?

Yes, that's fair. Also note that other than INSERT .. SELECT (and probably merge), we already have the alternative planning (e.g., single shard router planning). So, other code-paths are not impacted by PG16.

I think maybe one more note here is that in Citus' planning logic, the single shard router kicks before the multi-shard pushdown. So, for non-INSERT..SELECT cases, the behavior has not changed anyway. The filters on the tables are already present on PG15-.

onurctirtir · 2023-07-24T08:06:40Z

Does that replace #6772?

onderkalaci · 2023-07-25T08:59:11Z

Does that replace #6772?

yes

marcocitus · 2023-07-25T09:08:25Z

src/backend/distributed/planner/insert_select_planner.c

@@ -690,15 +727,16 @@ DistributedInsertSelectSupported(Query *queryTree, RangeTblEntry *insertRte,
 	}

 	/* first apply toplevel pushdown checks to SELECT query */
-	DeferredErrorMessage *error = DeferErrorIfUnsupportedSubqueryPushdown(subquery,
+	DeferredErrorMessage *error = routerSelect ? NULL :


maybe just have an if (!routerSelect) block around these error checks

onderkalaci · 2023-07-26T11:38:42Z

src/test/regress/expected/query_single_shard_table.out

@@ -1398,6 +1398,7 @@ INSERT INTO nullkey_c1_t1 SELECT * FROM nullkey_c1_t2;
 SET client_min_messages TO DEBUG2;
 --    between two non-colocated single-shard tables
 INSERT INTO nullkey_c1_t1 SELECT * FROM nullkey_c2_t1;
+DEBUG:  Creating router plan


@onurctirtir can you please review the test output changes and let me know if there are any unexpected changes.

I already did a pass, but would be nice to get a review from you.

Also please check insert_select_single_shard.out changes as well

onderkalaci · 2023-07-26T11:43:26Z

src/test/regress/expected/local_shard_copy.out

@@ -28,9 +28,11 @@ SELECT create_distributed_table('distributed_table','key');
 (1 row)

 INSERT INTO distributed_table SELECT *,* FROM generate_series(20, 40);
+DEBUG:  Creating router plan


We have lots of new Creating router plan logs. I was not sure whether we should consider omitting it.

But I decided to keep these are it is really what happens underlying.

naisila · 2023-07-27T12:32:36Z

Note: This patch only fixes the multi_insert_select failure from this list #7017

Fix complex joins are only supported when all distributed tables are co-located and joined on their distribution columns, seen in multi_insert_select

onderkalaci · 2023-07-27T12:39:51Z

Note: This patch only fixes the multi_insert_select failure from this list #7017

Fix complex joins are only supported when all distributed tables are co-located and joined on their distribution columns, seen in multi_insert_select

thanks for verifying, that was my intention as well

Tradionally our planner works in the following order: router - > pushdown -> repartition -> pull to coordinator However, for INSERT .. SELECT commands, we did not support "router". In practice, that is not a big issue, because pushdown planning can handle router case as well. However, with PG 16, certain outer joins are converted to JOIN without any conditions (e.g., JOIN .. ON (true)) and the filters are pushed down to the tables. When the filters are pushed down to the tables, router planner can detect. However, pushdown planner relies on JOIN conditions. An example query: ``` INSERT INTO agg_events (user_id) SELECT raw_events_first.user_id FROM raw_events_first LEFT JOIN raw_events_second ON raw_events_first.user_id = raw_events_second.user_id WHERE raw_events_first.user_id = 10; ``` As a side effect of this change, now we can also relax certain limitation that "pushdown" planner emposes, but not "router". So, with this PR, we also allow those.

onurctirtir

LGTM except the flaky tests.

I think we should add the following to the PR description:

Closes https://github.com/citusdata/citus/pull/6772.

And the following changelog item like we did in #6752.

DESCRIPTION: Prevents unnecessarily pulling the data into coordinator for some INSERT .. SELECT queries that target a single-shard group

(Omitting the effect on PG16 support because no Citus versions had support for PG16 so far)

onurctirtir · 2023-07-28T10:54:47Z

src/backend/distributed/planner/insert_select_planner.c

@@ -61,6 +61,8 @@ static DistributedPlan * CreateInsertSelectPlanInternal(uint64 planId,
 static DistributedPlan * CreateDistributedInsertSelectPlan(Query *originalQuery,
 														   PlannerRestrictionContext *
 														   plannerRestrictionContext);
+static bool InsertSelectRouter(Query *originalQuery,


Maybe we should name this as InsertSelectIsSelectRouter or such to emphasize that this doesn't imply that the whole query is router but the select part.

onurctirtir · 2023-07-28T11:02:57Z

src/test/regress/expected/query_single_shard_table.out

-- We could relax distributed insert .. select checks to allow pushing
-- down more clauses down to the worker nodes when inserting into a single
-- shard by selecting from a colocated one. We might want to do something
-- like https://github.com/citusdata/citus/pull/6772.
--
--  e.g., insert into null_shard_key_1/citus_local/reference
--        select * from null_shard_key_1/citus_local/reference limit 1
--
-- Below "limit / offset clause" test and some others are examples of this.


Getting rid of those comments made me feel very happy :)

Similar to #7077. As PG 16+ has changed the join restriction information for certain outer joins, MERGE is also impacted given that is is also underlying an outer join. See #7077 for the details.

onderkalaci requested a review from marcocitus July 20, 2023 14:44

tejeswarm reviewed Jul 20, 2023

View reviewed changes

src/backend/distributed/planner/insert_select_planner.c Show resolved Hide resolved

marcocitus reviewed Jul 25, 2023

View reviewed changes

onderkalaci mentioned this pull request Jul 25, 2023

Expand EnableNonColocatedRouterQueryPushdown to cover shard colocation (e.g., shard index) #7076

Merged

onderkalaci force-pushed the router_insert_select branch 3 times, most recently from ce3eff4 to 9bd9770 Compare July 26, 2023 11:35

onderkalaci marked this pull request as ready for review July 26, 2023 11:35

onderkalaci commented Jul 26, 2023

View reviewed changes

onderkalaci requested review from marcocitus and tejeswarm July 26, 2023 11:40

onderkalaci commented Jul 26, 2023

View reviewed changes

onderkalaci requested a review from onurctirtir July 26, 2023 11:44

onderkalaci force-pushed the router_insert_select branch from 9bd9770 to e90557e Compare July 26, 2023 11:49

onderkalaci force-pushed the router_insert_select branch from e90557e to 74ced12 Compare July 28, 2023 06:06

onurctirtir approved these changes Jul 28, 2023

View reviewed changes

onderkalaci force-pushed the router_insert_select branch from 74ced12 to 05b97fd Compare July 28, 2023 11:57

onderkalaci enabled auto-merge (squash) July 28, 2023 12:06

onderkalaci merged commit cb5eb73 into main Jul 28, 2023
26 checks passed

onderkalaci deleted the router_insert_select branch July 28, 2023 12:07

onderkalaci mentioned this pull request Jul 28, 2023

Add single-shard router Merge command support #7088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for router INSERT .. SELECT commands #7077

Add support for router INSERT .. SELECT commands #7077

onderkalaci commented Jul 20, 2023 •

edited

Loading

tejeswarm commented Jul 20, 2023

onderkalaci commented Jul 21, 2023

onurctirtir commented Jul 24, 2023

onderkalaci commented Jul 25, 2023

marcocitus Jul 25, 2023

onderkalaci Jul 26, 2023

onderkalaci Jul 26, 2023

naisila commented Jul 27, 2023

onderkalaci commented Jul 27, 2023

onurctirtir left a comment •

edited

Loading

onurctirtir Jul 28, 2023

onurctirtir Jul 28, 2023

Add support for router INSERT .. SELECT commands #7077

Add support for router INSERT .. SELECT commands #7077

Conversation

onderkalaci commented Jul 20, 2023 • edited Loading

tejeswarm commented Jul 20, 2023

onderkalaci commented Jul 21, 2023

onurctirtir commented Jul 24, 2023

onderkalaci commented Jul 25, 2023

marcocitus Jul 25, 2023

Choose a reason for hiding this comment

onderkalaci Jul 26, 2023

Choose a reason for hiding this comment

onderkalaci Jul 26, 2023

Choose a reason for hiding this comment

naisila commented Jul 27, 2023

onderkalaci commented Jul 27, 2023

onurctirtir left a comment • edited Loading

Choose a reason for hiding this comment

onurctirtir Jul 28, 2023

Choose a reason for hiding this comment

onurctirtir Jul 28, 2023

Choose a reason for hiding this comment

onderkalaci commented Jul 20, 2023 •

edited

Loading

onurctirtir left a comment •

edited

Loading