Skip to content

oracledb_cdc: Recreate stored procedure for updating checkpoint cache, removing the need to delete previous one first#4481

Open
josephwoodward wants to merge 4 commits into
mainfrom
jw/checkpointcachestoredproc
Open

oracledb_cdc: Recreate stored procedure for updating checkpoint cache, removing the need to delete previous one first#4481
josephwoodward wants to merge 4 commits into
mainfrom
jw/checkpointcachestoredproc

Conversation

@josephwoodward

@josephwoodward josephwoodward commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

This change ensures the stored procedure responsible for updating the OracleDB backed checkpoint cache is aligned with the table it's updating. This is especially useful now that the checkpoint cache table name can be configured.

This change also supports idempotent updating of the stored procedure, removing needing to verify of the checkpoint cache exists beforehand.

@claude

claude Bot commented Jun 2, 2026

Copy link
Copy Markdown

Commits
LGTM

Review
Small, focused change to the Oracle CDC checkpoint cache: the upsert stored procedure is now created idempotently via CREATE OR REPLACE PROCEDURE (removing the check-then-create race) and its name is derived per cache table (schema.<name>_UPDATE) instead of a fixed constant. The default cache table still maps to the same proc name as before, so the change is backward-compatible. The removed defaultStoredProcName constant has no remaining references and the strings import is still used, so it compiles cleanly.

LGTM

@josephwoodward josephwoodward changed the title oracledb_cdc: align OracleDB based checkpoint cache update stored proc with checkpoint cache table name oracledb_cdc: Recreate stored procedure for updating checkpoint cache, removing the need to delete previous one first Jun 3, 2026
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Commits
LGTM

Review
Reviewed the changes deriving the checkpoint stored procedure name from the cache table name and switching to atomic CREATE OR REPLACE PROCEDURE. The default-table case preserves the prior proc name (RPCN.CDC_CHECKPOINT_CACHE_UPDATE), so existing deployments remain compatible, and the integration test follows the project test patterns.

LGTM


@josephwoodward josephwoodward marked this pull request as ready for review June 3, 2026 22:22
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Commits
LGTM

Review
Reviewed the Oracle CDC checkpoint cache change: deriving the stored procedure name from the cache table name (<TABLE>_UPDATE) and switching from an existence-check + CREATE PROCEDURE to CREATE OR REPLACE PROCEDURE. The default table maps to the same proc name as the removed constant (backward-compatible), the table is created before the proc, the prepared statement uses the derived name consistently, and the rename/orphan behaviour is documented. The new integration test follows project patterns (CheckSkip, StreamBuilder, InjectTestService, async Run with context.Canceled handling, assert.Eventually without require inside the callback). The .adoc and Go Description() notes match.

LGTM

Comment thread internal/impl/oracledb/logminer/logminer.go Outdated
@claude

claude Bot commented Jun 4, 2026

Copy link
Copy Markdown

Commits

LGTM — all five commits follow the oracledb_cdc: <message> convention with imperative, lowercase messages and good granularity (documentation isolated in its own commit, atomic-proc / derived-name / test / docs / SCN-fallback each self-contained).

Review

The stored-proc-name derivation refactor (storedProcName() + CREATE OR REPLACE PROCEDURE) is clean and idempotent, and the new integration test exercises the rename path well.

  1. logminer.go L201-L206: the new firstSCN == 0 fallback to CURRENT_SCN may be unreachable. The added WHERE FIRST_CHANGE# > 0 filter can make the inner query return zero rows, so MIN(FIRST_CHANGE#) yields SQL NULL (not 0), and Scan into a uint64 errors on NULL before the == 0 check runs. Consider scanning into a nullable type and treating both NULL and 0 as the fallback signal.

@josephwoodward josephwoodward force-pushed the jw/checkpointcachestoredproc branch from bb9add4 to 53a6d55 Compare June 4, 2026 10:14
@claude

claude Bot commented Jun 4, 2026

Copy link
Copy Markdown

Commits
LGTM

Review
Reviewed the diff: per-table derivation of the checkpoint stored procedure name with an idempotent CREATE OR REPLACE PROCEDURE, the FindStartPos fallback to CURRENT_SCN when no valid redo logs exist (sql.NullInt64 correctly distinguishes NULL/0), the matching documentation update, and the new integration test for stored-proc recreation on rename.

The changes are focused and follow the established project patterns (error wrapping, db.Close() cleanup on error, integration test conventions with integration.CheckSkip, t.Context(), assert.Eventually without require inside, and license.InjectTestService).

LGTM

@josephwoodward josephwoodward force-pushed the jw/checkpointcachestoredproc branch from 53a6d55 to 9490d24 Compare June 16, 2026 23:23
@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown

Commits
LGTM

Review
This PR derives the checkpoint stored procedure name from the cache table name (<schema>.<name>_UPDATE) instead of a fixed CDC_CHECKPOINT_CACHE_UPDATE, and switches creation to CREATE OR REPLACE for idempotency/atomicity. The change is consistent between proc creation and the prepared statement, backward-compatible for the default table name, and covered by a new integration test verifying recreation-on-rename and that the old table/proc are left intact. Removed defaultStoredProcName has no remaining references and the strings import is still used. Documentation is updated in a separate commit.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant