[PP-1] Use the 'uuid' database schema type (with native PostgreSQL implementation) for UUID fields

Just noticed this causes some tests to fail on PG because the test data contains UUID values that are syntactically invalid.

Interestedin.
Can you describe what kind of data do we try to insert there? I could imagine things like an empty string maybe?

So let's postpone because of #2403271: Return NULL on EntityRepository::loadEntityByUuid() when the UUID is invalid or it doesn't exist (I left a comment over there)

A year has gone and we have PostgreSQL testing now. Going to re-upload patch to see what happens...

Back to "postponed" – let's fix #2403271: Return NULL on EntityRepository::loadEntityByUuid() when the UUID is invalid or it doesn't exist first.

Marking as PP-2 (i.e. Postponed on 2 issues) on:

1. #1805576: Add a 'uuid' database schema type
2. #2403271: Return NULL on EntityRepository::loadEntityByUuid() when the UUID is invalid or it doesn't exist

Looked into this a bit, after updating #1805576: Add a 'uuid' database schema type yesterday.

My goal is to build on the patch there, change core's UUID item to use the uuid schema type and get the resulting patch green. We currently allow arbitrary strings for UUIDs so this is not going to be able to committed, as is, and we will either need a new setting on UuidItem or an entirely new field type for "strict" UUIDs, so the patch I'm initially going for will definitely not be committed in this form. But I want to make sure that we are covering all functionality that currently involves UUIDs. Once we have a green patch we can discuss how to implement this properly.

Providing a first version of that attempt here. I've encountered two problems thus far:

1. The problem of the PostgreSQL driver incorrectly using LOWER() on UUID columns which can also be seen in some of the test failures here. I "fixed" this by making \Drupal\Core\Entity\Query\Sql\Tables consider native UUID columns as case sensitive on PostgreSQL. This is a workaround, I'm not sure what a proper fix would look like.
2. The issue that PostgreSQL cannot query for invalid UUIDs on native UUID columns, i.e. #2403271: Return NULL on EntityRepository::loadEntityByUuid() when the UUID is invalid or it doesn't exist. The patches there only deal with EntityRepository::loadEntityByUuid(), but in running JsonApiDocumentTopLevelNormalizerTest I found that JsonApiDocumentTopLevelNormalizer::denormalize() calls EntityStorage::loadByProperties() on a UUID field directly without going through the entity repository. So I implemented another workaround in SqlContentEntityStorage::loadByProperties() directly. This, also, will not fly as is, but is just to get this working for now

Let's see how badly this breaks!

Sorry for the 4 test runs that fail on a CS error! I had never checked out the new commit-code-check.sh, did that now...

This one should be better. I am also skipping all update path tests with this patch as they will inherently fail on "UUID field schema needs to be updated" for all entity types, so let's not bother the test runner with that. Other than that I am not aware of any test failures (but am assuming there are a bunch left...)

Not providing a new "for-review" patch as the CS fix is the only difference...

tl:dr; JSON API performs direct entity queries with invalid UUID values, so we need to support that. The attached patch attempts to do so ;-)

OK, so the fail in the one above was a bit disheartening as it proved a suspicion/fear I had had before: We need to support people doing direct entity queries on UUID fields with invalid values. It would have been a tough argument to have to no longer allow this and not consider this a BC break for the theoretical use-case of doing. We are spared this argument, though, due to JSON API very practically doing this already. JSON API's collections allow filtering the result-set by any field value which translates into a direct entity query with respective conditions for the filters. And EntityResourceTest::testGetEmptyCollection() tests exactly the case discussed here: it adds a filter on the UUID field with the value 'invalid', expecting an empty collection as a result. Thus, if we want to pursue native UUIDs on PostgreSQL then we need to go one level further than the previous patch and attack the problem at the query level.

An aside before I continue with what I've done with the new patch: In investigating the aforementioned and possibles solutions I tried manually running various queries against my local PostgreSQL instance but could not reproduce the errors thrown on those invalid UUID values. This confused me a great deal and I briefly thought this was due to the version difference of PostgreSQL as I have a PostgreSQL 12 locally and the testbot runs 10 by default. This is why I started a test run on PostgreSQL 12 for the latest patch above. After that failed in the same way I realized that the PostgreSQL CLI tool psql - and by extension drush sql:query does not show any errors by default unless you explicitly specify the -b or --echo-errors option. So anyone trying this out at home will need to do that.

I spent a lot of time looking at the code that converts entity queries into SQL queries as that is the place where we need to resolve this issue. Unfortunately that part of the code is very complex and at all easily extendable. Also in the context of an entity query we do not generally have the entire entity schema at hand, so even figuring out which field column is a UUID column is a non-trivial task, without even worrying about how to fix that. After a while, though, I realized two things that lead me to what I think is the solution to this problem:

1. We already have a mechanism for overriding the "convert an entity query to a database query"-process on a per-database driver basis. This was absolutely news to me, but behold \Drupal\Core\Entity\Query\Sql\pgsql\Condition if you, like me, find this astonishing. That means as long as we can hook into the right spot in the process and the figure out what actually to do, we will not have to employ hacky driver checks a la #25, which is great.
2. We are already altering the result of the "convert an entity query to a database query"-process based on the fact whether or not a respective (textual) column is case sensitive or not. For example, for case insensitive fields we turn ->condition('field', 'value') into field LIKE 'value' instead of field = 'value'. This was again very much news to me, but in any case that means we already have a precedent of this process depending on the column schema. Which is exactly what we want to do here.

So given the above, I did the following:

• Put the case-sensitivity tracking in \Drupal\Core\Entity\Query\Sql\Tables in a separate method so it is more easily overridable
• Added a PostgreSQL-specific Tables implementation that adds tracking of UUID columns on top of the case-sensitivity tracking
• Added a new method to \Drupal\Core\Entity\Query\Sql\Condition to allow altering the condition. This ia a bit unfortunate because there is already ::translateCondition() but that one does not have access to Tables so it does not help here. I think I still found a way to make the new method fit reasonably well but your mileage may vary.
• Implemented said method in PostgreSQL's (existing) Condition override. In that I check if the column is a UUID column and, if so, and if the value(s) is/contains invalid UUIDs, skips the condition and alters the query result to achieve the expected result. This took me quite a while to get right, but I think I have a version that is both correct and fairly understandable
• Added a new EntityQueryUuidTest that tests all sorts of entity queries with different sets of conditions and with all possible operators. That passes locally on all three database engines which gave me confidence that the PostgreSQL-specific overrides function in the way they should.
• To get the tests to pass with all possible operators, I needed the PostgreSQL's entity query Condition to have a way to mark fields to be casted to text by the database driver. This is needed for comparison operators, i.e. in PostgreSQL you cannot do uuid BETWEEN '1a' AND '1f' to find a specific range of UUIDs, instead it needs to be uuid::text BETWEEN '1a' AND '1f'. To support this I added support for specifying this cast (i.e. "::text") as part of the field in a database condition and made the PostgreSQL database driver pick that up and not escape it. If anyone has a better way to achieve this please step forward, I personally think this is the ugliest part of the patch.
• After doing all that I removed the workarounds from #25 as those are not needed anymore.

So I am very much looking forward to the test results on this one. But to summarize, I think this patch gets us a big step in the right direction and comes with fairly extensive test coverage. It no longer depends on #2403271: Return NULL on EntityRepository::loadEntityByUuid() when the UUID is invalid or it doesn't exist and is fairly hack-free (* #25 didn't depend on #2403271: Return NULL on EntityRepository::loadEntityByUuid() when the UUID is invalid or it doesn't exist either but had some ugly hacks). I am not yet turning the "PP-2" into a "PP-1", though, until I get some thumbs up ;-)

Two more notes:

1. I am pretty sure the code that is moved into \Drupal\Core\Entity\Query\Sql\Condition::preTranslateCondition() in the patch is broken as it does not account for OR groups. It is not at all tested, though, so I will investigate and (if I am right) open a follow-up for that.
2. The NOT BETWEEN operator is not documented as existing, but JSON API has test coverage for it (and it does work), so I included it here. I will open a follow-up to clarify this situation, i.e. probably just amending the docs to mention it "officially".

1. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/pgsql/Condition.php
   @@ -2,14 +2,117 @@
   +  protected function preTranslateCondition(ConditionInterface $conditionContainer, TablesInterface $tables, array &$condition) {
   ...
   +    assert($tables instanceof Tables);
   

   The function argument already require this interface so not clear why that needed

2. +++ b/core/lib/Drupal/Core/Entity/Query/Sql/pgsql/Condition.php
   @@ -2,14 +2,117 @@
   +    assert(is_string($condition['field']));
   

   this assert more confusing, is it expected that 'field' key always exists in condition array?

3. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
   @@ -126,6 +126,8 @@ abstract class UpdatePathTestBase extends BrowserTestBase {
   +    $this->markTestSkipped();
   

   it will need upgrade and update hook

fixed tags

Thanks for the review @andypost!

Re #28:

1. I am checking for the PostgreSQL-specific Tables implementation as only that has the UUID tracking, the base class does not have that. That's why the assert makes sense and is needed to get autocompletion for the method call below.
2. Yes, the field should always be set at this point and we use it in a method call which expects a string just below, so this is not adding any assumptions, it's just enforcing those that are already implicit. I'm fine to remove these assertions if people don't like them, I just wanted to make clear why I added them.
3. Actually the current patch that automatically changes all UUID fields is just to validate that we don't break anything. This approach has already been validated above as we would have never found all the use-cases above if this had just been "tacked-on" as an additional feature from the start. The plan, though, is to make it opt-in, as we cannot inadverdantly change the behavior of existing UUID fields from under people's feet. I.e. as of now you can store arbitrary values in UUID columns and changing that, must be an explicit opt-in step. Thus, removing the tags again. Once we have a green patch with the "change everything" approach we can discuss how to implement this "opt-in" behavior and then we can remove the skipping of the update path tests.

Having written 2. above I realized that I hadn't added the scalar type hint to isUuidField(), so doing that now.

This patch should pass the code-style tests. Still getting used to the pre-commit script, I had run it locally, but I guess I had some stuff cached, not sure...

Anyway, this one should hopefully be better.

So, that was not too bad, but three problems were surfaced by the test fails in #30:

1. A simple typo in \Drupal\Core\Entity\Query\Sql\pgsql\Tables::trackColumnState(), which caused all the "%delta" errors, fixed that.
2. JSON API revealed that we did not yet cover aggregate queries. I think this yet again validates the approach of testing this first will the "convert everything" approach, I don't think we would have caught that otherwise. Fixed that by adding an overridden QueryAggregate for PostgreSQL that casts UUID fields to ::text as needed, in line with the casting done for the various comparison operators already in the previous patch.
3. Workspaces overriding of the entity query system did not account for the newly added PostgreSQL overrides. Implemented that now. Because the query override system is an explicit API which we are using here, I don't think this can be considered a BC break it's just that the Workspaces override is not very resilient/conformant to this API. That in itself is not fixed here, but I've made Workspaces aware of the new overrides by providing Workspaces-specific overrides on top of those. In the process I (more or less) had to deprecated the Workspaces-PostgreSQL query factory and move it to a different namespace (because the entity query overriding is namespace-based), so I deprecated the class in the old location. Did not add an actual change notice for that yet, would love to get some validation on the approach here first.

Let's see, there should definitely be a lot less fails, maybe even none ;-)

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/pgsql/QueryAggregate.php
@@ -0,0 +1,29 @@
+use Drupal\Core\Entity\Query\Sql\QueryAggregate as BaseQueryAggregateAlias;
...
+class QueryAggregate extends BaseQueryAggregateAlias {

Ahh sorry, the "...Alias" part should not be there, will fix in the next round...

Still have this on my todo list, will try to make some more progress soon. Here is a work-in-progress interdiff that I have locally so far in case I don't get to it and someone else wants to pick this up. The remaining blocker with this is views-filter support.

Finally got around to opening follow-ups for the issues mentioned in #27:

• #3256056: Entity query system does not document the NOT BETWEEN operator, but JSON:API supports/uses it
• #3256162: Entity query %delta conditions do not work properly with most operators or OR condition groups

Not sure I will get around to working on this again soon, but when I do I can now dive in directly without those distractions...

Is this more or less a duplicate of #1805576: Add a 'uuid' database schema type, which applies more broadly to all DB drivers?

Re #38: So #1805576: Add a 'uuid' database schema type is about the addition to the Schema API, this issue is about using the new database schema field type provided there for the entity field schema of UUID fields. So this is postponed on that.

Your confusion is understandable, however, because the issue title here is not very accurate and also I don't think this should be assigned to the "postgresql db driver" Component. Since there hasn't been much traction here anyway just going ahead and changing title and component. In case I'm missing something please do feel free to revert. Leaving "PostgreSQL" in the title for searchability, though.

While I'm at it also decreasing the postponed count per #27

Seems the PP-1 task got lost in other tasks, so let me necropost here some benefits of implementing this from a duplicated issue #3573735: Improve UUID performance by storing the value as binary format in the database instead of string:

For now, Drupal stores the UUID values in the entity tables just as plain strings in the database: varchar(128).

This type of storage leads to significant performance issues, compared to the binary UUID format:

Storing UUIDs as strings instead of binary in MySQL results in a 55% storage penalty and significantly degrades performance due to index fragmentation. A string UUID occupies 36 bytes compared to 16 bytes for binary, which reduces buffer pool efficiency, increases disk I/O during reads, and forces expensive, non-sequential "index thrashing" during writes. Furthermore, binary comparisons are computationally cheaper than character-based collation checks, making joins and lookups consistently faster when using the compact BINARY(16) format.

So, switching to the binary format should significantly improve the performance, here are some estimations:

Storage: A 55–70% reduction in storage for the UUID column itself. Because MySQL InnoDB secondary indexes also include the primary key, this space-saving is compounded across every index on your table.

Write Speed (Insert): Up to a 3x–5x faster insertion rate once your dataset exceeds the size of your RAM (Buffer Pool).

Read Speed (Select/Join): Joins and lookups typically see a 20–60% speed improvement because the database compares raw 128-bit numbers instead of performing character-by-character lexical collation checks.

The same is true for all other database types.

---
So, let's try to finalize and merge this issue?

If we are attempt to use binary format for UUIDs then it makes sense to use UUID v7 - only one transition

Since PG18 there's native function for it https://www.postgresql.org/docs/current/functions-uuid.html#FUNC_UUID_GE...

Also Symfony since 7.4 also support it https://symfony.com/blog/new-in-symfony-7-4-uid-improvements

PS: the only dark side I see is that it has timestamp inside