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

Update handling of unlogged tables as per 2024.2 #2074

Merged
merged 18 commits into from
Dec 16, 2024

Conversation

makalaaneesh
Copy link
Collaborator

@makalaaneesh makalaaneesh commented Dec 13, 2024

Describe the changes in this pull request

  • Specify MinVersionFixedIn = 2024.2.0.0
  • If issue is detected, but filtered out because of target-db-version, add a note in assessment report.
  • Propagate NOTICE client messages to stdout in import-schema (with the exception of the ALTER TABLE "table rewrite will cause inconsistencies" notice)
  • Moved some functions from importData.go to importSchemaYugabyteDB.go
  • Upgrade pgx from v4 to v5 in importSchemaYUgabyteDB.go

Describe if there are any user-facing changes

import-schema output will now have the NOTICEs printed out if received from psql.
image

How was this pull request tested?

  • Added/fixed unit test cases

Does your PR have changes that can cause upgrade issues?

Component Breaking changes?
MetaDB Yes/No
Name registry json Yes/No
Data File Descriptor Json Yes/No
Export Snapshot Status Json Yes/No
Import Data State Yes/No
Export Status Json Yes/No
Data .sql files of tables Yes/No
Export and import data queue Yes/No
Schema Dump Yes/No
AssessmentDB Yes/No
Sizing DB Yes/No
Migration Assessment Report Json Yes/No
Callhome Json Yes/No
YugabyteD Tables Yes/No
TargetDB Metadata Tables Yes/No

@makalaaneesh makalaaneesh marked this pull request as ready for review December 13, 2024 08:28
assert.NoError(t, err)

defer conn.Close(context.Background())
_, err = conn.Exec(ctx, "CREATE UNLOGGED TABLE unlogged_table (a int)")
// in 2024.2, UNLOGGED no longer throws an error, just a notice
if noticeFound {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you have changed the assertion to assert based on target-db-version if its fixed or not, but should we also keep the notice found check also?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't see the point of keeping the notice check around. we are anyway printing all notices for all DDLs that are run. So we just need to now test that it does not throw any error. RIght?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, okay.

yb-voyager/cmd/importSchemaYugabyteDB.go Show resolved Hide resolved
Copy link
Contributor

@priyanshi-yb priyanshi-yb left a comment

Choose a reason for hiding this comment

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

LGTM!

assert.NoError(t, err)

defer conn.Close(context.Background())
_, err = conn.Exec(ctx, "CREATE UNLOGGED TABLE unlogged_table (a int)")
// in 2024.2, UNLOGGED no longer throws an error, just a notice
if noticeFound {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, okay.

@makalaaneesh makalaaneesh merged commit dd61428 into main Dec 16, 2024
42 checks passed
@makalaaneesh makalaaneesh deleted the aneesh/unlogged-tables branch December 16, 2024 05:51
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.

2 participants