Support for BINARY and VARBINARY in Database Schema

We're not making any changes to the schema support in Drupal 7. We can come back to this in Drupal 8.

Subscribe. Storing e.g. a SHA-256 hash in a UTF-8 CHAR(64) column uses six times more bytes than a BINARY(32) column.

Now that D8 is in alpha 3 release, was this addressed at all?

This looks great, thanks!

added test cases.

Also fixed sloppy quoting of DEFAULT strings using the pdo->quote.

Also fixed sloppy quoting of DEFAULT strings using the pdo->quote.

...and that goes into a separate issue + patch. :-)

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SchemaTest.php
@@ -339,6 +339,41 @@ function testSchemaAddField() {
+    $this->pass(format_string('Table %table created.', array('%table' => $table_name)));

No need to use format_string() here, because we know what $table_name contains.

"Table $table_name created."

The patch actually adjusts Postgres + SQLite accordingly, so I'm adjusting the issue title.

quoting fixed in patch in issue 2232425 which is now a requirement for this to work

format_string corrected though was copying from same code base.

Also discover a PDO exception with this in postgres while testing the #2224847: Automatically shorten cid's in Cache\DatabaseBackend and PhpBackend

[message:protected] => SQLSTATE[22021]: Character not in repertoire: 7 ERROR:  invalid byte sequence for encoding "UTF8": 0xb4


                    [args] => Array
                        (
                            [0] => SELECT 1 AS expression
FROM
{cache_page} cache_page
WHERE ( (cid = :db_condition_placeholder_0) AND (cidhash = :db_condition_placeholder_1) )
                            [1] => Array
                                (
                                    [:db_condition_placeholder_0] => test1
                                    [:db_condition_placeholder_1] => �D�a?��7���о�U6�
                                )

                            [2] => Array
                                (
                                    [return] => 1
                                )

                        )

What postgres need to escape bindata in some way. Looks like a job for core/lib/Drupal/Core/Database/Query/Condition.php

Postgres needs http://www.php.net/manual/en/function.pg-escape-bytea.php

No obvious place to insert this. https://github.com/doctrine/dbal/pull/452 doesn't seem to include it.

Deferred postgresql support in Update/Select arguments to #2238253: Add bindValue to a PDO::PARAM_* type in database query arguments - seems a little tricky to tackle on its own.

Test cases updated. Postgresql seems to handle schema bits associated with default value using standard quote. Still waiting on #2232425: Database Schema field/column default value is not properly quoted via PDO::quote() to be committed.

Dependency of #2222635: Waiting for table metadata lock on cache_field table

We need to have someone verify that this works on Postgres and SQLite before we commit. I'm not opposed, though.

I have verified passes the Schema test suite on postgres and sqlite.

I've even DatabaseBackendUnitTest with patch [2222635-14] (with the following change) on postgres and sqlite since postgres doesn't like escaping binary in SELECT statements.

commit 37afc3a11037d949d3f143f686e2afbbfc6b8eb1
Author: Daniel Black <daniel.black@openquery.com.au>
Date:   Fri Apr 11 02:51:59 2014 +0000

    postgres compatible hash value

diff --git a/core/lib/Drupal/Core/Cache/DatabaseBackend.php b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
index a9f1eb3..ec2e07a 100644
--- a/core/lib/Drupal/Core/Cache/DatabaseBackend.php
+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -183,7 +183,10 @@ public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array
    * Calculate the hash for caching
    */
   protected static function cachehash($cid) {
-    return hash('sha1', $cid, TRUE);
+    // Having a non-hex format would be nice, however postgres has troubles
+    // #2232425
+    // return hash('sha1', $cid, TRUE);
+    return hash('sha1', $cid);
   }

   /**
@@ -568,7 +571,6 @@ public function schemaDefinition() {
           'type' => 'binary',
           'length' => strlen(self::cachehash('value')),
           'not null' => TRUE,
-          'default' => '',
         ),
         'cid' => array(
           'description' => 'Cache ID.',

12: binary.patch queued for re-testing.

8: binary.patch queued for re-testing.

Thanks to the commit in #2232425: Database Schema field/column default value is not properly quoted via PDO::quote() this now passes automated tested and is ready for review/committing as is a prerequisite for hash based db caches (#2222635: Waiting for table metadata lock on cache_field table)

12: binary.patch queued for re-testing.

So, I'm a little confused about the posgres docs for bytea: http://www.postgresql.org/docs/9.1/static/datatype-binary.html

It seems in #12 that's deferred? however, we could convert the binary to hex instead of using the standard escape?

however, we could convert the binary to hex instead of using the standard escape?

Maybe. However I'm still stuck with then how do we override the standard escape in the db api layer and somehow have an awareness of which field is the bytea to contain escaping?

So if cidhash is to be escaped how is the following expression written to use hex.

 $result = $this->connection->query('SELECT cid, data, created, expire, serialized, tags, checksum_invalidations, checksum_deletions FROM {' . $this->connection->escapeTable($this->bin) . '} WHERE cidhash = :cidhash AND cid = :cid', array(':cidhash' => $cidhash, ':cid' => $cid));

Directly inserting it into the query string is one unelegant way:

 $result = $this->connection->query('SELECT cid, data, created, expire, serialized, tags, checksum_invalidations, checksum_deletions FROM {' . $this->connection->escapeTable($this->bin) . "} WHERE cidhash = E'\\\\x" . array_shift(unpack('H*', $cidhash) . "' AND cid = :cid", array(':cid' => $cid));

Looks like we need a new escapeBinary() method for all db drivers - essentially following the existing escapeLike() model?

Some drivers might be able to piggy-back onto quote() in case PDO implements native binary quoting support for them.

@sun - so how would that work? postgres would make it hex and mysql would be a no-op?

@sun, @pwolanin - can we take the issue of postgresql compatibility of bytea to #2238253: Add bindValue to a PDO::PARAM_* type in database query arguments as this patch doesn't introduce the incompatibility of bytea and postgres select, it exists currently as postgres already maps blobs to bytea.

As such this is adding a feature and no regressions even though currently you'll need to be careful how you use it if binary types are used in selects with non-ascii values as parameters.

Hmm, well, in the cache table issue we can make binary work for mysql only, so I don't think we should show the schema API as fully supporting it unless we resolve this (small?) postgres issue.

Of course, getting the testbots to run postgres too wold be a big help.

schema API as fully supporting it unless we resolve this (small?) postgres issue

It may be small but its really ugly to fix - progress is been made - look in the appropriate bug report.

Note that we support the binary => true/false flag and now also use that for the cid column in cache tables by default. If that is set, then the BINARY flag is used for MySQL.

@Berdir these are two unrelated things. The BINARY type is for storing binary data; the BINARY attribute is for storing text with a defined character set but a binary case-sensitive collation. The BINARY attribute automatically chooses an appropriate collation for the column's character set e.g. utf8_bin.

I'm using Drupal 7, no plans to go to 8 yet.
I'm writing up a module with a .install file and using hook_schema() in the .install file.

The install file includes the function for a hook_schema().
Working with encryption and I need to store as type BINARY or VARBINARY.

Looking at the Schema API, BINARY nor VARBINARY is listed.

What I've tried:

'iv' => array(
        'description' => 'initialization vector',
        'type' => 'binary',
        'not null' => FALSE,
      ),
      

Band-aiding a solution to use base64_encode() or bin2hex(), will cost storage space and performance (I believe).

If i apply this patch (Patch #12), would it enable me to store BINARY / VARBINARY for hook_schema()?

thanks in advance,

gangaloo: This issue is about Drupal 8. Please open a new issue for Drupal 7, as the patch won't apply anyway due to other changes in the code base.

For cache tables we're now using varchar_ascii for the cache IDs.

Is there any other use case for this?

Given #1031122: postgres changeField() is unable to convert to bytea column type correctly I'd be very wary about adding even more bytea dependencies to core.

Is there any other use case for this?

Seems to be enough desire over the years to add this. Doctrine dbal added it years ago. As referenced in the other issues there are still some limitations around binary/varbinary types and these issues have a body of knowledge around them and aren't impossible to fix and nor are they necessary for a variety of uses.

In the mean time however consider that there is a range of functionality provided by this patch that isn't available currently.

There's desire expressed in this issue but the issue doesn't mention varchar_ascii which can store hashes very efficiently, hence me asking what people want to store in it now.

these issues have a body of knowledge around them and aren't impossible to fix and nor are they necessary for a variety of uses.

Once we add a feature we can't control what people use it for, so we should try to fix known issues first.

re: #2 storing a hash in an ASCII VARCHAR(64) column is a big improvement, but still uses more than twice as much space as a BINARY(32) column

moving to an appropriate version for possible features

How is varchar_ascii more efficient than utf8? I thought the only difference was allowed index size. The size on disk is the same.

The efficiency with varbinary comes from not having to encode binary data (hashes, encryption, files) into text via hex or base64.

You are correct, using ascii instead of utf8mb4 doesn't help size on disk at all (I just checked to verify this :) So we have some improvement here only for index size.

Here are some real-world numbers, for 4000 rows of SHA-256 hashes as primary key plus an additional index*.

CREATE TABLE `binhash` ( `hash` binary(32) NOT NULL, PRIMARY KEY (`hash`), KEY `hashkey` (`hash`)) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4

binary(32) size on disk: 640K

CREATE TABLE `aschash` ( `hash` varchar(64) CHARACTER SET ascii NOT NULL, PRIMARY KEY (`hash`), KEY `hashkey` (`hash`)) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4

varchar(64) ascii size on disk: 9.0M

* per MySQL docs:

The primary index of a table should be as short as possible. This makes identification of each row easy and efficient. For InnoDB tables, the primary key columns are duplicated in each secondary index entry, so a short primary key saves considerable space if you have many secondary indexes.

Trying a re-roll for 9.1.x. Let's see what the testbot says...

Re-roll was manual, so no interdiff. Summary of important differences:

- fieldSetDefault() changes: dropped due to fieldSetDefault() being deprecated in favor of changeField() (I'm assuming changeField() is already updated via createFieldSql() changes). See change record for fieldSetDefault() deprecation.
- ::_escapeDefaultValue(): already implemented in 9.1.x. (Database\Schema.php).
- Test was ported to new SchemaTest.php, I tried following the pattern in other tests for some calls. Also, commented out tests for now-non-existant fieldSetDefault() et al, with a TODO for using changeField() instead.

Sorry testbot!, I got you yelling all the way through tests due to a silly mistake. It should be fixed now.

quasi-interdiff is just:

< +      if (!empty($spec['binary'] && !in_array($spec['mysql_type'], $this->mysqlBinaryTypes))) {
---
> +      if (!empty($spec['binary']) && !in_array($spec['mysql_type'], $this->mysqlBinaryTypes)) {

Trying to fix the deprecation...

Now that we have passes for the three DBMSes, let's try to get some feedback; I'm wondering if testing changeField() is really necessary (see TODO). If not, we can just remove the TODO and continue review + refinements.

For anybody interested, #50 applies on 8.8.x. With it, you can define binary fields in hook_schema():

function my_module_schema() {
  $schema['my_module'] = [
    'description' => '',
    'fields' => [
      'someid' => [
        'description' => 'The id.',
        'type' => 'serial',
        'unsigned' => TRUE,
        'not null' => TRUE,
      ],
      'uid' => [
        'description' => 'The user id.',
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => TRUE,
        'default' => 0,
      ],
      // =========== BINARY field here ===========
      'hash' => [
        'description' => 'The hash',
        'type' => 'binary',
        'length' => 16,
        'not null' => TRUE,
        'default' => hex2bin('00'),
      ],
    ],
    'primary key' => ['someid'],
    'indexes' => [
      'uid' => ['uid'],
      'hash' => ['hash'],
    ],
  ];

  return $schema;
}

For MySQL queries, HEX()/UNHEX() + db->select() and db->query() (for INSERT) should suffice:

// INSERT
$someid = \Drupal::database()->query('
  INSERT INTO {my_module}
  (uid, hash)
  VALUES (:uid, UNHEX(:hash))
', [
  ':uid' => $account->id(),
  ':hash' => $human_readable_hash,
], [
  'return' => \Drupal\Core\Database\Database::RETURN_INSERT_ID,
]);

// SELECT
$query = \Drupal::database()->select('my_module', 'mm')
  ->fields('mm');
$query->addExpression('HEX(hash)', 'hash');
$query->where('hash = UNHEX(:hash)', [':hash' => $human_readable_hash]);
$query->condition('uid', $some_uid);
$results = $query->execute()->fetchAllAssoc('someid');

Did some more research on this today, and I'm now wondering if \PDO::PARAM_LOB can help to have a single way of properly escaping binary data; PDO-style sample from https://stackoverflow.com/a/59360328:

$binip = hex2bin($hexip);
$st->bindParam(':ip', $binip, \PDO::PARAM_LOB);

And we should be able to use \PDO::PARAM_LOB via \Drupal\Core\Database\Connection::quote().

1. Can we update the issue summary with the default items. I am very much interested why we should add support for BINARY and VARBINARY. What are the benefits of doing so.

   <h3 id="summary-problem-motivation">Problem/Motivation</h3>
   
   
   <h4 id="summary-steps-reproduce">Steps to reproduce</h4>
   
   
   <h3 id="summary-proposed-resolution">Proposed resolution</h3>
   
   
   <h3 id="summary-remaining-tasks">Remaining tasks</h3>
   
   
   <h3 id="summary-ui-changes">User interface changes</h3>
   
   
   <h3 id="summary-api-changes">API changes</h3>
   
   
   <h3 id="summary-data-model-changes">Data model changes</h3>
   
   
   <h3 id="summary-release-notes">Release notes snippet</h3>
   

2. Should we not only add the database schema stuff, but also the CRUD stuff (a.k.a insert, update , select and delete).

Updating IS.

Given #50 provides almost complete support for hook_schema() functionality, I suggest we split adding support for proper conversion to/from binary for all types of queries into a new issue. #52 shows how to leverage existing facilities to handle hex hashes. I haven't checked other uses of binary data, though.

I think that it is better to do more in this issue. Lets start using this functionality in at least one place in core. To me that is the best way to see that it works as it should.
Maybe we should first do #2238253: Add bindValue to a PDO::PARAM_* type in database query arguments, before doing this issue.

Just to report we have been using #50 in a site; the patch has been working fine through various changes in the site codebase/infrastructure:

• Drupal: 8.8 to 9.3.14
• MySQL 5.6 up to MariaDB 10.4.25
• PHP from 7.2 up to 8.0.20

The site handles ~900k unique visits per month. During this time, the site generated ~1M records in the table having the binary field.

Let's try with a quick re-roll.

Fixed cspell errors by adding the words to the dictionary.

@jedihe: Thank you for working on this. Let me start by saying that I am not against this issue, only that I am not convinced that the benefits of using binary and varbinary fields outway the effort to make the change and the problems we will get in making the change. The only benefit that I see is that we save a bit of harddisk space. We do not live in 1970 any more and harddisk space is cheap. If it makes Drupal faster in a significant way, than this would something we should do.

On mysql, historically a binary/varbinary column gave you better performance than a blob column by avoiding slow on-disk temporary tables. Version 8.0.13 improved handling of blob columns: they can now be stored in an in-memory temporary table. I'd guess binary/varbinary might still be slightly faster than blob overall due to how the data is stored in table - could be a fun little benchmarking project..

@daffie: thanks for the detailed feedback. I understand that this patch touches on low-level functionality, so potential inclusion will only occur if a very compelling argument is made for it. For now, I'm happy with just having this patch in a working state for MySQL.

Consider https://www.drupal.org/pift-ci-job/2591966 as a use case for (var)binary. I am not even sure how to do this without VARBINARY support, to be fair.

Edit: the module has been released with the appropriate drupalci.yml file to apply a minimal version of this patch.

Can someone sum up what has been tricky about adding support for binary/varbinary, given that blob is already supported? Is there something broken about blob on postgres? (I happen to have a contrib module that stores some binary data in a cache table, so I was assuming this basically worked..)