mfvanek / pg-index-health

pg-index-health is a Java library for analyzing and maintaining indexes and tables health in Postgresql databases.
Apache License 2.0
128 stars 15 forks source link

add code for check_not_valid_constraints.sql #374

Closed BLoHny closed 6 months ago

BLoHny commented 6 months ago

Relates to https://github.com/mfvanek/pg-index-health/issues/362

BLoHny commented 6 months ago

The current test code doesn't seem to be good code. Could you help me?

mfvanek commented 6 months ago

The current test code doesn't seem to be good code. Could you help me?

  1. You can copy and modify existing class https://github.com/mfvanek/pg-index-health/blob/master/pg-index-health/src/test/java/io/github/mfvanek/pg/support/statements/AddLinksBetweenAccountsAndClientsStatement.java in order to bring not valid FK.
  2. You can add one more column to https://github.com/mfvanek/pg-index-health/blob/master/pg-index-health/src/test/java/io/github/mfvanek/pg/support/statements/CreateTableWithCheckConstraintOnSerialPrimaryKey.java and not valid check constraint on it
BLoHny commented 6 months ago

The current test code doesn't seem to be good code. Could you help me?

  1. You can copy and modify existing class https://github.com/mfvanek/pg-index-health/blob/master/pg-index-health/src/test/java/io/github/mfvanek/pg/support/statements/AddLinksBetweenAccountsAndClientsStatement.java in order to bring not valid FK.
  2. You can add one more column to https://github.com/mfvanek/pg-index-health/blob/master/pg-index-health/src/test/java/io/github/mfvanek/pg/support/statements/CreateTableWithCheckConstraintOnSerialPrimaryKey.java and not valid check constraint on it
// AddInvalidForeignKeyStatement.java
    @Override
    public void execute(@Nonnull final Statement statement) throws SQLException {
        statement.execute(String.format("alter table if exists %1$s.accounts " +
                        "add constraint c_accounts_fk_invalid foreign key (invalid_client_id) references %1$s.invalid_clients (invalid_id);",
                schemaName));
    }
// CreateTableWithCheckConstraintOnSerialPrimaryKey.java
    @Override
    public void execute(@Nonnull final Statement statement) throws SQLException {
        statement.execute(String.format("create table if not exists %1$s.another_table(" +
                        "id bigserial primary key, " +
                        "invalid_column integer, " +
                        "constraint not_reserved_id check (id > 1000), " +
                        "constraint less_than_million check (id < 1000000), " +
                        "constraint always_false check (invalid_column > 100 AND invalid_column < 0));",
                schemaName));
    }

Is this modified code suitable?

mfvanek commented 6 months ago

Is this modified code suitable?

Unfortunately, no, it isn't.

When I say "not valid constraint" it's not about true or false constraint is, it's about whether database uses this constraint or not. Not valid constraint is like a draft. Please take a look at the original article https://habr.com/ru/articles/800121/

It should be like this

alter table if exists %1$s.accounts add constraint c_accounts_fk_client_id_not_validated_yet foreign key (client_id) references %1$s.clients (id) not valid;

In tests after you find all not valid constraints you can can call validate constraint and recheck the database: all issues should go away

BLoHny commented 6 months ago
    @ParameterizedTest
    @ValueSource(strings = {PgContext.DEFAULT_SCHEMA_NAME, "custom"})
    void onDatabaseWithNotValidatedForeignKey(final String schemaName) {
        executeTestOnDatabase(schemaName, DatabasePopulator::withInvalidForeignKey, ctx ->
                assertThat(check)
                        .executing(ctx)
                        .hasSize(1)
                        .containsExactly(
                                Constraint.of(ctx.enrichWithSchema("accounts"), "c_accounts_fk_client_id_not_validated_yet",
                                        ConstraintType.FOREIGN_KEY)
                        )
        );
    }

Is it okay to proceed with the test code like this?

mfvanek commented 6 months ago

Hi @BLoHny

Is it okay to proceed with the test code like this?

I'd suggest a few enhancements:

  1. Rename withInvalidForeignKey to withNotValidConstraints
  2. Add not valid FK and not valid check constraint
  3. Add dbp -> dbp.withNotValidConstraints().withUniqueConstraintOnSerialColumn() to ensure that sql query ignores unique constraints and PK's
BLoHny commented 6 months ago
        statement.execute(String.format("alter table if exists %1$s.accounts " +
                        "add constraint c_accounts_chk_client_id_not_validated_yet check (client_id > 0) not valid;",
                schemaName));

After add more sql query..

                        .containsExactly(
                                Constraint.of(ctx.enrichWithSchema("accounts"), "c_accounts_chk_client_id_not_validated_yet",
                                        ConstraintType.CHECK),
                                Constraint.of(ctx.enrichWithSchema("accounts"), "c_accounts_fk_client_id_not_validated_yet",
                                        ConstraintType.FOREIGN_KEY)
                        )

add test code

proceed like this?

mfvanek commented 6 months ago

proceed like this?

Yes, looks good

BLoHny commented 6 months ago

open new pull request https://github.com/mfvanek/pg-index-health/pull/382 by conflict