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

Reporting the GIST / BRIN / SPGIST index methods in analyze-schema using pg-parser and also adding them in assessment report #1734

Merged
merged 10 commits into from
Oct 9, 2024

Conversation

priyanshi-yb
Copy link
Contributor

@priyanshi-yb priyanshi-yb commented Oct 6, 2024

Description-

  1. Replace the logic of reporting the GIST / BRIN / SPGIST index methods using pg-parser.
  2. Reporting the BRIN and SPGIST in the assessment report. https://yugabyte.atlassian.net/browse/DB-12603
  3. Classifying the ALTER FOREIGN TABLE for Default constraint case in foreign_table.sql. https://yugabyte.atlassian.net/browse/DB-12734

Jenkins:
https://jenkins.dev.yugabyte.com/job/users/job/yb-voyager-testing/job/yb-voyager-testing-pipeline/3631/
https://jenkins.dev.yugabyte.com/job/users/job/yb-voyager-testing/job/yb-voyager-testing-pipeline/3633/

@priyanshi-yb priyanshi-yb marked this pull request as ready for review October 7, 2024 12:04
@@ -41,6 +29,18 @@ OPTIONS (
table_name 't'
);

/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did this move down, suddenly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we are now putting the ALTER FOREIGN TABLE in foreign_table.sql at the end. it will fail after the CREATE FOREIGN TABLE now. so order of that in this file matters.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but the filename still says table.sql below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I didn't change it as we ignore the file name while validating these files I just moved it as is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh what do we use it for then?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's just there, and the file path can vary that's why it's not getting validated. cc: @shubham-yb

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

yb-voyager/cmd/analyzeSchema.go Outdated Show resolved Hide resolved
yb-voyager/cmd/analyzeSchema.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@makalaaneesh makalaaneesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please verify that the docs links are correct, for the new issues and for the changes you made as well.

@priyanshi-yb priyanshi-yb merged commit c88186e into main Oct 9, 2024
16 checks passed
@priyanshi-yb priyanshi-yb deleted the priyanshi/report-other-gist-methods branch October 9, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants