Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Catalog] Fix locking issues + identify problem in MappingValue #9523

Merged
merged 1 commit into from Nov 3, 2023

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Oct 30, 2023

The test this adds is skipped because it's not entirely fixed by this PR.

The issues fixed by this PR were found through the test, but there is a bigger underlying problem that needs more thought.

To summarize the issues fixed by this PR:
When we read from a catalog, we need to grab the CatalogSet's catalog_lock, even when we already have the Catalog's write_lock.

The issue that isn't fixed is that DeleteMapping is too eager, it deletes mappings too eagerly, I think we need a refcount similar to what we do for EntryValue<->EntryIndex so we can be aware that there are other transactions that make use of the mapping.


The issue highlighted by the added test is that sometimes this could result in a crash.
When we grab a reference to a CatalogEntry and that entry is subsequently deleted after we release the lock, we're holding random memory.

* thread #23, stop reason = EXC_BAD_ACCESS (code=1, address=0x40)
    frame #0: 0x0000000102bceba8 libduckdb.dylib`duckdb::Binder::Bind(this=0x0000000118f5f9e8, stmt=<unavailable>) at bind_simple.cpp:23:26 [opt]
   20           auto entry = Catalog::GetEntry(context, stmt.info->GetCatalogType(), stmt.info->catalog, stmt.info->schema,
   21                                          stmt.info->name, stmt.info->if_not_found);
   22           if (entry) {
-> 23                   auto &catalog = entry->ParentCatalog();
   24                   if (!entry->temporary) {
   25                           // we can only alter temporary tables/views in read-only mode
   26                           properties.modified_databases.insert(catalog.GetName());

When I comment out the DeleteMapping call I can not reproduce this crash


I've also tried taking out the DeleteMapping and moving it into CommitState::CommitEntry so it only happens when the transaction is successful, but that still didn't fix it.

@Tishj Tishj requested a review from Mytherin October 31, 2023 07:13
@Tishj
Copy link
Contributor Author

Tishj commented Oct 31, 2023

@Mytherin this should be fine to merge, I would like your thoughts on the CatalogEntry getting deleted after it was looked up in Binder::Binder(AlterStatement &)

Just saw my local CI failed on an unrelated error though?
https://github.com/Tishj/duckdb/actions/runs/6698175808/job/18204504822

CMake Error at CMakeLists.txt:607 (add_dependencies):
Error:   The dependency target "postgres_scanner_extension" of target
  "duckdb_local_extension_repo" does not exist.
Call Stack (most recent call first):
  CMakeLists.txt:1080 (add_extension_dependencies)

@carlopi
Copy link
Contributor

carlopi commented Oct 31, 2023

duckdb_local_extension_repo has been solved by: #9496, now merged, so that it's not a problem

(I am referring to Tishj's last comment, overall issue I have no idea of)

@Mytherin Mytherin merged commit db3d653 into duckdb:main Nov 3, 2023
43 checks passed
@Mytherin
Copy link
Collaborator

Mytherin commented Nov 3, 2023

Thanks!

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.

None yet

3 participants