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

bulkio/import: bug when mixing existing and new tables? #50733

Closed
nvanbenschoten opened this issue Jun 27, 2020 · 1 comment · Fixed by #50851
Closed

bulkio/import: bug when mixing existing and new tables? #50733

nvanbenschoten opened this issue Jun 27, 2020 · 1 comment · Fixed by #50851
Assignees
Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@nvanbenschoten
Copy link
Member

I was casually skimming over the code trying to understand #50731 and stumbled upon this block of code:

if len(newTableDescs) != 0 {
res, err := prepareNewTableDescsForIngestion(ctx, txn, p, newTableDescs, importDetails.ParentID)
if err != nil {
return err
}
for i, table := range res {
importDetails.Tables[i] = jobspb.ImportDetails_Table{Desc: table,
Name: details.Tables[i].Name,
SeqVal: details.Tables[i].SeqVal,
IsNew: details.Tables[i].IsNew,
TargetCols: details.Tables[i].TargetCols}
}
}

I couldn't make sense of it, and I now think it's a bug. Specifically, I think the indexing is wrong here, as it doesn't handle the case where len(newTableDescs) < len(importDetails.Tables) correctly. Looking up, we see an unused map newTableDescToIdx, which is exactly what we want here. So I think we're just forgetting to use this map. I think we want something like i := newTableDescToIdx[i] in the loop over res.

I don't know if this is actually possible to hit as a user, as I believe IMPORT only allows new tables and IMPORT INTO only allows existing tables.

@nvanbenschoten nvanbenschoten added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-disaster-recovery labels Jun 27, 2020
@adityamaru
Copy link
Contributor

Thanks for flagging, looking into it.

adityamaru added a commit to adityamaru/cockroach that referenced this issue Jun 30, 2020
There was an indexing bug which would have been triggered if the
resolution of new and old table descriptors was ever mixed within the
same import. This situation has not hit any users because an IMPORT only
allows writing to new tables, while an IMPORT INTO only allows writing
to existing (old) tables, thereby never mixing the two.

This change fixes that bug.

Closes cockroachdb#50733

Release note (bug fix): The indexing when writing resolved table
descriptors to the IMPORT job was fixed, to prevent a future bug from
arising.
adityamaru added a commit to adityamaru/cockroach that referenced this issue Jul 1, 2020
There was an indexing bug which would have been triggered if the
resolution of new and old table descriptors was ever mixed within the
same import. This situation has not hit any users because an IMPORT only
allows writing to new tables, while an IMPORT INTO only allows writing
to existing (old) tables, thereby never mixing the two.

This change fixes that bug.

Closes cockroachdb#50733

Release note: None
adityamaru added a commit to adityamaru/cockroach that referenced this issue Jul 2, 2020
There was an indexing bug which would have been triggered if the
resolution of new and old table descriptors was ever mixed within the
same import. This situation has not hit any users because an IMPORT only
allows writing to new tables, while an IMPORT INTO only allows writing
to existing (old) tables, thereby never mixing the two.

This change fixes that bug.

Closes cockroachdb#50733

Release note: None
craig bot pushed a commit that referenced this issue Jul 9, 2020
50851: importccl: fix bug when mixing old and new tables during import r=adityamaru a=adityamaru

There was an indexing bug which would have been triggered if the
resolution of new and old table descriptors was ever mixed within the
same import. This situation has not hit any users because an IMPORT only
allows writing to new tables, while an IMPORT INTO only allows writing
to existing (old) tables, thereby never mixing the two.

This change fixes that bug.

Closes #50733

Release note: None

Co-authored-by: Aditya Maru <[email protected]>
@craig craig bot closed this as completed in 0d23c26 Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants