Skip to content

sql: fix drop schema bug which corrupts schemas using the name of the db#63119

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
ajwerner:ajwerner/fix-drop-schema
Apr 6, 2021
Merged

sql: fix drop schema bug which corrupts schemas using the name of the db#63119
craig[bot] merged 2 commits intocockroachdb:masterfrom
ajwerner:ajwerner/fix-drop-schema

Conversation

@ajwerner
Copy link
Copy Markdown
Contributor

@ajwerner ajwerner commented Apr 5, 2021

This bug has obscure impact but is rather egregious. When dropping a schema
we would add entry with the name of it's parent database rather than itself
into the database's name map. This generally means we'll have an orphaned
entry in that map corresponding to the database itself. That's only going to
be a problem if you want to create a schema of the same name as the database.
We can make a follow-up patch to have the code ignore entries (remove at
deserialization time) that correspond to the database itself and then we
can write a long-running migration. For now we're going to fix the root
cause.

This bug was exposed by cross-descriptor validation of schemas. Prior
to this fix, you'd get an ugly error like the following:

        (XX000) internal error: schema "samename" (155): not present in parent database [152] schemas mapping
        schema_desc.go:190: in ValidateCrossReferences()
        DETAIL: stack trace:

I am worried that we're going to see support issues for this (starting in 20.2) and we'll need
a follow-up change to do some automated repair or robustness to this problem.

Fixes #62920

Release note (bug fix): Fixed a bug in user-defined schemas whereby the
dropping of any schema may prevent creation of schemas with the name of
the database and may corrupt already existing schemas of that name.

@ajwerner ajwerner requested a review from rafiss April 5, 2021 21:58
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! the path/filepath import needs to be removed from rsg_test, and it looks good to go

ajwerner added 2 commits April 6, 2021 09:51
…ress

The testdata is copied over to the stress nodes. Running these rsg tests at
scale reveals problems rapidly.

Release note: None
This bug has obscure impact but is rather egregious. When dropping a schema
we would add entry with the name of it's parent database rather than itself
into the database's name map. This generally means we'll have an orphaned
entry in that map corresponding to the database itself. That's only going to
be a problem if you want to create a schema of the same name as the database.
We can make a follow-up patch to have the code ignore entries (remove at
deserialization time) that correspond to the database itself and then we
can write a long-running migration. For now we're going to fix the root
cause.

This bug was exposed by cross-descriptor validation of schemas. Prior
to this fix, you'd get an ugly error like the following:

```
        (XX000) internal error: schema "samename" (155): not present in parent database [152] schemas mapping
        schema_desc.go:190: in ValidateCrossReferences()
        DETAIL: stack trace:
```

I am worried that we're going to see support issues for this and we'll need
a follow-up change to do some automated repair or robustness to this problem.

Release note (bug fix): Fixed a bug in user-defined schemas whereby the
dropping of any schema may prevent creation of schemas with the name of
the database and may corrupt already existing schemas of that name.
@ajwerner
Copy link
Copy Markdown
Contributor Author

ajwerner commented Apr 6, 2021

bors r=rafiss

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Apr 6, 2021

Build succeeded:

@craig craig bot merged commit e663531 into cockroachdb:master Apr 6, 2021
@rafiss rafiss added this to the 21.1 milestone Apr 22, 2021
craig bot pushed a commit that referenced this pull request Aug 16, 2021
68567: colrpc: enhance warnings from the outbox r=yuzefovich a=yuzefovich

This commit marks several string constants as "safe" from the
redactability perspective so that the warnings logged by the outboxes
are more helpful. Additionally, several minor nits around error
formatting are addressed.

Release note: None

68675: sql: implement IterateStatementStats() for PersistedSQLStats r=maryliag a=Azhng

Depends on: #68555, #68620
Related to: #64743

Previously, IterateStatementStats() for PersistedSQLStats was
left unimplemented and it defaults to the implementation of
SQLStats.IterateStatementStats(). This means calls to
IterateStatementStats() on PersistedSQLStats cannot read
the statement statistitcs stored in system table.

This commit implements the IterateStatementStats() through
the new CombinedStmtStatsIterator which enables this method
to read both in-memory and persited statement statistics.

Release note: None

68738: sql/catalog/dbdesc: repair old descriptors corrupted due to DROPPED SCHEMA name r=ajwerner a=sajjadrizvi

#63119 fixes a bug that corrupts a database descriptor when a child schema was 
dropped, adding an entry in schema-info structure erroneously with database name
instead of schema name . Although the bug was fixed , there can be database backups 
with corrupted descriptors.

This commit adds a post-deserialization function to repair a corrupted descriptor. Moreover,
it adds a test function to ensure that descriptors with such corruption are fixed during
migration.
 
Release note: None
 
Fixes: #63148


68903: github: redirect KV code reviews to @cockroachdb/kv-prs r=irfansharif a=irfansharif

We try to use the @cockroachdb/kv alias for notifying the entire team on
issues. There's no way to disable notifications for github's automatic
codeowners driven review requests, and that tends to be a firehose, so
lets use this sub-team alias instead.

Release note: None

68978: changefeedccl: detect sink URLs with no scheme r=HonoreDB a=stevendanna

Previously, if a user provided a sink URL with no scheme (such as
` kafka%3A%2F%2Fnope%0A`), a changefeed job would be started. However,
this changefeed job would be writing into a bufferSink.  The
bufferSink is used by core changefeeds.

The user may have provided such a URL because of confusion over how to
URL encode their sink URL.

Now, they will receive an error such as

```
pq: no scheme found for sink URL "kafka%3A%2F%2Fnope%0A"
```

Release note (enterprise change): CHANGEFEED statements now error
if the provided sink URL does not contain a scheme. Such URLs are
typically a mistake and will result in non-functional changefeeds.

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Azhng <archer.xn@gmail.com>
Co-authored-by: Sajjad Rizvi <sajjad@cockroachlabs.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
Co-authored-by: Steven Danna <danna@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sql/tests: TestRandomSyntaxGeneration failed

4 participants