Skip to content
This repository was archived by the owner on Jun 2, 2025. It is now read-only.

Information schema cleanup and improvements #49

Merged
merged 9 commits into from
May 26, 2025

Conversation

JanJakes
Copy link
Contributor

@JanJakes JanJakes commented May 12, 2025

The information schema builder requires a few small touches for the new driver release:

  • It creates some tables that we don't support yet — let's remove those queries.
  • It doesn't maintain the table_constraints table. This table will be needed in the future, and since it also store some information about primary and unique constraints, let's store it now, so we don't have to backfill it with a migration later on.
  • Make sure identifiers are case-insensitive
  • Verify index and constraint naming — e.g., what happens when both constraint name and index name are specified, and how duplicate names are handled.

@JanJakes JanJakes force-pushed the information-schema-cleanup branch from efe431a to 87a14cb Compare May 21, 2025 16:54
JanJakes added 3 commits May 21, 2025 20:59
The previous CTE approach modified only columns that were recorded in
the statistics table, which was a bug for some DROP statements, when
statistic data could already have been removed and as a result the column
data was not correctly updated.

The approach with a subquery solves that by updating all column records
for a given table (resetting "column_key" to "" when there is no column key).
@JanJakes JanJakes force-pushed the information-schema-cleanup branch from 87a14cb to bd2c533 Compare May 21, 2025 18:59
@JanJakes JanJakes force-pushed the information-schema-cleanup branch from 8a731c7 to 5ed53f0 Compare May 26, 2025 09:00
@JanJakes JanJakes force-pushed the information-schema-cleanup branch from 489deb8 to ad3f600 Compare May 26, 2025 10:15
@JanJakes JanJakes force-pushed the information-schema-cleanup branch from 129cc2f to 03d8ee5 Compare May 26, 2025 12:01
@JanJakes JanJakes marked this pull request as ready for review May 26, 2025 12:11
@JanJakes JanJakes requested a review from adamziel May 26, 2025 12:11

/*
* At the moment, we only support ASCII bytes in all identifiers.
* This is because SQLite doesn't support case-insensitive Unicode
Copy link
Contributor

Choose a reason for hiding this comment

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

It's great to surface this error in an explicit way, thank you!

Thinking about the future, if identifiers are case-insensitive in PHP, could we lowercase them effectively in PHP and pass the transformed names to SQLite? Although that opens more questions – are column names raw bytes, or are they normalized? Yeah, perhaps not going there is for the best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamziel It's definitely something we could solve on the PHP side (the simplest way probably requires mbstring), but yeah, we'd need to be careful exactly about what you mention — the column name will be stored as raw bytes in MySQL, but comparisons would need to be done normalized.

return $name;
}

// The name is used - find the first unused name.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't suppose this would ever take a long time, but if it ever does we could explore uuids

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamziel Even in a table with hundreds of indexes, the time would probably be negligible. The reason why it's done exactly this way is that it's what MySQL seems to do, so it's nice to have it compatible.

@JanJakes
Copy link
Contributor Author

@adamziel Thanks for such a quick review! ❤️

@JanJakes JanJakes merged commit a2ea9f8 into develop May 26, 2025
12 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants