-
Notifications
You must be signed in to change notification settings - Fork 686
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
Fix the way we check for local/reference table joins in the executor #3280
Conversation
@@ -617,12 +617,22 @@ IsLocalReferenceTableJoinPlan(PlannedStmt *plan) | |||
return false; | |||
} | |||
|
|||
foreach(oidCell, plan->relationOids) | |||
foreach(rangeTableCell, plan->rtable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this contain RTEs for the entire query? (incl. CTEs, subqueries)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, standard_planner stores the flattened rte lists in plannedStmt->rtable. set_plan_references
concatenates all these lists in glob->finalrtable
, and then standard_planner does plannedStmt->rtable = glob->finalrtable
.
Added a comment.
if (RelationIsAKnownShard(relationId, onlySearchPath)) | ||
if (rangeTableEntry->rtekind == RTE_FUNCTION) | ||
{ | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why return false here? What if we a join between a local table, a reference table and a function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match distributed_planner.c's IsLocalReferenceTableJoin()
. Joins between local, reference, and functions will error out in planner. We can remove that restriction in both planner and executor, but maybe in another PR.
Removing the check here, although I think it won't change any behaviour.
if I try without any works (just coordinator), I get: postgres=# ALTER TABLE ref ADD COLUMN z int;
WARNING: could not find any shard placements for shardId 102040
... stuck forever |
that actually seems to be a separate bug unrelated to having the coordinator in the metadata |
f556d61
to
2c63485
Compare
Codecov Report
@@ Coverage Diff @@
## master #3280 +/- ##
==========================================
+ Coverage 92.04% 92.05% +0.01%
==========================================
Files 163 163
Lines 32865 32873 +8
==========================================
+ Hits 30250 30262 +12
+ Misses 2615 2611 -4 |
I can look into this tomorrow. |
if (RelationIsAKnownShard(relationId, onlySearchPath)) | ||
if (rangeTableEntry->rtekind != RTE_RELATION) | ||
{ | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since views are RTE_RELATION they are also treated as local tables.
CREATE VIEW v AS SELECT * FROM ref WHERE a = 1;
BEGIN;
SELECT * FROM v JOIN ref ON (v.a = ref.a);
ERROR: cannot join local tables and reference tables in a transaction block, udf block, or distributed CTE subquery
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed + tests. Also added tests and comments for the function join.
52930a4
to
90d2410
Compare
90d2410
to
939d3c9
Compare
Fixes #3279
Previously we used
plannedStmt->releationOids
which is the set of relations query depends on to check for the relations "involved" in the query. But this is not correct. For example, citus plans query of the following form in thecitus_drop_trigger()
to haverelationOids = [pg_class, reference_table]
, so we assumed it is a join between reference table and local table.This PR changes the behaviour to use the range table entries instead.