sql: Implement DISCARD TEMP #86246

e-mbrown · 2022-08-16T19:25:29Z

Release note (sql change): We now support DISCARD TEMP/TEMPORARY, which drops all temporary tables created in the current session. The command does not drop temporary schemas.

Release note (bug fix): DISCARD ALL now deletes temporary tables

Release justification: Low risk, high benefit changes to existing functionality(DISCARD ALL)

cockroach-teamcity · 2022-08-16T19:25:38Z

This change is

ajwerner · 2022-08-18T16:00:02Z

How is this a bug fix? Seems like a new feature to me.

e-mbrown

Updated

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

rafiss · 2022-08-26T18:19:23Z

the bug fix part of this is that DISCARD ALL was supposed to also run the DISCARD TEMP logic, but it wasn't

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @rafiss)

pkg/sql/discard.go line 55 at r1 (raw file):

		//	DISCARD TEMP
		err := prepareDeleteTempTables(ctx, p)

the prepare part of the function name seems unnecessary

pkg/sql/discard.go line 94 at r1 (raw file):

}

func getTempSchema(

why is this named getTempSchema? it looks like it is returning sessionIDs

pkg/sql/discard.go line 106 at r1 (raw file):

		}

		for _, scEntry := range schemaEntries {

you shouldn't need to iterate here -- actually it would be wrong since this will mean that it will delete all of the temporary tables including those from other sessions.

instead use p.SessionData().DatabaseIDToTempSchemaID[dbDesc.ID] to get the temp schema for each DB (if one exists)

pkg/sql/discard.go line 109 at r1 (raw file):

			isTempSchema, sessionID, err := temporarySchemaSessionID(scEntry.Name)
			if err != nil {
				// This should not cause an error.

this should return the error instead of logging and ignoring it

pkg/sql/discard.go line 119 at r1 (raw file):

		}
	}
	log.Infof(ctx, "found %d temporary schemas", len(sessionIDs))

no need to log this

pkg/sql/discard.go line 137 at r1 (raw file):

	for _, dbDesc := range allDbDescs {
		err = removeTempTable(ctx, p, ie, descCol, dbDesc, sessionIDs)

can you instead call cleanupSchemaObjects, which already exists?

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @rafiss)

pkg/sql/discard.go line 145 at r1 (raw file):

}

func removeTempTable(

with the above comment, there shouldn't be a need for this function

e-mbrown

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

pkg/sql/discard.go line 55 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

the prepare part of the function name seems unnecessary

Done.

pkg/sql/discard.go line 94 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

why is this named getTempSchema? it looks like it is returning sessionIDs

Done.

pkg/sql/discard.go line 106 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

you shouldn't need to iterate here -- actually it would be wrong since this will mean that it will delete all of the temporary tables including those from other sessions.

instead use p.SessionData().DatabaseIDToTempSchemaID[dbDesc.ID] to get the temp schema for each DB (if one exists)

Done.

pkg/sql/discard.go line 109 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this should return the error instead of logging and ignoring it

Done.

pkg/sql/discard.go line 119 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

no need to log this

Done.

pkg/sql/discard.go line 137 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can you instead call cleanupSchemaObjects, which already exists?

Done.

pkg/sql/discard.go line 145 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

with the above comment, there shouldn't be a need for this function

Done.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @rafiss)

pkg/sql/discard.go line 69 at r3 (raw file):

		})
	case tree.DiscardModeTemp:
		err := deleteTempTables(params.ctx, params.p)

this needs to be called as part of DISCARD ALL too

pkg/sql/logictest/testdata/logic_test/discard line 120 at r3 (raw file):

statement ok
SET experimental_enable_temp_tables=on

can you test what happens if DISCARD TEMP is called before creating any temp tables? i would expect SELECT count(*) FROM [SHOW SCHEMAS] WHERE schema_name LIKE 'pg_temp_% to return 0 both before and after the DISCARD TEMP

e-mbrown

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

pkg/sql/discard.go line 69 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this needs to be called as part of DISCARD ALL too

Done.

pkg/sql/logictest/testdata/logic_test/discard line 120 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can you test what happens if DISCARD TEMP is called before creating any temp tables? i would expect SELECT count(*) FROM [SHOW SCHEMAS] WHERE schema_name LIKE 'pg_temp_% to return 0 both before and after the DISCARD TEMP

Done.

rafiss

very nice, lgtm!

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

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @rafiss)

-- commits line 4 at r4:
can you also add another note like Release note (bug fix): DISCARD ALL now deletes temporary tables

Release note (sql change): We now support DISCARD TEMP/TEMPORARY, which drops all temporary tables created in the current session. The command does not drop temporary schemas. Release note (bug fix): DISCARD ALL now deletes temporary tables Release justification: Bug Fix

rafiss

bors r+

craig · 2022-08-31T19:16:57Z

Build succeeded:

Bazel Essential CI (Cockroach)

In cockroachdb#86246 we introduced logic to discard schemas when running DISCARD ALL and DISCARD TEMP. This logic did expensive kv operations unconditionally; if the session knew it had never created a temporary schema, we'd still fetch all databases and proceed to search all databases for a temp schema. This is very expensive. Fixes: cockroachdb#95864 Release note (performance improvement): In 22.2, we introduced support for `DISCARD TEMP` and made `DISCARD ALL` actually discard temp tables. This implementation ran expensive logic to discover temp schemas rather than consulting in-memory data structures. As a result, `DISCARD ALL`, which is issued regularly by connection pools, became an expensive operation when it should be cheap. This problem is now resolved.

94153: sql: Remove type annotations from `column_default` in `information_schema.columns` r=ZhouXing19 a=e-mbrown Informs: #87774 The type annotation on `column_default` caused confusion in postgres compatible apps. This change formats the `column_default` without the type annotation. It does not address the incorrect type annotation. Release note (bug fix): The content of `column_default` in `information_schema.columns` no longer have type annotations. 95855: backupccl: run restore/tpce/32tb/aws/nodes=15/cpus=16 once a week r=benbardin a=msbutler This patch adds the restore/tpce/32TB/aws/nodes=15/cpus=16 test to our weekly test suite. The backup fixture was generated by initing a tpce cluster with 2 Million customers, and running this workload while 48 incremental backups were taken at 15 minute increments. The roachtest restores from a system time captured in the 24th backup. The cluster used to restore the backup will run on aws with 15 nodes each with 16 vcpus. To verify this test exists, run: `roachtest list tag:weekly` Release note: None Epic: none 95861: kv: remove lastToReplica and lastFromReplica from raftMu r=nvanbenschoten a=nvanbenschoten Extracted from #94165 without modification. ---- In 410ef29, we moved these fields from under the Replica.mu to under the Replica.raftMu. This was done to avoid lock contention. In this commit, we move these fields under their own mutex so that they can be accessed without holding the raftMu. This allows us to send Raft messages from other goroutines. The commit also switches from calling RawNode.ReportUnreachable directly in sendRaftMessage to using the more flexible unreachablesMu set, which defers the call to ReportUnreachable until the next Raft tick. As a result, the commit closes #84246. Release note: None Epic: None 95876: sql: remove round-trips from common DISCARD ALL r=ajwerner a=ajwerner #### sql: avoid checking if a role exists when setting to the current role This is an uncached round-trip. It makes `DISCARD` expensive. In #86485 we implemented `SET SESSION AUTHORIZATION DEFAULT`, this added an extra sql query to `DISCARD ALL` to check if the role we'd become exists. This is almost certainly unintentional in the case where the role we'd become is the current session role. I think this just happened because of code consolidation. We now no longer check if the current session role exists. This removes the last round-trip from DISCARD ALL. #### sql: use in-memory session data to decide what to do in DISCARD In #86246 we introduced logic to discard schemas when running DISCARD ALL and DISCARD TEMP. This logic did expensive kv operations unconditionally; if the session knew it had never created a temporary schema, we'd still fetch all databases and proceed to search all databases for a temp schema. This is very expensive. Fixes: #95864 Release note (performance improvement): In 22.2, logic was added to make `SET SESSION AUTHORIZATION DEFAULT` not a no-op. This implementation used more general code for setting the role for a session which made sure that the role exists. The check for whether a role exists is currently uncached. We don't need to check if the role we already are exists. This improves the performance of `DISCARD ALL` in addition to `SET SESSION AUTHORIZATION DEFAULT`. Release note (performance improvement): In 22.2, we introduced support for `DISCARD TEMP` and made `DISCARD ALL` actually discard temp tables. This implementation ran expensive logic to discover temp schemas rather than consulting in-memory data structures. As a result, `DISCARD ALL`, which is issued regularly by connection pools, became an expensive operation when it should be cheap. This problem is now resolved. 95997: ui: hide list of statement for non-admins r=maryliag a=maryliag To get the list of fingerprints used by an index, we use the `system.statement_statistics` directly, but non-admins don't have access to system table. Until #95756 or #95770 are done, we need to hide this feature for non-admins. Part Of #93087 Release note (ui change): Hide list of used fingerprint per index on Index Details page, for non-admin users. 95998: roachtest: update activerecord blocklist r=ZhouXing19 a=andyyang890 This patch removes a few tests from the blocklist that do not exist in the test suite for rails 7.0.3, which were being run prior to the fix for version pinning. Informs #94211 Release note: None 96003: kv: add MVCC local timestamp details to ReadWithinUncertaintyIntervalError r=nvanbenschoten a=nvanbenschoten This PR adds details about MVCC local timestamps to ReadWithinUncertaintyIntervalErrors. This provides more information about the cause of uncertainty errors. The PR also includes a series of minor refactors to clean up this code. Release note: None Epic: None Co-authored-by: e-mbrown <[email protected]> Co-authored-by: Michael Butler <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: maryliag <[email protected]> Co-authored-by: Andy Yang <[email protected]>

In cockroachdb#86246 we introduced logic to discard schemas when running DISCARD ALL and DISCARD TEMP. This logic did expensive kv operations unconditionally; if the session knew it had never created a temporary schema, we'd still fetch all databases and proceed to search all databases for a temp schema. This is very expensive. Fixes: cockroachdb#95864 Release note (performance improvement): In 22.2, we introduced support for `DISCARD TEMP` and made `DISCARD ALL` actually discard temp tables. This implementation ran expensive logic to discover temp schemas rather than consulting in-memory data structures. As a result, `DISCARD ALL`, which is issued regularly by connection pools, became an expensive operation when it should be cheap. This problem is now resolved.

e-mbrown requested a review from a team August 16, 2022 19:25

e-mbrown requested a review from a team as a code owner August 16, 2022 19:25

e-mbrown changed the title ~~sql: Implement Discard Temp~~ sql: Implement DISCARD TEMP Aug 16, 2022

e-mbrown force-pushed the eb/discardtemp branch 2 times, most recently from 5c7c213 to 5719290 Compare August 18, 2022 15:09

e-mbrown requested a review from rafiss August 18, 2022 15:46

e-mbrown commented Aug 18, 2022

View reviewed changes

e-mbrown force-pushed the eb/discardtemp branch from 5719290 to caa21ad Compare August 22, 2022 17:15

rafiss requested changes Aug 26, 2022

View reviewed changes

e-mbrown force-pushed the eb/discardtemp branch from caa21ad to 8f3b075 Compare August 30, 2022 18:24

e-mbrown commented Aug 30, 2022

View reviewed changes

e-mbrown requested a review from rafiss August 30, 2022 18:25

e-mbrown force-pushed the eb/discardtemp branch from 8f3b075 to 94bd6fc Compare August 30, 2022 19:19

rafiss requested changes Aug 30, 2022

View reviewed changes

e-mbrown force-pushed the eb/discardtemp branch from 94bd6fc to 09c4650 Compare August 31, 2022 13:53

e-mbrown commented Aug 31, 2022

View reviewed changes

e-mbrown requested a review from rafiss August 31, 2022 13:53

rafiss approved these changes Aug 31, 2022

View reviewed changes

e-mbrown force-pushed the eb/discardtemp branch from 09c4650 to 50639e4 Compare August 31, 2022 16:20

rafiss approved these changes Aug 31, 2022

View reviewed changes

craig bot merged commit 0602871 into cockroachdb:master Aug 31, 2022

ajwerner mentioned this pull request Jan 25, 2023

'DISCARD ALL' performance regression on v22.2 #95864

Closed

ajwerner mentioned this pull request Jan 25, 2023

sql: remove round-trips from common DISCARD ALL #95876

Merged

ajwerner mentioned this pull request Jan 27, 2023

release-22.2: sql: fix extra round trips during DISCARD, fix temp discard bug #96102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Implement DISCARD TEMP #86246

sql: Implement DISCARD TEMP #86246

e-mbrown commented Aug 16, 2022 •

edited

Loading

cockroach-teamcity commented Aug 16, 2022

ajwerner commented Aug 18, 2022

e-mbrown left a comment

rafiss commented Aug 26, 2022

rafiss left a comment

rafiss left a comment

e-mbrown left a comment

rafiss left a comment

e-mbrown left a comment

rafiss left a comment

rafiss left a comment

rafiss left a comment

craig bot commented Aug 31, 2022

sql: Implement DISCARD TEMP #86246

sql: Implement DISCARD TEMP #86246

Conversation

e-mbrown commented Aug 16, 2022 • edited Loading

cockroach-teamcity commented Aug 16, 2022

ajwerner commented Aug 18, 2022

e-mbrown left a comment

Choose a reason for hiding this comment

rafiss commented Aug 26, 2022

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

e-mbrown left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

e-mbrown left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

craig bot commented Aug 31, 2022

e-mbrown commented Aug 16, 2022 •

edited

Loading