sql: do not fallback to old namespace table for non-public schema #51359

arulajmani · 2020-07-13T00:20:09Z

Previously, GetObjectNames would fallback to the old namespace table
unconditionally. This was incorrect when looking for objects under a
schema which wasn't public, as entries in the old namespace table are
implied to exist under the public schema. This manifested itself in
unintended scenarios, such as #51219, where we would try to delete all
objects constructed before 20.1 upgrade -- erroneously assuming they
are temporary objects.

This PR fixes that by ensuring the fallback logic only applies to
the public schema name case. Additionally this patch adds tests
to guard against the aforementioned bug.

Closes #51358

Release note (bug fix): A bug with temporary object cleaner was stuck
trying to remove objects that it mistakenly thought were temporary is
fixed. Note that no persistent data was deleted -- the temporary
cleaner simply errored out because it thought certain persistent data
was temporary.

cockroach-teamcity · 2020-07-13T00:20:16Z

This change is

rohany · 2020-07-13T17:11:34Z

pkg/sql/temporary_schema_test.go

@@ -272,3 +328,47 @@ func TestTemporarySchemaDropDatabase(t *testing.T) {
 		assert.Equal(t, 0, tempObjectCount)
 	}
 }
+
+// ensureTemporaryObjectsAreDeleted ensures all the tempNames have been deleted.
+// This can take a longer amount of time if the job takes time/ lease doesn't


I think you can use lease.TestingDisableTableLeases() to avoid this

rohany · 2020-07-13T17:12:35Z

pkg/sql/catalog/catalogkv/physical_accessor.go

+	// will only be present in the older system.namespace. To account for this
+	// scenario, we must do this filtering logic.
+	// TODO(solon): This complexity can be removed in  20.2.
+	if scName == tree.PublicSchema {


would it be clearer to just return early if scName != tree.PublicSchema?

arulajmani

TFTR. Should be RFAL

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rohany)

pkg/sql/temporary_schema_test.go, line 333 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I think you can use lease.TestingDisableTableLeases() to avoid this

Is in the function fine or does this need to happen during test setup? I've done it in the function for now, let me know if this needs changing.

pkg/sql/catalog/catalogkv/physical_accessor.go, line 144 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

would it be clearer to just return early if scName != tree.PublicSchema?

Done.

rohany · 2020-07-14T04:13:26Z

pkg/sql/temporary_schema_test.go

+func ensureTemporaryObjectsAreDeleted(
+	ctx context.Context, t *testing.T, conn *sql.Conn, schemaName string, tempNames []string,
+) {
+	lease.TestingDisableTableLeases()


put this in the test, not the helper -- you have to defer lease.TestingDisableTableLeases()() so that it is reset after the test.

rohany · 2020-07-14T04:13:43Z

pkg/sql/temporary_schema_test.go

+) {
+	lease.TestingDisableTableLeases()
+	for _, name := range tempNames {
+		testutils.SucceedsSoon(t, func() error {


do you still need this succeeds soon then?

arulajmani

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @otan, and @rohany)

pkg/sql/temporary_schema_test.go, line 340 at r2 (raw file):

Previously, rohany (Rohan Yadav) wrote…

do you still need this succeeds soon then?

I guess I do because the job might take time to succeed, but I'll cc @otan because he wrote this initially, I just moved it into a function.

rohany · 2020-07-14T15:43:03Z

If you need the succeeds soon anyway, then we can avoid the lease disabling all together

otan · 2020-07-14T16:49:06Z

pkg/sql/temporary_schema_test.go, line 340 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

I guess I do because the job might take time to succeed, but I'll cc @otan because he wrote this initially, I just moved it into a function.

iirc it was flaking without this, but with leasing disabled maybe not.
(why are we testing with leasing disabled?)

arulajmani

I do need the succeeds soon stuff regardless -- removed the lease disabling. RFAL

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rohany)

pkg/sql/temporary_schema_test.go, line 338 at r2 (raw file):

Previously, rohany (Rohan Yadav) wrote…

put this in the test, not the helper -- you have to defer lease.TestingDisableTableLeases()() so that it is reset after the test.

Done.

pkg/sql/temporary_schema_test.go, line 340 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

iirc it was flaking without this, but with leasing disabled maybe not.
(why are we testing with leasing disabled?)

Tried removing it and I'm getting some flakes because the job hasn't completed -- so I think we do need the succeed soon stuff here regardless of the defer lease.TestingDisableTableLeases()()

rohany · 2020-07-14T23:16:29Z

Lgtm

rohany · 2020-07-14T23:20:48Z

Actually, this seems weird that we need the succeeds soon — the job executes serially and we are just running drop statements in the job.

otan · 2020-07-14T23:23:29Z

pkg/sql/temporary_schema_test.go, line 340 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Tried removing it and I'm getting some flakes because the job hasn't completed -- so I think we do need the succeed soon stuff here regardless of the defer lease.TestingDisableTableLeases()()

i think the JOB to actually run the schema changes isn't run serially before this statement

arulajmani

Got TestingDisableTableLeases to pass on stress.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rohany)

otan · 2020-08-05T02:24:19Z

should this be movin'?

arulajmani · 2020-08-05T14:12:03Z

@rohany had some questions over why we need the succeeds soon/disable leases in the first place. Do we want to block this until then or maybe put a TODO/open an issue and address later?

rohany · 2020-08-05T14:26:50Z

Putting a TODO and an opening an issue to investigate is fine. I didn't want to hold up on this bugfix + backport.

Previously, `GetObjectNames` would fallback to the old namespace table unconditionally. This was incorrect when looking for objects under a schema which wasn't `public`, as entries in the old namespace table are implied to exist under the `public` schema. This manifested itself in unintended scenarios, such as cockroachdb#51219, where we would try to delete all objects constructed before 20.1 upgrade -- erroneously assuming they are temporary objects. This PR fixes that by ensuring the fallback logic only applies to the `public` schema name case. Additionally this patch adds tests to guard against the aforementioned bug. Closes cockroachdb#51358 Release note (bug fix): A bug with temporary object cleaner was stuck trying to remove objects that it mistakenly thought were temporary is fixed. Note that no persistent data was deleted -- the temporary cleaner simply errored out because it thought certain persistent data was temporary.

arulajmani · 2020-08-05T16:00:14Z

Opened #52412 and added a TODO for myself. RFAL

arulajmani · 2020-08-05T17:26:32Z

Thanks for the reviews.

bors r=rohany

craig · 2020-08-05T20:56:45Z

Build succeeded:

GitHub CI (Cockroach)

otan · 2020-08-11T19:26:43Z

did we backport this?

arulajmani · 2020-08-12T02:19:25Z

Thanks for the ping, it totally slipped me. Opened a backport rn

arulajmani requested a review from rohany July 13, 2020 00:20

rohany reviewed Jul 13, 2020

View reviewed changes

arulajmani commented Jul 13, 2020

View reviewed changes

arulajmani force-pushed the name-resolution-scan-objects branch from a3ac044 to ca587ee Compare July 13, 2020 19:54

arulajmani requested a review from rohany July 13, 2020 19:55

rohany reviewed Jul 14, 2020

View reviewed changes

arulajmani commented Jul 14, 2020

View reviewed changes

arulajmani force-pushed the name-resolution-scan-objects branch from ca587ee to 70f9146 Compare July 14, 2020 23:04

arulajmani commented Jul 14, 2020

View reviewed changes

arulajmani requested a review from rohany July 14, 2020 23:16

arulajmani force-pushed the name-resolution-scan-objects branch from 70f9146 to 8702dca Compare July 15, 2020 23:51

arulajmani commented Jul 15, 2020

View reviewed changes

arulajmani force-pushed the name-resolution-scan-objects branch from 8702dca to abc98df Compare August 5, 2020 15:59

rohany approved these changes Aug 5, 2020

View reviewed changes

craig bot merged commit 3b193fe into cockroachdb:master Aug 5, 2020

arulajmani mentioned this pull request Aug 12, 2020

release-20.1: sql: do not fallback to old namespace table for non-public schema #52662

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: do not fallback to old namespace table for non-public schema #51359

sql: do not fallback to old namespace table for non-public schema #51359

arulajmani commented Jul 13, 2020

cockroach-teamcity commented Jul 13, 2020

rohany Jul 13, 2020

rohany Jul 13, 2020

arulajmani left a comment

rohany Jul 14, 2020

rohany Jul 14, 2020

arulajmani left a comment

rohany commented Jul 14, 2020 •

edited

otan commented Jul 14, 2020

arulajmani left a comment

rohany commented Jul 14, 2020

rohany commented Jul 14, 2020

otan commented Jul 14, 2020

arulajmani left a comment

otan commented Aug 5, 2020

arulajmani commented Aug 5, 2020

rohany commented Aug 5, 2020

arulajmani commented Aug 5, 2020

arulajmani commented Aug 5, 2020

craig bot commented Aug 5, 2020

otan commented Aug 11, 2020

arulajmani commented Aug 12, 2020

sql: do not fallback to old namespace table for non-public schema #51359

sql: do not fallback to old namespace table for non-public schema #51359

Conversation

arulajmani commented Jul 13, 2020

cockroach-teamcity commented Jul 13, 2020

rohany Jul 13, 2020

Choose a reason for hiding this comment

rohany Jul 13, 2020

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

rohany Jul 14, 2020

Choose a reason for hiding this comment

rohany Jul 14, 2020

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

rohany commented Jul 14, 2020 • edited

otan commented Jul 14, 2020

arulajmani left a comment

Choose a reason for hiding this comment

rohany commented Jul 14, 2020

rohany commented Jul 14, 2020

otan commented Jul 14, 2020

arulajmani left a comment

Choose a reason for hiding this comment

otan commented Aug 5, 2020

arulajmani commented Aug 5, 2020

rohany commented Aug 5, 2020

arulajmani commented Aug 5, 2020

arulajmani commented Aug 5, 2020

craig bot commented Aug 5, 2020

otan commented Aug 11, 2020

arulajmani commented Aug 12, 2020

rohany commented Jul 14, 2020 •

edited