Thanks for dusting this off, @Uzair5162! (And retroactive thanks, @faizaanmadhani! Your work is not forgotten! 🙂) It's looking pretty good. I'm still reading, but have some initial comments.

Reviewed 10 of 31 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @dt, and @faizaanmadhani)

pkg/sql/create_stats.go line 222 at r5 (raw file):

	}

	if n.Options.UsingExtremes && !n.p.SessionData().EnableCreateStatsUsingExtremes && n.Name.String() != jobspb.AutoPartialStatsName {

I think the check for n.Options.UsingExtremes here should be enough. We shouldn't need to check the name.

pkg/sql/create_stats.go line 741 at r5 (raw file):

					len(details.ColumnStats), i,
				); err != nil {
					retErr = errors.CombineErrors(retErr, err)

Is the idea that we should make a best-effort attempt to collect the rest of the partial stats even if one fails?

I think this might not interact correctly with cancellation of the CREATE STATISTICS: if the error is ContextCanceled, we should fail right away. (That reminds me: it would probably be a good idea to check ctx.Err() at the top of this loop.)

I'm also not sure this is going to interact correctly with the single transaction within which all of these distsql invocations are running. The transaction could be dead, for example, in which case all subsequent distsql invocations will fail, and we'll retry the entire thing anyway.

pkg/sql/catalog/catpb/catalog.go line 73 at r5 (raw file):

	// AutoPartialStatsEnabledTableSettingName is the name of the automatic
	// partial stats collection enabled table setting.
	AutoPartialStatsEnabledTableSettingName = "sql_partial_stats_automatic_collection_enabled"

To be consistent with the other per-table settings, I think we should make these names the same as the matching cluster setting names but with periods replaced with underscores.

pkg/sql/stats/automatic_stats.go line 144 at r5 (raw file):

	catpb.AutoPartialStatsMinStaleSettingName,
	"target minimum number of stale rows per table that will trigger a partial statistics refresh",
	500,

It might be worth making this smaller than 500? 100?

pkg/sql/stats/automatic_stats.go line 392 at r5 (raw file):

	if desc == nil {
		// If the descriptor could not be accessed, defer to the cluster setting.
		return AutomaticPartialStatisticsClusterMode.Get(&r.st.SV)

I think we should require both sql.stats.automatic_partial_collection.enabled and sql.stats.automatic_collection.enabled to be true in order to automatically collect partial stats, because (a) partial stats don't work without full stats anyway, and (b) that will preserve the behavior of SET CLUSTER SETTING sql.stats.automatic_collection.enabled = off; disabling all automatic stats collection, which is relied upon by many tests (and customers).

And same with table settings. So the precedence hierarchy would be:

1. per-table auto full
2. per-table auto partial
3. cluster auto full
4. cluster auto partial

In other words, I think sql.stats.automatic_partial_collection.enabled should only describe what happens when automatic stats are already enabled.

pkg/sql/stats/automatic_stats.go line 986 at r5 (raw file):

	if isPartial {
		stmt = fmt.Sprintf(
			"CREATE STATISTICS %s FROM [%d] WITH OPTIONS THROTTLING %g AS OF SYSTEM TIME '-%s' USING EXTREMES",

nit: Instead of duplicating the call to Sprintf it might be nice to add %s to the end of the original format string which takes " USING EXTREMES" if isPartial else "".

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @dt, and @michae2)

pkg/sql/create_stats.go line 222 at r5 (raw file):

Done. Does this work because we use session settings' default values in job runs?

pkg/sql/create_stats.go line 741 at r5 (raw file):

Is the idea that we should make a best-effort attempt to collect the rest of the partial stats even if one fails?

That was my thinking, yeah. Partial stats will fail if prior full stats don't exist on the column and I didn't want that to prevent stat collections on other columns that have existing stats. Now that I think about it though, this is unlikely to happen for auto partial stats since the default column set for these collections is a subset of the default auto full stats column set, so this is probably unnecessary.

pkg/sql/catalog/catpb/catalog.go line 73 at r5 (raw file):

Done.

pkg/sql/stats/automatic_stats.go line 144 at r5 (raw file):

Done, I agree!

pkg/sql/stats/automatic_stats.go line 392 at r5 (raw file):

This was my thinking as well and I think this is the current behavior. We check if auto full stats are enabled in automatic_stats.go#L793 after mutations, and early return if they're not (either (sql_stats_automatic_partial_collection_enabled table setting is unset and the sql.stats.automatic_partial_collection.enabled cluster setting is false) or (sql_stats_automatic_partial_collection_enabled table setting is false)).

We also check if auto full stats are enabled again before calling maybeRefreshStats() (which has the auto partial stat logic) in automatic_stats.go#L592 in case settings change between the mutation notification and the refresher picking it up.

The logic test in distsql_automatic_partial_stats#L5-L26 should confirm this where auto partial stats are enable for the table but auto full stats are disabled for the cluster, so auto partial stats are not collected.

Do you think it would be a good idea to add another check for auto full stats being enabled in autoPartialStatsEnabled() to make this more clear?

pkg/sql/stats/automatic_stats.go line 986 at r5 (raw file):

Done.

Awesome work! This is very close. Main thing I'm worried about is the error handling in that loop in makeJobRecord.

Reviewed 15 of 31 files at r3, 1 of 5 files at r5, 5 of 6 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @dt, @faizaanmadhani, and @Uzair5162)

-- commits line 9 at r7:
nit: looks like the per-table setting names need to be updated here

(Also, you might consider moving the details about the settings to the release note.)

-- commits line 39 at r7:
@rytaft do you have any objection to multiple automatic partial stats collections at once? It seems like a good way to start trying out multiple concurrent stats jobs?

I remember we discussed this before in:

• #118983
• some slack conversations I can't seem to find now

pkg/sql/create_stats.go line 222 at r5 (raw file):

For auto stats, these session variables come from the internal executor when the stats.Refresher runs CREATE STATISTICS. We have a way to set session variables for the internal executor, too, so I think it would be nice if CREATE STATISTICS respected the enable_create_stats_using_extremes session variable regardless of whether it was run manually or automatically.

In other words, I think SET CLUSTER SETTING sql.internal_executor.session_overrides = 'enable_create_stats_using_extremes=off'; should prevent automatic collection of partial stats (even if sql.stats.automatic_partial_collection.enabled is on).

pkg/sql/create_stats.go line 741 at r5 (raw file):

Partial stats will fail if prior full stats don't exist on the column and I didn't want that to prevent stat collections on other columns that have existing stats.

Could we protect against that case by changing distsql_plan_stats.go to skip over columns that don't have a prior full stat if this is an automatic collection?

I think simpler behavior here (failing the entire partial stats collection) will lead to fewer surprises, both here and in the optimizer when using these partial stats.

pkg/sql/catalog/catpb/catalog.go line 73 at r5 (raw file):

Thanks!

pkg/sql/stats/automatic_stats.go line 392 at r5 (raw file):
Thank you for explaining!

Do you think it would be a good idea to add another check for auto full stats being enabled in autoPartialStatsEnabled() to make this more clear?

No, it's fine the way you have it. I just didn't read all of the code.

pkg/sql/stats/automatic_stats.go line 866 at r7 (raw file):

	var rowCount float64
	mustRefresh := false
	mustRefreshPartial := false

bikeshedding: The purpose of this is a little different than mustRefresh, maybe isPartial would be better?

pkg/sql/storageparam/tablestorageparam/table_storage_param.go line 686 at r7 (raw file):

		return nil
	}
	return errors.Newf("unable to set table setting %s", key)

nit: This could probably be errors.AssertionFailedf.

pkg/sql/storageparam/tablestorageparam/table_storage_param.go line 712 at r7 (raw file):

			return nil
		}
		return errors.Newf("unable to set table setting %s", key)

nit: could also be errors.AssertionFailedf

pkg/sql/storageparam/tablestorageparam/table_storage_param.go line 740 at r7 (raw file):

			return nil
		}
		return errors.Newf("unable to set table setting %s", key)

nit: could also be errors.AssertionFailedf

pkg/sql/storageparam/tablestorageparam/table_storage_param.go line 771 at r7 (raw file):

		return nil
	}
	return errors.Newf("unable to reset table setting %s", key)

nit: this could also become errors.AssertionFailedf

pkg/sql/logictest/testdata/logic_test/distsql_automatic_partial_stats line 38 at r7 (raw file):

# not triggered.
statement ok
SET CLUSTER SETTING sql.stats.automatic_collection.min_stale_rows = 2147483647

nit: might be safer to do this before turning on sql.stats.automatic_collection.enabled

pkg/sql/logictest/testdata/logic_test/distsql_automatic_partial_stats line 90 at r7 (raw file):

ALTER TABLE data SET (sql_stats_automatic_partial_collection_enabled = true)

# Insert enough data to guarantee an partial stats collection.

typo: s/an/a/g

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @dt, @faizaanmadhani, and @michae2)

-- commits line 9 at r7:

Done.

pkg/sql/create_stats.go line 222 at r5 (raw file):

Interesting! I've added a test for this here.

pkg/sql/create_stats.go line 741 at r5 (raw file):

I think simpler behavior here (failing the entire partial stats collection) will lead to fewer surprises, both here and in the optimizer when using these partial stats.

I agree, let's keep it as is.

pkg/sql/stats/automatic_stats.go line 866 at r7 (raw file):

Agree, renamed to isPartial.

pkg/sql/storageparam/tablestorageparam/table_storage_param.go line 686 at r7 (raw file):

Done.

pkg/sql/storageparam/tablestorageparam/table_storage_param.go line 712 at r7 (raw file):

Done.

pkg/sql/storageparam/tablestorageparam/table_storage_param.go line 740 at r7 (raw file):

Done.

pkg/sql/storageparam/tablestorageparam/table_storage_param.go line 771 at r7 (raw file):

Done.

pkg/sql/logictest/testdata/logic_test/distsql_automatic_partial_stats line 38 at r7 (raw file):

Good point, done.

pkg/sql/logictest/testdata/logic_test/distsql_automatic_partial_stats line 90 at r7 (raw file):

Fixed.

After addressing @rytaft's concerns, this

Nice work bringing this back to life!!

Reviewed 1 of 5 files at r5, 1 of 6 files at r6, 3 of 3 files at r8, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @dt, and @faizaanmadhani)

pkg/sql/logictest/testdata/logic_test/distsql_automatic_partial_stats line 89 at r8 (raw file):

# setting instead
statement ok
SET CLUSTER SETTING sql.internal_executor.session_overrides = 'EnableCreateStatsUsingExtremes=false';

Nice!

pkg/sql/stats/create_stats_job_test.go line 404 at r8 (raw file):

	allowRequest = make(chan struct{})
	initialFullStatErrCh := make(chan error)
	go func() {

speculation: You might not need the goroutine here if you close(allowRequest) first? I'm not 100% sure, though.

pkg/sql/stats/create_stats_job_test.go line 451 at r8 (raw file):

	}

	<-runningAutoPartialStatErrCh

Should we also check that this is nil?

pkg/sql/stats/create_stats_job_test.go line 489 at r8 (raw file):

	}

	<-runningAutoPartialStatErrCh

Same question here

Thanks for the review @michae2 :)

I've changed it now so that auto partial stats jobs can't run concurrently on the same table, but they can still run at the same time as any other stats job.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @dt, @faizaanmadhani, and @michae2)

pkg/sql/stats/create_stats_job_test.go line 404 at r8 (raw file):

Correct :)

pkg/sql/stats/create_stats_job_test.go line 451 at r8 (raw file):

Done.

pkg/sql/stats/create_stats_job_test.go line 489 at r8 (raw file):

Done.

I just looked at the last revision, and that , thanks!

Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @dt, @faizaanmadhani, and @michae2)

-- commits line 39 at r7:

Yea I think this should be fine now.

pkg/jobs/utils.go line 113 at r9 (raw file):

		s.WriteByte(')')
		typeStrs = s.String()
	}

Can you pull this code to get typeStrs into a helper function? Looks identical to what's in the function above.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @DrewKimball, @dt, @faizaanmadhani, @michae2, and @rytaft)

pkg/jobs/utils.go line 113 at r9 (raw file):

Done.

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @DrewKimball, @dt, @faizaanmadhani, and @michae2)

Nice work!

I'm a bit concerned about the extra pressure this will place on the jobs system because this will fairly significantly increase the number of jobs. If we see problems when enabling this we might have to prioritize addressing #108435.

Reviewed 12 of 31 files at r3, 1 of 5 files at r5, 3 of 6 files at r6, 2 of 3 files at r8, 2 of 3 files at r9, 1 of 1 files at r11, 5 of 5 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @DrewKimball, @dt, @faizaanmadhani, and @michae2)

pkg/sql/logictest/testdata/logic_test/distsql_automatic_partial_stats line 73 at r13 (raw file):

query TTIIIT colnames
SELECT DISTINCT ON (statistics_name, column_names) statistics_name, column_names, row_count, distinct_count, null_count, partial_predicate
FROM [SHOW STATISTICS FOR TABLE data] ORDER BY statistics_name, column_names, created DESC

This test and the one below might erroneously pass because the queries complete before the partial stats could be collected. I don't see an easy way of making the tests more robust though... I suppose if there is a bug here the test will flake periodically in nightly tests and we'll find it.