cli: fix dump to gracefully handle temp tables, views and sequences #51185

adityamaru · 2020-07-09T02:16:54Z

pg_dump ignores temporary tables, views and sequences. This change
makes cockroach dump follow the same behavior by skipping descriptors
with schema name having pg_temp as its prefix, when aggregating the
metadata for the tables to be dumped.

When no table names are specified in the cockroach dump command, we
silently ignore all temp objects. If the user explicitly asks for a
temp object to be dumped, we error out with an informative message.

Fixes:#50899
Fixes:#51166

Release note (bug fix): cockroach dump no longer errors out when
dumping temporary tables, view or sequences. It either ignores them or
throws an informative error if the temp object is explicitly requested
to be dumped via the CLI.

cockroach-teamcity · 2020-07-09T02:17:01Z

This change is

rohany · 2020-07-09T02:48:35Z

pkg/cli/dump.go

+	if !ok {
+		return basicMetadata{}, fmt.Errorf("unexpected value: %T", schemaNameI)
+	}
+	if strings.Contains(schemaName, sessiondata.PgTempSchemaName) {


not contains, but Prefix

rohany · 2020-07-09T02:49:22Z

pkg/cli/dump_test.go

@@ -858,3 +860,171 @@ INSERT INTO foo (id) VALUES
 		})
 	}
 }
+
+func TestDumpTempTables(t *testing.T) {


use the datadriven dump tests instead -- take a look at pkg/cli/testdata/dump/

rohany · 2020-07-09T02:50:25Z

This PR should be backported to 20.1

donbowman · 2020-07-09T12:16:37Z

I guess if this is accepted #51173 becomes moot.

donbowman · 2020-07-09T14:11:07Z

One thing that nags at me
#42425
suggests that the temp tables/sequences/... are deleted when no longer in use (session gone).
but that does not seem to be the case:

select distinct schema_name from crdb_internal.create_statements;

shows 25 items
show sessions reports 1 (the session i have open doing the show sessions)

Are they not supposed to be removed from here?

adityamaru

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

pkg/cli/dump.go, line 550 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

not contains, but Prefix

good catch, done.

pkg/cli/dump_test.go, line 864 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

use the datadriven dump tests instead -- take a look at pkg/cli/testdata/dump/

as discussed offline, temp tables do not persist across RunWithCaptureArgs() invocations. Therefore, using the datadriven infrastructure does not allow us to test our handling of temp object since they get deleted between sql and dump runs anyways.

rohany · 2020-07-09T14:26:36Z

suggests that the temp tables/sequences/... are deleted when no longer in use (session gone).

They are cleaned up periodically by a background process, not immediately as the session exits.

rohany · 2020-07-09T14:28:20Z

pkg/cli/dump_test.go

@@ -858,3 +860,171 @@ INSERT INTO foo (id) VALUES
 		})
 	}
 }
+
+func TestDumpTempTables(t *testing.T) {


I would just comment here why you didn't use the datadriven tests so that we remember

adityamaru

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

pkg/cli/dump_test.go, line 864 at r2 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I would just comment here why you didn't use the datadriven tests so that we remember

Done.

adityamaru · 2020-07-09T19:58:37Z

Can't reproduce the CI stress fail locally or on gceworker. I suspect it could be because the PGURL name was a constant rather than the convention which is t.Name.

adityamaru · 2020-07-10T00:14:43Z

Pushed to get a CI run in. Testing whether the time statement in main_test.go is the cause of stressrace failure on CI. The test is now passing on gceworker.

adityamaru

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

pkg/cli/main_test.go, line 36 at r3 (raw file):

@RaduBerinde keeping this line is causing the newly added TestDumpTempTables to fail under make stress with - https://teamcity.cockroachdb.com/repository/download/Cockroach_UnitTests/2078378:id/testrace/logTestDumpTempTables_dump_db_only_temp068989627/clitest-stderr.4ab17fe2fbed.roach.2020-07-09T20_12_24Z.018111.log

It also causes the other older unit tests to fail when I run them on a gceworker, with the same error as linked above. I tried moving the time statement into init based on a thread I saw on Slack, but that doesn't work either. Any recommendations on how to remedy this?

knz · 2020-07-13T13:26:40Z

pkg/cli/main_test.go

@@ -31,10 +30,6 @@ func TestMain(m *testing.M) {
 	// a version injected. Pretend to be a very up-to-date version.
 	defer build.TestingOverrideTag("v999.0.0")()

-	// The times for Example_statement_diag are reported in the local timezone.


Why did you change this?

I didn't plan to check-in the deletion, I just wanted to see if that was the cause of themake stress failures for the dump unit tests on CI. I have asked Radu for his input on the same in the comment above - #51185 (review)

Previously, in the case of cli tests we overrode Local to be equal to UTC time so that the output is always the same. This assignment however resulted in data race conditions in several CLI unit tests. Egs: https://teamcity.cockroachdb.com/repository/download/Cockroach_UnitTests/ 2078378:id/testrace/logTestDumpTempTables_dump_db_only_temp068989627/ clitest-stderr.4ab17fe2fbed.roach.2020-07-09T20_12_24Z.018111.log This change ensures all statement diag times are always show in UTC, thereby eliminating the need to override Local. Release note (cli change): statement-diag command will now show all times in UTC.

pg_dump ignores temporary tables, views and sequences. This change makes `cockroach dump` follow the same behavior by skipping descriptors with schema name having `pg_temp` as its prefix, when aggregating the metadata for the tables to be dumped. When no table names are specified in the `cockroach dump` command, we silently ignore all temp objects. If the user explicitly asks for a temp object to be dumped, we error out with an informative message. Fixes:cockroachdb#50899 Release note (bug fix): `cockroach dump` no longer errors out when dumping temporary tables, view or sequences. It either ignores them or throws an informative error if the temp object is explicitly requested to be dumped via the CLI.

adityamaru · 2020-07-13T22:19:10Z

@RaduBerinde as discussed offline I changed Local() to UTC() in the statement_diag. I have separated the change out into its own commit.

RaduBerinde · 2020-07-13T22:36:00Z

Statement diag commit LGTM, thanks!

rohany · 2020-07-13T23:12:28Z

@adityamaru do you know why this was segfaulting though with the Local though?

RaduBerinde · 2020-07-13T23:18:36Z

Looks like it was racing with Go's timer code which sends Now() through some channel. It's enough for any other package to have set up a timer in an init(). Modifying Local is a pretty broken "API".

adityamaru · 2020-07-14T01:48:40Z

TFTRs!

bors r+

craig · 2020-07-14T02:30:49Z

Build succeeded

GitHub CI (Cockroach)

otan · 2020-07-15T05:21:27Z

should we backport this to v20.1?

knz · 2020-07-15T08:26:51Z

it's a bug fix without backward-incompatibility so yes

adityamaru requested review from rohany, miretskiy and a team July 9, 2020 02:16

adityamaru requested a review from a team as a code owner July 9, 2020 02:16

adityamaru removed the request for review from a team July 9, 2020 02:17

otan mentioned this pull request Jul 9, 2020

dump: getBasicMetadata: programming error -- expected just 1 row of result, got more with TEMP SEQUENCE #51166

Closed

adityamaru mentioned this pull request Jul 9, 2020

dump: add dump-public option to skip non-public schema #51173

Closed

rohany suggested changes Jul 9, 2020

View reviewed changes

adityamaru force-pushed the temp-table-dump branch 2 times, most recently from b84834e to da96896 Compare July 9, 2020 14:22

adityamaru commented Jul 9, 2020

View reviewed changes

adityamaru requested a review from rohany July 9, 2020 14:25

rohany reviewed Jul 9, 2020

View reviewed changes

rohany approved these changes Jul 9, 2020

View reviewed changes

adityamaru force-pushed the temp-table-dump branch from da96896 to 967b818 Compare July 9, 2020 14:38

adityamaru commented Jul 9, 2020

View reviewed changes

rohany approved these changes Jul 9, 2020

View reviewed changes

adityamaru force-pushed the temp-table-dump branch from 967b818 to 7b92f4f Compare July 9, 2020 19:56

adityamaru force-pushed the temp-table-dump branch from 7b92f4f to 1854c08 Compare July 10, 2020 00:13

adityamaru commented Jul 10, 2020

View reviewed changes

knz reviewed Jul 13, 2020

View reviewed changes

adityamaru force-pushed the temp-table-dump branch from 1854c08 to 8e97ca2 Compare July 13, 2020 22:14

adityamaru force-pushed the temp-table-dump branch from 8e97ca2 to d8c41da Compare July 13, 2020 22:17

craig bot merged commit 161d68e into cockroachdb:master Jul 14, 2020

adityamaru mentioned this pull request Jul 15, 2020

release-20.1: cli: fix dump to gracefully handle temp tables, views and sequences #51457

Merged

lnhsingh mentioned this pull request Aug 26, 2020

cli: statement-diag CLI is now in UTC cockroachdb/docs#8194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: fix dump to gracefully handle temp tables, views and sequences #51185

cli: fix dump to gracefully handle temp tables, views and sequences #51185

adityamaru commented Jul 9, 2020 •

edited by otan

Loading

cockroach-teamcity commented Jul 9, 2020

rohany Jul 9, 2020

rohany Jul 9, 2020

rohany commented Jul 9, 2020

donbowman commented Jul 9, 2020

donbowman commented Jul 9, 2020

adityamaru left a comment

rohany commented Jul 9, 2020

rohany Jul 9, 2020

adityamaru left a comment

adityamaru commented Jul 9, 2020

adityamaru commented Jul 10, 2020

adityamaru left a comment

knz Jul 13, 2020

adityamaru Jul 13, 2020

adityamaru commented Jul 13, 2020

RaduBerinde commented Jul 13, 2020

rohany commented Jul 13, 2020

RaduBerinde commented Jul 13, 2020

adityamaru commented Jul 14, 2020

craig bot commented Jul 14, 2020

otan commented Jul 15, 2020

knz commented Jul 15, 2020

cli: fix dump to gracefully handle temp tables, views and sequences #51185

cli: fix dump to gracefully handle temp tables, views and sequences #51185

Conversation

adityamaru commented Jul 9, 2020 • edited by otan Loading

cockroach-teamcity commented Jul 9, 2020

rohany Jul 9, 2020

Choose a reason for hiding this comment

rohany Jul 9, 2020

Choose a reason for hiding this comment

rohany commented Jul 9, 2020

donbowman commented Jul 9, 2020

donbowman commented Jul 9, 2020

adityamaru left a comment

Choose a reason for hiding this comment

rohany commented Jul 9, 2020

rohany Jul 9, 2020

Choose a reason for hiding this comment

adityamaru left a comment

Choose a reason for hiding this comment

adityamaru commented Jul 9, 2020

adityamaru commented Jul 10, 2020

adityamaru left a comment

Choose a reason for hiding this comment

knz Jul 13, 2020

Choose a reason for hiding this comment

adityamaru Jul 13, 2020

Choose a reason for hiding this comment

adityamaru commented Jul 13, 2020

RaduBerinde commented Jul 13, 2020

rohany commented Jul 13, 2020

RaduBerinde commented Jul 13, 2020

adityamaru commented Jul 14, 2020

craig bot commented Jul 14, 2020

Build succeeded

otan commented Jul 15, 2020

knz commented Jul 15, 2020

adityamaru commented Jul 9, 2020 •

edited by otan

Loading