stats: break Gossip dependency

[tbg]

Table stats uses gossip to notify the nodes about new statistics:

cockroach/pkg/sql/stats/stats_cache.go

Line 79 in f176a39

func NewTableStatisticsCache(

Multi-tenancy Phase 2 won't have gossip available. On the plus side, there will only be a single SQL server running on behalf of a tenant (meaning the use of Gossip is not necessary, you just need to notify the running process).

Post phase 2, when we consider multiple SQL servers for a given tenant, we need to properly substitute the use of Gossip, but for now here the work is to use Gossip only when it's available, but to unconditionally notify the local server. This suggests that the statistics cache should not know about gossip but should operate on a suitable notifier abstraction.

Tentatively marking as a blocker since I think we'll want to avoid disabling stats, even for the small workloads we expect to support in the beginning.

[tbg]

@RaduBerinde I feel like you told me before but can you write down what the consequences would be of not doing this. Would stats just arrive on the node with a delay?

[RaduBerinde]

Currently they won't be updated at all without gossip (except on the local node where they are generated). There is no "polling" in place for table stats.

[RaduBerinde]

For Phase 2, I believe things would just work without Gossip, as we also notify the local stats cache directly (this was to guarantee in tests that the next statement would use new stats).

[asubiotto]

I'm trying to run the distsql_automatic_stats logic test in a multitenant configuration but it seems to fail to show any stats for the first SHOW STATISTICS statement (even after a 45s retry):

# Disable automatic stats
statement ok
SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false

statement ok
CREATE TABLE data (a INT, b INT, c FLOAT, d DECIMAL, PRIMARY KEY (a, b, c), INDEX d_idx (d))

# Enable automatic stats
statement ok
SET CLUSTER SETTING sql.stats.automatic_collection.enabled = true

# Generate all combinations of values 1 to 10.
statement ok
INSERT INTO data SELECT a, b, c::FLOAT, NULL FROM
   generate_series(1, 10) AS a(a),
   generate_series(1, 10) AS b(b),
   generate_series(1, 10) AS c(c)

query TTIII colnames,rowsort,retry
SELECT statistics_name, column_names, row_count, distinct_count, null_count
FROM [SHOW STATISTICS FOR TABLE data] ORDER BY column_names::STRING, created
----
statistics_name  column_names  row_count  distinct_count  null_count
__auto__         {a,b,c}       1000       1000            0
__auto__         {a,b}         1000       100             0
__auto__         {a}           1000       10              0
__auto__         {b}           1000       10              0
__auto__         {c}           1000       10              0
__auto__         {d}           1000       1               1000

I wondered whether it had anything to do with disabling auto stats on the backing kv cluster and (since setting cluster settings on a tenant only affect the in-memory setting), but enabling that doesn't result in a difference.

To reproduce paste the above logictest into a __test file and run: make test PKG=./pkg/ccl/logictestccl TESTS=TestTenantLogic//distsql_automatic_stats. @RaduBerinde do you mind taking a look?

[RaduBerinde]

The logs show this error:

failed to create statistics on table 53: create-stats: unexpected concurrency for a flow that was forced to be planned locally

I guess the stats plan can't be fused into a single processor. @andreimatei I think you ran into this at some point and fixed something, do you remember?

[andreimatei]

I think maybe this is what #52621 is trying to address.
Is there a good reason why these stats processors can't be fused?

[RaduBerinde]

@asubiotto I don't understand why you expect this to work when there is a known issue that you have a PR out for? Or is the problem still happening with your PR?

[asubiotto]

I guess the stats plan can't be fused into a single processor. @andreimatei I think you ran into this at some point and fixed something, do you remember?

Woops, I did not see the logs. I just posted here since I saw no stats and that the expectation was that this should work. #52621 should fix this, sorry for the noise.

[tbg]

Should be fixed now then, right? Can we confirm, add the test, and close?

[asubiotto]

distsql_automatic_stats runs with 3node-tenant.

[knz]

The INJECT STATS statement still uses gossip in a way that refers to this issue.

[knz]

CREATE STATS too

[rytaft]

So just to confirm, the remaining work here is to remove all references to Gossip in stats for post-Phase 2, in which tenants may have multiple SQL servers? What is the timeline for this?

Is the suggested solution to use polling? Are there other DB features that have moved from Gossip to polling? Or have they used something else?

[knz]

There is a new subsystem alongside gossip called "gossip subscriptions".
The SQL servrer can register for certain gossip updates and the KV server will push updates over the subscription stream.

[rytaft]

Got it - thanks!

[RaduBerinde]

It looks like the best option is to set up a RangeFeed on the entire table_statistics table and look at modified keys to see which tables need refresh. There is a similar usecase in sql/catalog/lease/lease.go.

[RaduBerinde]

Node @ajwerner is working on a library for setting up rangefeeds which we should use once it's ready (#58361).

[RaduBerinde]

Keeping open until we have something backported.

[ajwerner]

The backport merged in #64360.