oracledb_cdc: Pre-filter relevant transactions via a common table expression

[josephwoodward]

No description provided.

[claude]

Commits
LGTM

Review
This PR adds an optional CTE-based LogMiner query path (use_cte_query) to the oracledb_cdc input. The config field, spec wiring, and defaults follow project conventions (field-name constant, ConfigSpec field with description/default, FieldBool extraction, Default* var). The refactor of the non-CTE query branch preserves the original predicate/PDB-filter behavior, and SQL-literal escaping is retained. The CTE branch passes exactly two bind args (startSCN, endSCN) matching its two :1/:2 placeholders.

1. The new CTE query branch (logminer.go#L97-L162) has no unit or integration test coverage, despite introducing non-trivial SQL with documented binding subtleties. See inline comment.

[claude]

Commits
LGTM

Review
Reviewed the addition of the use_cte_query option to the oracledb_cdc input. The CTE query construction, parameter binding (:1/:2 matched by the two startSCN, endSCN args), fmt.Sprintf placeholder/arg counts, SQL escaping, and the len(userTables) > 0 guard all check out. The non-CTE branch is behaviorally equivalent to the prior code, and the new path is covered by an integration test.

LGTM

[claude]

Commits
LGTM

Review
Reviewed the addition of the use_cte_query option to the oracledb_cdc LogMiner input. The config plumbing (field constant, static default, FieldBool extraction, spec wiring) follows project conventions. The new CTE query is well-constructed: it references the two bind parameters exactly once to avoid go-ora NULL binding, uses ORDERED USE_NL hints to preserve the natural SCN scan order that transaction buffering depends on, and is correctly guarded behind len(userTables) > 0. The non-CTE refactor preserves the original predicate structure and parenthesization. An integration test exercising the CTE path (insert/update/delete) was added with proper lifecycle handling.

LGTM

[claude]

Commits
LGTM

Review
This PR adds an optional use_cte_query LogMiner setting for oracledb_cdc that rewrites the V$LOGMNR_CONTENTS query as a CTE — first resolving XIDs that touched monitored tables, then fetching rows only for those transactions. The non-CTE path was refactored to share tablePred/pdbFilter/dmlCodes builders; I verified the refactor is behavior-preserving (paren structure and PDB filtering are identical). The CTE format string placeholders match their arguments, SQL-quote escaping is preserved, and the :1/:2 binds appear exactly once as documented. Default is static (false), the field uses a constant, NewDefaultConfig is updated, and an integration test exercises the new path.

LGTM

[claude]

The outer CTE query selects all rows for the relevant XIDs (JOIN relevant_xids R ON R.RXID = V.XID) with only the pdbFilter applied — it omits the per-table predicate (tablePred) that the non-CTE logMinerQuery applies to DML codes. processRedoEvent adds every OpInsert/OpUpdate/OpDelete to the txn cache and publishes them all on commit, with no further table filtering downstream. As a result, when a single transaction touches both an included table and a table outside include, the CTE path will capture and publish the DML for the non-included table as well, whereas the non-CTE path filters it out. This is a silent divergence in include/exclude semantics between the two modes. If this is intentional, it's worth documenting; otherwise the outer query (and/or processRedoEvent) should re-apply the table filter.

confirmed=true

[josephwoodward]

Addressed in 664dbb1

[claude]

Commits

1. oracledb_cdc: support CTEs bundles a documentation change (docs/modules/components/pages/inputs/oracledb_cdc.adoc) together with code changes (logminer.go, config.go, input_oracledb_cdc.go, tests). In multi-commit PRs, documentation changes should be in a separate commit from code changes.
2. oracledb_cdc: tests passing — the message is vague and not in imperative mood (it describes a state, not an action). The commit also re-touches logminer.go from commit 1, so it reads as an unsquashed fixup. Consider squashing it into support CTEs or rewording to describe the actual change (e.g. oracledb_cdc: fix ...).

Review

The CTE-based LogMiner query is a solid optimization and the refactor into scanLogMinerRows is clean. One correctness concern on the new query path.

1. The outer CTE query (logminer.go#L147-L159) fetches all rows for the relevant XIDs without re-applying the per-table predicate that the non-CTE path uses. Since processRedoEvent publishes all buffered DML on commit with no downstream table filtering, a transaction that touches both an included and a non-included table will emit the non-included table’s DML in CTE mode — a silent divergence in include/exclude semantics versus the non-CTE path.

[claude]

Commits

1. oracledb_cdc: tests passing (5200836) — vague, WIP-style message that describes a state rather than a self-contained logical change. Per the commit policy, messages like this should be avoided; this looks like a development checkpoint that should be squashed into the feature commit.
2. oracledb_cdc: address comment (664dbb1) — fixup-style message. This should be squashed into the commit it amends rather than landing as a standalone commit.

The two substantive commits (oracledb_cdc: support CTEs, oracledb_cdc: decrease overall timeout) are well-formed.

Review

The change adds an opt-in logminer.use_cte_query field that switches LogMiner to a CTE-based query: a WITH relevant_xids clause identifies XIDs touching monitored tables, then joins back to V$LOGMNR_CONTENTS for the full event set, with a separate cross-window commit catch-up pass for transactions whose DML fell in a prior SCN window.

I traced the new query construction, bind-variable mapping (:1/:2 inner scan, :3/:4 outer fetch), the scanLogMinerRows extraction (CSF continuation, rows.Err(), and trailing-pending handling are all preserved), resource cleanup across the branching paths, and config field/default conventions. The logic is sound and consistent with existing patterns, and the new behavior is covered by an integration subtest.

LGTM