sql: Use normal priority for metadata schema change updates

[shghasemi]

Previously, all schema change updates were executed with a bulk normal priority. This had unintended side effects on overloaded systems, where trivial schema changes were unable to make progress. With this change, we only apply the low priority for operations that involve doing backfills or validation.

Fixes: #162042

Release note: None

[trunk-io]

Merging to master in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[fqazi]

Looks, good one minor suggestion

@fqazi reviewed 2 files and all commit messages, and made 3 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @shghasemi).

---

pkg/sql/schema_changer.go line 2905 at r1 (raw file):

// txn is a convenient wrapper around descs.Txn().
func (sc *SchemaChanger) txn(
	ctx context.Context, useBulkPri bool, f func(context.Context, descs.Txn) error,

Lets switch this to use the option pattern:

type txnOptions struct {  
  bulkPri bool
}  
  
type txnOption interface {  
  apply(*txnOptions)  
}

type bulkPriOption bool

bool (b bulkPriOption) apply(t *txnOptions){
    t.bulkPri = bool(b)
}

func WithBulkPri(b bool) txnOption{
 return bulkPriOption(b)
}

func (sc *SchemaChanger)  txn(
	ctx context.Context, f func(context.Context, descs.Txn), txnOptions...) {

---

pkg/sql/backfill.go line 155 at r1 (raw file):

) historicalTxnRunner {
	runner := func(ctx context.Context, retryable scTxnFn) error {
		return sc.fixedTimestampTxnWithExecutor(ctx, useBulkPri, readAsOf, func(

For simplicity lets exclude the fixed timestamp operations. I know we read descriptors below, but these are mainly for CREATE TABLE AS / REFRESH MATERIALIZED VIEW type backfills.

[shghasemi]

@shghasemi made 2 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi).

---

pkg/sql/backfill.go line 155 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

For simplicity lets exclude the fixed timestamp operations. I know we read descriptors below, but these are mainly for CREATE TABLE AS / REFRESH MATERIALIZED VIEW type backfills.

They are also used to run validation.

cockroach/pkg/sql/backfill.go

Line 766 in f275471

runHistoricalTxn := sc.makeFixedTimestampRunner(readAsOf)

Should we set them to always run with bulkPri?

---

pkg/sql/schema_changer.go line 2905 at r1 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Lets switch this to use the option pattern:

type txnOptions struct {  
  bulkPri bool
}  
  
type txnOption interface {  
  apply(*txnOptions)  
}

type bulkPriOption bool

bool (b bulkPriOption) apply(t *txnOptions){
    t.bulkPri = bool(b)
}

func WithBulkPri(b bool) txnOption{
 return bulkPriOption(b)
}

func (sc *SchemaChanger)  txn(
	ctx context.Context, f func(context.Context, descs.Txn), txnOptions...) {

Done

[fqazi]

@fqazi reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @shghasemi).

---

pkg/sql/backfill.go line 155 at r1 (raw file):

Previously, shghasemi wrote…

They are also used to run validation.

cockroach/pkg/sql/backfill.go

Line 766 in f275471

runHistoricalTxn := sc.makeFixedTimestampRunner(readAsOf)

Should we set them to always run with bulkPri?

Yeah, lets leave those at the bulk priority.