Skip to content
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

bad rewriting of query involving targetList indirection: "UPDATE SET (col2, col1, col3)" #7674

Open
c2main opened this issue Aug 23, 2024 · 1 comment

Comments

@c2main
Copy link
Contributor

c2main commented Aug 23, 2024

A customer reported a bug from development team affecting queries with UPDATE SET (col2, col1, col3) = (.....)
The columns are reordered following their physical ordering in pg_attribute, leading to for example UPDATE SET (col1, col2, col3) = (.....) with the right part, after = unchanged, as a results wrong columns are updated...

I have a complete test case, and a PR which I'm going to link in a moment. I think the issue may affect silently many applications, even more because the behavior can change over time based on the DDL applied on the updated tables.

There are several issue, I'm going to collect what I fond and link here so maybe we can close several at once...

Please see https://github.com/citusdata/citus/actions/runs/10527721147#summary-29171656391 for some examples of the problem.

@c2main
Copy link
Contributor Author

c2main commented Aug 23, 2024

Similar to #4092 which focus on reference table.

c2main added a commit to c2main/citus that referenced this issue Nov 19, 2024
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 added a commit to c2main/citus that referenced this issue Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant