Avoid timestamptz subtraction for span and event start offsets #1047

mattdailis · 2023-07-31T15:54:50Z

Tickets addressed: Resolve inconsistencies when setting Postgres intervals in DB #1045
Review: By commit
Merge strategy: Merge (no squash)

Description

Postgres stores intervals in tuples of (months, days, microseconds) (docs)
The first two are primarily needed for calendrical durations. For example, January 1st -> February 1st -> March 1st is 2 months. Noon on Sunday + 1 day is Noon on Monday, even if that crosses a daylight savings boundary (so a day can be 23, 24, or 25 hours, depending on when it is)

On Aerie, to the extent possible, we want to use the microseconds part of that tuple. This is primarily to canonicalize the representation of intervals in Aerie, to avoid the multiple representations 24:00:00.000 vs 24 days.

This PR updates the PostSpansAction (renamed to InsertSpansAction to mirror database parlance) to write durations using PreparedStatements.setDuration rather than by doing subtraction between two timestamps. The primary purpose of this change is to avoid start_offsets that mix the days and microseconds portions of the interval type.

Aside:
I found the code that implements subtraction between two timestamptzs (it happens to be the same code to subtract regular timestamps). At the bottom, it calls interval_justify_hours (code here).

As far as I can tell, it:

Does the subtraction at the microsecond level, and gets the exact difference in microseconds

“Justifies” it so that the microsecond portion is less than 24 hours, by incrementing the day portion as much as possible

Seconds aside:
Another reason is to avoid calendrical durations, which could cause issues if we were to apply Aerie durations to calendrical time systems. In Aerie, we generally stick to UTC, and do not have a good story for leap seconds, so we can make the claim that days are always 24 hours long. However, it is not out of the question that one might overlay activity times on a more human time scale, e.g. operational meeting times. Using the microsecond representation guarantees that the offsets are correct even in the presence of daylight savings time or leap seconds.

Verification

I built Aerie on develop, ran simulation and saw that it inserted mixed days/microseconds intervals into the span.start_offset column. I then switched to this branch, rebuilt, and saw that it inserted only microsecond-based intervals.

Documentation

We definitely need a document on "Time in Aerie", since this is a topic that comes up often. I'll make a documentation ticket.

NASA-AMMOS/aerie-docs#66

Future work

Consider how to avoid the non-microseconds portions of the postgres interval type. Perhaps a domain type, or check constraints could help here, or moving to a different representation of offsets altogether.

…ubtraction

Mythicaeda

General question with this PR: Are we trying to avoid using days and micros for an Aerie reason? The PR mentions accuracy concerns at the start, but based on how the math is done those accuracy concerns don't seem like they'd appear (since it tries to fill the days part of the tuple, not the months part)

Mythicaeda · 2023-07-31T20:05:57Z

...ain/java/gov/nasa/jpl/aerie/merlin/server/remotes/postgres/InsertSimulationEventsAction.java

  ) throws SQLException {
    for (final var eventPoint : eventPoints.entrySet()) {
      final var time = eventPoint.getKey();
      final var transactions = eventPoint.getValue();
      for (int transactionIndex = 0; transactionIndex < transactions.size(); transactionIndex++) {
        final var eventGraph = transactions.get(transactionIndex);
        final var flattenedEventGraph = EventGraphFlattener.flatten(eventGraph);
-        batchInsertEventGraph(datasetId, time, transactionIndex, simulationStart, flattenedEventGraph, this.statement);
+        batchInsertEventGraph(datasetId, time, transactionIndex, flattenedEventGraph, this.statement);


Why was this done via a static method rather than inlined in the apply like our other batch inserts?

Mythicaeda · 2023-07-31T20:11:53Z

...ain/java/gov/nasa/jpl/aerie/merlin/server/remotes/postgres/InsertSimulationEventsAction.java

-    for (final Pair<String, Pair<Integer, SerializedValue>> entry : flattenedEventGraph) {
+    for (final var entry : flattenedEventGraph) {
      final var causalTime = entry.getLeft();
-      final Pair<Integer, SerializedValue> event = entry.getRight();
+      final var event = entry.getRight();


Idle question: why are these getting converted from their specific types to vars?

Mythicaeda · 2023-07-31T20:18:12Z

...in/java/gov/nasa/jpl/aerie/merlin/server/remotes/postgres/PostgresResultsCellRepository.java

@@ -338,7 +338,7 @@ private static void postActivities(
      final Timestamp simulationStart
  ) throws SQLException {
    try (
-        final var postActivitiesAction = new PostSpansAction(connection);
+        final var insertSpansAction = new InsertSpansAction(connection);


Minor: Looking at this with the rename, I wonder if something like InsertActivitySpansAction rather than just InsertSpansAction (or renaming the function from postActivities to postActivitySpans) would help with clarity, since Aerie does have the concept of non-Activity Spans

mattdailis · 2023-08-01T00:55:29Z

The original driver for this PR was to have consistency in time representation. Spans inserted by the scheduler have the form HH:MM:SS.XXX, while spans inserted by the simulator have the form N days HH:MM:SS.XXX.

If we were to choose one of these representations to standardize on, I would prefer the former. It is true that under controlled circumstances, we shouldn't see issues with precision, but the former form has a different semantic meaning from the latter form, one that conforms better to our notion of duration in Aerie.

Fun note from the postgres code about the accuracy of this subtraction. They're referring to the case where the two instances fall on different sides of a daylight savings shift - the subtraction will incorrectly assume that the length of a day is 24 hours.
"This is wrong, but removing it breaks a lot of regression tests."
https://github.com/postgres/postgres/blob/341996248e4d720556689e5fb3da7a408cf94228/src/backend/utils/adt/timestamp.c#L2668

Mythicaeda · 2023-08-01T01:03:09Z

The original driver for this PR was to have consistency in time representation. Spans inserted by the scheduler have the form HH:MM:SS.XXX, while spans inserted by the simulator have the form N days HH:MM:SS.XXX.

If we were to choose one of these representations to standardize on, I would prefer the former.

Ah, yeah, the consistency argument makes sense to me 👍. Was that implied by the line On Aerie, to the extent possible, we want to use the microseconds part of that tuple.? If not, you ought to mention that in the PR body, as standardizing how we insert into these columns is very compelling.

Mythicaeda · 2023-08-30T22:33:20Z

Thought about this PR: is it possible to encode this conversion from (months, days, microseconds) -> (microseconds) via DB triggers? I'm thinking of this bug in the UI, which wouldn't have occurred if the DB enforced the standardization

mattdailis · 2024-06-26T23:29:46Z

Closing to reduce clutter - we can revisit this if the need arises

mattdailis added 2 commits July 28, 2023 09:04

Save span start_offset and duration as interval without timestamptz s…

06e52d5

…ubtraction

Save Event start_offset as interval without timestamptz subtraction

42ac4f2

mattdailis requested a review from a team as a code owner July 31, 2023 15:54

mattdailis requested review from cohansen and Mythicaeda July 31, 2023 15:54

mattdailis temporarily deployed to e2e-test July 31, 2023 15:54 — with GitHub Actions Inactive

mattdailis self-assigned this Jul 31, 2023

mattdailis added refactor A code change that neither fixes a bug nor adds a feature database Anything related to the database simulation Anything related to the simulation domain labels Jul 31, 2023

Rename PostSpansAction to InsertSpansAction

9f0371d

mattdailis force-pushed the avoid-timestamptz-subtraction branch from c9a9f50 to 9f0371d Compare July 31, 2023 16:02

mattdailis temporarily deployed to e2e-test July 31, 2023 16:02 — with GitHub Actions Inactive

Mythicaeda reviewed Jul 31, 2023

View reviewed changes

mattdailis closed this Jun 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid timestamptz subtraction for span and event start offsets #1047

Avoid timestamptz subtraction for span and event start offsets #1047

mattdailis commented Jul 31, 2023 •

edited

Loading

Mythicaeda left a comment

Mythicaeda Jul 31, 2023

Mythicaeda Jul 31, 2023

Mythicaeda Jul 31, 2023

mattdailis commented Aug 1, 2023

Mythicaeda commented Aug 1, 2023

Mythicaeda commented Aug 30, 2023

mattdailis commented Jun 26, 2024

Avoid timestamptz subtraction for span and event start offsets #1047

Avoid timestamptz subtraction for span and event start offsets #1047

Conversation

mattdailis commented Jul 31, 2023 • edited Loading

Description

Verification

Documentation

Future work

Mythicaeda left a comment

Choose a reason for hiding this comment

Mythicaeda Jul 31, 2023

Choose a reason for hiding this comment

Mythicaeda Jul 31, 2023

Choose a reason for hiding this comment

Mythicaeda Jul 31, 2023

Choose a reason for hiding this comment

mattdailis commented Aug 1, 2023

Mythicaeda commented Aug 1, 2023

Mythicaeda commented Aug 30, 2023

mattdailis commented Jun 26, 2024

mattdailis commented Jul 31, 2023 •

edited

Loading