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

backup: Rewrite erroneously expanded SQL stmts on-the-fly #172

Merged
merged 2 commits into from
Dec 11, 2020
Merged

Conversation

cdecker
Copy link
Contributor

@cdecker cdecker commented Dec 1, 2020

Rewrite queries that predate ElementsProject/lightning#4090 to include
the missing whitespace. Doing this on-the-fly seems to have little to
no impact on performance, so let's just do it this way. Has the
advantage of working transparently for the user.

Fixes #158

Copy link
Member

@m-schmoock m-schmoock left a comment

Choose a reason for hiding this comment

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

ACK 5557bef
Kinda overkill for a bug that's already been fixed imho, but why not :D


for i, o in tests:
assert(b._rewrite_stmt(i) == o)

Copy link
Member

Choose a reason for hiding this comment

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

Nit: E302 missing one more blank line :D

(
r'UPDATE outputs SET status=123, reserved_til=1891733WHERE prev_out_tx=1 AND prev_out_index=2',
r'UPDATE outputs SET status=123, reserved_til=1891733 WHERE prev_out_tx=1 AND prev_out_index=2',
),
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you could also test for the second statement: ... peer_id=12345WHERE channels.id...

@cdecker
Copy link
Contributor Author

cdecker commented Dec 1, 2020

ACK 5557bef
Kinda overkill for a bug that's already been fixed imho, but why not :D

The issue is that users still have backups that predate the fix, so we need a way for them to restore those backups. Eventually we can retire this things, but for now it's required.

@cdecker cdecker merged commit 092b295 into master Dec 11, 2020
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.

backup: cli tool failed to restore backup
2 participants