Skip to content

[Fix] [Connector-V2-CDC-Postgres] Deduplicate same TableId when information_schema.tables returns duplicated rows for HighGo database - #12421

Draft
1362227089 wants to merge 4 commits into
apache:devfrom
1362227089:fix/postgres-cdc-duplicate-table-discovery
Draft

1362227089 wants to merge 4 commits into
apache:devfrom
1362227089:fix/postgres-cdc-duplicate-table-discovery

Conversation

@1362227089

Copy link
Copy Markdown

Purpose of this pull request

Fix the Duplicate key exception when syncing with HighGo database.
HighGo database may return multiple identical rows for one physical table in information_schema.tables. This causes TableDiscoveryUtils.listTables() to generate multiple identical TableId, then fails when constructing map with Collectors.toMap().

Add deduplication logic at table‑discovery boundary inside TableDiscoveryUtils.listTables(), deduplicate by full TableId(catalog‑schema‑table) before returning table collection.
This fix only eliminates fully duplicate table entries, keep original behavior for standard PostgreSQL, no change for normal PostgreSQL discovery flow.

Does this PR introduce any user‑facing change?

No

How was this patch tested?

  1. Added new unit test case: mock information_schema.tables returns multiple identical table rows, verify output list only keeps one unique TableId.
  2. Manual verification:
    • HighGo database scenario: reproduce original Duplicate key error, after fix, Postgres‑CDC source startup successfully.
    • Standard PostgreSQL scenario: existing table‑discovery logic unchanged, CDC task works as before.

…scovery to support HighGo

------
- The root cause of the problem (HighGo information_schema.tables returned duplicate rows → TableId duplicated → downstream toMap threw duplicate key error), the fix location is at the discovery boundary, LinkedHashSet maintains order and identity, and the normal PG behavior remains unchanged as test evidence.
- This submission has no configuration items/defaults/API changes, does not involve backward compatibility and documentation updates, and the PR scope remains at 1 production file + 1 test file.

------

ISSUES: 12267
…table discovery to support HighGo"

This reverts commit 40c6cf4.

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What Problem Does This PR Solve?

HighGo (a PostgreSQL-compatible database) can return multiple identical rows for the same physical
table from information_schema.tables. TableDiscoveryUtils.listTables() appends every row it
sees without deduplicating, so the discovered TableId list contains the same table several
times. Later, PostgresIncrementalSource.tableChanges() does
discoverTables.stream().collect(Collectors.toMap(Function.identity(), ...)) with no merge
function, which throws IllegalStateException: Duplicate key <TableId> and makes the job fail to
start. The intended fix deduplicates TableId values at the table-discovery boundary, using an
order-preserving LinkedHashSet, so ordinary PostgreSQL discovery is untouched and HighGo's
repeated rows collapse to a single entry.

This root cause is unusually well substantiated: it traces back to issue #12267, where the
duplicate rows were independently confirmed against HighGo (six identical BASE TABLE rows from
information_schema.tables, with pg_class/pg_namespace checked and ruled out as the source of
the duplication), and the discovery-boundary fix approach was the one explicitly agreed on in that
thread before this PR was opened.

1. Code Change Review

1.1 Core Logic Analysis

There is a critical mismatch between what this PR is supposed to do and what its current diff
actually contains, which I found by walking the branch's commit history rather than only the
GitHub diff:

819a781f74b (HEAD) Revert "[Fix][Connector-V2]: Deduplicate table ids in PostgreSQL CDC table discovery to support HighGo"
41da549c2ca Merge branch 'dev' ...
40c6cf4f069 [Fix][Connector-V2]: Deduplicate table ids in PostgreSQL CDC table discovery to support HighGo

Commit 40c6cf4f069 contains exactly the fix described in the PR body and in issue #12267:

- final List<TableId> capturedTableIds = new ArrayList<>();
+ final Set<TableId> capturedTableIds = new LinkedHashSet<>();
  ...
- return capturedTableIds;
+ return new ArrayList<>(capturedTableIds);

But the very next commit on this branch, 819a781f74b, titled "Revert ...", reverts exactly that
change while, in the same commit, adding the new TableDiscoveryUtilsTest.java. The net result is
that git diff dev...HEAD (and gh pr diff) shows only two files changed: the new test class and
an unrelated example file — TableDiscoveryUtils.java is not touched at all in the mergeable
diff. I confirmed this by reading TableDiscoveryUtils.java directly off origin/dev: it still
uses a plain ArrayList and unconditionally calls capturedTableIds.add(tableId) for every row,
with no deduplication.

This is almost certainly an unintentional git mistake rather than a deliberate withdrawal — the PR
is still a draft, and in the linked issue the author wrote "Waiting for committer feedback on the
solution" right after posting the PR link. But as it stands today, this PR does not fix the bug
it claims to fix
.

The fix itself (commit 40c6cf4f069), once restored, is correct and minimal. I checked
io.debezium.relational.TableId (debezium-core 1.9.8.Final, the version this connector depends
on) directly from its sources jar:

public int hashCode() { return id.hashCode(); }
public boolean equals(Object obj) {
    if (obj instanceof TableId) { return this.compareTo((TableId) obj) == 0; }
    return false;
}

equals()/hashCode() are based on the fully-qualified catalog.schema.table identifier, so a
LinkedHashSet<TableId> correctly collapses only exact duplicates while preserving discovery
order — it will not accidentally merge two genuinely different tables. This is exactly the
"deduplicate at the discovery boundary, keep identity intact, don't mask it in the downstream map"
approach from the issue thread, and it stays scoped to connector-cdc-postgres; nothing in
connector-cdc-base or the generic JDBC connector is touched.

Runtime path traced for this fix:
PostgresDialect.discoverDataCollections()TableDiscoveryUtils.listTables() (dedup happens
here) → PostgresIncrementalSource.tableChanges()'s
discoverTables.stream().collect(Collectors.toMap(...)) (previously threw here on duplicates).
This confirms the normal startup path for Postgres-CDC hits the changed code on every job start,
not just a recovery/retry path — the fix is on the primary path, and so is the described bug.

1.2 Compatibility Impact

Fully compatible, once the fix commit is actually included. No config option, default, API, or
serialization format changes. For ordinary PostgreSQL users the discovered list is already
duplicate-free, so behavior is unchanged. For HighGo users the job previously failed 100% of the
time at startup, so there is no working behavior to regress.

1.3 Performance / Side-Effect Analysis

Negligible. Table discovery runs once per job initialization over a small, bounded table list;
swapping ArrayList for LinkedHashSet (then copying back to ArrayList) adds no meaningful
CPU/memory cost and does not touch any per-row or per-checkpoint hot path. No new locks, threads,
or retries are introduced.

1.4 Error Handling and Logging

Issue 1 — Blocker. The production fix in TableDiscoveryUtils.listTables() is missing from
the mergeable diff because it was reverted by the branch's own HEAD commit 819a781f74b (see
1.1). Location: seatunnel-connectors-v2/connector-cdc/connector-cdc-postgres/src/main/java/org/apache/seatunnel/connectors/seatunnel/cdc/postgres/utils/TableDiscoveryUtils.java.
Risk: the PR would merge without fixing the reported Duplicate key failure at all. Best
improvement: drop the revert commit (or re-apply the LinkedHashSet change from 40c6cf4f069) so
the final diff actually contains the production change. Severity: Blocker. Raised by another
reviewer: No.

Issue 2 — Blocker. The new tests assert behavior the current production code does not have.
Location: seatunnel-connectors-v2/connector-cdc/connector-cdc-postgres/src/test/java/org/apache/seatunnel/connectors/seatunnel/cdc/postgres/utils/TableDiscoveryUtilsTest.java:105-116 and
:140-158 (shouldDeduplicateRepeatedCatalogRows, shouldKeepFirstOccurrenceWhenDuplicatesInterleave).
Given current dev/PR-HEAD behavior (plain ArrayList, no dedup), feeding 6 identical rows
returns a list of size 6, not 1, and interleaved duplicates return 5 entries, not 3 — both
assertions would fail once CI actually executes the test. Risk: a passing-looking test suite that
is silently testing against nonexistent production behavior. Best improvement: this is
automatically resolved once Issue 1 is fixed; please re-run ./mvnw test -pl seatunnel-connectors-v2/connector-cdc/connector-cdc-postgres -nsu -Dmaven.gitcommitid.skip=true
locally after restoring the fix to confirm both tests actually pass. Severity: Blocker. Raised by
another reviewer: No.

Issue 3 — Blocker. seatunnel-examples/seatunnel-engine-examples/src/main/java/org/apache/seatunnel/example/engine/SeaTunnelEngineLocalExample.java:39
changes the hardcoded default configurePath from /examples/fake_to_console.conf to
/examples/cdc_highgo_to_mysql.conf. I searched the whole repository (including test resources
and generated target/ output) and no file named cdc_highgo_to_mysql.conf exists anywhere.
SeaTunnelEngineLocalExample.main() is the shared "run locally with no arguments" entry point many
contributors use (e.g., from an IDE run configuration). After this change, running it without an
explicit argument will throw FileNotFoundException for everyone, not just the PR author. This is
unrelated to the HighGo/Postgres-CDC fix and looks like a leftover personal local-debug edit that
was accidentally committed (it was already present in the original fix commit 40c6cf4f069, not
introduced by the revert). Best improvement: drop this hunk entirely; keep any personal example
path as a local run argument instead of changing the shared default. Severity: Blocker (breaks a
common devex path for a change unrelated to this PR's stated purpose). Raised by another reviewer:
No.

2. Code Quality Assessment

2.1 Coding Standards

The new test file has the ASF license header, uses explicit imports (no wildcards), and follows
the project's formatting conventions. No System.out.println, no @DisplayName usage, no
prohibited patterns.

2.2 Test Coverage and Test Stability

Three unit tests cover: all-duplicate collapse (6 identical rows → 1), plain PostgreSQL discovery
unchanged (distinct tables, order preserved), and duplicates interleaved with distinct tables
(collapse to first occurrence, order preserved) — this is a good, minimal set that also protects
the "must not change ordinary PostgreSQL behavior" requirement called out in the issue thread.
Mechanically, the test mocks PostgresConnection.query(String, ResultSetConsumer) (a non-final,
overridable method) and a ResultSet; I verified against the actual debezium-core 1.9.8.Final and
debezium-connector-postgres 1.9.8.Final jars that the constructor
PostgresConnection(JdbcConfiguration, String) and the query(String, ResultSetConsumer) override
point both exist with the signatures used here, so this should compile cleanly.

Flaky-test-risk rating: Stable. No sleeps, timers, shared static state, or execution-order
dependence; the mock's cursor state is local to each test instance.

However, as noted in Issue 2, these tests currently assert behavior the reverted production code
does not implement, so — independent of flakiness — they would fail on a real test run today. This
must be fixed together with Issue 1.

2.3 Documentation Updates

None included, and none are strictly required since this is an internal robustness fix with no new
config/API surface (matches the PR's own "No user-facing change" answer). A one-line mention in
docs/en/connectors/source/PostgreSQL-CDC.md / docs/zh/connectors/source/PostgreSQL-CDC.md about
HighGo table-discovery compatibility would be a nice-to-have, not a blocker.

3. Architectural Soundness

3.1 Elegance of the Solution

Once restored, the fix is a clean two-line behavioral change exactly at the boundary where
duplicates are known to originate, with no new abstractions or config surface.

3.2 Maintainability

Good — self-contained inside an existing utility method, easy to reason about, backed by targeted
unit tests.

3.3 Extensibility

Not really applicable here; this is a narrow correctness fix, not a new extension point.

3.4 Historical-Version Compatibility

No serialization format, checkpoint/savepoint, or protocol changes are involved, so there is no
upgrade/restore compatibility concern.

4. Issue Summary

Number Issue Location Severity
1 Production dedup fix (commit 40c6cf4f069) is reverted by the PR's own HEAD commit; final diff has no production code change TableDiscoveryUtils.java (missing from dev...HEAD) Blocker
2 New unit tests assert dedup behavior the current (reverted) production code doesn't have; they will fail once actually run TableDiscoveryUtilsTest.java:105-116, 140-158 Blocker
3 Unrelated default-config change points at a nonexistent file, breaks shared local example entry point SeaTunnelEngineLocalExample.java:39 Blocker
4 No doc mention of HighGo compatibility caveat (optional) docs/en, docs/zh Postgres-CDC pages Minor

5. Merge Recommendation

Conclusion: Not recommended for merge

  1. Blockers — must be fixed:
    • Restore the production fix: drop the revert commit (or re-apply the ArrayList
      LinkedHashSet change from 40c6cf4f069) so TableDiscoveryUtils.listTables() actually
      deduplicates. Right now the PR's mergeable diff does not contain this change at all.
    • After restoring the fix, run
      ./mvnw test -pl seatunnel-connectors-v2/connector-cdc/connector-cdc-postgres -nsu -Dmaven.gitcommitid.skip=true
      locally and confirm TableDiscoveryUtilsTest passes.
    • Remove the unrelated SeaTunnelEngineLocalExample.java default-config change; it points at a
      file that doesn't exist anywhere in the repo and will break the shared local run entry point
      for other contributors.
  2. Recommended fixes — non-blocking:
    • Consider a one-line doc note about HighGo table-discovery compatibility.
    • Since CI shows "Build: action_required" (this repo requires a maintainer to approve workflow
      runs for first-time contributors), a maintainer will need to approve the workflow once the
      above is fixed so real CI results are available before merge.

Overall assessment: this is one of the best-substantiated root causes I've seen come through as a
PR — issue #12267 already nailed down, with direct evidence, that HighGo's information_schema.tables
genuinely returns six identical rows for the same table, and ruled out alternate explanations
(cross-catalog discovery, physical duplication in pg_class/pg_namespace) before this PR was
opened. The originally-proposed fix (order-preserving dedup via LinkedHashSet at the discovery
boundary) is exactly right and appropriately minimal. The only reason this isn't ready to merge is
what looks like a simple, self-inflicted git mistake — the fix commit got reverted, apparently by
accident, in the same commit that added the tests — plus one unrelated stray file. Welcome to
SeaTunnel, and thank you for the very thorough investigation in #12267! Once the fix commit is
restored and the unrelated example-file change is dropped, this should be quick to get to a
mergeable state.

This branch has not been deployed

No deployments
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.

2 participants