Skip to content

Support precision option in datetime types (PostgreSQL) #6742

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

Open
wants to merge 2 commits into
base: 4.2.x
Choose a base branch
from

Conversation

upyx
Copy link

@upyx upyx commented Jan 27, 2025

Q A
Type feature
Fixed issues #2873 #6631
Refers #5961

Summary

The year was 2025... I made yet another try to add support of microseconds to date-time types.

Looks like a really big work on #5961 is abandoned. Sadly. I've reviewed this PR. I think it modifies too many things and attempts to add microseconds everywhere. It reminds me of PHP 6 in trying to add support for Unicode globally.

Let's make things easy!

What it does

It adds to PostgreSQL platform support of microseconds for the next types:

  • DateTimeType
  • DateTimeImmutableType
  • DateTimeTzType
  • DateTimeImmutableTzType

To configure precision (in PostgreSQL documentation it calls "precision," not "scale") of those types, the precision option is available. For backward compatibility, the default precision is zero.

Additionally, it respects precision in schema introspection (fix #6631). In order to distinguish timestamp (without precision) and timestamp(6) the without_precision platform option was added.

VarDateTimeType and VarDateTimeImmutableType

According to description of these types, DateTime::__construct() twice slower then DateTime::createFromFormat(). I cannot remember the times when it was true. In my tests on PHP 8.1 (which is a lower supported version for 4.x), the constructor is slightly faster.

Script:

$i = 1000000;
while ($i--) {
    $dates[] = date('Y-m-d H:i:s.', random_int(0, 2 << 30)) . random_int(0, 999999);
}

echo end($dates) . PHP_EOL;
echo reset($dates) . PHP_EOL;

$startTime = hrtime(true);
foreach ($dates as $date) {
    $d = new DateTimeImmutable($date);
}

echo hrtime(true) - $startTime, ' ns', PHP_EOL;

$startTime = hrtime(true);
foreach ($dates as $date) {
    $d = DateTimeImmutable::createFromFormat('Y-m-d H:i:s.u', $date);
}

echo hrtime(true) - $startTime, ' ns', PHP_EOL;

Result:

2029-07-14 20:51:46.103511
1990-10-19 13:16:13.771286
2021394668 ns
2206943091 ns

What's next

  • Discuss the PR
  • Add other platforms
  • Fix bugs & add more tests

It adds to PostgreSQL platform support of microseconds for the next types:

 - DateTimeType
 - DateTimeImmutableType
 - DateTimeTzType
 - DateTimeImmutableTzType

Additionally, respects precision in schema introspection.
@morozov
Copy link
Member

morozov commented Jan 28, 2025

In order to distinguish timestamp (without precision) and timestamp(6) the without_precision platform option was added.

Why is this distinction necessary? Not only that the type always has precision (whether it's specified explicitly or not), it makes it possible to specify both precision and without_precision on a column (or omit both), which makes no sense.

When the column definition defined by the application and containing without_precision is compared with the one introspected from the database, how should this comparison work?

@upyx
Copy link
Author

upyx commented Jan 29, 2025

Why is this distinction necessary?

Because PostgreSQL does it. Column type can be altered from timestamp to timestamp(6) and vice versa (e.g. by schema:diff). So we need a way to save that differecne on introspection. And we can't use precision: null because it is already used for timestamp(0).

So the mapping here:

DBAL PostgreSQL
precision: null, without_precision: null timestamp(0)
precision: 0, without_precision: null timestamp(0)
precision: 6, without_precision: null timestamp(6)
precision: null, without_precision: true timestamp
precision: 0, without_precision: true timestamp
precision: 42, without_precision: true timestamp

Maybe that awkwardness was the reason why in #5961 used a scale property instead of presicion - the scale default is zero. I think it will be best to use "scale". What do you think?

@morozov
Copy link
Member

morozov commented Jan 29, 2025

Because PostgreSQL does it.

In my understanding, the difference between timestamp and timestamp(6) is purely syntactic. The default precision is 6.

CREATE TABLE test_timestamps (
  ts1 TIMESTAMP,
  ts2 TIMESTAMP(6)
);

SELECT
  column_name,
  datetime_precision
FROM information_schema.columns
WHERE table_name = 'test_timestamps';

The above query will produce 6 for both columns.

Column type can be altered from timestamp to timestamp(6) and vice versa

They shouldn't be considered different. If they are, it's a bug.

Or, more specifically, the default precision of timestamp columns in DBAL is undefined, that's why they may produce a diff. But the point is, this issue needs to be addressed by improving the type system and type comparison logic, not introducing extra type parameters.

There's no difference between `timestamp` and `timestamp(6)` during
introspection anymore.
Copy link

github-actions bot commented May 4, 2025

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label May 4, 2025
@sukei
Copy link

sukei commented May 7, 2025

Despite the CS, the PR is green. What prevent this to be merged?

@github-actions github-actions bot removed the Stale label May 8, 2025
Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

This PR seems overloaded: it attempts to address some performance issue, adds a new feature and deprecates the existing API (which isn't allowed in a patch release).

Would it make sense to tackle these issues one by one?

case 'timestamp':
case 'timestamptz':
if (
preg_match(
Copy link
Member

@morozov morozov May 12, 2025

Choose a reason for hiding this comment

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

Please see if this could be replaced with $this->parseColumnTypeParameters($completeType) (introduced in #6933). You will need to rebase on 4.3.x and retarget the PR.

@@ -11,34 +11,10 @@
use Exception;

/**
* Immutable type of {@see VarDateTimeType}.
* @deprecated Use {@see DateTimeImmutableType} instead.
Copy link
Member

Choose a reason for hiding this comment

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

How is this relevant to introducing the support for precision?

@@ -649,6 +649,34 @@ public function testPartitionTable(): void
));
self::assertSame(1, $partitionsCount);
}

public function testTimestampPrecisionComparison(): void
Copy link
Member

Choose a reason for hiding this comment

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

Please move this to SchemaManagerTest. It's fine if for now it will be skipped on all platforms except for Postgres, but we want to be able to reuse this test once the support for precision is added to more platforms.

$offlineTable->addColumn('t_u_tz', Types::DATETIMETZ_MUTABLE, ['precision' => 6]);

$createTableSQL = <<<'SQL'
CREATE TABLE timestamp_columns_test (
Copy link
Member

@morozov morozov May 12, 2025

Choose a reason for hiding this comment

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

Why create this table via SQL if the table is already defined above via the DBAL API?

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.

Custom datetime with support of microseconds not properly works after update of dbal from 3.6 to 4.2.1
3 participants