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

ADBDEV-6011: Fix restore helper hides actual err #100

Merged
merged 3 commits into from
Aug 7, 2024
Merged

Conversation

RekGRpth
Copy link
Member

@RekGRpth RekGRpth commented Jul 29, 2024

Fix restore helper hides actual err

The commit 4371a94 moved the LoopEnd label so that the err variable began to be
overwritten and it will no longer be used below. To fix it, this patch renames
the err variable returned by flushAndCloseRestoreWriter to errPipe.

The integration test has been adapted to error-free behavior.


Fix restore helper hides actual err

The commit 4371a94 moved the LoopEnd label so that the err

err = errors.Wrap(err, strings.Trim(errBuf.String(), "\x00"))
} else {
err = errors.Wrap(err, "Error copying data")
error began to be overwritten
err = flushAndCloseRestoreWriter(currentPipe, tableOid)
and it will no longer be used below
if err != nil {

To fix it, this patch renames err variable returned by flushAndCloseRestoreWriter to errPipe.

@RekGRpth RekGRpth marked this pull request as ready for review July 29, 2024 11:06
@dkovalev1
Copy link

Looks logical, but what is the scenario when it does matter? Can we reproduce the problem fixed and make a test?

@RekGRpth
Copy link
Member Author

Looks logical, but what is the scenario when it does matter? Can we reproduce the problem fixed and make a test?

Problem was found theoretically

@red1452
Copy link

red1452 commented Jul 30, 2024

Change the commit message:

The commit https://github.com/arenadata/gpbackup/commit/4371a94305a7a77ce3e6990a9691a3986d9426d9 moved the LoopEnd label so that the err variable began to be
overwritten and it will no longer be used below. To fix it, this patch renames
the err variable returned by flushAndCloseRestoreWriter to errPipe.

@RekGRpth
Copy link
Member Author

Change the commit message:

The commit https://github.com/arenadata/gpbackup/commit/4371a94305a7a77ce3e6990a9691a3986d9426d9 moved the LoopEnd label so that the err variable began to be
overwritten and it will no longer be used below. To fix it, this patch renames
the err variable returned by flushAndCloseRestoreWriter to errPipe.

changed

red1452
red1452 previously approved these changes Jul 30, 2024
@dkovalev1
Copy link

Looks logical, but what is the scenario when it does matter? Can we reproduce the problem fixed and make a test?

Problem was found theoretically

Can you at least explain when the problem happens and which cases to patch solves? If it's not possible to supply a test.

@RekGRpth
Copy link
Member Author

Looks logical, but what is the scenario when it does matter? Can we reproduce the problem fixed and make a test?

Problem was found theoretically

Can you at least explain when the problem happens and which cases to patch solves? If it's not possible to supply a test.

test was added

@RekGRpth RekGRpth requested a review from red1452 August 1, 2024 07:45
The commit 4371a94 moved the LoopEnd label so that the err variable began to be
overwritten and it will no longer be used below. To fix it, this patch renames
the err variable returned by flushAndCloseRestoreWriter to errPipe.
Base automatically changed from 1.30.5-sync to master August 2, 2024 06:41
@Stolb27 Stolb27 dismissed red1452’s stale review August 2, 2024 06:41

The base branch was changed.

Copy link

@dkovalev1 dkovalev1 left a comment

Choose a reason for hiding this comment

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

This change seems logical and allows gpbackup to report error. However, the test system is far from parfect. Tests are very difficult to read, trace and understand.

@RekGRpth RekGRpth merged commit 395c819 into master Aug 7, 2024
5 checks passed
@RekGRpth RekGRpth deleted the ADBDEV-6011 branch August 7, 2024 02:53
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