Skip to content

Conversation

@jsilvela
Copy link

Closes #167

Avoid the trailing Z in RFC3339 / ISO 8601 in UTC zone, which does not play as a recovery target time.

@jsilvela jsilvela requested a review from a team as a code owner October 29, 2025 16:25
@jsilvela jsilvela changed the title fix: avoid trailing Z when converting RFC3339 timestamps for postgres fix: avoid trailing Z when converting RFC3339 timestamps in UTC for postgres Oct 29, 2025
// to avoid problems when used for recovery_target_time
func ConvertToPostgresFormat(timestamp string) string {
formatWithoutZ := func(t time.Time) string {
formatted := t.Format("2006-01-02 15:04:05.000000Z07:00")
Copy link
Author

@jsilvela jsilvela Oct 29, 2025

Choose a reason for hiding this comment

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

Here, questions. In the ParseTargetTime function in pkg/types/time.go , we make use of pq.ParseTimestamp().

Why not use pq.FormatTimestamp() here, regardless of input formats?
Moreover, why not validate the input in this function? We validated RFC3339 and RFC3339Micro, but standalone, this function, given "foo", will assume it's a valid timestamp and just return it.

@jsilvela
Copy link
Author

There is an E2E test exercising the conversion function: cloudnative-pg/cloudnative-pg#8981

@mnencia
Copy link
Member

mnencia commented Dec 23, 2025

Regarding pq.FormatTimestamp():

I tested it and it still outputs Z for UTC times:

UTC: 2023-07-06 08:00:39Z
+03:00: 2023-07-06 08:00:39+03:00

So using pq.FormatTimestamp alone wouldn't solve the recovery_target_time issue - we'd still need to replace Z with +00:00.

Additionally, pq.ParseTimestamp does not accept RFC3339 format (with T):

2023-07-06T08:00:39Z    → ERROR: expected '32' at position 10; got '84'
2023-07-06 08:00:39     → 2023-07-06 08:00:39Z  (works)

So we need Go's time.Parse with RFC3339 layouts to convert from the user's input format.

Regarding validation:

You're right that given "foo", the function just returns it unchanged. The docstring says "It does not validate its input" - this is intentional to allow PostgreSQL-format strings to pass through unchanged.

However, I've pushed two additional commits that improve this:

  1. ff8453b - Use +00:00 instead of +00 for format consistency
  2. 914e886 - Handle RFC3339-like format without timezone ("2023-07-06T08:00:39") which ParseTargetTime accepts but was passing through unchanged

If we want stricter validation (return error for invalid input), that would be a breaking API change. We could consider it for a future version.

mnencia
mnencia previously approved these changes Dec 24, 2025
@jsilvela
Copy link
Author

Thanks Marco, that makes good sense. Good changes.
My Christmas gift. Thanks :)
Have a great one too.

@mnencia
Copy link
Member

mnencia commented Dec 24, 2025

Hey @jsilvela! Merry Christmas and Happy New Year to you too!

This PR has been expanded to address a related consistency issue discovered while investigating.

The original fix avoids the trailing Z for UTC timestamps, but there was also an inconsistency with RFC3339-like timestamps without timezone (e.g., 2023-07-06T08:00:39). ParseTargetTime (used by barman-cloud for backup selection) interpreted them as UTC, while ConvertToPostgresFormat output them without timezone, letting PostgreSQL use server local time. This could cause backup selection and recovery to target different points in time if the server wasn't in UTC.

Now both functions consistently treat RFC3339-like timestamps without timezone as UTC, and ConvertToPostgresFormat outputs them with explicit +00:00. The docstrings have also been updated to document this behavior.

Related PRs: cloudnative-pg/cloudnative-pg#8937 and cloudnative-pg/plugin-barman-cloud#700

jsilvela and others added 4 commits December 24, 2025 16:47
Use +00:00 instead of +00 for UTC timezone offset to maintain
consistency with non-UTC timestamps which use the full offset
format (e.g., +03:00).

Signed-off-by: Marco Nenciarini <[email protected]>
Add support for converting timestamps in RFC3339-like format without
timezone (e.g., "2023-07-06T08:00:39") to PostgreSQL format. This
format is accepted by ParseTargetTime in pkg/types/time.go but was
previously passed through unchanged, causing PostgreSQL to reject
the timestamp due to the 'T' separator.

Signed-off-by: Marco Nenciarini <[email protected]>
RFC3339-like timestamps without timezone (e.g., "2006-01-02T15:04:05")
are now interpreted as UTC and output with explicit +00:00 suffix.

This ensures consistency between:
- ParseTargetTime (used by barman-cloud for backup selection)
- ConvertToPostgresFormat (used for PostgreSQL recovery_target_time)

Both functions now treat such timestamps as UTC, avoiding potential
timezone mismatches where backup selection and recovery could target
different points in time.

Signed-off-by: Marco Nenciarini <[email protected]>
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.

[BUG]: timestamp conversion for recovery target is not valid for UTC times

2 participants