Skip to content

Conversation

@andreitokar
Copy link
Contributor

Fixes #4021

public DbException getDuplicateKeyException(String key) {
StringBuilder builder = new StringBuilder();
getSQL(builder, TRACE_SQL_FLAGS).append(" ON ");
getIndexOrConstraintSQL(builder).append(" ON ");
Copy link
Contributor

@katzyn katzyn Dec 29, 2024

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);

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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() ?

Copy link
Contributor

@katzyn katzyn Dec 30, 2024

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.

Copy link
Contributor

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)

Copy link
Contributor Author

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) {
Copy link
Contributor

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)");
Copy link
Contributor

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).

@andreitokar andreitokar merged commit a5e6662 into h2database:master Dec 31, 2024
2 checks passed
@andreitokar andreitokar deleted the issue-4021 branch January 4, 2025 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

h2 does not correctly report the name of a violated unique constraint

2 participants