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

[Attach] Fix bug causing sequences to break attaching databases. #11327

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Mar 23, 2024

This PR fixes #10263

Essentially, this issue stems from code being shared between database loading and attaching a new database.
This function is SingleFileCheckpointReader::LoadFromStorage

It was originally made to handle loading the initial database from disk, meaning no existing transaction manager was present.
Because of this, it always created a new connection to the database and manually started a transaction, to meet the requirement imposed by the binder - which is that there has to be an active transaction attached to these actions.

Since we are coming from PhysicalAttach, we already have an existing database, and connection with transaction running, in which we are creating this AttachedDatabase.
Because of the behavior of LoadFromStorage, we create a new transaction in which we are loading the contents of the database.

Because there are now two transactions active, one that has created the AttachedDatabase and another that is populating the database, neither transaction can see the changes made by the other.
That causes the lookup for db1.seq to fail, because it can't see db1.

We fix this by providing proper context to LoadFromStorage to let it know there is an existing transaction it can use, instead of starting a new one.

Copy link
Contributor

@douenergy douenergy left a comment

Choose a reason for hiding this comment

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

Thanks, @Tishj, I have learned more about CheckpointManager from your PR.

@@ -46,7 +46,7 @@ class StorageManager {
static StorageManager &Get(Catalog &catalog);

//! Initialize a database or load an existing database from the given path
void Initialize();
void Initialize(optional_ptr<ClientContext> context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also set 'nullptr' as the default value for StorageManager.Initialize?

@Mytherin Mytherin merged commit cc8af50 into duckdb:main Mar 25, 2024
45 checks passed
@Mytherin
Copy link
Collaborator

Thanks! LGTM

github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Mar 28, 2024
Merge pull request duckdb/duckdb#11327 from Tishj/attach_sequence_fix
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.

Sequence missing from catalog with ATTACH
3 participants