WIP Fix issue #7674 about UPDATE SET(..), with indirection #7675
Conversation
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, 8000 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. |
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 ? |
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
❌ Your patch check has failed because the patch coverage (55.55%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7675 +/- ##
==========================================
- Coverage 89.70% 87.28% -2.43%
==========================================
Files 283 283
Lines 60518 60470 -48
Branches 7543 7532 -11
==========================================
- Hits 54287 52780 -1507
- Misses 4076 5324 +1248
- Partials 2155 2366 +211 |
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,
8000
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. |
Nicely put, thanks for laying out the options. We are currently weighing up 1 and 2. There may be unintended consequences to 2 (and 3) because Citus is passing a partially un-written Query tree to You mentioned the performance impact of 2 in some earlier comments, perhaps this is something that could be assessed with benchmarking. As with any benchmarking coming up with a set of queries that push the feature (or patch) is the main challenge. There is potentially another way to achieve 2, which is to use Postgres' I verified that your PR fixes #4092, a similar issue. Could you update the test cases to cover that ? Also, it would be great if this PR could assess and include the logic and test cases from #5692, as that is also fixing a problematic UPDATE statement (postgres rewrite merges UPDATE targets into one single target). What's your take on including #5692 ? |
Thank you for the feedback @colm-mchugh. FYI I am still studying alternative ways to achieve the goal, including hook mentioned. Also a step back to look at the picture more globally (Postgres FDW does not support those statements because of the hard work required to undo those "optimizations" from the rewriter...so it looks interesting to have a patch for Postgres FDW and have similar for Citus, if at all possible). |
Sure, thanks for the update. We are leaning to applying the fix in ruleutils, or at least avoiding passing an un-rewritten query tree to the Postgres planner in the distributed planner hook. The targetlist un-rewriting could inhibit Postres' use of the physical tlist optimization when creating a scan node, which is not a concern for Citus distributed planning, but there could be potential problems in the future; we'd like to keep to Postgres planner's implicit expectation to receive a rewritten query tree. But please continue to investigate and propose the solution according to your judgement. Postgres FDW is interesting, it does not include ruleutils but its deparse.c is “annoyingly duplicative of ruleutils.c”, to quote its comment. This duplication seems to be in some functions for deparsing expressions (deparseConst(), deparseFuncExpr(), deparseAggreg()). Of the extensions that come with Postgres, only one (pageinspect) uses ruleutils (specifically, function |
I completely agree with the points you're raising here and I was concerned by this angle too (and I confirm planner does expect the ordered tlist, at least from comment in
Yeah, I appreciated the preliminary comment in this deparse.c file. /*
* If it's a MULTIEXPR Param, punt. We can't tell from here
* whether the referenced sublink/subplan contains any remote
* Vars; if it does, handling that is too complicated to
* consider supporting at present. Fortunately, MULTIEXPR
* Params are not reduced to plain PARAM_EXEC until the end of
* planning, so we can easily detect this case. (Normal
* PARAM_EXEC Params are safe to ship because their values
* come from somewhere else in the plan tree; but a MULTIEXPR
* references a sub-select elsewhere in the same targetlist,
* so we'd be on the hook to evaluate it somehow if we wanted
* to handle such cases as direct foreign updates.)
*/I also have a better understanding on the motivation of the citus team to try to use another place than ruleutils to patch. On my journey, I was (and am still) also considering barriers to be added to just prevent the known bad cases to happen (for example it looks easy, safe and free to check if the tlist has been modified and reordered, and if it is ERROR with an informative message). Maybe this is the first thing to do as it'll hopefully requires a less intrusive approach. And we can get more time to think about it. |
That comment in deparse.c on MULTI_EXPR param nodes reminds me of #7676, the other UPDATE issue you found, where the target expression passed to Postgres execution initialization has a MULTI_EXPR param node, and Postgres exec says Returning to this issue, we (Citus committers) are internally agreed that rulesutils is the best place, or m 628C ost appropriate place given the constraints, for the fix. However you make a good point about preventing the known problematic cases as an interim; it is less intrusive, and lower risk, and affords more time to land the solution, as you say. Please explore, and propose in this direction as you see fit. |
|
I have added more tested related to this PR and found another problem in |
0b434dc to
c1aa700
Compare
|
just pushed some more tests (see indirections.sql) and ERROR on cases not well managed so far. |
|
OK, I have added a list sort function to order target list per paramid value: those are always incremented, with a large increment when switching sublinks, it looks simple enough, though I may rework the As such it's fixing all the related issues I think, but the one with a I will update this PR accordingly, at least with an identified ERROR message for this case, and will fix in another PR (related to this other issue: #7676 ) |
0344947 to
724e589
Compare
|
@c2main we'd like get this into the upcoming release Citus 13.1, so just want to check if it is possible for you to rebase onto the current main branch, taking into account the following details, within the next couple of weeks, or by the end of this month (Apr '25):
Let us know how that sounds, if its reasonable to target it for 13.1. And thanks again for your efforts. |
👍 That sounds good, there is also this point that we need to WARN/ERROR or have an option to WARN/ERROR when we detect a query which may have corrupt data previously. |
724e589 to
490f6ab
Compare
Adding similar indirection test to existing ones but using a sublink. i.e. INSERT INTO .... SELECT and UPDATE ... SELECT are added. Queries of the form: ``` ... SET (a, b) = (SELECT '1', '2') ... 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 some alternatives are not supported yet by citus, for example: `(INSERT .. ON CONFLICT SET (a,b).....`
-- TODO crash Citus --- INSERT INTO domain_indirection_test (f1,f3.if1, f3.if2) SELECT 0, 1, 2; +INSERT INTO domain_indirection_test (f1,f3.if1, f3.if2) SELECT 0, 1, 2; +ERROR: cannot plan distributed INSERT INTO ... SELECT query +HINT: Do not use array references and field stores on the INSERT target list. INSERT INTO domain_indirection_test (f1,f3.if1) VALUES (0, 1); ERROR: inserting or modifying composite type fields is not supported HINT: Use the column name to insert or update the composite type as a single value -- TODO here: not the expected ERROR -- INSERT INTO domain_indirection_test (f1,f3.if1) SELECT 0, 1; -- ERROR: could not find a conversion path from type 23 to 17619 UPDATE domain_indirection_test SET domain_array[0].if2 = 5; ERROR: inserting or modifying composite type fields is not supported HINT: Use the column name to insert or update the composite type as a single value -- TODO here: not the expected ERROR
Add more testing for UPDATE with indirection
It'll probably be interesting to add more test related to indirection
here.
Produce an ERROR when attribute in the target list have been reordered by PostgreSQL rewritter.
All OK for basic UPDATE multi SELECT
490f6ab to
dcdaaaa
Compare
Yes, I will discuss with the citus committers and get back on this specific point. In the meantime, I'll leave some comments on the PR. |
| { | ||
| int paramid = 0; | ||
| if (saw_junk) | ||
| elog(ERROR, "out of order target list"); |
There was a problem hiding this comment.
Curious about the motivation for this error; there isn't a test case for it in the PR it looks like, but do you have a query that hit this condition? Would it imply an error in the Postgres parser/analyzer if it was hit?
There was a problem hiding this comment.
Curious about the motivation for this error; there isn't a test case for it in the PR it looks like, but do you have a query that hit this condition? Would it imply an error in the Postgres parser/analyzer if it was hit?
I maybe have been too much volunteer here: there is similar check in executor on PostgreSQL, so doing it at the planner level is probably useless. See https://github.com/postgres/postgres/blob/3631612eae9c2def99151c4f36b1b3771f53cba7/src/backend/executor/execExpr.c#L581
We're not supposed to hit this ERROR at any point, there is no associated test in PostgreSQL either.
I will probably keep an Assert() though as we're reordering the target list, and we probably don't want to reorder further a list which is not in the expected order.
There was a problem hiding this comment.
I see, thanks. Assert() on resjunk(s) together at the tail of the list sounds good.
| SET citus.next_shard_id TO 750000; | ||
| SET citus.next_placement_id TO 750000; | ||
|
|
||
| CREATE SCHEMA indirections; |
There was a problem hiding this comment.
nit: can we name this regress test ? For example multi_update_select.sql may be more descriptive, similar to the existing regress test multi_insert_select.sql.
Also it looks like it is not in any test schedule; I suggest adding it to src/test/regress/multi_schedule
There was a problem hiding this comment.
nit: can we name this regress test ? For example
multi_update_select.sqlmay be more descriptive, similar to the existing regress testmulti_insert_select.sql.Also it looks like it is not in any test schedule; I suggest adding it to
src/test/regress/multi_schedule
I think I did this way first but "indirection" is really the thing we are trying to workaround and get correct (and move other related test into this one). I can rename though.
| } | ||
|
|
||
| return paramid; | ||
| } |
There was a problem hiding this comment.
Can we have citus_ruleutils.c export one function that can be used by the supported PG ruleutils ? For example, a function:
void
ensure_update_targetlist_in_param_order(List *targetList)
{
bool need_to_sort_target_list = false;
int previous_paramid = 0;
ListCell *l;
foreach(l, targetList)
{
TargetEntry *tle = (TargetEntry *) lfirst(l);
if (!tle->resjunk)
{
int paramid = GetParamId((Node *) tle->expr);
if (paramid < previous_paramid)
{
need_to_sort_target_list = true;
break;
}
previous_paramid = paramid;
}
}
if (need_to_sort_target_list)
list_sort(targetList, target_list_cmp);
}
which is called by each supported get_update_query_targetlist_def() when the parse tree has MULTIEXPR assignments, maybe right after the sublinks have been collected. Basically, to keep the changes to each supported PG ruleutils to a minimum.
There was a problem hiding this comment.
If I'm correct, it's adding a new traversal of the target list, I understand the motivation to reduce the diff, I was trying to not add too much extra processing. I check what is possible.
There was a problem hiding this comment.
Yes there is a maintainability/processing tradeoff, but given that this is at plan-time and most of the time a sort will probably not be necessary the tradeoff for maintainability is worth making, unless there is a compelling counter-case.
| "cannot plan distributed UPDATE SET (..) = (SELECT ...) query when not sorted by physical order"), | ||
| errhint("Sort the columns on the left side by physical order."))); | ||
|
|
||
| previous_attnum = attnum; |
There was a problem hiding this comment.
Currently in the PR PG17 and PG15 ruleutils error out if the target columns are not in attribute order, is this intended to alert users to the problem ?
There was a problem hiding this comment.
Currently in the PR PG17 and PG15 ruleutils error out if the target columns are not in attribute order, is this intended to alert users to the problem ?
It was some tests in this direction: fixing the bug is great, informing users if we detect they have probably been exposed is important IMO.
There was a problem hiding this comment.
Something like:
DefineCustomStringVariable(
"citus.log_pr_error_list",
gettext_noop("Comma-separated list of PR numbers for ERROR trapping"),
gettext_noop("set ERROR when a query enter a path changed by said pull request number"),
&pr_error_list_raw,
"",
PGC_USERSET,
GUC_STANDARD,
NULL,
PRErrorListGucAssignHook,
NULL);There was a problem hiding this comment.
Hm, perhaps this kind of mechanism could be put into a separate PR ? We appreciate the intent, but it may be easier to consider in a separate contribution.
There was a problem hiding this comment.
true, I will work on this new PR so it can be used for this UPDATE SET case.
So after discussing this with the team the preferred approach is to highlight the issue in the release notes and changelog. This is how such issues have been dealt with in the past. Do you have a suggestion/thoughts as to how the condition can be detected ? |
How are identified such issues in the release notes ? Honestly this is not super user friendly behavior to keep that on a minor release messages only and it will probably be complex for users to evaluate if they are concerned or not. |
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.