mirror of https://github.com/citusdata/citus.git
e4ce4d184e
DESCRIPTION: Fixes a bug that causes allowing UPDATE / MERGE queries
that may change the distribution column value.
Fixes: #8087.
Probably as of #769, we were not properly checking if UPDATE
may change the distribution column.
In #769, we had these checks:
```c
if (targetEntry->resno != column->varattno)
{
/* target entry of the form SET some_other_col = <x> */
isColumnValueChanged = false;
}
else if (IsA(setExpr, Var))
{
Var *newValue = (Var *) setExpr;
if (newValue->varattno == column->varattno)
{
/* target entry of the form SET col = table.col */
isColumnValueChanged = false;
}
}
```
However, what we check in "if" and in the "else if" are not so
different in the sense they both attempt to verify if SET expr
of the target entry points to the attno of given column. So, in
Also see this PR comment from #5220:
https://github.com/citusdata/citus/pull/5220#discussion_r699230597.
In #769, probably we actually wanted to first check whether both
SET expr of the target entry and given variable are pointing to the
same range var entry, but this wasn't what the "if" was checking,
so removed.
As a result, in the cases that are mentioned in the linked issue,
we were incorrectly concluding that the SET expr of the target
entry won't change given column just because it's pointing to the
same attno as given variable, regardless of what range var entries
the column and the SET expr are pointing to. Then we also started
using the same function to check for such cases for update action
of MERGE, so we have the same bug there as well.
So with this PR, we properly check for such cases by comparing
varno as well in TargetEntryChangesValue(). However, then some of
the existing tests started failing where the SET expr doesn't
directly assign the column to itself but the "where" clause could
actually imply that the distribution column won't change. Even before
we were not attempting to verify if "where" cluse quals could imply a
no-op assignment for the SET expr in such cases but that was not a
problem. This is because, for the most cases, we were always qualifying
such SET expressions as a no-op update as long as the SET expr's
attno is the same as given column's. For this reason, to prevent
regressions, this PR also adds some extra logic as well to understand
if the "where" clause quals could imply that SET expr for the
distribution key is a no-op.
Ideally, we should instead use "relation restriction equivalence"
mechanism to understand if the "where" clause implies a no-op
update. This is because, for instance, right now we're not able to
deduce that the update is a no-op when the "where" clause transitively
implies a no-op update, as in the case where we're setting "column a"
to "column c" and where clause looks like:
"column a = column b AND column b = column c".
If this means a regression for some users, we can consider doing it
that way. Until then, as a workaround, we can suggest adding additional
quals to "where" clause that would directly imply equivalence.
Also, after fixing TargetEntryChangesValue(), we started successfully
deducing that the update action is a no-op for such MERGE queries:
```sql
MERGE INTO dist_1
USING dist_1 src
ON (dist_1.a = src.b)
WHEN MATCHED THEN UPDATE SET a = src.b;
```
However, we then started seeing below error for above query even
though now the update is qualified as a no-op update:
```
ERROR: Unexpected column index of the source list
```
This was because of #8180 and #8201 fixed that.
In summary, with this PR:
* We disallow such queries,
```sql
-- attno for dist_1.a, dist_1.b: 1, 2
-- attno for dist_different_order_1.a, dist_different_order_1.b: 2, 1
UPDATE dist_1 SET a = dist_different_order_1.b
FROM dist_different_order_1
WHERE dist_1.a dist_different_order_1.a;
-- attno for dist_1.a, dist_1.b: 1, 2
-- but ON (..) doesn't imply a no-op update for SET expr
MERGE INTO dist_1
USING dist_1 src
ON (dist_1.a = src.b)
WHEN MATCHED THEN UPDATE SET a = src.a;
```
* .. and allow such queries,
```sql
MERGE INTO dist_1
USING dist_1 src
ON (dist_1.a = src.b)
WHEN MATCHED THEN UPDATE SET a = src.b;
```
(cherry picked from commit
|
||
|---|---|---|
| .. | ||
| commands | ||
| metadata | ||
| utils | ||
| adaptive_executor.h | ||
| argutils.h | ||
| backend_data.h | ||
| background_jobs.h | ||
| cancel_utils.h | ||
| causal_clock.h | ||
| citus_acquire_lock.h | ||
| citus_clauses.h | ||
| citus_custom_scan.h | ||
| citus_depended_object.h | ||
| citus_nodefuncs.h | ||
| citus_nodes.h | ||
| citus_ruleutils.h | ||
| citus_safe_lib.h | ||
| colocation_utils.h | ||
| combine_query_planner.h | ||
| commands.h | ||
| connection_management.h | ||
| coordinator_protocol.h | ||
| cte_inline.h | ||
| deparse_shard_query.h | ||
| deparser.h | ||
| directed_acyclic_graph_execution.h | ||
| distributed_deadlock_detection.h | ||
| distributed_execution_locks.h | ||
| distributed_planner.h | ||
| distribution_column.h | ||
| enterprise.h | ||
| error_codes.h | ||
| errormessage.h | ||
| executor_util.h | ||
| extended_op_node_utils.h | ||
| foreign_key_relationship.h | ||
| function_call_delegation.h | ||
| function_utils.h | ||
| hash_helpers.h | ||
| insert_select_executor.h | ||
| insert_select_planner.h | ||
| intermediate_result_pruning.h | ||
| intermediate_results.h | ||
| jsonbutils.h | ||
| listutils.h | ||
| local_distributed_join_planner.h | ||
| local_executor.h | ||
| local_multi_copy.h | ||
| local_plan_cache.h | ||
| locally_reserved_shared_connections.h | ||
| lock_graph.h | ||
| log_utils.h | ||
| maintenanced.h | ||
| memutils.h | ||
| merge_executor.h | ||
| merge_planner.h | ||
| metadata_cache.h | ||
| metadata_sync.h | ||
| metadata_utility.h | ||
| multi_executor.h | ||
| multi_explain.h | ||
| multi_join_order.h | ||
| multi_logical_optimizer.h | ||
| multi_logical_planner.h | ||
| multi_logical_replication.h | ||
| multi_partitioning_utils.h | ||
| multi_physical_planner.h | ||
| multi_progress.h | ||
| multi_router_planner.h | ||
| multi_server_executor.h | ||
| namespace_utils.h | ||
| param_utils.h | ||
| pg_dist_background_job.h | ||
| pg_dist_background_task.h | ||
| pg_dist_backrgound_task_depend.h | ||
| pg_dist_cleanup.h | ||
| pg_dist_colocation.h | ||
| pg_dist_local_group.h | ||
| pg_dist_node.h | ||
| pg_dist_node_metadata.h | ||
| pg_dist_partition.h | ||
| pg_dist_placement.h | ||
| pg_dist_rebalance_strategy.h | ||
| pg_dist_schema.h | ||
| pg_dist_shard.h | ||
| pg_dist_transaction.h | ||
| placement_access.h | ||
| placement_connection.h | ||
| priority.h | ||
| query_colocation_checker.h | ||
| query_pushdown_planning.h | ||
| query_stats.h | ||
| query_utils.h | ||
| recursive_planning.h | ||
| reference_table_utils.h | ||
| relation_access_tracking.h | ||
| relation_restriction_equivalence.h | ||
| relation_utils.h | ||
| relay_utility.h | ||
| remote_commands.h | ||
| remote_transaction.h | ||
| repartition_executor.h | ||
| repartition_join_execution.h | ||
| replicate_none_dist_table_shard.h | ||
| replication_origin_session_utils.h | ||
| resource_lock.h | ||
| run_from_same_connection.h | ||
| shard_cleaner.h | ||
| shard_pruning.h | ||
| shard_rebalancer.h | ||
| shard_split.h | ||
| shard_transfer.h | ||
| shard_utils.h | ||
| shardinterval_utils.h | ||
| shardsplit_logical_replication.h | ||
| shardsplit_shared_memory.h | ||
| shared_connection_stats.h | ||
| shared_library_init.h | ||
| statistics_collection.h | ||
| string_utils.h | ||
| subplan_execution.h | ||
| task_execution_utils.h | ||
| tdigest_extension.h | ||
| tenant_schema_metadata.h | ||
| time_constants.h | ||
| transaction_identifier.h | ||
| transaction_management.h | ||
| transaction_recovery.h | ||
| transmit.h | ||
| tuple_destination.h | ||
| tuplestore.h | ||
| type_utils.h | ||
| version_compat.h | ||
| worker_create_or_replace.h | ||
| worker_log_messages.h | ||
| worker_manager.h | ||
| worker_protocol.h | ||
| worker_shard_copy.h | ||
| worker_shard_visibility.h | ||
| worker_transaction.h | ||