WIP Fix issue #7674 about UPDATE SET(..), with indirection #7675
Conversation
Queries of the form: ``` UPDATE ... SET (a, b) = (SELECT '1', '2') UPDATE ... SET (b, a) = (SELECT '2', '1') ``` Should do the same thing, but currently the order of the attributes in `SET (...)` as rewriten for pushdown is only based on the physical ordering of the attributes in the relation. This leads to several subtle problems including situation where a DROPped then reADDed attributes will change its placement in the attribute list. There are maybe more tests to add in other situation where a SET (MULTIEXPR) is possible, though UPDATE form is pretty unique as alternatives are not supported by citus: `(INSERT .. ON CONFLICT SET (a,b).....`
ruleutils in Citus is based on PostgreSQL source code, but in PostgreSQL ruleutils is not used at the planner stage. For instance, it is assumed after parser that targetList are ordered as they were read, but it's not true after rewriter, the resulting rewrite tree is then provided to planner (and citus), but the ordering of the list is not granted anymore. It's similar to others previous issues reported and still open, as well as to other bugfixes/improvment over time, the most noticable being the ProcessIndirection() which is for domain and similar. However, the implications of this bug are huge for users of `UPDATE SET (...)`: 1. if you used to order by columns order, you're maybe safe: `SET (col1, col2, col3, ...)` 2. if you used not to order by column order: `SET (col2, col1, col3, ...)` then you probably found a problem, or you have one. Note about 1. that despite appearance and your QA, you are at risk: if physical columns ordering is changed (for example after DROPping/ADDing some), the same query which use to apparently works well will silently update other columns... As it is this code is not optimized for performance, not sure it'll be needed.
c2main
force-pushed
the
fix-update-multi-attr-select
branch
from
099eab0 to
73f4f67
Compare
|
@naisila just a message to help you find this PR when you'll get a chance to look at it... |
There was a problem hiding this comment.
#5692 is also trying to fix quite a similar problem. Reviewers had objected to that PR because we want to avoid modifying ruleutils as much as possible and try to fix such issues through the distributed planner. Have you tried to investigate this issue through the lens of the distributed planner hook?
Thank you for the feedback. No I didn't look at distributed planner hook, I will evaluate this approach. |
I just looked:
I understand the motivation to not touch too much ruleutils, but also my understanding is that ruleutils has been imported precisely to achieve a goal which is not achieved because it contains several limitation due to reasons well explained in Onur's PR (tree rewritten and optimized by postgres before being in the hands of citus). By touching only distributed planner I fear that the bug will just reappear in some corner case situation, but I am still exploring, sharing here as I'm sure those points have been discussed in the past but I didn't search history... |
|
If the patch is elsewhere then I suppose it will be required to do essentially what IMHO, this is a "codepath" expecting to diverge a lot from upstream because they deserve distinct purpose, for many cases the solution is just to not allow that in Citus and maybe it's an alternative to consider here. Like some other similar queries. Maybe I missed alternatives ? |
|
Thanks for your investigations. The team will look at this in more detail as soon as we get a chance. |
This reverts commit 73f4f67.
I've tried to patch in lower layer, it's possible, but less convenient and more error prone. If it works for you this way I can squash the commits and maybe improve names/comments ? |
It'll probably be interesting to add more test related to indirection here.
We may need to reorder parts of the planner tree we are receiving in dsitributed_planner hook because it has been optimized by PostgreSQL. However we only need to rewrite tree which will be used to produce a query string, otherwise it's pointless. Proceed very early in the planner hook, which is probably a good place to pull up some similar fixes applied here and there. See the exported function from citus_ruleutils.c: void RebuildParserTreeFromPlannerTree(Query *query) Added a new include in citus_ruleutils.c: miscadmin.h from PostgreSQL, because it contains the check_stack_depth() function which is a good idea to use as we don't know the size of the query..
c2main
force-pushed
the
fix-update-multi-attr-select
branch
from
6d69c20 to
1cf93c1
Compare
@naisila @onurctirtir is it aligned with what you had in mind ? |
This PR has been open for several month already, it fixes a transparent bug with data alteration. What is required to increase priority or getting some more attention ? |
|
@c2main Thanks for your work on this, we appreciate your reworking the fix to apply it in the distributed planner hook. We would like the fix to be detected and applied at a lower level than in the current PR, for example in CreateModifyPlan() (or at a lower level, in RouterJob()). If this proves to be infeasible then we'd prefer to go with the first approach of applying the fix in citus_ruleutils.c. Thanks again for your efforts, we can work with you in progressing this. |
When patching in with qq3 as (
update test_dist_indirection
SET (col_text, col_bool)
= (SELECT 'full', true)
where id = 3
returning *
),
qq4 as (
update test_dist_indirection
SET (col_text, col_bool)
= (SELECT 'fully', true)
where id = 4
returning *
)
select * from qq3 union all select * from qq4;However, adding my rewrite function just before Maybe it's possible to push down a bit more in if (fastPathRouterQuery)
...
else
...
if (isMultiShardQuery)
...
else
...Is it worth pursuing in this direction ? Meantime, I have update PR with the |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7675 +/- ##
===========================================
- Coverage 89.69% 49.10% -40.60%
===========================================
Files 283 283
Lines 60510 59770 -740
Branches 7541 7367 -174
===========================================
- Hits 54276 29349 -24927
- Misses 4079 27794 +23715
- Partials 2155 2627 +472 |
Nice.
Yes, but if it proves too cumbersome because of the different query types it's probably not worth pursuing. For example, if special case code is required for each query type and |
Pushing down a little bit more, 2 call places are required: diff --git a/src/backend/distributed/planner/multi_router_planner.c b/src/backend/distributed/planner/multi_router_planner.c
index 84352415f..36aaeeecd 100644
--- a/src/backend/distributed/planner/multi_router_planner.c
+++ b/src/backend/distributed/planner/multi_router_planner.c
@@ -1869,13 +1869,6 @@ RouterJob(Query *originalQuery, PlannerRestrictionContext *plannerRestrictionCon
}
else
{
- /*
- * We may need to reorder parts of the planner tree we are receiving here.
- * We expect to produce an SQL query text but our tree has been optimized by
- * PostgreSL rewriter already...
- * FIXME is there conditions to reduce the number of calls ?
- */
- RebuildParserTreeFromPlannerTree(originalQuery);
(*planningError) = PlanRouterQuery(originalQuery, plannerRestrictionContext,
&placementList, &shardId, &relationShardList,
&prunedShardIntervalListList,
@@ -2364,6 +2357,8 @@ PlanRouterQuery(Query *originalQuery,
if (!IsMergeQuery(originalQuery))
{
+ /* OK */
+ RebuildParserTreeFromPlannerTree(originalQuery);
planningError = ModifyQuerySupported(originalQuery, originalQuery,
isMultiShardQuery,
plannerRestrictionContext);
@@ -2450,6 +2445,8 @@ PlanRouterQuery(Query *originalQuery,
bool isUpdateOrDelete = UpdateOrDeleteOrMergeQuery(originalQuery);
if (!(isUpdateOrDelete && RequiresCoordinatorEvaluation(originalQuery)))
{
+ /* OK */
+ RebuildParserTreeFromPlannerTree(originalQuery);
UpdateRelationToShardNames((Node *) originalQuery, *relationShardList);
}
At the moment, the So what are we trying to solve here ?
There is very probably a design choice to be made, if possible not by me but Citus committers on what contract Citus want to have with parser tree vs rewritter tree vs planner tree:
My understanding is that the current contract is the 3rd (which include a bit of contract 1 and contract 2). My preferences in design is 2. (it may allow to handle more queries in Citus), then the 1. for workarounds (and evaluate fixes upstream). From a PostgreSQL point of view, we also have problem around this topic (to show VIEW definition IIRC, and for ANALYZE text output). Having the parser tree available would be very handy. The other way around is maybe to push-back ruleutils fixes to PostgreSQL, if at all possible, to reduce the diff with upstream. |
Issue reported to Data Bene support. See #7674
The fix looks good, at least it breaks nothing and the bug not visible.
I am unsure to add all regressions tests where I did, maybe their own file?
Also the PR is not yet ready regarding citus_indent, but if you can already provide some feedback.