oracledb_cdc: derive identical message schemas from catalog and driver metadata - #4661
Conversation
…r metadata The per-message schema was derived from two sources that disagree on how they report the same column: ALL_TAB_COLUMNS embeds fractional-seconds precision in timestamp type names (TIMESTAMP(6)), which fell through the type mapping to String, while the go-ora driver's names (TimeStampDTY) mapped to Timestamp. Snapshot messages seed the schema cache from driver metadata and restarts rebuild it from the catalog, so the schema attached to messages flipped depending on which path produced them — and under BACKWARD compatibility, schema_registry_encode permanently rejected every message from whichever path lost the initial registration. Both sources now route through a single mapping (columnToCommon), with the source-specific encodings bridged explicitly: - normalizeOracleTypeName strips parenthesised size qualifiers from catalog type names before matching - catalogNumberInfo substitutes precision 38 for NUMBER(*,s) columns (INTEGER/INT/SMALLINT), matching what the driver reports on the wire - negative-scale NUMBER(p,-s) maps to BigDecimal from both sources (the driver's uint8 scale wraps and can't represent it faithfully) - go-ora driver spellings for binary and JSON types (VarRaw, LongRaw, LongVarRaw, OCIBlobLocator, TNSType(119)) map like their catalog equivalents; LOB locator names added to the snapshot scanner and lob_enabled filtering TestOracleSchemaSourceParity pins catalog-shaped and driver-shaped inputs to identical schemas for every supported type, and the datatypes integration test gains a restart leg that resumes from the checkpoint so the catalog-derived schema actually appears in asserted messages — within a single process lifetime, streaming reuses the snapshot-seeded schema and the catalog mapping was never exercised, which is how this went unseen. The restart leg caught the INTEGER divergence empirically during development. A benchmark covers the per-row seeding hot path (no regression, allocations unchanged). Behaviour note: INTEGER/INT/SMALLINT columns now consistently emit Decimal(38,0) instead of flip-flopping between Decimal and BigDecimal, and timestamp columns consistently emit Timestamp; subjects whose registered schema carries the old catalog-derived typing need a one-time migration (see PR description).
| switch dbType { | ||
| case "CLOB", "NCLOB", "BLOB", "LONG", "LONG RAW", | ||
| "LongVarChar", "LongRaw": // go-ora driver-level names for CLOB/NCLOB/LONG and BLOB/LONG RAW | ||
| "LongVarChar", "LongRaw", // go-ora driver-level names for CLOB/NCLOB/LONG and BLOB/LONG RAW (inline LOB mode) |
There was a problem hiding this comment.
LongVarRaw was added as a driver spelling for LONG RAW/BLOB in the scanner switch (case "RAW", "LONG RAW", "BLOB", "VarRaw", "LongRaw", "LongVarRaw", "OCIBlobLocator") and in oracleTypeToCommonType ("LONGVARRAW" → ByteArray), and the new parity test lists it as a LONG RAW driver name — but it was not added to isLOBType.
isLOBType is the gate for lob_enabled: false (snapshot.go#L286 nulls the value post-scan), so with lob_enabled: false a LONG RAW column that go-ora reports as LongVarRaw (TNS 95, the raw counterpart of LongVarChar/94 which is listed) has its full payload emitted, while the same column reported as LongRaw is nulled — behaviour that flips on a driver-reported type name for the same Oracle column.
Suggested fix: add "LongVarRaw" alongside "LongRaw" in isLOBType, or, if LongVarRaw is deliberately treated as non-LOB, drop it from the scanner/type-mapping additions so the three name lists stay in agreement.
| | `NUMBER(p, s<0)` | String (BigDecimal) ⁵ | `json.Number` | `json.Number` ¹ | `1200` | | ||
| | `NUMBER` (bare) | String | `json.Number` | `json.Number` ¹ | `42` | | ||
| | `INTEGER` / `INT` / `SMALLINT` | Int64 ² | `int64` | `int64` ¹ | `42` | | ||
| | `FLOAT` | String ² | `json.Number` | `json.Number` ¹ | `1.5` | | ||
| | `INTEGER` / `INT` / `SMALLINT` | String (Decimal(38,0)) ² | `string` | `string` ¹ | `"42"` | | ||
| | `FLOAT` | String (BigDecimal) ² | `json.Number` | `json.Number` ¹ | `1.5` | |
There was a problem hiding this comment.
These two rows document the wrong Go/wire types for their new BigDecimal mapping.
NUMBER(p, s<0) and FLOAT both now map to BigDecimal, and BigDecimal columns are canonicalised to a string — snapshot scans them as sql.NullString and runs sqlutil.CoerceToCommon (snapshot.go#L498-L509), and CoerceToCommon's schema.BigDecimal branch returns CanonicaliseBigDecimal(...), a string (coerce.go#L74-L83). The integration test in this PR asserts exactly that: for any Decimal/BigDecimal column, every phase's value must be a string (integration_datatypes_test.go#L321-L329) — flt (FLOAT) is covered by that assertion.
So both rows should read string / string with a quoted wire example ("1200", "1.5"), matching the INTEGER/INT/SMALLINT row corrected in the same hunk, rather than json.Number / bare 1200.
| // matching the driver's undeclared-scale sentinel. | ||
| func catalogNumberInfo(precision, scale sql.NullInt64) (p, s int64, hasDecimalInfo bool) { | ||
| if !precision.Valid && scale.Valid { | ||
| return 38, scale.Int64, true |
There was a problem hiding this comment.
The 38 substitution is a bare literal in logic. Per the project Go patterns ("Magic Numbers": "Name all numeric constants. Every literal number in logic must have a clear meaning through a named constant or variable") this should be a named constant — e.g. a maxOracleNumberPrecision = 38 next to the existing precedent MaxInt64DecimalPrecision, which documents its bound the same way. The doc comment explains the value but the constant is what the driver-side sentinel is compared against, so naming it keeps the two sources' shared assumption discoverable from both call sites.
| case "CLOB", "NCLOB", "BLOB", "LONG", "LONG RAW", | ||
| "LongVarChar", "LongRaw": // go-ora driver-level names for CLOB/NCLOB/LONG and BLOB/LONG RAW | ||
| "LongVarChar", "LongRaw", // go-ora driver-level names for CLOB/NCLOB/LONG and BLOB/LONG RAW (inline LOB mode) | ||
| "OCIClobLocator", "OCIBlobLocator": // go-ora driver-level LOB locator names (non-inline mode) |
There was a problem hiding this comment.
LongVarRaw is missing from isLOBType. This PR treats LongVarRaw as a driver spelling for LONG RAW everywhere else — it is added to the byte-array scanner case (snapshot.go#L467-L470), to the ByteArray mapping in oracleTypeToCommonType, and asserted as a LONG RAW driver name in TestOracleSchemaSourceParity (schema_test.go#L355-L360) — but the LOB filter here still only knows LongRaw.
Since the filter keys off the driver name (isLOBType(types[idx].DatabaseTypeName()) at snapshot.go#L285-L289), a LONG RAW column that the driver reports as LongVarRaw would keep emitting its full payload in snapshot rows even when the user set lob_enabled: false. Suggest adding LongVarRaw to this case list so the two enumerations stay in sync.
There was a problem hiding this comment.
good catch — added LongVarRaw to isLOBType in 4924a67 so all three name lists agree.
| // matching the driver's undeclared-scale sentinel. | ||
| func catalogNumberInfo(precision, scale sql.NullInt64) (p, s int64, hasDecimalInfo bool) { | ||
| if !precision.Valid && scale.Valid { | ||
| return 38, scale.Int64, true |
There was a problem hiding this comment.
The literal 38 (Oracle's maximum NUMBER precision) is used bare in logic here. Per the project Go patterns (.claude/agents/godev.md, Code Style Rules → Magic Numbers): "Name all numeric constants. Every literal number in logic must have a clear meaning through a named constant or variable."
Suggest a named constant (e.g. oracleMaxNumberPrecision = 38) next to the existing precedent replication.MaxInt64DecimalPrecision (stream_message.go#L23-L27) and use it here, so the substitution documents itself at the call site rather than only in the doc comment. See schema.go#L84-L89.
There was a problem hiding this comment.
done in 4924a67 — added replication.MaxOracleNumberPrecision next to MaxInt64DecimalPrecision and used it in catalogNumberInfo.
| | `NUMBER(p, s<0)` | String (BigDecimal) ⁵ | `json.Number` | `json.Number` ¹ | `1200` | | ||
| | `NUMBER` (bare) | String | `json.Number` | `json.Number` ¹ | `42` | | ||
| | `INTEGER` / `INT` / `SMALLINT` | Int64 ² | `int64` | `int64` ¹ | `42` | | ||
| | `FLOAT` | String ² | `json.Number` | `json.Number` ¹ | `1.5` | | ||
| | `INTEGER` / `INT` / `SMALLINT` | String (Decimal(38,0)) ² | `string` | `string` ¹ | `"42"` | | ||
| | `FLOAT` | String (BigDecimal) ² | `json.Number` | `json.Number` ¹ | `1.5` | |
There was a problem hiding this comment.
These two rows document the wrong Go/wire types for their (new) schema mapping. Both NUMBER(p, s<0) and FLOAT now resolve to BigDecimal, and BigDecimal values are canonicalised to a string in both paths — snapshot scans them as text and runs sqlutil.CoerceToCommon (snapshot.go#L498-L509), and streaming goes through the same function (coerce.go#L74-L83). The integration test asserts exactly this — a Decimal/BigDecimal column must never surface as json.Number (integration_datatypes_test.go#L317-L330).
So the Snapshot/Streaming Go Type columns should read string and the JSON wire format "1200" / "1.5", matching how the INTEGER row directly between them was corrected in this same hunk. As written, a reader is told a FLOAT column arrives as a bare JSON number, which is the behaviour this PR is fixing away from (§1.1.1 configuration/behaviour documentation, §1.2.3 documented edge cases).
There was a problem hiding this comment.
you're right — fixed in 4924a67, and I've corrected the neighbouring decimal rows too (NUMBER(p>18,0), NUMBER(p,s>0), bare NUMBER) since the integration test asserts string values for all of them.
- add LongVarRaw to isLOBType so lob_enabled filtering agrees with the scanner and type mapping on every driver spelling of LONG RAW - name the NUMBER(*,s) precision substitution (replication.MaxOracleNumberPrecision) instead of a bare 38 - correct the TYPES.md decimal rows: Decimal/BigDecimal values are canonicalised to strings in both paths, as the datatypes integration test asserts
| } | ||
|
|
||
| db.MustExec(insertSQL) | ||
| phases["stream-post-restart"] = waitForMessage() |
There was a problem hiding this comment.
The restart leg never verifies its own premise, so it can silently stop testing anything.
The whole value of this leg is that it resumes from the persisted SCN checkpoint and therefore skips the snapshot, leaving the catalog-derived schema as the only one in play. But nothing asserts that. If the checkpoint isn't there or isn't resumed (e.g. a future change to when the SCN is flushed, or StopWithin cutting the flush short), the second stream re-runs the snapshot, waitForMessage() returns a snapshot-seeded message, and phases["stream-post-restart"] becomes a duplicate of phases["snapshot"] — every assertion below still passes while the catalog mapping is no longer exercised at all. That is exactly the blind spot this leg was added to close (per its own comment: "within a single process lifetime streaming reuses the snapshot-seeded schema and the catalog mapping never appears in any published message").
batcher.go:172 already sets an operation metadata key from replication.MessageOperationRead, so this is cheap to close: capture operation alongside body/schema in capturedMessage and require the post-restart message's operation to not be the snapshot-read value, so the leg fails loudly instead of degrading into a no-op.
Per CONTRIBUTING.md §1.3.2, tests must "prove that the connector works across supported configurations" — a leg that can pass without exercising the path under test doesn't prove it.
There was a problem hiding this comment.
really good catch — the leg could indeed rot silently. fixed in 3a72416: capturedMessage now carries the operation metadata, the snapshot phase must be a read, and the post-restart phase must not be, so a broken checkpoint resume fails the test loudly. re-ran the integration test against real Oracle to confirm the premise actually holds.
| - oracledb_cdc: Fix the per-message schema metadata flipping between the snapshot and streaming paths for TIMESTAMP, INTEGER and binary columns, which caused permanent Schema Registry BACKWARD-compatibility rejections (and a failed registration round-trip per row) after a pipeline restart. INTEGER/INT/SMALLINT columns now consistently map to Decimal(38,0); subjects registered with the old catalog-derived typing need a one-time migration described in the PR. ([@Jeffail](https://github.com/Jeffail), [#4661](https://github.com/redpanda-data/connect/pull/4661)) | ||
|
|
||
| ## 4.103.1 - 2026-07-31 | ||
|
|
There was a problem hiding this comment.
The migration note is incomplete: NUMBER(p,-s) columns also change their catalog-derived typing in this PR, but only INTEGER/INT/SMALLINT are called out.
oracletypes.go changes negative scale from Decimal(precision, 0) (scale = 0 then fall through) to BigDecimal. On the catalog path that scale was previously reachable — DATA_PRECISION/DATA_SCALE are both non-NULL for a declared NUMBER(5,-2), so hasDecimalInfo was true and the column registered as Decimal(5,0). It now registers as BigDecimal.
That is the same class of breakage the entry already warns about for INTEGER (whose catalog typing moves BigDecimal → Decimal(38,0)): any subject already registered from the catalog path needs the same one-time migration. Users with negative-scale NUMBER columns will hit it with no warning here. Worth adding them to the sentence about subjects needing migration.
TYPES.md does document the new mapping (footnote ⁵), so this is only the release-note/migration text.
There was a problem hiding this comment.
agreed — added negative-scale NUMBER(p,-s) to the changelog migration note and to the behaviour-change section of the PR description in 3a72416. (the changelog entry also moved under the existing 4.103.2 - TBD heading while resolving the merge conflict with main, rather than guessing a version.)
…ma-source-parity # Conflicts: # CHANGELOG.md
- assert the restart leg's premise via operation metadata: the snapshot phase must be a 'read' and the post-restart phase must NOT be, so the leg fails loudly if checkpoint resume breaks instead of silently re-testing the snapshot path - extend the changelog migration note to cover negative-scale NUMBER(p,-s) columns, whose catalog typing changes Decimal(p,0) -> BigDecimal in this PR
| require.NotEqual(t, "read", phases["stream-post-restart"].operation, | ||
| "post-restart phase captured a snapshot read: the restarted stream did not resume from the checkpoint, so the catalog-derived schema path is not being exercised") | ||
|
|
||
| phaseNames := []string{"snapshot", "stream-insert", "stream-update", "stream-post-restart"} |
There was a problem hiding this comment.
The new restart leg can't verify the parity claim that motivates it, because every schema comparison in this test goes through childTypeMap, which keeps only Children[i].Type and discards the logical decimal precision/scale:
connect/internal/impl/oracledb/integration_datatypes_test.go
Lines 352 to 359 in 3a72416
So the per-column assertion at
connect/internal/impl/oracledb/integration_datatypes_test.go
Lines 310 to 318 in 3a72416
compares
schema.Decimal == schema.Decimal and passes regardless of precision/scale.
That is precisely the dimension the new catalogNumberInfo substitution controls. For num_star_2 NUMBER(*,2) and num_int INTEGER, the catalog path produces Decimal(38, s) because of the hardcoded MaxOracleNumberPrecision; if go-ora actually reported a different precision on the wire (e.g. Decimal(22,2)), the snapshot and post-restart messages would still both be schema.Decimal and this leg would pass — while the emitted schema metadata, and therefore the Schema Registry subject, would still flip between the two paths with the same BACKWARD incompatibility the PR is fixing. Avro decimal precision/scale are part of the type.
This also makes the "verified against real Oracle by the restart leg of TestIntegrationOracleDBCDCDataTypeConsistency" claim in the new catalogNumberInfo doc comment (
connect/internal/impl/oracledb/schema.go
Lines 75 to 89 in 3a72416
TestOracleSchemaSourceParity, not by real Oracle.
Suggested fix: compare the full schema.Common children (name + type + logical decimal precision/scale) across phases instead of only CommonType — e.g. index children by name into schema.Common rather than schema.CommonType — so the restart leg fails when the catalog- and driver-derived precision/scale disagree. Cited per CONTRIBUTING.md §1.3.2/§1.3.3 (tests must prove the behaviour they claim to cover).
There was a problem hiding this comment.
fair — the leg was asserting a weaker claim than the doc comment made, and a precision-only flip would have slipped through. fixed in cc9a6ed: the cross-phase comparison now uses the full schema.Common per column (type + optionality + logical decimal precision/scale), and the diagnostic table renders them (DECIMAL(38,2) etc.). re-ran against real Oracle and it passes — NUM_INT is Decimal(38,0) and NUM_STAR_2 is Decimal(38,2) from both schema sources, so the precision-38 substitution in catalogNumberInfo is now pinned empirically rather than only by the hand-written parity rows.
The cross-phase schema assertion previously reduced each column to its CommonType, so a Decimal(38,2) vs Decimal(22,2) flip between the snapshot-seeded and catalog-derived paths would pass unnoticed even though Avro decimal precision/scale are part of the type. Compare the full schema.Common per column (type, optionality, logical decimal parameters) instead, and render precision/scale in the diagnostic table. Verified against real Oracle: NUMBER(*,s) columns report precision 38 from both schema sources, pinning the MaxOracleNumberPrecision substitution empirically.
Three independent review passes over the branch (cross-enumeration consistency, assertion strength vs claim strength, docs/migration notes as verifiable claims) surfaced gaps in what the tests and notes actually prove: - pin exact decimal (precision, scale) constants in NumberToCommon and parity tests: source agreement alone would let both sides agree on the WRONG parameters and re-register an incompatible schema - capture each integration phase by operation metadata (drain-until-op, forbidden ops): at-least-once redelivery could otherwise substitute a duplicate earlier event for the phase under test, silently gutting its coverage - add num_neg NUMBER(5,-2) to the integration table, pinning the negative-scale mapping and value canonicalisation end to end - add TestSnapshotScannerSchemaParity tying the three case-sensitive driver-spelling enumerations (schema mapping, snapshot scan destinations via the extracted SnapshotScanDest, lob_enabled filter via exported IsLOBTypeName) to one table — the family both prior review misses lived in - assert timestamp values render as RFC 3339 in every phase, not just as matching Go types - correct the migration notes: for BLOB/LONG RAW and native JSON the snapshot/driver path was the incorrect side (string), so subjects registered from the SNAPSHOT path need migrating for those columns — the changelog previously attributed all migration need to the catalog path; also name the NUMBER(*,s) BigDecimal->Decimal(38,s) change and the lob_enabled nulling fix for non-inline LOB fetch modes - soften overstated doc comments and fix TYPES.md scale-range rows and the base64 wire example
…racledb_cdc entry
|
|
||
| // Oracle database type names | ||
| switch dbTypeName { | ||
| case "RAW", "LONG RAW", "BLOB", "VarRaw", "LongRaw", "LongVarRaw", "OCIBlobLocator": |
There was a problem hiding this comment.
Test coverage: the non-inline LOB path this PR fixes is never exercised end to end.
This PR adds OCIBlobLocator here (and OCIClobLocator at snapshot.go#L517-L519, plus both spellings to IsLOBTypeName), and the CHANGELOG advertises it as a user-visible fix: "lob_enabled: false now correctly nulls LOB columns for connections using non-inline LOB fetching (lob fetch=stream/post), which previously did not null them."
The only verification is TestSnapshotScannerSchemaParity, which asserts SnapshotScanDest's own return values against a hand-written table — it cannot prove go-ora actually delivers a locator value into the new destination. The existing integration test that covers this behaviour, TestIntegrationOracleDBCDCLargeObjectColumnsToggle (integration_test.go#L879-L894), builds its connection string from oracledbtest.SetupTestWithOracleDBVersion (oracledbtest.go#L412-L416), which appends no lob fetch option — so every test in the tree runs in inline mode and sees LongVarChar/LongRaw, never the locator spellings.
That leaves a behaviour change untested in a supported configuration: for OCIBlobLocator the scan destination changes from sql.Null[string] (previous default branch) to sql.Null[[]byte], and the accompanying schema type changes String → ByteArray. If go-ora's locator-mode value doesn't convert to []byte, this surfaces as a hard scan error at snapshot time rather than the old (wrong but non-fatal) string.
Suggested fix: parameterise TestIntegrationOracleDBCDCLargeObjectColumnsToggle (or add a sibling case) over a connection string carrying lob fetch=stream, asserting both lob_enabled: false nulls the LOB columns and lob_enabled: true still yields usable values.
Per CONTRIBUTING.md §1.3.2 — "Tests should cover end-to-end functionality and prove that the connector works across supported configurations" — and §1.3.3, "Integration tests verify core workflows and are runnable in CI".
There was a problem hiding this comment.
agreed — the locator spellings were reasoned from go-ora source but never exercised, and the OCIBlobLocator scan-destination change (string → []byte) was exactly the risky bit. added two lob fetch=stream legs to TestIntegrationOracleDBCDCLargeObjectColumnsToggle in b7c108f: lob_enabled: false must null the CLOB and BLOB columns, and lob_enabled: true must deliver the CLOB string and base64 BLOB payload. both pass against real Oracle, so the locator-mode scan destinations and the IsLOBTypeName additions are now pinned empirically rather than statically. (snapshot-only legs — the LogMiner path is independent of the client's LOB fetch mode, which the inline legs already cover.)
…log edit Add lob fetch=stream legs to the LOB toggle integration test: go-ora reports LOB columns under their locator spellings (OCIClobLocator/OCIBlobLocator) in this mode, exercising the driver-name additions to the snapshot scanner, the schema mapping, and the lob_enabled filter against real Oracle. lob_enabled: false must null the LOB columns; lob_enabled: true must deliver the CLOB string and BLOB bytes. Snapshot-only legs: the LogMiner streaming path is independent of the client's LOB fetch mode, which the inline legs already cover. Also revert the hand-added CHANGELOG.md entry: changelog entries are generated during the release process, and hand-added ones only create merge conflicts between in-flight PRs. The release-note wording lives in the PR description.
| _, _ = db.Exec(`TRUNCATE TABLE RPCN.CDC_CHECKPOINT_CACHE`) | ||
|
|
||
| t.Run("lob_enabled=false lob-fetch=stream", func(t *testing.T) { |
There was a problem hiding this comment.
The two new lob fetch=stream legs run while the lob_enabled=true stream from the previous subtest is still running — that stream is only stopped at the end of the test function (integration_test.go#L1141-L1143), not inside its own t.Run (contrast the lob_enabled=false leg, which stops at L963 before the first TRUNCATE).
All four streams share the same checkpoint store and key: checkpoint_cache_table_name defaults to RPCN.CDC_CHECKPOINT_CACHE and checkpoint_cache_key defaults to oracledb_cdc (input_oracledb_cdc.go#L211-L221), and none of the new configs override them.
Failure scenario: the still-live TESTDB.LOBENABLED stream keeps upserting its SCN under key oracledb_cdc. Between the TRUNCATE here and the new leg's Connect() (table creation + 5 inserts + build/start — seconds), it re-writes a checkpoint row with a far-ahead SCN. The new leg then sees an existing checkpoint, skips the snapshot entirely, and assert.Eventually spins for 5 minutes before require.Truef(t, got == lobStreamRows) fails — a flaky, slow failure that hides the behaviour the leg is meant to prove. The two live streams also clobber each other's checkpoints under the shared key.
Suggested fix: stop the lob_enabled=true stream at the end of its own subtest (before the first new TRUNCATE), or give the two new legs a distinct checkpoint_cache_key so they can't collide with a concurrently-running stream. Relatedly, runLobStreamLeg returns the stream for the caller to stop, so a require failure inside it leaks a live stream into the next leg's truncate — stopping via t.Cleanup inside the helper would close that hole too.
There was a problem hiding this comment.
right again — the shared default checkpoint key plus the still-live previous stream was a race I wrote straight past, and the helper leaked its stream on a require failure. fixed in the latest commit: each stream-fetch leg gets its own checkpoint_cache_key, the lob_enabled=true stream now stops inside its own subtest, and the helper stops its stream via t.Cleanup. re-ran the full four-leg test against real Oracle — passes.
The two lob fetch=stream legs ran while the lob_enabled=true subtest's stream was still live, and all four streams shared the default checkpoint cache key — an in-flight ack from the earlier stream could re-write a checkpoint between the TRUNCATE and the new leg's Connect(), making the leg silently skip the snapshot it exists to assert (a slow, flaky failure). Give each stream-fetch leg its own checkpoint_cache_key, stop the lob_enabled=true stream inside its own subtest, and stop the helper's stream via t.Cleanup so a require failure can't leak a live stream into subsequent legs.
| // when ALL_TAB_COLUMNS reports a NULL DATA_PRECISION with a non-NULL scale — | ||
| // see catalogNumberInfo. | ||
| const MaxOracleNumberPrecision = 38 | ||
|
|
There was a problem hiding this comment.
I was looking into this in the go-ora driver and Claude raised this, I'm not sure how much of a concern it is?
But there's a wrinkle worth checking. That same branch (in go-ora) also unconditionally forces Scale = 0xFF, and it fires whenever Precision == 0 and (Scale == 0 or Scale == 0xFF) — i.e. it can't distinguish two different wire states:
- a truly undeclared bare NUMBER/FLOAT, which reaches this point with Scale already 0xFF (set a few lines earlier when the raw wire scale is the -127 sentinel, parameter.go:180-184), from
- a NUMBER(*,0)-shaped column (INTEGER/SMALLINT, undeclared precision but a declared scale of 0), which reaches this point with Scale == 0 because the wire sent an actual 0, not the -127 sentinel.
Both collapse to the identical (Precision=38, Scale=0xFF) result. That's a problem for this PR's premise: schema_test.go:327 hardcodes driverSource{typeName: "NUMBER", precision: 38, scale: 0} for the INTEGER case (with the comment at :323-324 asserting "the driver reports (38, 0)"). If go-ora's real behavior is what I just traced, the driver would actually report (38, 255) for INTEGER — same as bare NUMBER — which NumberToCommon maps to BigDecimal via the scale > precision branch (oracletypes.go:45-46), not the intended Decimal(38,0). That would reopen exactly the catalog/driver mismatch this PR exists to close, just for INTEGER/INT/SMALLINT columns specifically, and only via the snapshot/driver path.
There was a problem hiding this comment.
good spot digging into that — the trace is right, that collapse branch in go-ora genuinely can't tell bare NUMBER from NUMBER(*,0) if both arrive with wire precision 0. so I ran a probe against the real Oracle image dumping DecimalSize() per column, and the answer is the branch never fires for INTEGER: the server wire-describes INTEGER/INT/SMALLINT as precision 38, scale 0 (unlike ALL_TAB_COLUMNS, which reports NULL — that asymmetry is exactly what catalogNumberInfo bridges).
full dump from the probe:
C_INTEGER | NUMBER | precision=38 scale=0 ok=true
C_INT | NUMBER | precision=38 scale=0 ok=true
C_SMALLINT | NUMBER | precision=38 scale=0 ok=true
C_NUM_STAR2 | NUMBER | precision=38 scale=2 ok=true
C_NUM_BARE | NUMBER | precision=38 scale=255 ok=true
C_FLOAT | NUMBER | precision=38 scale=255 ok=true
C_NUM_5 | NUMBER | precision=5 scale=0 ok=true
C_NUM_10_2 | NUMBER | precision=10 scale=2 ok=true
C_NUM_NEG | NUMBER | precision=5 scale=254 ok=true
every hand-written driver row in TestOracleSchemaSourceParity matches. and if some other Oracle version did describe INTEGER as (0,0) → go-ora's (38,255) → BigDecimal, the restart leg's full-schema comparison would fail loudly (Decimal(38,0) catalog vs BigDecimal snapshot) rather than letting the mismatch back in silently — so it's a tripwired hypothetical rather than an open risk. happy to share the probe snippet if useful.
| // type-name mapping cost here is paid once per snapshot message, not once per | ||
| // table. Column set mirrors the integration test's ALL_TYPES table using | ||
| // go-ora driver type-name spellings, as buildColumnMeta reports them. | ||
| func BenchmarkSeedFromColumnMeta(b *testing.B) { |
There was a problem hiding this comment.
NOTE — benchmark results not recorded (CONTRIBUTING.md §1.3.4/§1.3.5, docs/benchmarking.md "Keeping Results Up to Date" item 2 and item 4)
This benchmark's own comment identifies the changed code as a hot path — "the hottest schema-cache path: the publisher re-seeds the cache from driver column metadata for EVERY snapshot row published" — and the commit message for oracledb_cdc: derive identical message schemas from catalog and driver metadata asserts "no regression, allocations unchanged". docs/benchmarking.md says:
When modifying a connector's performance path — Re-run the benchmark and append a new dated section to the results file. This includes changes to batching, buffering, connection handling, serialization, or any code that sits in the hot path.
and, under "During code review":
It will also note when performance-critical connector changes may warrant a benchmark re-run.
docs/benchmark-results/oracledb-cdc.md exists (linked from the "Existing Benchmarks" table alongside internal/impl/oracledb/bench/) but is not updated in this PR, so the no-regression claim isn't recorded anywhere reviewable.
Suggested fix: run the existing internal/impl/oracledb/bench/ suite (snapshot and streaming, per the "Snapshot vs Streaming" section) and append a dated section to docs/benchmark-results/oracledb-cdc.md with the PR link and the before/after numbers — or state in the PR description why a re-run isn't warranted for a per-column type-mapping change. The testing.B micro-benchmark itself matches the sanctioned Go Benchmark Tests pattern (b.ReportAllocs() + for b.Loop()) — the gap is only the recorded result.
…r metadata (#4661) * oracledb_cdc: derive identical message schemas from catalog and driver metadata The per-message schema was derived from two sources that disagree on how they report the same column: ALL_TAB_COLUMNS embeds fractional-seconds precision in timestamp type names (TIMESTAMP(6)), which fell through the type mapping to String, while the go-ora driver's names (TimeStampDTY) mapped to Timestamp. Snapshot messages seed the schema cache from driver metadata and restarts rebuild it from the catalog, so the schema attached to messages flipped depending on which path produced them — and under BACKWARD compatibility, schema_registry_encode permanently rejected every message from whichever path lost the initial registration. Both sources now route through a single mapping (columnToCommon), with the source-specific encodings bridged explicitly: - normalizeOracleTypeName strips parenthesised size qualifiers from catalog type names before matching - catalogNumberInfo substitutes precision 38 for NUMBER(*,s) columns (INTEGER/INT/SMALLINT), matching what the driver reports on the wire - negative-scale NUMBER(p,-s) maps to BigDecimal from both sources (the driver's uint8 scale wraps and can't represent it faithfully) - go-ora driver spellings for binary and JSON types (VarRaw, LongRaw, LongVarRaw, OCIBlobLocator, TNSType(119)) map like their catalog equivalents; LOB locator names added to the snapshot scanner and lob_enabled filtering TestOracleSchemaSourceParity pins catalog-shaped and driver-shaped inputs to identical schemas for every supported type, and the datatypes integration test gains a restart leg that resumes from the checkpoint so the catalog-derived schema actually appears in asserted messages — within a single process lifetime, streaming reuses the snapshot-seeded schema and the catalog mapping was never exercised, which is how this went unseen. The restart leg caught the INTEGER divergence empirically during development. A benchmark covers the per-row seeding hot path (no regression, allocations unchanged). Behaviour note: INTEGER/INT/SMALLINT columns now consistently emit Decimal(38,0) instead of flip-flopping between Decimal and BigDecimal, and timestamp columns consistently emit Timestamp; subjects whose registered schema carries the old catalog-derived typing need a one-time migration (see PR description). * changelog: add oracledb_cdc schema consistency fix entry * oracledb_cdc: address review feedback - add LongVarRaw to isLOBType so lob_enabled filtering agrees with the scanner and type mapping on every driver spelling of LONG RAW - name the NUMBER(*,s) precision substitution (replication.MaxOracleNumberPrecision) instead of a bare 38 - correct the TYPES.md decimal rows: Decimal/BigDecimal values are canonicalised to strings in both paths, as the datatypes integration test asserts * oracledb_cdc: address second round of review feedback - assert the restart leg's premise via operation metadata: the snapshot phase must be a 'read' and the post-restart phase must NOT be, so the leg fails loudly if checkpoint resume breaks instead of silently re-testing the snapshot path - extend the changelog migration note to cover negative-scale NUMBER(p,-s) columns, whose catalog typing changes Decimal(p,0) -> BigDecimal in this PR * oracledb_cdc: compare full column schemas across integration phases The cross-phase schema assertion previously reduced each column to its CommonType, so a Decimal(38,2) vs Decimal(22,2) flip between the snapshot-seeded and catalog-derived paths would pass unnoticed even though Avro decimal precision/scale are part of the type. Compare the full schema.Common per column (type, optionality, logical decimal parameters) instead, and render precision/scale in the diagnostic table. Verified against real Oracle: NUMBER(*,s) columns report precision 38 from both schema sources, pinning the MaxOracleNumberPrecision substitution empirically. * oracledb_cdc: strengthen tests and docs from adversarial self-review Three independent review passes over the branch (cross-enumeration consistency, assertion strength vs claim strength, docs/migration notes as verifiable claims) surfaced gaps in what the tests and notes actually prove: - pin exact decimal (precision, scale) constants in NumberToCommon and parity tests: source agreement alone would let both sides agree on the WRONG parameters and re-register an incompatible schema - capture each integration phase by operation metadata (drain-until-op, forbidden ops): at-least-once redelivery could otherwise substitute a duplicate earlier event for the phase under test, silently gutting its coverage - add num_neg NUMBER(5,-2) to the integration table, pinning the negative-scale mapping and value canonicalisation end to end - add TestSnapshotScannerSchemaParity tying the three case-sensitive driver-spelling enumerations (schema mapping, snapshot scan destinations via the extracted SnapshotScanDest, lob_enabled filter via exported IsLOBTypeName) to one table — the family both prior review misses lived in - assert timestamp values render as RFC 3339 in every phase, not just as matching Go types - correct the migration notes: for BLOB/LONG RAW and native JSON the snapshot/driver path was the incorrect side (string), so subjects registered from the SNAPSHOT path need migrating for those columns — the changelog previously attributed all migration need to the catalog path; also name the NUMBER(*,s) BigDecimal->Decimal(38,s) change and the lob_enabled nulling fix for non-inline LOB fetch modes - soften overstated doc comments and fix TYPES.md scale-range rows and the base64 wire example * oracledb_cdc: add doc comment to exported IsLOBTypeName * changelog: correct migration attribution and Avro coercion note for oracledb_cdc entry * oracledb_cdc: cover non-inline LOB fetch mode end to end; drop changelog edit Add lob fetch=stream legs to the LOB toggle integration test: go-ora reports LOB columns under their locator spellings (OCIClobLocator/OCIBlobLocator) in this mode, exercising the driver-name additions to the snapshot scanner, the schema mapping, and the lob_enabled filter against real Oracle. lob_enabled: false must null the LOB columns; lob_enabled: true must deliver the CLOB string and BLOB bytes. Snapshot-only legs: the LogMiner streaming path is independent of the client's LOB fetch mode, which the inline legs already cover. Also revert the hand-added CHANGELOG.md entry: changelog entries are generated during the release process, and hand-added ones only create merge conflicts between in-flight PRs. The release-note wording lives in the PR description. * oracledb_cdc: isolate LOB-toggle test legs from shared checkpoint state The two lob fetch=stream legs ran while the lob_enabled=true subtest's stream was still live, and all four streams shared the default checkpoint cache key — an in-flight ack from the earlier stream could re-write a checkpoint between the TRUNCATE and the new leg's Connect(), making the leg silently skip the snapshot it exists to assert (a slow, flaky failure). Give each stream-fetch leg its own checkpoint_cache_key, stop the lob_enabled=true stream inside its own subtest, and stop the helper's stream via t.Cleanup so a require failure can't leak a live stream into subsequent legs.
What
Fixes a permanent
schema_registry_encodefailure mode inoracledb_cdc: the schema attached to each message could differ between the snapshot path and the streaming path for the same column, so whichever path registered the subject first would win, and every message from the other path was then rejected by Schema Registry with a BACKWARDMISSING_UNION_BRANCHerror — a 100%-failure-rate condition that never self-heals, plus a wasted HTTP round-trip per affected row.Root cause
The schema cache is populated from two sources that report the same column differently:
fetchTableSchema, fromALL_TAB_COLUMNS) — used for theConnect()pre-fetch and drift refreshes, i.e. everything after a restart. Oracle embeds the fractional-seconds precision in the type name itself (TIMESTAMP(6),TIMESTAMP(6) WITH TIME ZONE), which fell throughoracleTypeToCommonType's match list toString.seedFromColumnMeta, from go-ora'sDatabaseTypeName()) — seeded by every snapshot message. go-ora reportsTimeStampDTY-style names, which matched →Timestamp.So the first run (snapshot seeds the cache) registers timestamps as
long/timestamp-millis, and after any restart or reconnect (catalog-only cache, snapshot skipped) every streamed message carries astring-typed schema for the same columns — permanently incompatible. The same class of mismatch existed forINTEGER/INT/SMALLINT(catalog reports NULLDATA_PRECISIONforNUMBER(*,s), the driver reports 38 → BigDecimal vs Decimal(38,0)), negative-scaleNUMBER(p,-s), and BLOB/LONG RAW via the driver'sLongRaw/OCIBlobLocatorspellings.Changes
normalizeOracleTypeName: strips parenthesised size qualifiers from catalog type names before matching (TIMESTAMP(6) WITH TIME ZONE→TIMESTAMP WITH TIME ZONE).columnToCommon: single shared mapping point — both schema sources now route through it, so they can't silently diverge again.catalogNumberInfo: bridges NULLDATA_PRECISIONforNUMBER(*,s)to the precision 38 the driver reports (verified against real Oracle).NUMBER(p,-s)→ BigDecimal from both sources (the driver's uint8 scale wraps, e.g. −2 → 254, and trips the undeclared-scale sentinel; the catalog now lands in the same place).VarRaw,LongRaw,LongVarRaw,OCIBlobLocator,TNSType(119)) map like their catalog equivalents; LOB locator names added to the snapshot scanner andlob_enabledfiltering.TYPES.mdcorrected — it documented the catalog names without the precision suffix.Testing
TestOracleSchemaSourceParity(new): pins catalog-shaped and driver-shaped inputs to identicalschema.Commonfor every supported Oracle type, including the NULL-precision and wrapped-negative-scale encodings.TestIntegrationOracleDBCDCDataTypeConsistency(new): stops the stream after the snapshot/insert/update phases, rebuilds it so it resumes from the checkpoint (snapshot skipped → catalog-derived schema in every message), and asserts schema + value-type equality across all four phases. This is the scenario where the bug lived — within one process lifetime, streaming reuses the snapshot-seeded schema, so the catalog mapping never appeared in any asserted message before. The leg earned its keep during development by catching the INTEGER divergence empirically. Passes against real Oracle (container-registry.oracle.com/database/free).TIMESTAMP(6), …) and driver aliases; negative-scale/sentinel cases added.BenchmarkSeedFromColumnMeta(new) covers the per-snapshot-row seeding hot path: ~16.97µs → ~17.12µs per op (within noise), allocations unchanged. The regex normalisation only runs for catalog names (once per table per connect/drift refresh).task fmt/task lint(0 issues) / full unit suite / template tests all green.Suggested release note
(The changelog is generated at release time, so no CHANGELOG.md edit in this PR — suggested wording for the release entry:)
Columns that previously flip-flopped now consistently emit one type: timestamps →
Timestamp(long/timestamp-millisin Avro),INTEGER/INT/SMALLINT→Decimal(38,0),NUMBER(*,s)→Decimal(38,s), negative-scaleNUMBER(p,-s)→BigDecimal(previouslyInt64, orDecimal(p,0)for p>18, on the catalog path),BLOB/LONG RAW→ByteArray, and nativeJSON→Any(Avrobytes).Which registered subjects need a one-time migration depends on which side was wrong for that column type — the two sources erred in opposite directions:
string-typed timestamps and need migrating.BLOB/LONG RAWand nativeJSON: the catalog typing was already correct — it was the snapshot/driver path that fell through tostring(LongRaw,TNSType(119)). Subjects registered from the snapshot path (again, the common case) carrystring-typed fields for these columns and need migrating.NUMBER(*,s)(includingINTEGER): the snapshot-seededDecimal(38,s)was already correct and heals automatically. The catalog path previously producedBigDecimal, which the Avro converter rejects outright — so on Avro pipelines those messages were failing to encode rather than registering an incompatible version, and they simply start working after this fix. Pipelines encoding withjson_schema(whereBigDecimalregisters as a pattern-constrained string) may hold the old typing and need the script.NUMBER(p,-s): subjects registered from the catalog path carrylong(Avro) fields from the oldInt64mapping and need migrating. Note these columns now map toBigDecimalfrom both sources, which — like bareNUMBERandFLOAT— has no Avro representation and needs an upstream cast/coercion forschema_registry_encode(avro); the driver cannot report a negative scale faithfully, so a bounded decimal isn't derivable consistently from both sources.Also note:
lob_enabled: falsenow correctly nulls LOB columns for connections using non-inline LOB fetching (lob fetch=stream/postin the connection string); previously those columns were not nulled (CLOB content was delivered as a plain string).The migration script below covers the migrating groups: for each subject it shows the string-typed candidate fields, then (with
--apply) temporarily relaxes the subject's compatibility, waits for the running pipeline to register the corrected schema, and restores the previous compatibility setting. One caveat on the dry-run listing: it surfaces string-typed fields, which covers the timestamp/binary/JSON cohorts — negative-scale NUMBER columns registered aslongwon't appear in it, so check those manually; the--applyflow itself handles any incompatibility the same way.migrate-oracledb-cdc-subjects.sh
One more note for reviewers: downstream consumers that project column types from the schema (JDBC-style sinks, Iceberg, etc.) will observe the type change on migrated subjects — per-message Avro consumers are unaffected since they resolve by embedded schema ID. From what I can tell this is unavoidable in any fix that makes the types consistent; happy to talk through alternatives if anyone sees a gentler path.
🤖 Generated with Claude Code