distsql: avoid planning on a draining node. by shubhamdhama · Pull Request #128283 · cockroachdb/cockroach

shubhamdhama · 2024-08-04T19:24:58Z

Commit-wise description

Commit 1: sql: add is_draining to system.sql_instances table.

This is a preliminary commit toward improving the DistSQL to avoid planning
on an instance that is draining. The idea here is to set this column to
true as soon as the draining request is received by the server and use that
through instance reader in the planner at gateway.

Adding this column requires writing a migration for the upgrade. And since
this new column is nullable we don't need to backfill and we are good with
just swapping the descriptors.

Informs: #100578
Release note: None

Development notes:

To update pkg/sql/catalog/bootstrap/testdata/testdata I had to manually
update the hash values for system/tenant in this file and pass the
--rewrite option to do the rest:

dev test //pkg/sql/catalog/bootstrap:bootstrap_test \
        --filter=TestInitialValuesToString/testdata --rewrite

To update tests in
pkg/sql/catalog/systemschema_test/testdata/bootstrap_{system,tenant}

dev test \
        //pkg/sql/catalog/systemschema_test:systemschema_test_test --rewrite

Commit 2: distsql: avoid planning on a draining node.

This change would prevent distsql planner to include a draining node in a
plan by leveraging the system.sql_instances table's is_draining field.

Somewhat related work to avoid planning on an unavailable node: #124986

Fixes: #100578
Epic: CRDB-31575
Release note: None

cockroach-teamcity · 2024-08-04T19:25:10Z

This change is

shubhamdhama · 2024-08-04T19:35:12Z

pkg/sql/distsql_physical_planner_test.go line 832 at r2 (raw file):

			return checkQueryPlan(t, r, expectedPlans)
		}, 4*time.Second)

The test for external-process mode is failing because the plan still includes the drained 5th node. This happens because setting is_draining to true isn't being reflected on the gateway node. However, this works correctly on a roachprod cluster. Despite extensive investigation, I couldn't figure out why this is happening, so I would really appreciate any help.

Code quote:

		testutils.SucceedsWithin(t, func() error {
			// TODO: REMOVE
			res := r.QueryStr(t, "SELECT now(), id, encode(session_id, 'hex'), is_draining FROM system.sql_instances WHERE addr IS NOT NULL")
			t.Logf("res : %v", res)

			return checkQueryPlan(t, r, expectedPlans)
		}, 4*time.Second)

pkg/sql/distsql_physical_planner.go

shubhamdhama

This is also ready for review.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @jeffswenson, @stevendanna, and @yuzefovich)

pkg/sql/distsql_physical_planner_test.go line 832 at r2 (raw file):

Previously, shubhamdhama (Shubham Dhama) wrote…

The test for external-process mode is failing because the plan still includes the drained 5th node. This happens because setting is_draining to true isn't being reflected on the gateway node. However, this works correctly on a roachprod cluster. Despite extensive investigation, I couldn't figure out why this is happening, so I would really appreciate any help.

Oh btw, this was fixed. Earlier, I was draining system tenant instance which I now changed with the correct sql instance.

jeffswenson

LGTM

I think you should also get a review from someone on sql foundations or sql queries. The functionality straddles the two teams. Historically sql foundations owned the sql_instances infrastructure and sql queries still owns distsql planning which is impacted by this change.

pkg/server/drain.go

pkg/sql/distsql_physical_planner.go

pkg/sql/sqlinstance/instancestorage/test_helpers.go

DrewKimball

I just have a few questions.

Reviewed 6 of 22 files at r1, 6 of 18 files at r3, 13 of 13 files at r5, 4 of 4 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt, @jeffswenson, @shubhamdhama, and @stevendanna)

pkg/sql/distsql_physical_planner.go line 1725 at r7 (raw file):

	}

	// GetAvailableInstances returns mostly healthy nodes, except those that have

[nit] Why the comment change?

pkg/sql/sqlinstance/instancestorage/instancereader.go line 138 at r5 (raw file):

		Locality:        row.locality,
		BinaryVersion:   row.binaryVersion,
		Region:          row.region,

Is it problematic that we weren't filling in Region before?

pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go line 39 at r6 (raw file):

		mutableDesc.TableDescriptor = *expectedDesc
		mutableDesc.Version = version
		return txn.Descriptors().WriteDesc(ctx, false, mutableDesc, txn.KV())

Will WriteDesc increment the descriptor version?

shubhamdhama

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

pkg/sql/distsql_physical_planner.go line 1725 at r7 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[nit] Why the comment change?

Done. (Nice catch, in my previous revision I removed this function).

pkg/sql/sqlinstance/instancestorage/instancereader.go line 138 at r5 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Is it problematic that we weren't filling in Region before?

It wasn't problematic earlier, but I figured it's most likely a miss.

pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go line 39 at r6 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Will WriteDesc increment the descriptor version?

Yes,

cockroach/pkg/sql/catalog/descs/collection.go

Lines 559 to 566 in 8551145

    
           // WriteDesc constructs a new Batch, calls WriteDescToBatch and runs it. 
        
           func (tc *Collection) WriteDesc( 
        
           	ctx context.Context, kvTrace bool, desc catalog.MutableDescriptor, txn *kv.Txn, 
        
           ) error { 
        
           	b := txn.NewBatch() 
        
           	if err := tc.WriteDescToBatch(ctx, kvTrace, desc, b); err != nil { 
        
           		return err 
        
           	}

cockroach/pkg/sql/catalog/descs/collection.go

Lines 276 to 289 in 8551145

    
           // WriteDescToBatch calls MaybeIncrementVersion, adds the descriptor to the 
        
           // collection as an uncommitted descriptor, and writes it into b. 
        
           func (tc *Collection) WriteDescToBatch( 
        
           	ctx context.Context, kvTrace bool, desc catalog.MutableDescriptor, b *kv.Batch, 
        
           ) error { 
        
           	if desc.GetID() == descpb.InvalidID { 
        
           		return errors.AssertionFailedf("cannot write descriptor with an empty ID: %v", desc) 
        
           	} 
        
           	desc.MaybeIncrementVersion() 
        
           	if !tc.skipValidationOnWrite && tc.validationModeProvider.ValidateDescriptorsOnWrite() { 
        
           		if err := validate.Self(tc.version, desc); err != nil { 
        
           			return err 
        
           		} 
        
           	}

rimadeodhar · 2024-09-03T16:52:06Z

@fqazi: Please could you take a look from the SQL Foundations side? Thanks.

fqazi

One minor nit

Reviewed 6 of 22 files at r1, 4 of 18 files at r3, 3 of 13 files at r5, 2 of 4 files at r6, 10 of 10 files at r8, 4 of 4 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @dt, @jeffswenson, @shubhamdhama, and @stevendanna)

pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go line 37 at r8 (raw file):

		}
		version := mutableDesc.Version
		mutableDesc.TableDescriptor = *expectedDesc

Use protoutil.Clone here

shubhamdhama · 2024-09-03T17:27:46Z

pkg/upgrade/upgrades/v24_3_sql_instances_add_draining.go line 37 at r8 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Use protoutil.Clone here

Done.

shubhamdhama · 2024-09-04T08:49:49Z

Thanks everyone for the review.

bors r=jeffswenson,DrewKimball,fqazi

stevendanna

Thanks for working on this. Overall this looks reasonable to me.

pkg/sql/sqlinstance/instancestorage/test_helpers.go

pkg/upgrade/upgrades/v24_3_sql_instances_add_draining_test.go

pkg/sql/distsql_physical_planner_test.go

shubhamdhama · 2024-09-04T09:01:24Z

bors r-

craig · 2024-09-04T09:01:27Z

Canceled.

This is a preliminary commit toward improving the DistSQL to avoid planning on an instance that is draining. The idea here is to set this column to true as soon as the draining request is received by the server and use that through instance reader in the planner at gateway. Adding this column requires writing a migration for the upgrade. And since this new column is nullable we don't need to backfill and we are good with just swapping the descriptors. Informs: cockroachdb#100578 Release note: None Development notes: To update `pkg/sql/catalog/bootstrap/testdata/testdata` I had to manually update the hash values for `system`/`tenant` in this file and pass the `--rewrite` option to do the rest: ``` dev test //pkg/sql/catalog/bootstrap:bootstrap_test \ --filter=TestInitialValuesToString/testdata --rewrite ``` To update tests in `pkg/sql/catalog/systemschema_test/testdata/bootstrap_{system,tenant}` ``` dev test \ //pkg/sql/catalog/systemschema_test:systemschema_test_test --rewrite ```

This change would prevent distsql planner to include a draining node in a plan by leveraging the system.sql_instances table's `is_draining` field. Somewhat related work to avoid planning on an unavailable node: cockroachdb#124986 Fixes: cockroachdb#100578 Epic: CRDB-31575 Release note: None

shubhamdhama · 2024-09-04T14:03:09Z

TFTR.

bors r=jeffswenson,DrewKimball,fqazi,stevendanna

craig · 2024-09-04T14:36:14Z

Build succeeded:

shubhamdhama requested review from a team as code owners August 4, 2024 19:24

shubhamdhama requested review from dt and yuzefovich and removed request for a team August 4, 2024 19:25

shubhamdhama requested review from jeffswenson and stevendanna August 5, 2024 11:41

shubhamdhama commented Aug 5, 2024

View reviewed changes

pkg/sql/distsql_physical_planner.go Outdated Show resolved Hide resolved

rafiss removed the request for review from a team August 21, 2024 20:23

shubhamdhama force-pushed the draining-use-in-distsql-100578 branch from cc10bf1 to 661e2dc Compare August 26, 2024 09:46

shubhamdhama mentioned this pull request Aug 26, 2024

sql: add is_draining to system.sql_instances table. #126765

Closed

shubhamdhama commented Aug 27, 2024

View reviewed changes

jeffswenson approved these changes Aug 27, 2024

View reviewed changes

pkg/server/drain.go Outdated Show resolved Hide resolved

pkg/sql/distsql_physical_planner.go Outdated Show resolved Hide resolved

pkg/sql/sqlinstance/instancestorage/test_helpers.go Outdated Show resolved Hide resolved

shubhamdhama force-pushed the draining-use-in-distsql-100578 branch 2 times, most recently from 85f61c1 to 29d22c9 Compare August 28, 2024 16:58

shubhamdhama requested a review from a team August 28, 2024 17:06

rytaft requested review from a team and DrewKimball and removed request for a team and yuzefovich August 28, 2024 17:14

shubhamdhama force-pushed the draining-use-in-distsql-100578 branch from 29d22c9 to 2f8894c Compare August 29, 2024 11:07

DrewKimball requested a review from jeffswenson August 29, 2024 22:14

DrewKimball approved these changes Aug 29, 2024

View reviewed changes

shubhamdhama commented Sep 2, 2024

View reviewed changes

shubhamdhama force-pushed the draining-use-in-distsql-100578 branch from 2f8894c to 6337d93 Compare September 2, 2024 06:30

rimadeodhar requested a review from fqazi September 3, 2024 16:51

fqazi requested a review from DrewKimball September 3, 2024 17:12

fqazi approved these changes Sep 3, 2024

View reviewed changes

shubhamdhama force-pushed the draining-use-in-distsql-100578 branch from 6337d93 to f1046f1 Compare September 3, 2024 17:26

shubhamdhama force-pushed the draining-use-in-distsql-100578 branch from f1046f1 to bebbfa5 Compare September 4, 2024 06:58

stevendanna approved these changes Sep 4, 2024

View reviewed changes

pkg/sql/sqlinstance/instancestorage/test_helpers.go Outdated Show resolved Hide resolved

pkg/upgrade/upgrades/v24_3_sql_instances_add_draining_test.go Outdated Show resolved Hide resolved

pkg/sql/distsql_physical_planner_test.go Outdated Show resolved Hide resolved

shubhamdhama force-pushed the draining-use-in-distsql-100578 branch from bebbfa5 to 95e3751 Compare September 4, 2024 12:08

shubhamdhama force-pushed the draining-use-in-distsql-100578 branch from 95e3751 to 9ef988d Compare September 4, 2024 13:16

craig bot merged commit ca0a938 into cockroachdb:master Sep 4, 2024

shubhamdhama mentioned this pull request Sep 9, 2024

sql,server: unify sqlInstanceDialer initialization using instancereader-based dialer. #130235

Closed

shubhamdhama mentioned this pull request Jan 9, 2025

server,ua: add support for --locality-advertise-addr for sql-instances #138715

Open

shubhamdhama deleted the draining-use-in-distsql-100578 branch March 1, 2025 10:45

jeffswenson mentioned this pull request Aug 25, 2025

roachtest: restore/nodeShutdown/coordinator failed #152388

Open

	// WriteDesc constructs a new Batch, calls WriteDescToBatch and runs it.
	func (tc *Collection) WriteDesc(
	ctx context.Context, kvTrace bool, desc catalog.MutableDescriptor, txn *kv.Txn,
	) error {
	b := txn.NewBatch()
	if err := tc.WriteDescToBatch(ctx, kvTrace, desc, b); err != nil {
	return err
	}

	// WriteDescToBatch calls MaybeIncrementVersion, adds the descriptor to the
	// collection as an uncommitted descriptor, and writes it into b.
	func (tc *Collection) WriteDescToBatch(
	ctx context.Context, kvTrace bool, desc catalog.MutableDescriptor, b *kv.Batch,
	) error {
	if desc.GetID() == descpb.InvalidID {
	return errors.AssertionFailedf("cannot write descriptor with an empty ID: %v", desc)
	}
	desc.MaybeIncrementVersion()
	if !tc.skipValidationOnWrite && tc.validationModeProvider.ValidateDescriptorsOnWrite() {
	if err := validate.Self(tc.version, desc); err != nil {
	return err
	}
	}

Conversation

shubhamdhama commented Aug 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Aug 4, 2024

Uh oh!

shubhamdhama commented Aug 4, 2024

Uh oh!

Uh oh!

shubhamdhama left a comment

Choose a reason for hiding this comment

Uh oh!

jeffswenson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrewKimball left a comment

Choose a reason for hiding this comment

Uh oh!

shubhamdhama left a comment

Choose a reason for hiding this comment

Uh oh!

rimadeodhar commented Sep 3, 2024

Uh oh!

fqazi left a comment

Choose a reason for hiding this comment

Uh oh!

shubhamdhama commented Sep 3, 2024

Uh oh!

shubhamdhama commented Sep 4, 2024

Uh oh!

stevendanna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shubhamdhama commented Sep 4, 2024

Uh oh!

craig bot commented Sep 4, 2024

Uh oh!

shubhamdhama commented Sep 4, 2024

Uh oh!

craig bot commented Sep 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shubhamdhama commented Aug 4, 2024 •

edited

Loading