[Bug Fix] : writing incorrect data to target Merge repartition Command #7659
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7659 +/- ##
===========================================
- Coverage 89.70% 52.71% -36.99%
===========================================
Files 283 283
Lines 60506 60481 -25
Branches 7539 7531 -8
===========================================
- Hits 54276 31884 -22392
- Misses 4073 25772 +21699
- Partials 2157 2825 +668 |
| @@ -361,8 +362,9 @@ CreateAllTargetListForRelation(Oid relationId, List *requiredAttributes) | |||
| } | |||
| else | |||
| { | |||
| int varAttNum = isMergeQuery ? attrNum : colAppendIdx++; | |||
There was a problem hiding this comment.
I understand that there is an off-by-one error in the column values inserted into the table but I really would like understand the Postgres-internals fact that requires us doing something specific for MERGE while it's not needed for other types of commands.
There was a problem hiding this comment.
In Merge Command we should refer actual table attNum but in other cases we are building query like below which refers attnum of a subquery. not the actual table.
(SELECT t.a, NULL, NULL FROM (SELECT a FROM table) t).
That is the reason this is applicable only for merge command.
| @@ -857,8 +857,11 @@ ConvertRelationRTEIntoSubquery(Query *mergeQuery, RangeTblEntry *sourceRte, | |||
| /* set the FROM expression to the subquery */ | |||
| newRangeTableRef->rtindex = SINGLE_RTE_INDEX; | |||
| sourceResultsQuery->jointree = makeFromExpr(list_make1(newRangeTableRef), NULL); | |||
|
|
|||
There was a problem hiding this comment.
requiredAttributes = RequiredAttrNumbersForRelationInternal(mergeQuery, relationRestriction->index);
Here were we getting 1, 3?
|
Likely the problem is stemming from the fact that
but in this case we are selecting column 1 and 3. May be we should change the function to handle
|
|
I think I know what the problem is, can you please do me a small favor Please change the below code to and see how things work |
Yes this works fine !! modified the changes with this function. |
There was a problem hiding this comment.
LGTM, can you please add more test scenarios
In the existing test scenarios, the order of columns in both target and source are matching
INSERT (salary, id) VALUES (source.salary, source.id);
INSERT (id, salary) VALUES (source.id, source.salary);
Can we add tests where order of columns in target and source are not same i.e INSERT target(col1, col3) FROM source(col3, col1)
Semantically they may be incorrect i.e. you may be inserting age into id, but we want to make sure that the "correct columns" are picked up on both sides.
…ragikjain/citus into users/paragjain/mergeWriteFix
We were writing incorrect data to target collection in some cases of merge command. In case of repartition when source query it RELATION. We were referring to incorrect attribute number that was resulting into this incorrect behavior.
Example :
I have added fixed tests as part of this PR , Thanks.