db: migrate `aggregation_jobs` table to new times by tgeoghegan · Pull Request #4393 · divviup/janus

tgeoghegan · 2026-02-24T01:12:58Z

Store client report intervals as INT8RANGE, representing a range of timestamps in time precision units.

To that end, we add SqlIntervalTimePrecision, a simplified version of SqlInterval that gives us conversion to/from INT8RANGE. Once all TSTZRANGEs are converted to INT8RANGE, the old SqlInterval gets deleted and SqlIntervalTimePrecision is renamed.

Part of #4206

tgeoghegan · 2026-02-24T01:16:01Z

aggregator_core/src/datastore.rs

-        COALESCE($2::TIMESTAMP WITH TIME ZONE - tasks.report_expiry_age * '1 second'::INTERVAL,
-                 '-infinity'::TIMESTAMP WITH TIME ZONE)
-    FOR UPDATE OF aggregation_jobs SKIP LOCKED LIMIT $3
+        COALESCE(($3 - tasks.report_expiry_age) / tasks.time_precision, 0)


This little bit of math needs close scrutiny. #3 is now in seconds since the epoch, so we're subtracting report_expiry_age (also in seconds, a BIGINT) then dividing by the BIGINT time_precision to get a quantity we can compare to UPPER(aggregation_jobs.client_timestamp_interval). The way we do that in Janus boils down to timestamp / precision_secs (Time::from_seconds_since_epoch) so I think this is OK, because Postgres / is documented to "truncate the result" (https://www.postgresql.org/docs/9.5/functions-math.html).

The math LGTM, but we do have a functionality change here, I think? Previously tasks without a report_expiry_age used -infinity in the comparison, while with the division here we're now getting 0, so our >= is only going to match tasks with positive timestamps, while previously with the result going to -infinity it'd always match.

I guess that's just an edge case not particularly important for the real world... but you did ask me to apply scrutiny here.

I appreciate the scrutiny. This is worth diving into.

The nuance here is signedness of time values. Previously, we stored SQL TIMESTAMP, which ranges from [-infinity, infinity] (is it possible to have a closed range on infinity? whatever). The intention was to use the smallest possible value so that it will always compare less than UPPER(aggregation_jobs.client_timestamp_interval), which was TIMESTAMP.

Now, UPPER(aggregation_jobs.client_timestamp_interval) is a DAP Time, which ranges from 0, 2^64-1. But we store it as SQL BIGINT which is -2^63, +2^63-1. So should we compare against the smallest possible Time or the smallest possible BIGINT?

Furthermore, TIMESTAMP's zero point does not line up with the Unix epoch that DAP uses, so some of our unit tests did care about negative SQL timestamps. Now, both $3 and UPPER(aggregation_jobs.client_timestamp_interval) are using the Unix epoch as their zero point, so I think it would be fine to use either 0 or x'8000000000000000'::bigint as our fallback value to compare against.

jcjones

Great work here. Thank you for taking this on... I don't feel super strongly about the extra safety so I'll go ahead and hit Approve With Feedback.

jcjones · 2026-02-24T17:13:08Z

aggregator_core/src/datastore.rs

-        COALESCE($2::TIMESTAMP WITH TIME ZONE - tasks.report_expiry_age * '1 second'::INTERVAL,
-                 '-infinity'::TIMESTAMP WITH TIME ZONE)
-    FOR UPDATE OF aggregation_jobs SKIP LOCKED LIMIT $3
+        COALESCE(($3 - tasks.report_expiry_age) / tasks.time_precision, 0)


The math LGTM, but we do have a functionality change here, I think? Previously tasks without a report_expiry_age used -infinity in the comparison, while with the division here we're now getting 0, so our >= is only going to match tasks with positive timestamps, while previously with the result going to -infinity it'd always match.

I guess that's just an edge case not particularly important for the real world... but you did ask me to apply scrutiny here.

jcjones · 2026-02-24T17:16:39Z

aggregator_core/src/datastore/models.rs

+                    Duration::from_time_precision_units(
+                        (end_time - start_time)
+                            .try_into()
+                            .map_err(|_| "interval duration must be positive")?,
+                    ),


While the try_into is doing some lifting here, the subtraction is also somewhat unsafe, and we could make that safe in the degenerate case, so why not?

Suggested change

Duration::from_time_precision_units(

(end_time - start_time)

.try_into()

.map_err(|_| "interval duration must be positive")?,

),

Duration::from_time_precision_units(

(end_time

.checked_sub(start_time)

.ok_or("interval end must be >= start")?)

.try_into()

.map_err(|_| "interval duration must be non-negative")?

),

We could alternately convert both start_time and end_time to a u64 right after decoding them, and do the checked_sub() last.

jcjones · 2026-02-24T17:23:22Z

aggregator_core/src/datastore/models.rs

+        _: &postgres_types::Type,
+        out: &mut bytes::BytesMut,
+    ) -> Result<postgres_types::IsNull, Box<dyn std::error::Error + Sync + Send>> {
+        // Convert the interval start and end to SQL timestamps.


these aren't timestamps

Suggested change

// Convert the interval start and end to SQL timestamps.

// Convert the interval start and end to values in time precision units.

jcjones · 2026-02-24T17:26:06Z

db/00000000000001_initial_schema.up.sql

-    client_timestamp_interval  TSTZRANGE NOT NULL,
+    -- The minimal interval, in time precision units, containing all of client timestamps included
+    -- in this aggregation job
+    client_timestamp_interval  INT8RANGE NOT NULL,


On line 3 we have an out of date comment that maybe should note that the INT8RANGE is also allowed to be an index (line 288) due to the GiST extension

Store client report intervals as `INT8RANGE`, representing a range of timestamps in time precision units. To that end, we add `SqlIntervalTimePrecision`, a simplified version of `SqlInterval` that gives us conversion to/from `INT8RANGE`. Once all `TSTZRANGE`s are converted to `INT8RANGE`, the old `SqlInterval` gets deleted and `SqlIntervalTimePrecision` is renamed. Part of #4206

divergentdave · 2026-02-24T23:39:06Z

aggregator_core/src/datastore/models.rs

+                    .map_err(|_| "interval start must be positive")?;
+                let end_time: u64 = int8_from_sql(end_raw)?
+                    .try_into()
+                    .map_err(|_| "interval start must be positive")?;


nit: typo

Suggested change

.map_err(|_| "interval start must be positive")?;

.map_err(|_| "interval end must be positive")?;

Shoot, I jumped the gun. I'll get this in the next PR.

tgeoghegan requested a review from a team as a code owner February 24, 2026 01:12

tgeoghegan added the allow-changed-migrations Override the ci-migrations check to allow migrations that have changed. label Feb 24, 2026

tgeoghegan commented Feb 24, 2026

View reviewed changes

jcjones approved these changes Feb 24, 2026

View reviewed changes

tgeoghegan force-pushed the timg/datastore-time-precision-client-reports-table branch from 4f5c305 to 72be386 Compare February 24, 2026 18:47

tgeoghegan force-pushed the timg/datastore-time-precision-agg-jobs-table branch from 6d9946f to f0dc49e Compare February 24, 2026 19:13

Base automatically changed from timg/datastore-time-precision-client-reports-table to main February 24, 2026 22:48

tgeoghegan added 3 commits February 24, 2026 15:07

remove comment

5419e1c

jcjones review

519c7df

tgeoghegan force-pushed the timg/datastore-time-precision-agg-jobs-table branch from cbb35b8 to 519c7df Compare February 24, 2026 23:08

david review

2155793

tgeoghegan enabled auto-merge (squash) February 24, 2026 23:27

divergentdave approved these changes Feb 24, 2026

View reviewed changes

tgeoghegan merged commit 63a244f into main Feb 24, 2026
8 checks passed

tgeoghegan deleted the timg/datastore-time-precision-agg-jobs-table branch February 24, 2026 23:47

	// Convert the interval start and end to SQL timestamps.
	// Convert the interval start and end to values in time precision units.

	.map_err(\|_\| "interval start must be positive")?;
	.map_err(\|_\| "interval end must be positive")?;

Comments

Conversation

tgeoghegan commented Feb 24, 2026

Uh oh!

tgeoghegan Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcjones left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tgeoghegan Feb 24, 2026 •

edited

Loading