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

Feature/299 PIT backups #301

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kidynamit
Copy link
Contributor

fixed the aware vs unaware compare
#299

@@ -70,6 +70,7 @@ def choose_backup(output, recovery_target_time):
match_timestamp = match = None
for backup in backup_list:
last_modified = parse(backup['last_modified'])
last_modified = last_modified.replace(tzinfo=recovery_target_time.tzinfo)
Copy link
Contributor

@CyberDem0n CyberDem0n Feb 13, 2019

Choose a reason for hiding this comment

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

First of all I don't really understand how you managed to get such exception.

  1. Spilo is using UTC timezone.
  2. wal-e backup-list produces last_modified in the format 2019-02-09T01:01:36.000Z
  3. wal-g backup-list produces last_modified in the format 2019-02-09T01:01:36Z

All aforementioned facts will produce datetime objects with tzinfo either tzlocal or tzutc (for AWS S3).
You are changing the timezone in the last_modified timestamp from wal-e/wal-g backup-list output to a timezone from --recovery-target-time. This is undesirable.

Can it be that you passed the CLONE_TARGET_TIME without specifying a timezone?
If you are using GCS could you verify the timestamp format in the backup-list output? Does it have a timezone? If it doesn't, how it relates to UTC? Is it possible to convert it to UTC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are changing the timezone in the last_modified timestamp from wal-e/wal-g backup-list output to a timezone from --recovery-target-time.

That is one of putting it. Another way of putting it is that I am matching the last modified timestamp to match the timezone of the recovery target timestamp for the PIT recovery to work.

Can it be that you passed the CLONE_TARGET_TIME without specifying a timezone?

Refer to issue #299 where I do specify the command I run and particularly the --recovery-target-time flag is set with the timezone specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is one of putting it.

The problem is that for us the last_modified already contains UTC timezone! Therefore it is really important to answer following questions:

  • If you are using GCS could you verify the timestamp format in the backup-list output?
  • Does it (timestamp) have a timezone?
  • If it doesn't, how it relates to UTC?
  • Is it possible to convert it to UTC?`

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 will attempt to perform the same operation with the CLONE_TARGET_TIME set. Then get back to you.

Choose a reason for hiding this comment

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

I am using GCS and am having the same problem as @kidynamit. The last_modified is a datatime object without tzinfo and thus it seems to be a bug in wal-e as they mention in their docs that last_modified should have tzinfo associated with it.

As for your questions @CyberDem0n

  • Format is yyyy-mm-dd hh:mm:ss
  • No, it does not have a timezone
  • Currently, there seems to be no way to relate it to UTC
  • See above, so no.

Maybe we could detect whether last_modified has a tzinfo and on None replace it with UTC? Currently this blocks me from using the Kubernetes Operator in combination with GCS for PITR restores using WAL-E and GCS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nielsmeima I've posted a fix. Turns out there was an expanded size bytes we could use.

@kidynamit
Copy link
Contributor Author

@CyberDem0n I think I fixed it appending the expanded_size_bytes field.

@sshota0809
Copy link

@CyberDem0n

Hi, I hitted this bug today. wal-e backup-list with GCS looks like the following format.

name	last_modified	expanded_size_bytes	wal_segment_backup_start	wal_segment_offset_backup_start	wal_segment_backup_stop	wal_segment_offset_backup_stop
base_00000005000000000000001B_00000040	2021-06-23 01:00:14.498000+00:00		00000005000000000000001B	00000040
base_00000005000000000000001D_00000040	2021-06-24 01:00:10.964000+00:00		00000005000000000000001D	00000040
base_000000060000000000000021_00000040	2021-06-24 08:17:39.684000+00:00		000000060000000000000021	00000040
base_000000060000000000000024_00000040	2021-06-25 01:00:11.216000+00:00		000000060000000000000024	00000040
base_00000006000000000000002B_00000040	2021-06-26 01:00:11.899000+00:00		00000006000000000000002B	00000040
base_00000006000000000000002D_00000040	2021-06-27 01:00:13.629000+00:00		00000006000000000000002D	00000040
base_000000060000000000000030_00000040	2021-06-28 01:00:12.463000+00:00		000000060000000000000030	00000040
base_000000060000000000000032_00000040	2021-06-29 01:00:09.027000+00:00		000000060000000000000032	00000040
base_000000060000000000000037_00000040	2021-06-30 01:00:08.684000+00:00		000000060000000000000037	00000040

And in below code, backup['last_modified'] is just date like 2021-06-23.

last_modified = parse(backup['last_modified'])

I think that commits in this PR looks like CORRECT! So Could you review and merge this?

@jtrouth
Copy link

jtrouth commented Jun 9, 2022

Is there any update on this? This still causes an issue on restores from GCS 3+ years after the PR was opened. Is there anything I can do to get some movement on this?

@ggramal ggramal mentioned this pull request Oct 10, 2023
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.

5 participants