Drain JDBC realm time-series data via a batched purge command - #5089

make sense. added tests for all versions

The javadoc on deleteAll mentions only "entity and grant record metadata".

deleteAll() is specified by BasePersistence, so I do not think it applies to events and metrics, which are under MetricsPersistence. It's currently detached from BasePersistence in terms of interfaces and its implementation is moving to extensions in #5068 🤔

I believe purging metric should be done under a separate method call and probably after merging #5068.

IMHO, conceptually, events should also not be bound to BasePersistence either. I'd like to isolate them link metrics and then add dedicated maintenance tasks for purging (see my other comment).

WDYT?

Events a time-series type of data. There may be a lot more of them in the database than Meta Store data. I'm not sure issuing a broad DELETE for them in one Tx is a robust approach.

I tend to think it would be preferable to handle their purge asynchronously and in a retriable / staged manner similar to NoSQL maintenance: https://github.com/apache/polaris/blob/apache-polaris-1.6.0/persistence/nosql/persistence/maintenance/api/src/main/java/org/apache/polaris/persistence/nosql/maintenance/api/package-info.java

If I didn't miss any context, this needs a migration path for existing databases and support in the current bootstrap schema. Existing v5 databases and the documented v3/v4-to-v5 upgrade do not create realm_purge, so the new enqueue fails and rolls back purge. Current main also bootstraps v6, whose scripts do not create this table. Could we install the queue on both paths and cover them with upgrade and fresh-bootstrap tests?

IIUC, this can also delete events and metrics from a realm bootstrapped after the purge. bootstrapRealms does not check the pending marker, so purge R, bootstrap R, new writes, then purge-drain reaches this DELETE with the same realm_id.

If so, could we preserve the new realm's data, either by preventing bootstrap while cleanup is outstanding or by scoping the task to the retired realm incarnation? A regression test for that sequence would make the lifecycle guarantee explicit IMO

IIRC, this lease does not enforce the documented one-drainer-per-realm guarantee. nowMs is reused for the whole run and the batch loop never renews or checks ownership, so another node can take an expired claim while the first keeps deleting, duplicating database work. The first node can then call complete, which removes the marker without checking the current owner. Could we use fresh claim times and retain ownership checks through draining and completion, with an expiry/takeover test?

If a realm name is purged, re-bootstrapped, used, and purged again before the drain runs, this keeps the first requested_at. The drain only deletes rows older than that first purge. Then complete drops the marker, so whatever the second incarnation wrote is orphaned for good, which is the bug this PR set out to fix. Can we make this an upsert that moves requested_at forward and resets status to pending? And add a test for purge→bootstrap→write→purge→drain?

_{🤖 AI-generated review comment — agents make mistakes, so please verify before acting on it.}

events only has a primary key on event_id. Unlike the two metrics tables, it has no (realm_id, timestamp_ms) index, so every purgeBatch subquery on events can scan the whole table, and the drain repeats that once per batch. Since v7 is already a required migration, can we add CREATE INDEX IF NOT EXISTS ... ON events(realm_id, timestamp_ms) here (and in the CRDB/H2 scripts and the CHANGELOG DDL)?

_{🤖 AI-generated review comment — agents make mistakes, so please verify before acting on it.}

This adds a method to the core MetaStoreManagerFactory SPI that only the JDBC backend implements. NoSQL already keeps its maintenance behind a backend-specific nosql subcommand. Could purge-drain be a JDBC-scoped subcommand the same way, or has this interface change been discussed on the dev list?

_{🤖 AI-generated review comment — agents make mistakes, so please verify before acting on it.}

Production code never calls enqueue, because deleteAll goes straight to enqueueInTransaction. Same with RealmDataPurger.storeName(), which nothing calls. Can we drop them, or use storeName in the failure warning so you can tell which table broke?

_{🤖 AI-generated review comment — agents make mistakes, so please verify before acting on it.}

v4–v6 link to schema-vN.sql snapshots, but these point at schema.sql on the 1.9.0 tag. Is a schema-v7.sql snapshot planned at release, or should these links change?

_{🤖 AI-generated review comment — agents make mistakes, so please verify before acting on it.}

If we only use it for purge, do we care about ordered deletes? Could we select a few PKs then delete them in a batch?.. or just do repeated DELTEs with a LIMIT?

I know it's overhead, but for purging it's probably manageable (async). On the other hand maintaining an index that is only used for purging will incur overhead in ordinary writes.

Fair point — dropped. The index is gone from all three schema.sql scripts and from the v7 migration DDL, and the drain may sequential-scan events.

On the alternatives: the current statement already selects PKs and deletes them in a batch — the subquery is a PK select with a LIMIT, in one round-trip (DELETE ... LIMIT isn't available on PostgreSQL, hence that shape). So the cost isn't ordering, it's finding the realm's rows, and that now falls on an operator-scheduled background command rather than any request path. Seems the right trade.

Would it be preferable to keep actual upgrade SQL in https://polaris.apache.org/releases/1.8.0/metastores/relational-jdbc/#schema-upgrades ?

Yes — and DatasourceOperations spells out exactly that: step 3 is "add the corresponding migration SQL to the Relational JDBC metastore documentation", step 4 "add a note to the changelog". I had it backwards.

Moved: the metastore docs now have a "Migrating to schema v7" section under Schema Upgrades with the DDL, and the CHANGELOG is back to a note linking there.

The timestamp relates to the data in the row (e.g. an event), but the cutoffMs relates to the wall clock time of the purge request. These values may not be logically comparable to each other 🤔

That said, I think in most cases this logic should work fine.

Currently localNodeId is rather random. It does not help in identifying the runner.

WDYT about using ProcessHandle.current().pid() instead or in addition to the UUID in the context of the Admin Tool?

Also, it does not feel like localNodeId() belongs in RealmPurgeService. It might as well be a local method here 🤔

I guess a --realm option might be convenient to focus the action at user's discretion. WDYT?

It would be nice to report progress back to CLI 🤔

When invoked from CLI, this message probably needs to do to the console 🤔

Could we report the other node ID?

tangential: Persistence type is only loosely coupled to metrics and events persistence. Ideally I think the can use different storage technology than the "Meta Store". In that case NoSQL MetaStore could be combined with JDBC Metrics persistence.

This is just a point for discussion. No need to change anything right now 😉

FYI: #3947

I guess schema changes are similar to API spec changes, so it would be nice to have a dev discussion about this, if only for community awareness. WDYT?

Once a purger returns 0 for this realm it stays at 0 (the cutoff is fixed), but it's still called every round. Since events has no (realm_id, timestamp_ms) index after dropping it, each of those calls is a full scan of events. A realm with ~1M metrics rows at the default batch of 1000 would scan events ~1000 times for nothing. WDYT about dropping a purger from the loop once it returns 0 (or fewer than batchSize)?

nit: listClaimable leaves out realms whose claim is still live, so --realm r while another node drains r reports "no queued purge". Could we tell these apart, e.g. "Realm r is being drained by ", using claimedBy?