Doctrine DBAL
  1. Doctrine DBAL
  2. DBAL-730

[GH-464] [DBAL-139] [WIP] Finish CACHE option implementation for sequences

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Security Level: All
    • Labels:
      None

      Description

      This issue is created automatically through a Github pull request on behalf of deeky666:

      Url: https://github.com/doctrine/dbal/pull/464

      Message:

      @beberlei Started implementation for DBAL-139(http://www.doctrine-project.org/jira/browse/DBAL-139) in commit https://github.com/doctrine/dbal/commit/e8efcd7621e85003f3ce496bc26a108b53371838 and https://github.com/doctrine/dbal/commit/d397c005dd3ff3d48a81eeb47c2a0d74d6b7400d which was first drafted in PR #202. It's about adding a cache option for sequences.

      *The following missing aspects of the implementation were added:*

      • SQL Server support
      • SQL Anywhere support
      • Reverse engineering the cache option with the schema managers
      • Comparator implementation
      • Schema::createSequence() adjustment
      • More tests

      *Additional optimizations:*

      • Rename `$cache` to `$cacheSize` in Sequence (unified naming with `$allocationSize`)
      • More strict input validation for cache size values in Sequence to avoid SQL generation errors
      • Better API documentation
      • Unified `getSequenceCacheClause()` method on all platforms

      *Todo:*

      • Fix comparator issues (see below)
      • Finish test suite for this (depends on comparator issues solution)

      *Comparator issues*
      The comparator will get into trouble when comparing cache size values. To get an understanding of this I'll first explain how cache size values evaluate to the database:

      • `null` -> No cache size specified, don't generate cache clause for sequences and use platform's default
      • `0` -> Explicitly disable caching for sequences, generates `NOCACHE` clause on platforms
      • every other positive value is an explicite specification of the cache size and results in `CACHE <size>` clause

      Now there are two problems.

      First one is that a value of `1` has a special meaning on some platforms, too. Oracle does not accept it and throws an error as the minimum allowed value is `2`. PostgreSQL allows a value of `1` but in fact interprets it as `NOCACHE`. SQL Server and SQL Anywhere accept a value of `1` but the behaviour is unclear compared to an explicite `NOCACHE` on these platforms. Now I am assuming that a cache size of `1` maybe also physically has the same effect as if specifying no cache at all and that this is more of a "compatibility" value and we maybe should not allow it at all to preserve portability throughout the vendors.

      The second problem is the `null` value for `$cacheSize` in Sequences. This tells the vendor to use it's own specific default cache size which differs on all platforms. The comparator will always mark a sequence as changed then when `null` was initially defined for the sequence and the sequence is introspected from database again. This will lead to repeated `ALTER SEQUENCE` statements in the schema tool which is not good. Also we cannot determine in the platform itself if the sequence has really changed as there currently is no `SequenceDiff` class which could get passed to `AbstractPlatform::getAlterSequenceSQL()`. Instead we only get the changed sequence object. I have put together some [examples](https://gist.github.com/deeky666/6c4e47a275b1c9e7c068) to clarify on what I mean as it is hard to explain.

      So I'm asking for opinions and input on this

        Activity

        There are no comments yet on this issue.

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Doctrine Bot
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated: