-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
importccl: Add DROP TABLE [IF EXISTS] support for import pgdump. #56920
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @mokaixu)
pkg/ccl/importccl/read_import_pgdump.go, line 511 at r1 (raw file):
} case *tree.DropTable: names := stmt.Names
can we just add a short comment documenting the behavior. If we find a table with the same name in the DB being imported into and public schema then we....else....
pkg/ccl/importccl/read_import_pgdump.go, line 525 at r1 (raw file):
) if err != nil { return errors.Wrapf(err, `drop table "%s" to proceed with import`, tableName)
nit: drop table foo and then retry the import
pkg/ccl/importccl/read_import_pgdump.go, line 910 at r1 (raw file):
return errors.Errorf("unsupported %T statement: %s", i, i) } case *tree.DropTable:
can we just add this to the list of tree.CreateTable etc cause it is "handled during schema extraction"
Previously, whenever a DROP TABLE statement was parsed, an error was thrown and the import would fail since DROP TABLE statements were not supported. Now, when we encounter a DROP TABLE statement for a target table foo, if foo exists, then we throw an error indicating to the user to drop the table foo. Otherwise, if foo does not exist, we silently ignore the DROP statement and proceed with the pgdump import. Release note: None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru)
pkg/ccl/importccl/read_import_pgdump.go, line 511 at r1 (raw file):
Previously, adityamaru (Aditya Maru) wrote…
can we just add a short comment documenting the behavior. If we find a table with the same name in the DB being imported into and public schema then we....else....
done
pkg/ccl/importccl/read_import_pgdump.go, line 525 at r1 (raw file):
Previously, adityamaru (Aditya Maru) wrote…
nit: drop table foo and then retry the import
done
pkg/ccl/importccl/read_import_pgdump.go, line 910 at r1 (raw file):
Previously, adityamaru (Aditya Maru) wrote…
can we just add this to the list of tree.CreateTable etc cause it is "handled during schema extraction"
makes sense, done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru)
bors r=adityamaru |
Build succeeded: |
Previously, whenever a DROP TABLE statement was parsed, an
error was thrown and the import would fail since DROP TABLE
statements were not supported.
Now, when we encounter a DROP TABLE statement for a target table
foo, if foo exists, then we throw an error indicating to the user
to drop the table foo. Otherwise, if foo does not exist, we silently
ignore the DROP statement and proceed with the pgdump import.
Resolves: #53112
Release note: None