Skip to content

fix(db): only cast text and binary compare columns to char on Oracle [full-ci] - #41818

Open
oc-tmueller wants to merge 3 commits into
masterfrom
fix/oracle-upsert-clob-only-cast-master
Open

fix(db): only cast text and binary compare columns to char on Oracle [full-ci]#41818
oc-tmueller wants to merge 3 commits into
masterfrom
fix/oracle-upsert-clob-only-cast-master

Conversation

@oc-tmueller

Copy link
Copy Markdown
Contributor

Description

On Oracle, Adapter::upsert() wrapped every compare column in to_char(). The cast is
required for CLOB and BLOB columns — Oracle refuses to compare those with = at all
(ORA-00932) — but to_char(column) = 'literal' is not sargable, so an index on the column
can no longer be used.

Cache::put() compares storage and path_hash, which is exactly the unique index
fs_storage_path_hash, so every upload, rename and file scan degraded from an index unique
scan into an index skip scan. On the installation where this was found the statement went
from ~0.0002 s / 8–20 buffer gets to 5.7–63.7 s / 91,000–122,000 buffer gets.

The compare column types are resolved from the schema now, and only text and binary columns
are cast:

-- before
UPDATE "oc_filecache" SETWHERE to_char("storage") = '1' AND to_char("path_hash") = ''
-- after
UPDATE "oc_filecache" SETWHERE "storage" = '1' AND "path_hash" = ''

Comparing a NUMBER column to a quoted literal is safe: Oracle converts the literal to a
number, so the column and its index stay usable.

How

Rather than hand-building the cast, the per-column type is handed to the expression builder.
OCIExpressionBuilder::eq() already emits exactly to_char("col") for
IQueryBuilder::PARAM_STR, and the base ExpressionBuilder::eq() ignores its type argument
entirely, so this is a no-op on MySQL/MariaDB/PostgreSQL/SQLite — the generated SQL there is
byte-identical to before. The CLOB path on Oracle is likewise byte-identical; only non-LOB
columns change.

The resolution itself lives in AdapterOCI8, via a new protected
Adapter::getCompareColumnTypes() seam that returns nothing on every other platform. The
schema lookup passes a quoted identifier, because ownCloud creates all tables quoted and
therefore in lower case while Oracle folds unquoted identifiers to upper case — the same
reason OracleConnection::tableExists() already quotes. The result is memoized per table.

If the types cannot be resolved (unknown table, schema manager throws), every compare column
is cast, i.e. exactly the previous behaviour: slow, but it can never raise ORA-00932. A
warning is logged once per table.

Tests

tests/lib/DB/AdapterTest.php gets six tests that fake the platform, so that every platform is
pinned no matter which database the job runs against. They cover both halves of the behaviour:

  • non-LOB compare columns (storage, path_hash) are not cast — the regression this fixes
  • a TextType compare column (configvalue) is still cast — the ORA-00932 case
  • a LOB column whose name is a reserved word (stored quoted) is still matched
  • an unresolvable schema (throws, or unknown table) casts everything
  • other platforms cast nothing, text columns included

Now that #41808 has restored the Oracle job, three further tests run against a live Oracle
and are skipped everywhere else. They pin what the mocks cannot: that the schema lookup finds
an ownCloud table at all on Oracle, which is the one thing that decides between the fix and the
old behaviour.

  • the compare column types resolved from the real oc_filecache and oc_appconfig
  • the generated SQL: no to_char() around the indexed columns, still one around the CLOB
  • an upsert that has to match on an uncast VARCHAR2 column as well as on a cast CLOB column

The existing AdapterTest call-count expectations are unchanged, which confirms there is no
collateral change to the generated queries.

Notes for the reviewer

This is the master forward-port of #41783, which carries the same fix for 10.16 where the
customer escalation is. lib/private/DB/AdapterOCI8.php is byte-identical on both branches: the
code is deliberately DBAL 2/3 agnostic. getSchemaManager() is deprecated on DBAL 3, but it is
what core uses everywhere else (10+ call sites, no createSchemaManager() usage), and
doctrine/deprecations stays at TYPE_NONE unless DOCTRINE_DEPRECATIONS is set, so nothing
is emitted under PHPUnit either.

Two pre-existing Oracle limitations are documented in IDBConnection::upsert() but
deliberately not changed here:

  • to_char() on a CLOB longer than 4000 bytes raises ORA-22835, so comparing a long CLOB
    never worked. OC\AllConfig works around this with dbms_lob.substr(configvalue, 4000, 1);
    adopting that changes behaviour for long values and belongs in its own PR.
  • to_char() does not accept a BLOB, so a binary compare column has never worked either way.
    Keeping BlobType in the cast list preserves today's behaviour rather than changing it.

One behaviour does change on Oracle: a non-numeric string compared against a NUMBER column
now raises ORA-01722 instead of silently matching no rows. No core caller does this.

Fixes #41782

🤖 Generated with Claude Code

@oc-tmueller
oc-tmueller requested a review from a team as a code owner September 7, 2026 09:18
DeepDiver1975 and others added 3 commits September 7, 2026 11:18
Adapter::upsert() wrapped every compare column in to_char() on Oracle. The cast
is needed for CLOB and BLOB columns, which Oracle refuses to compare with = at
all (ORA-00932), but to_char(column) is not sargable, so an index on the column
can no longer be used. Cache::put() compares storage and path_hash - exactly the
columns of the unique index fs_storage_path_hash - so every upload, rename and
file scan degraded into an index skip scan.

The compare column types are resolved from the schema now, and only text and
binary columns are cast. If the types cannot be resolved, every column is cast,
which is the previous behaviour and can never raise ORA-00932.

#41782

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
AdapterTest has a constructor without arguments, so PHPUnit cannot hand a data
set to an instance and the provider arguments were dropped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
The tests added with the fix fake the platform, so they cannot prove that the
schema lookup finds an ownCloud table on a live Oracle: the tables are created
quoted and therefore in lower case, while Oracle folds an unquoted identifier to
upper case. A lookup that comes up empty falls back to casting every compare
column, which is exactly the regression that was fixed.

Now that the PHPUnit DB suite runs against Oracle again, pin that on the real
schema: the compare column types of oc_filecache and oc_appconfig, the generated
SQL they lead to, and an upsert that has to match on an uncast VARCHAR2 column
as well as on a CLOB column that is still cast.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
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.

Oracle: upsert() casts every compare column to char, so filecache writes cannot use an index

3 participants