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

Add support for creating temporary tables in schema #6409

Open
wants to merge 15 commits into
base: 4.3.x
Choose a base branch
from

Conversation

alessandromorelli
Copy link

This allows creating temporary tables in PostgreSQL specifying also the on commit options.

Q A
Type improvement
Fixed issues N/A

Summary

Sometimes it is useful to create temporary tables and this PR adds the support to do so with the schema manager.

I added the handling of the relevant options and I provided tests and documentation.

This allows creating temporary tables in PostgreSQL specifying also the on commit options.
`unlogged <https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-UNLOGGED>`_

- **temporary** (boolean): Set a PostgreSQL table type as
`temporary <https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-TEMPORARY>`_
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look like a PostgreSQL-specific concept to me 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Well, it isn't per se but since I have access to PostgreSQL mainly, I implemented it just on the PostgreSQL platform.

@derrabus derrabus changed the base branch from 4.0.x to 4.1.x August 14, 2024 10:54
@derrabus
Copy link
Member

What is the status of this PR? I see a failing CI and a feature that works on Postgres only. Are you planning to continue your work?

@alessandromorelli
Copy link
Author

I'll try to fix the failing CI later today but I'll keep the implementation to Postgres only.

I don't have access to other platforms to try

@derrabus
Copy link
Member

In that case, I don't think that we're interested in this change. Either we implement a feature with broad platform support or we don't implement it at all.

This allows creating temporary tables in PostgreSQL specifying also the on commit options.
@derrabus derrabus changed the base branch from 4.1.x to 4.2.x August 15, 2024 09:18
@derrabus
Copy link
Member

Can you please rebase your changes? We don't accept PRs with merge commits.

@alessandromorelli
Copy link
Author

Digging further in the code, I see that support for temporary tables was implemented but not documented for MySQL.

I've implemented the support for all the remaining platforms (except for SQL Server, which doesn't have a different DML to create temporary tables), added tests and documented the feature.

tests/Platforms/SQLitePlatformTest.php Outdated Show resolved Hide resolved
Comment on lines -257 to +267
if (! empty($options['temporary'])) {
$temporary = $options['temporary'] ?? false;
if (! is_bool($temporary)) {
throw new InvalidArgumentException(sprintf(
'invalid temporary specification for table %s',
$name,
));
}

if ($temporary === true) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a BC break. Do we need this additional strictness?

Comment on lines +259 to +267
$statement = match ($options['temporary'] ?? '') {
'' => 'CREATE TABLE ',
'created' => 'CREATE GLOBAL TEMPORARY TABLE ',
'declared' => 'DECLARE GLOBAL TEMPORARY TABLE ',
default => throw new InvalidArgumentException(sprintf(
'invalid temporary specification for table %s',
$name,
))
};
Copy link
Member

Choose a reason for hiding this comment

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

Okay, so for MySQL we demand that the temporary property is a bool, but for DB2 we want it to be a string? Can you elaborate on the difference between those two options? Do we need to support both?


$sqls = parent::_getCreateTableSQL($name, $columns, $options);
Copy link
Member

Choose a reason for hiding this comment

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

Removing the call to the parent implementation seems to bloat this method big time. Can we somehow restore that call and make the diff smaller?

@@ -313,9 +317,62 @@ public function getListSequencesSQL(string $database): string
*/
protected function _getCreateTableSQL(string $name, array $columns, array $options = []): array
{
$indexes = $options['indexes'] ?? [];
$options['indexes'] = [];
$sql = parent::_getCreateTableSQL($name, $columns, $options);
Copy link
Member

Choose a reason for hiding this comment

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

Same question here. Do we have to remove the call to the parent method?

Comment on lines +332 to +340
$temporary = match ($options['temporary'] ?? '') {
'' => '',
'global' => 'GLOBAL TEMPORARY ',
'private' => 'PRIVATE TEMPORARY ',
default => throw new InvalidArgumentException(sprintf(
'invalid temporary specification for table %s',
$name,
))
};
Copy link
Member

Choose a reason for hiding this comment

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

Same question here. Why are my options for temporary different for each platform?

@derrabus
Copy link
Member

except for SQL Server, which doesn't have a different DML to create temporary tables

TIL: SQL Server implements temporary tables via naming conventions. 🤦🏻‍♂️

https://learn.microsoft.com/en-us/sql/relational-databases/tables/tables?view=sql-server-ver16#temporary-tables

@derrabus
Copy link
Member

Okay, but seriously, what would happen if the app attempted to create a temporary table on SQL server via DBAL? Do we silently create a permanent one? Is that what we want?

@alessandromorelli
Copy link
Author

To respond to your comments:

  • DB2 has 2 kinds of temporary tables, created and declared: the main difference is that while data is always purged at the end of a session, created temporary table definitions are retained and declared temporary tables are dropped.
  • Oracle has also 2 kinds of temporary tables, global and private: global tables definitions are retained and visible to all connections, private are neither retained nor visible outside the current session, sorta like DB2 but a bit different.
  • PostgreSQL has only one kind of temporary tables but for compatibility accepts a GLOBAL | LOCAL modifier. The use of the modifier, while adhering to the SQL standard is discouraged because it might be implemented in the future, causing potential breakages, if the code assumes it will be ignored.
  • Both PostgreSQL and Oracle allow to specify whether rows are preserved or deleted on commit, but PostgreSQL allows also a non standard clause ON COMMIT DROP.
  • SQLite, MySQL and variants allow only a TEMPORARY modifier to the create table statement.

This being the situation, I think there are two alternatives:

  • having a single boolean flag across all platforms (choosing arbitrarily some defaults for Oracle and DB2) and a second option to specify the variants for Oracle and DB2
  • having different value options per platform, with sanity checks

To avoid silently creating a permanent table on SQL Server, I would add a check on the SQL Server platform, raising an exception if the temporary option is specified.

Last, to avoid overriding the base _getCreateTableSQL method would require a partial rewrite to add at least two extension points, one for the CREATE TABLE modifier and one for the ON COMMIT clause.

Plus, all the other platforms already override the method, without calling the parent method at all.

@derrabus
Copy link
Member

Thank you very much for your thorough analysis!

  • having a single boolean flag across all platforms (choosing arbitrarily some defaults for Oracle and DB2) and a second option to specify the variants for Oracle and DB2

This would actually be my favorite. We should be able to define a schema that we can deploy to all supported platforms. And the defaults shouldn't be arbitrary as you put it. We should pick defaults that bring us as close as possible to the behavior supported by the other platforms.

To avoid silently creating a permanent table on SQL Server, I would add a check on the SQL Server platform, raising an exception if the temporary option is specified.

Yeah, that's probably the best we can do.

I thought about changing the name according to SQL Server's convention. So, if a temporary table "foo" is to be created, we would create ##foo instead. Then again, silently changing the table name would be weird, I guess.

Last, to avoid overriding the base _getCreateTableSQL method would require a partial rewrite to add at least two extension points, one for the CREATE TABLE modifier and one for the ON COMMIT clause.

That sounds about right.

Plus, all the other platforms already override the method, without calling the parent method at all.

Understood. I wanted to make the life easier for reviewers like myself. But if you say that it's not worth introducing those extensions points, I trust your judgement.

@alessandromorelli
Copy link
Author

The difficulty in reducing the schema to a common baseline across all platforms lies in the profound difference between Oracle/DB2 and the rest of the platforms.

While on MySQL, PostgreSQL and SQLite temporary tables are only visible to the current session and each session can simultaneously create the same temporary table, on Oracle and DB2 this is not true, temporary table schema is shared among sessions, only data is session-specific.

Private temporary tables on Oracle and declared temporary tables on DB2 behave similarly to the other platforms, but only on DB2 we can approximate the behaviour of the other platforms: on Oracle private temporary tables are also required to adhere to a naming convention, bringing up the same problem as on SQL server.

It seems to me that keeping the option vendor specific is the better compromise.

Perhaps the temporary option can be split into multiple options like postgres:temporary, sqlite:temporary, ora:temporary, etc. but it feels a bit cumbersome.

@alessandromorelli
Copy link
Author

Sorry, my bad, should I rebase on the 4.2.x branch?

@derrabus
Copy link
Member

derrabus commented Sep 4, 2024

Yes, 4.2.x is the target branch for new features at the moment.

Okay, so we have basically two types of temporary tables.

  • Type A is a table that I create once in my schema and that can hold temporary data. Unlike other tables, that table is empty at the beginning of my session. All data I write to that table is visible to me only and at the end of my session, the table gets truncated.
  • Type B is a table that itself exists only temporarily. I have to recreate it for each session that needs that table because at the end of my session, the table is dropped.

Did I summarize this correctly? Can we fit all implementations into those two types, meaning that all supported DBMS support either type A or type B or both?

@derrabus derrabus changed the base branch from 4.2.x to 4.3.x October 10, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants