From 1a1e753bcb0800efc70c3ccc0f8ecb09a73ef4d8 Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Mon, 2 Sep 2024 18:36:54 +0200 Subject: [PATCH 1/2] pgevents: fix retention_period and disable_cleanup on crdb --- lib/events/pgevents/pgevents.go | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/events/pgevents/pgevents.go b/lib/events/pgevents/pgevents.go index df59ee4e74da6..9759705dd23f0 100644 --- a/lib/events/pgevents/pgevents.go +++ b/lib/events/pgevents/pgevents.go @@ -79,10 +79,20 @@ const ( ); CREATE INDEX events_search_session_events_idx ON events (session_id, event_time, event_id) WHERE session_id != '00000000-0000-0000-0000-000000000000';` - dateIndex = "CREATE INDEX events_creation_time_idx ON events USING brin (creation_time);" - schemaV1TableWithDateIndex = schemaV1Table + "\n" + dateIndex - schemaV1CockroachSetRowExpiry = "ALTER TABLE events SET (ttl_expiration_expression = '((creation_time AT TIME ZONE ''UTC'') + (%d * INTERVAL ''1 microsecond'')) AT TIME ZONE ''UTC'' ');" - schemaV1CockroachUnsetRowExpiry = "ALTER TABLE events RESET (ttl_expiration_expression);" + dateIndex = "CREATE INDEX events_creation_time_idx ON events USING brin (creation_time);" + schemaV1TableWithDateIndex = schemaV1Table + "\n" + dateIndex + + // crdb will throw an error if the (n)::INTERVAL can't be represented as an + // INTERVAL literal, and the sum between a TIMESTAMPTZ and an INTERVAL will + // likewise fail if the result value can't be represented as a TIMESTAMPTZ + // (any of this should only apply with wildly impractical expiry values in + // the range of thousands of years) + schemaV1CockroachSetRowExpirySeconds = "ALTER TABLE events SET (ttl_expiration_expression = '((creation_time AT TIME ZONE ''UTC'') + ((%d)::INTERVAL)) AT TIME ZONE ''UTC''');" + // the asymmetry here is intended, crdb requires "RESET (ttl)" to disable + // row-level TTL on a table, whereas "RESET (ttl_expiration_expression)" + // would remove the expression in favor of ttl_expire_after (and it will + // error out if ttl_expire_after is unset) + schemaV1CockroachUnsetRowExpiry = "ALTER TABLE events RESET (ttl);" ) // Config is the configuration struct to pass to New. @@ -264,11 +274,10 @@ func configureCockroachDBRetention(ctx context.Context, cfg *Config, pool *pgxpo expiryQuery = schemaV1CockroachUnsetRowExpiry } else { cfg.Log.DebugContext(ctx, "Configuring CockroachDB native row expiry") - expiryQuery = fmt.Sprintf(schemaV1CockroachSetRowExpiry, cfg.RetentionPeriod) + expiryQuery = fmt.Sprintf(schemaV1CockroachSetRowExpirySeconds, int64(cfg.RetentionPeriod.Seconds())) } _, err := pool.Exec(ctx, expiryQuery, pgx.QueryExecModeExec) return trace.Wrap(err) - } func buildSchema(isCockroach bool, cfg *Config) (schemas []string, err error) { From 82ae07a65bed5cf3799ca2335ae20c304e5f9e66 Mon Sep 17 00:00:00 2001 From: Edoardo Spadolini Date: Tue, 3 Sep 2024 09:58:12 +0200 Subject: [PATCH 2/2] avoid integer to INTERVAL casts --- lib/events/pgevents/pgevents.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/events/pgevents/pgevents.go b/lib/events/pgevents/pgevents.go index 9759705dd23f0..4672a661f54a9 100644 --- a/lib/events/pgevents/pgevents.go +++ b/lib/events/pgevents/pgevents.go @@ -82,12 +82,14 @@ const ( dateIndex = "CREATE INDEX events_creation_time_idx ON events USING brin (creation_time);" schemaV1TableWithDateIndex = schemaV1Table + "\n" + dateIndex - // crdb will throw an error if the (n)::INTERVAL can't be represented as an - // INTERVAL literal, and the sum between a TIMESTAMPTZ and an INTERVAL will - // likewise fail if the result value can't be represented as a TIMESTAMPTZ - // (any of this should only apply with wildly impractical expiry values in - // the range of thousands of years) - schemaV1CockroachSetRowExpirySeconds = "ALTER TABLE events SET (ttl_expiration_expression = '((creation_time AT TIME ZONE ''UTC'') + ((%d)::INTERVAL)) AT TIME ZONE ''UTC''');" + // the 'n'::INTERVAL expression will saturate at around 292 years (which is + // perfectly acceptable for a retention period of the audit log), and the + // sum between a TIMESTAMPTZ set around 2024 and an INTERVAL of up to 292 + // years is always representable + // + // the string to INTERVAL cast should be stable, unlike any integer to + // INTERVAL cast (see https://github.com/cockroachdb/cockroach/issues/57876) + schemaV1CockroachSetRowExpirySeconds = "ALTER TABLE events SET (ttl_expiration_expression = '((creation_time AT TIME ZONE ''UTC'') + (''%d''::INTERVAL)) AT TIME ZONE ''UTC''');" // the asymmetry here is intended, crdb requires "RESET (ttl)" to disable // row-level TTL on a table, whereas "RESET (ttl_expiration_expression)" // would remove the expression in favor of ttl_expire_after (and it will