Expand target entries with merged array/jsonb subscripting ops into multiple ones #5692

onurctirtir · 2022-02-07T17:29:19Z

DESCRIPTION: Fixes a bug that causes incorrect evaluation of UPDATE/UPSERT commands over array/jsonb columns

When re-writing query tree, postgres combines multiple subscripting
operators referencing to the same attribute into a single TargetEntry
by linking SubscriptingRef objects to each other via their refexpr
fields. (See rewriteTargetListIU & process_matched_tle functions.)

However, ruleutils function get_update_query_targetlist_def doesn't
know how to properly deparse such a TargetEntry. As a result, we were
only taking the last set-by-subscript operation into account when
generating the shard query for such an UPDATE command.

In postgres, this doesn't cause any problems (e.g.: when generating
definition of a rule based object) since the query-rewrite
transformations aren't performed on the query tree that
get_update_query_targetlist_def is expected to process.

For this reason; with this commit, before processing the target entry
list in our ruleutils based deparser, we first expand such target
entries into multiple ones.

To detect such SubscriptingRef objects, we also need to investigate
FieldStore and CoerceToDomain objects as postgres functions
processing SubscriptingRef objects do --although they do so for
different purposes. However, given that Citus already doesn't allow
INSERT/UPDATE via FieldStore, we only do that for CoerceToDomain
objects.

Reproducible (at least) for all versions >= 9.5.

onderkalaci · 2022-02-08T10:28:04Z

When re-writing query tree, postgres combines multiple subscripting
operators referencing to the same attribute into a single TargetEntry
by linking SubscriptingRef objects to each other via their refexpr
fields. (See rewriteTargetListIU & process_matched_tle functions.)

Don't we use originalQuery while deparsing? Why is that impacted by re-writing?

onurctirtir · 2022-02-08T11:17:30Z

When re-writing query tree, postgres combines multiple subscripting
operators referencing to the same attribute into a single TargetEntry
by linking SubscriptingRef objects to each other via their refexpr
fields. (See rewriteTargetListIU & process_matched_tle functions.)

Don't we use originalQuery while deparsing? Why is that impacted by re-writing?

Yes, we use the original query tree but the rewrite transformations happen way before, meaning that the query tree that we later pass down to the executor is the query tree that postgres already applied such transformations, i.e.: parse argument passed to distributed_planner.

src/backend/distributed/deparser/citus_ruleutils.c

…ultiple ones When re-writing query tree, postgres combines multiple subscripting operators referencing to the same attribute into a single `TargetEntry` by linking `SubscriptingRef` objects to each other via their `refexpr` fields. (See `rewriteTargetListIU` & `process_matched_tle` functions.) However, ruleutils function `get_update_query_targetlist_def` doesn't know how to properly deparse such a `TargetEntry`. As a result, we were only taking the last set-by-subscript operation into account when generating the shard query for such an `UPDATE` command. In postgres, this doesn't cause any problems (e.g.: when generating definition of a rule based object) since the query-rewrite transformations aren't performed on the query tree that `get_update_query_targetlist_def` is expected to process. For this reason; with this commit, before processing the target entry list in our ruleutils based deparser, we first expand such target entries into multiple ones. To detect such `SubscriptingRef` objects, we also need to investigate `FieldStore` and `CoerceToDomain` objects as postgres functions processing `SubscriptingRef` objects do --although they do so for different purposes. However, given that Citus already doesn't allow `INSERT/UPDATE` via `FieldStore`, we only do that for `CoerceToDomain` objects.

onurctirtir · 2022-02-09T10:10:44Z

src/backend/distributed/deparser/citus_ruleutils.c

+			ereport(ERROR, (errmsg("unexpectedly got FieldStore object when "
+								   "generating shard query")));


Indeed we have a test for that now, but seems that codecov did't get updated yet.

ummmm, somehow we might be telling codecov to skip files under '/test/

src/backend/distributed/utils/listutils.c

codecov · 2022-02-14T20:13:55Z

Codecov Report

Merging #5692 (108f537) into master (333bcc7) will increase coverage by 0.18%.
The diff coverage is 97.82%.

❗ Current head 108f537 differs from pull request most recent head 6fa965c. Consider uploading reports for the commit 6fa965c to get more accurate results

@@            Coverage Diff             @@
##           master    #5692      +/-   ##
==========================================
+ Coverage   92.60%   92.79%   +0.18%     
==========================================
  Files         227      223       -4     
  Lines       47376    47382       +6     
==========================================
+ Hits        43874    43968      +94     
+ Misses       3502     3414      -88

…older versions

onurctirtir · 2022-03-09T11:20:28Z

Looks like after the changes from #5728 has been merged, most of the tests added in this pr started failing since we don't yet support DOMAIN objects.

onderkalaci · 2022-03-09T12:58:37Z

Looks like after the changes from #5728 has been merged, most of the tests added in this pr started failing since we don't yet support DOMAIN objects.

that could land into Citus 11 via #5764

onurctirtir · 2022-03-09T16:05:12Z

Looks like after the changes from #5728 has been merged, most of the tests added in this pr started failing since we don't yet support DOMAIN objects.

that could land into Citus 11 via #5764

Dropped DOMAIN tests, but we should re-land those tests into subscripting_op.sql after merging #5764.

https://gist.github.com/onurctirtir/d6c350a78e03d9044a2bf3c72f0f39f8

onderkalaci

I think I'm mostly on-board with the changes, still want to have one more look to the approach. And, couldn't dive into the details of the code yet, but seems fine on the high-level

onderkalaci · 2022-03-09T15:05:53Z

src/test/regress/expected/pg14.out

+(1 row)
+
+INSERT INTO nested_obj_update VALUES
+  (1, '{"a": [1,2,3], "b": [4,5,6], "c": [7,8,9], "d": [1,2,1,2]}', '4'),


maybe add RETURNING as well:

RETURNING (data['a'], data['b'][0]);

onderkalaci · 2022-03-09T15:12:44Z

src/test/regress/expected/pg14.out

@@ -1272,6 +1272,83 @@ SELECT count(*) FROM
     1
 (1 row)

+CREATE TABLE jsonb_subscript_update (id INT, data JSONB);


seems like a great candidate for adding to arbitrary configs so that we ensure the results are the same with PG/?

onderkalaci · 2022-03-09T15:27:32Z

src/test/regress/expected/pg14.out

+         data['a']::NUMERIC + 3 AS d
+  FROM jsonb_subscript_update
+) updated_vals
+WHERE jsonb_subscript_update.id = updated_vals.id;


when I add AND jsonb_subscript_update.id = 1 we seem to fail to update properly.

UPDATE jsonb_subscript_update SET data['b'] = updated_vals.b::TEXT::jsonb, data['c'] = updated_vals.c::TEXT::jsonb, data['d'] = updated_vals.d::TEXT::jsonb FROM ( SELECT id, data['a'] AS a, data['a']::NUMERIC + 1 AS b, data['a']::NUMERIC + 2 AS c, data['a']::NUMERIC + 3 AS d FROM jsonb_subscript_update ) updated_vals WHERE jsonb_subscript_update.id = updated_vals.id AND jsonb_subscript_update.id = 1; ┌────┬──────────────────┐ │ id │ data │ ├────┼──────────────────┤ │ 1 │ {"a": 1, "d": 4} │ │ 2 │ {"a": 2} │ └────┴──────────────────┘ (2 rows)

Router queries might use a different code-path for deparsing the queries.

onderkalaci · 2022-03-09T15:39:13Z

src/test/regress/expected/pg14.out

+
+INSERT INTO jsonb_subscript_update VALUES (1, '{"a": 1}'), (2, '{"a": 2}');
+UPDATE jsonb_subscript_update
+SET data['b'] = updated_vals.b::TEXT::jsonb,


maybe also insert one more row with 2 levels of subscription:

INSERT INTO jsonb_subscript_update VALUES (1, '{"a": {"c":20, "d" : 200}}'), (2, '{"a": {"d":10, "c" : 100}}');

And use

UPDATE jsonb_subscript_update SET DATA['a']['c'] = (updated_vals.d + updated_vals.a::NUMERIC)::TEXT::JSONB FROM (SELECT id, DATA['a']['c'] AS a, DATA['a']['c']::NUMERIC + 1 AS b, DATA['a']['c']::NUMERIC + 2 AS c, DATA['a']['d']::NUMERIC + 3 AS d FROM jsonb_subscript_update) updated_vals WHERE jsonb_subscript_update.id = updated_vals.id;

onderkalaci · 2022-03-09T16:08:47Z

src/backend/distributed/deparser/citus_ruleutils.c

+
+
+/*
+ * ExpandMergedSubscriptingRefEntries takes a list of target entries and expands


lets document the PG functions that merge these together

onderkalaci · 2022-03-09T16:12:57Z

src/backend/distributed/deparser/ruleutils_12.c

@@ -3324,6 +3324,8 @@ get_update_query_targetlist_def(Query *query, List *targetList,
 	SubLink    *cur_ma_sublink;
 	List	   *ma_sublinks;

+    targetList = ExpandMergedSubscriptingRefEntries(targetList);


So, we call this for PG 12/13 as well. Why? Do we have the same bug for arrays on the earlier versions? Do we have tests for that?

onderkalaci · 2022-03-09T16:15:30Z

src/backend/distributed/deparser/ruleutils_14.c

@@ -3439,6 +3439,8 @@ get_update_query_targetlist_def(Query *query, List *targetList,
 	SubLink    *cur_ma_sublink;
 	List	   *ma_sublinks;

+    targetList = ExpandMergedSubscriptingRefEntries(targetList);


maybe don't change the input:

targetList = ExpandMergedSubscriptingRefEntries(list_copy(targetList));

onderkalaci · 2022-03-09T16:17:31Z

src/backend/distributed/utils/listutils.c

+List *
+list_reverse(const List *list)
+{
+#if PG_VERSION_NUM >= PG_VERSION_13


can we use something simpler for all versions?

List *reversedSubXactStates = NIL; /* * activeSubXactContexts is in reversed temporal order, so we reverse it to get it * in temporal order. */ SubXactContext *state = NULL; foreach_ptr(state, activeSubXactContexts) { reversedSubXactStates = lcons(state, reversedSubXactStates); } return reversedSubXactStates;

onurctirtir force-pushed the fix/subscript-jsonb branch 3 times, most recently from 2dc56db to 12aa54d Compare February 8, 2022 10:08

onurctirtir force-pushed the fix/subscript-jsonb branch 2 times, most recently from f410eab to 9914df2 Compare February 8, 2022 12:36

onurctirtir commented Feb 8, 2022

View reviewed changes

src/backend/distributed/deparser/citus_ruleutils.c Outdated Show resolved Hide resolved

onurctirtir marked this pull request as ready for review February 8, 2022 14:10

onurctirtir force-pushed the fix/subscript-jsonb branch from a2c4c06 to dc29396 Compare February 9, 2022 09:53

citusdata deleted a comment from codecov bot Feb 9, 2022

onurctirtir added the backport label Feb 9, 2022

onurctirtir requested a review from onderkalaci February 9, 2022 10:09

onurctirtir commented Feb 9, 2022

View reviewed changes

src/backend/distributed/utils/listutils.c Show resolved Hide resolved

Merge branch 'master' into fix/subscript-jsonb

9f27c20

onurctirtir added 4 commits February 15, 2022 00:04

optimize list_reverse for pg < 13 since we might backport the fix to …

af529d3

…older versions

Merge branch 'master' into fix/subscript-jsonb

8e60732

Merge branch 'master' into fix/subscript-jsonb

108f537

Merge branch 'master' into fix/subscript-jsonb

6fa965c

drop domain tests

4a4ce71

Merge branch 'master' into fix/subscript-jsonb

c63551c

onderkalaci reviewed Mar 9, 2022

View reviewed changes

Merge branch 'master' into fix/subscript-jsonb

bb294f7

naisila mentioned this pull request Nov 21, 2024

WIP Fix issue #7674 about UPDATE SET(..), with indirection #7675

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand target entries with merged array/jsonb subscripting ops into multiple ones #5692

Expand target entries with merged array/jsonb subscripting ops into multiple ones #5692

onurctirtir commented Feb 7, 2022 •

edited

Loading

onderkalaci commented Feb 8, 2022

onurctirtir commented Feb 8, 2022

onurctirtir Feb 9, 2022

onurctirtir Feb 14, 2022

codecov bot commented Feb 14, 2022 •

edited

Loading

onurctirtir commented Mar 9, 2022

onderkalaci commented Mar 9, 2022

onurctirtir commented Mar 9, 2022

onderkalaci left a comment

onderkalaci Mar 9, 2022

onderkalaci Mar 9, 2022

onderkalaci Mar 9, 2022

onderkalaci Mar 9, 2022

onderkalaci Mar 9, 2022

onderkalaci Mar 9, 2022

onderkalaci Mar 9, 2022

onderkalaci Mar 9, 2022

		ereport(ERROR, (errmsg("unexpectedly got FieldStore object when "
		"generating shard query")));



		/*
		* ExpandMergedSubscriptingRefEntries takes a list of target entries and expands

Expand target entries with merged array/jsonb subscripting ops into multiple ones #5692

Are you sure you want to change the base?

Expand target entries with merged array/jsonb subscripting ops into multiple ones #5692

Conversation

onurctirtir commented Feb 7, 2022 • edited Loading

onderkalaci commented Feb 8, 2022

onurctirtir commented Feb 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2022 • edited Loading

Codecov Report

onurctirtir commented Mar 9, 2022

onderkalaci commented Mar 9, 2022

onurctirtir commented Mar 9, 2022

onderkalaci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurctirtir commented Feb 7, 2022 •

edited

Loading

codecov bot commented Feb 14, 2022 •

edited

Loading