-
Notifications
You must be signed in to change notification settings - Fork 49
Dynamic database name #266
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
Conversation
c13c720
to
c3ff66a
Compare
c3ff66a
to
d06e74b
Compare
d06e74b
to
047f71d
Compare
$expanded_list = array(); | ||
foreach ( $columns as $column ) { | ||
$quoted_column = $this->quote_sqlite_identifier( $column ); | ||
if ( str_contains( strtolower( $column ), 'schema' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is quite permissive – can we explicitly list the tables and columns we're interested in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel Ah, sure, we can! In this place, we know for sure that we're dealing with an information schema table column, so maybe we could also check if it contains schema_
or _schema
, although listing is also easy—this should cover it:
'SCHEMA_NAME',
'TABLE_SCHEMA',
'VIEW_SCHEMA',
'INDEX_SCHEMA',
'CONSTRAINT_SCHEMA',
'UNIQUE_CONSTRAINT_SCHEMA',
'REFERENCED_TABLE_SCHEMA',
'TRIGGER_SCHEMA',
Or we could even list the exact columns for each table.
@JanJakes this looks great for information schema tables. However, is this enough to support a dynamic database name? What will happen for queries such as |
@adamziel Using the DB name in the query works. A query like SELECT *
FROM (
SELECT
`TABLE_CATALOG`,
IIF(`TABLE_SCHEMA` = 'information_schema', `TABLE_SCHEMA`, 'wp') AS `TABLE_SCHEMA`,
`TABLE_NAME`,
`TABLE_TYPE`,
`ENGINE`,
`VERSION`,
`ROW_FORMAT`,
`TABLE_ROWS`,
`AVG_ROW_LENGTH`,
`DATA_LENGTH`,
`MAX_DATA_LENGTH`,
`INDEX_LENGTH`,
`DATA_FREE`,
`AUTO_INCREMENT`,
`CREATE_TIME`,
`UPDATE_TIME`,
`CHECK_TIME`,
`TABLE_COLLATION`,
`CHECKSUM`,
`CREATE_OPTIONS`,
`TABLE_COMMENT`
FROM `_wp_sqlite_mysql_information_schema_tables` AS `tables`
) |
@adamziel Oh, but I didn't add any tests for this: USE information_schema;
SELECT * FROM tables;
SELECT * FROM my_db.tables; Will do and make sure it works. |
@adamziel I pushed 3 more commits, all should be addressed now. |
This PR replaces #262. It is a simpler solution to the same problem:
When a table reference targets an information schema table, we replace it with a subquery, injecting the configured database name dynamically:
The same logic will be applied to table references in JOIN clauses as well.
With the new SQLite driver, we keep running into issues with missing and incorrect database name values (#197, #203, #226, #260, #261, and other issues).
Thinking through this further, I came to the conclusion that to provide maximum flexibility and portability, it would be best to provide an API in the shape of:
"Mount
my-sqlite-file.sqlite
asmy-db-name
."Even if we consider adding multi-database support one day, it would still be great to allow mounting additional SQLite files under dynamic database names.
I don't see any downsides to doing this, except maybe that we'll have to choose some database name in some scenarios, such as in database admin tools. However, in these cases we can use a reasonable default or the file name.
This is a WIP pull request demonstrating the simplest approach. For it to be merged, I only need to resolve how the existing database name values should be treated.