SYM-7916: continue past DDL that fails only because the object already exists - #995
SYM-7916: continue past DDL that fails only because the object already exists#995gp510 wants to merge 2 commits into
Conversation
…y exists Re-sending a table definition to a target where the load already created the objects raises "already exists" (SQL Server 1913, Oracle ORA-00955, MySQL duplicate key name) and fails the batch carrying it. Table definitions ride the same channel as change data, so one failed batch holds every batch queued behind it and all replication for that table stops -- while the only possible effect of the statement succeeding would have been to create something that was already there. Observed six times across five dates and three engines at one site. Recovery also required non-obvious operator knowledge: retrying re-issues the same DDL and fails identically, so the batch has to be ignored rather than have its error cleared, and nothing said so. Taking the loader-tolerance route rather than emitting guarded CREATE INDEX per platform. The deciding reason is that MsSqlDdlBuilder and OracleDdlBuilder are not in this repository at all -- symmetric-db/platform has ase, cassandra, db2, derby, firebird, greenplum, h2, hbase, hsqldb, informix, ingres, interbase, kafka, mysql, nuodb, postgresql, raima, redshift, sqlanywhere, sqlite and voltdb -- so guarded DDL here would fix PostgreSQL, which is not the platform failing, and leave SQL Server waiting on a separate change. MySQL also has no CREATE INDEX IF NOT EXISTS, Oracle would need a PL/SQL block that has to survive SqlScriptReader delimiter splitting, and a name-based guard silently skips an index that exists with a different definition, which is exactly the situation that triggers this. Turned out to need no new plumbing. doesObjectAlreadyExist is already on ISqlTemplate with objectAlreadyExistsCodes / objectAlreadyExistsStates behind it, and JdbcSqlTemplate already holds the settings object that ClientSymmetricEngine populates with the engine properties. So: - objectAlreadyExistsMessageParts adds cross-vendor message matching, which is what gives platforms outside this repository the fix with no per-platform change. Error codes stay per-platform, since the same number means different things on different vendors. - The DDL statement loop tolerates create/alter failures that classify as already-exists, alongside the existing isDrop / isSequenceCreate handling, logging the skipped statement and the driver message at WARN. - When tolerance is off and such a statement fails, the log now says the batch will fail identically on retry and should be ignored rather than cleared. - db.tolerate.object.already.exists.on.ddl, default true, is the revert switch. No interface signature changed and no emitted DDL changed, so the ~20 PRO platform subclasses of AbstractDdlBuilder that are invisible from this repository are unaffected, and both PostgreSqlDdlBuilderTest exact-string assertions still pass untouched. A full PRO compile is still required before merge. 11 new tests pinning the exact vendor wording, that unrelated failures are not swallowed, and the parameter. 1115 tests pass across symmetric-db, symmetric-jdbc, symmetric-io and symmetric-core with 0 failures. SQL Server 1913 and Oracle ORA-00955 message matching cannot be verified without those vendors; the message strings are what the OSS-only reach rests on, and server-localized messages defeat them, so PRO should follow up by populating the per-platform error-code arrays. Noted on the ticket. Worth recording separately: the deeper cause is a false-positive AddIndexChange. ModelComparator.findCorrespondingIndex matches on full IIndex.equals rather than name, while 1913 is a name collision, so the diff fires when the read-back model differs in definition from an index whose name already exists at the target. This change suppresses the symptom, not the model mismatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ameter doc Quality pass, no behaviour change. All 1117 tests across symmetric-jdbc, symmetric-db, symmetric-io and symmetric-core still pass. doesObjectAlreadyExist(ex) was evaluated twice on the same exception in adjacent branches, each re-walking the cause chain, and the rule for "is this an already-exists failure" was written out twice in expressions that have to change together. Hoisted to one local. The parameter was documented DatabaseOverridable: true, which is not true. It is read through SqlTemplateSettings, which is built once at startup from the properties file before sym_parameter is available, so setting it in the console would silently do nothing and it needs a restart. Its neighbours in that block that are read the same way declare no DatabaseOverridable at all; the comment now says so explicitly rather than claiming a capability the plumbing does not have. Test: dropped the TestTemplate subclass, since JdbcSqlTemplate is concrete and the test sits in its package, so the fields and the tolerance check were already reachable -- matching how HanaSqlJdbcSqlTemplateTest constructs one. Collapsed the five vendor-message cases and the three tolerance cases into parameterized tests so the vendor name is the case label. Added the GPL header, which this was the only file in the package to be missing, and moved off the JUnit 4 assertions it had mixed in with JUnit 5 annotations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
| # | ||
| # Tags: database | ||
| # Type: boolean | ||
| db.tolerate.object.already.exists.on.ddl=true |
There was a problem hiding this comment.
I wonder if this parameter is unnecessary IF the new code checks for the version of database, to ensure it supports syntax like " ... IF NOT EXISTS ..."
or a wrapper SQL block which prevents creating index with the same name.
There was a problem hiding this comment.
Taking the two you flagged separately, because I think you are right about one of them.
duplicate key name — I have direct evidence it is a false positive, not drift. PostgreSQL 16 hub to MySQL 8 spoke, ordinary three-table initial load with create-tables, no deliberate trigger. The control build failed exactly the way the customer reports:
WARN [JdbcSqlTemplate] Duplicate key name 'item_category_fk'.
Failed to execute: CREATE INDEX item_category_fk ON item (category_id)
ERROR [ManageIncomingBatchListener] Failed to load batch hub-139
sym_incoming_batch 139 went to ER, sql_code 1061, and stayed there through 188 identical retries with CDC head-of-line blocked behind it.
The mechanism is in our own reader, not in a drifted target. MySqlDdlReader.isInternalForeignKeyIndex returns true when the FK name equals the index name, and AbstractJdbcDdlReader then calls table.removeIndex(indexIdx). So MySQL backs the FK with that index and hides it from the model we just read back. The next create event diffs as "index missing" and emits DDL for something that is demonstrably present:
KEY item_category_fk (category_id),
CONSTRAINT item_category_fk FOREIGN KEY (category_id) REFERENCES category (category_id)
The target is not out of sync. The reader cannot see what is there. That is the case this entry is for.
duplicate column name — you are right and I should not have included it. I only ever exercised 1061. I never tested 1060, and a duplicate column is a much better candidate for genuine drift than a duplicate index name is. I will either drop it or come back with a test that shows a benign case. Dropping it is my inclination.
Worth adding on "painting over": the tolerated path is not silent. It logs at WARN with the failing statement, and when the parameter is off we now emit operator guidance naming the batch as permanently unretryable, which the current code does not do at all. So the change makes a currently-invisible failure visible in both directions.
| * matching, so a platform that depends on this should still populate {@link #objectAlreadyExistsCodes}. | ||
| */ | ||
| protected String[] objectAlreadyExistsMessageParts = { "already exists", "already an object named", | ||
| "is already used by an existing object", "duplicate key name", "duplicate column name" }; |
There was a problem hiding this comment.
It is a great move to consolidate messages, which indicate that the target object already exists! This allows us to add messages in languages other than English, too.
I am not yet convinced that these messages simply flag existance vs. out-of-sync target database which does need manually attention to get replication flowing successfully? (We do not want to paint over structural problems only to troubleshoot data import failures 1 batch later)
- duplicate key name
- duplicate column name
There was a problem hiding this comment.
Taking the two you flagged separately, because I think you are right about one of them.
duplicate key name — I have direct evidence it is a false positive, not drift. PostgreSQL 16 hub to MySQL 8 spoke, ordinary three-table initial load with create-tables, no deliberate trigger. The control build failed exactly the way the customer reports:
WARN [JdbcSqlTemplate] Duplicate key name 'item_category_fk'.
Failed to execute: CREATE INDEX item_category_fk ON item (category_id)
ERROR [ManageIncomingBatchListener] Failed to load batch hub-139
sym_incoming_batch 139 went to ER, sql_code 1061, and stayed there through 188 identical retries with CDC head-of-line blocked behind it.
The mechanism is in our own reader, not in a drifted target. MySqlDdlReader.isInternalForeignKeyIndex returns true when the FK name equals the index name, and AbstractJdbcDdlReader then calls table.removeIndex(indexIdx). So MySQL backs the FK with that index and hides it from the model we just read back. The next create event diffs as "index missing" and emits DDL for something that is demonstrably present:
KEY item_category_fk (category_id),
CONSTRAINT item_category_fk FOREIGN KEY (category_id) REFERENCES category (category_id)
The target is not out of sync. The reader cannot see what is there. That is the case this entry is for.
duplicate column name — you are right and I should not have included it. I only ever exercised 1061. I never tested 1060, and a duplicate column is a much better candidate for genuine drift than a duplicate index name is. I will either drop it or come back with a test that shows a benign case. Dropping it is my inclination.
Worth adding on "painting over": the tolerated path is not silent. It logs at WARN with the failing statement, and when the parameter is off we now emit operator guidance naming the batch as permanently unretryable, which the current code does not do at all. So the change makes a currently-invisible failure visible in both directions.
|
Pavel asked me to take a look at this and figure out the root cause. There were some recent changes in this area (SYM-7797), but they aren't related to the root cause. The root cause is a case-sensitivity issue. The index names in the source database are in uppercase and are written that way to the XML. When the XML is loaded in the target database, Then |
Thank you, Evan! Glen Noronha (@glen), |


Fixes SYM-7916.
The problem
Re-sending a table definition to a target where the load already created the objects raises "already exists" (SQL Server 1913, Oracle ORA-00955, MySQL duplicate key name) and fails the batch carrying it. Table definitions ride the same channel as change data, so one failed batch holds every batch queued behind it and all replication for that table stops — while the only possible effect of the statement succeeding would have been to create something that was already there.
Observed six times across five dates and three engines at one site. Recovery also required non-obvious operator knowledge: retrying re-issues the same DDL and fails identically, so the batch has to be ignored rather than have its error cleared, and nothing said so.
Why loader tolerance rather than guarded CREATE INDEX
The ticket listed both options. The deciding reason is concrete:
MsSqlDdlBuilderandOracleDdlBuilderare not in this repository at all.symmetric-db/platformhas ase, cassandra, db2, derby, firebird, greenplum, h2, hbase, hsqldb, informix, ingres, interbase, kafka, mysql, nuodb, postgresql, raima, redshift, sqlanywhere, sqlite, voltdb. No mssql, no oracle.The reported failure is SQL Server 1913, so guarded DDL here would fix PostgreSQL — a platform that is not failing — and leave the affected customer waiting on a separate change.
Supporting: MySQL has no
CREATE INDEX IF NOT EXISTSat all; Oracle needs a PL/SQL block that then has to surviveSqlScriptReaderdelimiter splitting; and a name-based guard silently skips an index that exists with a different definition, which is precisely the situation that triggers this — so option 1's masking risk is not actually smaller.It needed no new plumbing
doesObjectAlreadyExistis already onISqlTemplatewithobjectAlreadyExistsCodes/objectAlreadyExistsStatesbehind it, andJdbcSqlTemplatealready holds the settings object thatClientSymmetricEnginepopulates. So:objectAlreadyExistsMessagePartsadds cross-vendor message matching, which is what gives platforms outside this repository the fix with no per-platform change. Error codes stay per-platform, since the same number means different things on different vendors.isDrop/isSequenceCreatehandling, logging the skipped statement and the driver message at WARN.db.tolerate.object.already.exists.on.ddl, default true, is the revert switch.Blast radius
No interface signature changed and no emitted DDL changed. The ~20 PRO platform subclasses of
AbstractDdlBuilderthat are invisible from this repo are unaffected, and bothPostgreSqlDdlBuilderTestexact-string assertions pass untouched.A full PRO compile is still required before merge — the OSS build passing proves nothing about the other platforms.
1117 tests pass across
symmetric-jdbc,symmetric-db,symmetric-ioandsymmetric-core, 0 failures.Two things reviewers should weigh
Cannot be verified without the vendors. SQL Server 1913 and Oracle ORA-00955 message matching is what the OSS-only reach rests on, and server-localized messages defeat it. PRO should follow up by populating the per-platform error-code arrays; that is the real fix and this is the fallback. Worth adding both to the PRO integration matrix.
The parameter is not database-overridable, and the comment says so explicitly. It is read through
SqlTemplateSettings, built once at startup from the properties file beforesym_parameteris available, so it needs a restart. Its neighbours read the same way declare noDatabaseOverridableeither. If we want it console-settable, the flag belongs onDatabaseWriterSettingsthreaded throughSqlScriptas a peer offailOnDrops— a bigger change that would touchISqlTemplate/IDatabasePlatformsignatures, which is exactly the blast radius this approach avoids. Happy to go that way if reviewers prefer it.Worth recording
The deeper cause is a false-positive
AddIndexChange:ModelComparator.findCorrespondingIndexmatches on fullIIndex.equals, not name, while 1913 is a name collision. So the diff fires when the read-back model differs in definition from an index whose name already exists at the target. This change suppresses the symptom, not the model mismatch.🤖 Generated with Claude Code