stmtdiagnostics: implement range feed on system.statement_diagnostics_requests to reduce latency

[tbg]

We should implement the range feed on system.statement_diagnostics_requests table in order to remove the "polling" that currently happens every 10s. See this comment for some pointers.

Jira issue: CRDB-4382

Epic CRDB-18185

[RaduBerinde]

@andreimatei and @ajwerner discussed this quite a bit during the initial implementation. The gossip solution is seen as temporary indeed.

[ajwerner]

Yep, this is on my list.

[ajwerner]

Worst case we disable the gossip without anything better initially and clients will be exposed to a bit of extra latency when requesting statement diagnostics.

[tbg]

@ajwerner were you planning on touching this (I assume not anytime soon) and is this good enough for now as-is? We are nominally considering this a blocker still but it doesn't seem to be.

[tbg]

As you touch this issue, please also put it in the appropriate project. I would put it in SQL-Execution but I don't know if they own this.

[ajwerner]

Do we need to ensure that the gossip isn't called or does it just no-op? If it no-ops then we're good and don't need to do anything. We've not exposing the statements page for tenants right now anyway and even if we were, they poll periodically so it's just higher latency

[tbg]

From my reading of the stmtdiag code, it will return an error (unsupported w/ multi-tenancy) which will be returned to the requester of the stmt diag report. This is fine, so no need to do anything right now.

[RaduBerinde]

To fix this, we can use a range feed on the dignostic requests table.

[irfansharif]

To fix this, we can use a range feed on the dignostic requests table.

We have libraries now to make this pretty plug-n-play:

cockroach/pkg/kv/kvclient/rangefeed/rangefeedcache/watcher.go

Lines 35 to 64 in 0bd68bd

	// Watcher is used to implement a consistent cache over spans of KV data
	// on top of a RangeFeed. Note that while rangefeeds offer events as they
	// happen at low latency, a consistent snapshot cannot be constructed until
	// resolved timestamp checkpoints have been received for the relevant spans.
	// The watcher internally buffers events until the complete span has been
	// resolved to some timestamp, at which point the events up to that timestamp
	// are published as an update. This will take on the order of the
	// kv.closed_timestamp.target_duration cluster setting (default 3s).
	//
	// If the buffer overflows (as dictated by the buffer limit the Watcher is
	// instantiated with), the old rangefeed is wound down and a new one
	// re-established. The client interacts with data from the RangeFeed in two
	// ways, firstly, by translating raw KVs into kvbuffer.Events, and by handling
	// a batch of such events when either the initial scan completes or the
	// frontier changes. The OnUpdateCallback which is handed a batch of events,
	// called an Update, is informed whether the batch of events corresponds to a
	// complete or incremental update.
	//
	// It's expected to be Start-ed once. Start internally invokes Run in a retry
	// loop.
	//
	// The expectation is that the caller will use a mutex to update an underlying
	// data structure.
	//
	// NOTE (for emphasis): Update events after the initial scan published at a
	// delay corresponding to kv.closed_timestamp.target_duration (default 3s).
	// Users seeking to leverage the Updates which arrive with that delay but also
	// react to the row-level events as they arrive can hijack the translateEvent
	// function to trigger some non-blocking action.
	type Watcher struct {

This guy for example maintains such a feed (in the tenant pod, over a tenant system table):

cockroach/pkg/server/systemconfigwatcher/cache.go

Lines 29 to 33 in 69e48ba

	// Cache caches a set of KVs in a set of spans using a rangefeed. The
	// cache provides a consistent snapshot when available, but the snapshot
	// may be stale.
	type Cache struct {
	w *rangefeedcache.Watcher

[yuzefovich]

@rafiss could you take a look at this comment please? I don't understand why this issue has "skipped test" label.

[rafiss]

I added the label because I see this in the code:

cockroach/pkg/sql/sqltestutils/telemetry.go

Line 107 in 451d761

skip.WithIssue(t, 47893, "tenant clusters do not support SQL features used by this test")

[rafiss]

I don't know if that code is referencing the correct issue. If not, feel free to create a separate issue for tracking the skipped test.

[yuzefovich]

I see, thanks. This issue originally has been about the support of stmt diagnostics in secondary tenants, but #83547 added the support with some caveats, so this issue was repurposed to be about optimizing the stmt diagnostics feature. I'll remove that skip.

[yuzefovich]

The issue about implementing the rangefeed on reduce latency is still present.

[yuzefovich]

@maryliag looks like you removed this from cluster observability project, but I think it'd be up to your team to address this issue (i.e. implementing the range feed as Irfan suggested here), do you agree? I'll update the issue description accordingly.