-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Report constraint name instead of index name in duplicate key exception #4176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
h2/src/main/org/h2/index/Index.java
Outdated
| public DbException getDuplicateKeyException(String key) { | ||
| StringBuilder builder = new StringBuilder(); | ||
| getSQL(builder, TRACE_SQL_FLAGS).append(" ON "); | ||
| getIndexOrConstraintSQL(builder).append(" ON "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Values of multi-column constraints can be listed out of order here.
For simplicity and for better compatibility with code that relies on old-style messages both constraint and index name can be specified:
if (…) {
for (…)
if (…) {
constraint.getSQL(builder, TRACE_SQL_FLAGS).append(" INDEX ");
break;
}
}
}
…
getSQL(builder, TRACE_SQL_FLAGS).append(" ON ");getDuplicatePrimaryKeyMessage() also needs to be updated. Something like
StringBuilder builder = new StringBuilder(64);
ArrayList<Constraint> constraints = table.getConstraints();
if (constraints != null) {
for (Constraint constraint : constraints) {
if (constraint.getConstraintType() == Type.PRIMARY_KEY) {
constraint.getSQL(builder, TRACE_SQL_FLAGS).append(' ');
break;
}
}
}
table.getSQL(builder.append("PRIMARY KEY ON "), TRACE_SQL_FLAGS);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, although I doubt anybody ever care about name of the primary key constraint.
BTW, this @nullable getConstraints()
ArrayList constraints = table.getConstraints();
if (constraints != null) {... ,
gets quite annoying/hard-to-read/error-prone and we have about 16 instances of it.
Would you mind replacing it with @NotNull version returning an empty list, or I am missing something?
It's all DDL statements which are rarely are on a critical path for performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return constraints != null ? constraints : List.of() should be harmless, because this method isn't used in performance-critical cases, but H2 uses this logic with null in other methods too and deviation between implementations of similar methods will be surprising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of deviations, you mean getTriggers(), sequences, sysnonyms, and getIndexes() ?
They are already implemented with slight differences, and differently from non-nullable dependentViews and dependentMaterializedViews.
I'll be happy to change them all to use gettter() with empty Iterable instead of null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All existing optimizations were actual for really old versions of Java.
These days constraints can be initialized to new ArrayList<>() instead of null, this constructor since Java 8 (and 7u40) doesn't allocate memory for underlying array and it is cheap enough.
Extra (10-4)*4=24 bytes of array memory (on 32-bit VMs and 64-bit VMs with compressed pointers) if number of elements is between 1 and 4 (Utils.newSmallArrayList() vs default behavior) is also not a big deal.
Maybe Utils.newSmallArrayList() will be better for constraints, because regular tables usually have at least PK constraint and sometimes few others, but I think various temporary tables usually don't have constraints at all.
For sequences with their typical number of elements of 0 or 1, new ArrayList<>(0) can be used instead, this constructor also doesn't allocate array memory for an empty list if 0 was passed to it, but it allocates a single-element array during addition of the first element instead of array with 10 elements.
I'm not sure about triggers, probably new ArrayList<>(0) is the best choice too.
| ArrayList<Constraint> constraints = table.getConstraints(); | ||
| if (constraints != null) { | ||
| for (Constraint constraint : table.getConstraints()) { | ||
| if (constraint.usesIndex(this) && constraint.getConstraintType() == Constraint.Type.UNIQUE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type.PRIMARY_KEY must be considered as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't primary key handled by getDuplicatePrimaryKeyMessage(), not getDuplicateKeyException() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Primary key constraints are backed by delegate or secondary index. getDuplicatePrimaryKeyMessage() is only used by the delegate index, the secondary index uses getDuplicateKeyException().
The delegate index is only used for single-column primary key constraints with TINYINT, SMALLINT, INTEGER, or BIGINT data types if PK constraint was specified directly in table definition. In all other cases MVSecordaryIndex is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CREATE TABLE TEST(ID UUID CONSTRAINT TEST_PK PRIMARY KEY);
INSERT INTO TEST VALUES '65b05eac-c855-4413-963c-701dc35a744f', '65b05eac-c855-4413-963c-701dc35a744f';Unique index or primary key violation: "PUBLIC.PRIMARY_KEY_2 ON PUBLIC.TEST(ID) VALUES ( /* 1 */ UUID '65b05eac-c855-4413-963c-701dc35a744f' )"
at org.h2.message.DbException.getJdbcSQLException(DbException.java:520)
at org.h2.message.DbException.getJdbcSQLException(DbException.java:489)
at org.h2.message.DbException.get(DbException.java:223)
at org.h2.message.DbException.get(DbException.java:199)
at org.h2.index.Index.getDuplicateKeyException(Index.java:525)
at org.h2.mvstore.db.MVSecondaryIndex.checkUnique(MVSecondaryIndex.java:223)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You right, I forgot that, thanks.
| return e; | ||
| } | ||
|
|
||
| private StringBuilder getIndexOrConstraintSQL(StringBuilder builder) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not used any more.
| stat.execute("create table test(id int, name int, " + | ||
| "constraint abc unique(name, id))"); | ||
| testErrorMessage("ABC_INDEX_2 ON PUBLIC.TEST(NAME NULLS FIRST, ID NULLS FIRST)"); | ||
| testErrorMessage("ABC INDEX PUBLIC.ABC_INDEX_2 ON PUBLIC.TEST(NAME NULLS FIRST, ID NULLS FIRST)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PUBLIC.CONSTRAINT_2 INDEX and PUBLIC.ABC (to make sure that names of constraints are qualified).
Fixes #4021