From 110bc614e450cd59ff305a0425e44cf68d2baf56 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Thu, 7 Jul 2022 13:45:06 +1200 Subject: [PATCH] #2743 - Postgres DDL - For create table, improve column ordering for tighter storage --- .../ddlgeneration/platform/PlatformDdl.java | 6 ++ .../ddlgeneration/platform/PostgresDdl.java | 56 +++++++++++++++++++ .../ddlgeneration/platform/YugabyteDdl.java | 10 +++- .../platform/PostgresDdlTest.java | 48 +++++++++++++++- .../ModelBuild_explicitSequencesTest.java | 6 +- .../pg-apply.sql | 2 +- .../dbmigration/postgres/1.0__initial.sql | 12 ++-- .../postgres/idx_postgres.migrations | 2 +- .../dbmigration/postgres9/1.0__initial.sql | 12 ++-- .../postgres9/idx_postgres.migrations | 2 +- 10 files changed, 134 insertions(+), 22 deletions(-) diff --git a/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PlatformDdl.java b/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PlatformDdl.java index f18b3c79e..239a3b90e 100644 --- a/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PlatformDdl.java +++ b/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PlatformDdl.java @@ -198,6 +198,7 @@ public class PlatformDdl { * Write all the table columns converting to platform types as necessary. */ public void writeTableColumns(DdlBuffer apply, List columns, DdlIdentity identity) { + columns = sortColumns(columns); for (int i = 0; i < columns.size(); i++) { if (i > 0) { apply.append(","); @@ -218,6 +219,11 @@ public class PlatformDdl { } } + protected List sortColumns(List columns) { + // do nothing by default + return columns; + } + /** * Write the column definition to the create table statement. */ diff --git a/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdl.java b/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdl.java index 3054a73d9..e0d58c53a 100644 --- a/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdl.java +++ b/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdl.java @@ -4,6 +4,14 @@ import io.ebean.config.dbplatform.DatabasePlatform; import io.ebeaninternal.dbmigration.ddlgeneration.DdlBuffer; import io.ebeaninternal.dbmigration.ddlgeneration.DdlWrite; import io.ebeaninternal.dbmigration.migration.AlterColumn; +import io.ebeaninternal.dbmigration.migration.Column; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; + +import static java.util.stream.Collectors.toList; /** * Postgres specific DDL. @@ -70,4 +78,52 @@ public class PostgresDdl extends PlatformDdl { .append(columnSetType).append(type) .append(" using ").append(alter.getColumnName()).append("::").append(type); } + + @Override + protected List sortColumns(List columns) { + List sorting = new ArrayList<>(columns.size()); + for (int i = 0, end = columns.size(); i < end; i++) { + Column column = columns.get(i); + sorting.add(new DDLColumnSort(column, ddlColumnOrdering(i, column))); + } + Collections.sort(sorting); + return sorting.stream().map(it -> it.column).collect(toList()); + } + + private int ddlColumnOrdering(int i, Column column) { + String type = column.getType().toLowerCase(); + if (type.startsWith("decimal")) { + return i + 1_000; + } + if (isVariableLength(type) || isLob(type)) { + return i + 10_000; + } + return i; + } + + private boolean isLob(String type) { + return type.startsWith("clob") || type.startsWith("longvarchar") || type.startsWith("blob") || type.startsWith("longvarbinary"); + } + + private boolean isVariableLength(String type) { + return type.startsWith("varchar") || type.startsWith("varbinary"); + } + + static final class DDLColumnSort implements Comparable { + + private final Column column; + private final int ordering; + + DDLColumnSort(Column column, int ordering) { + this.column = column; + this.ordering = ordering; + } + + @Override + public int compareTo(DDLColumnSort o) { + return Integer.compare(ordering, o.ordering); + } + } + + } diff --git a/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/YugabyteDdl.java b/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/YugabyteDdl.java index a78e5b339..479439eb2 100644 --- a/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/YugabyteDdl.java +++ b/ebean-ddl-generator/src/main/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/YugabyteDdl.java @@ -1,11 +1,19 @@ package io.ebeaninternal.dbmigration.ddlgeneration.platform; import io.ebean.config.dbplatform.DatabasePlatform; +import io.ebeaninternal.dbmigration.migration.Column; -public class YugabyteDdl extends PostgresDdl { +import java.util.List; + +public final class YugabyteDdl extends PostgresDdl { public YugabyteDdl(DatabasePlatform platform) { super(platform); this.historyDdl = new YugabyteHistoryDdl(); } + + @Override + protected List sortColumns(List columns) { + return columns; + } } diff --git a/ebean-ddl-generator/src/test/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdlTest.java b/ebean-ddl-generator/src/test/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdlTest.java index b8018cc3d..3a16a70f8 100644 --- a/ebean-ddl-generator/src/test/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdlTest.java +++ b/ebean-ddl-generator/src/test/java/io/ebeaninternal/dbmigration/ddlgeneration/platform/PostgresDdlTest.java @@ -1,18 +1,60 @@ package io.ebeaninternal.dbmigration.ddlgeneration.platform; import io.ebean.platform.postgres.PostgresPlatform; +import io.ebeaninternal.dbmigration.migration.Column; import org.junit.jupiter.api.Test; +import java.util.ArrayList; +import java.util.List; + import static org.assertj.core.api.Assertions.assertThat; -public class PostgresDdlTest { +class PostgresDdlTest { private PostgresDdl postgresDdl = new PostgresDdl(new PostgresPlatform()); @Test - public void setLockTimeout() { - + void setLockTimeout() { final String sql = postgresDdl.setLockTimeout(5); assertThat(sql).isEqualTo("set lock_timeout = 5000"); } + + @Test + void sortColumns_noAdjustment() { + List cols = postgresDdl.sortColumns(columns("integer", "bigint")); + assertThat(cols).extracting("type").containsExactly("integer", "bigint"); + + List colsReverse = postgresDdl.sortColumns(columns("bigint", "integer")); + assertThat(colsReverse).extracting("type").containsExactly("bigint", "integer"); + } + + @Test + void sortColumns_decimalVarchar() { + List cols = postgresDdl.sortColumns(columns("decimal(1)", "varchar(1)", "varchar(2)", "int", "decimal(2)")); + assertThat(cols).extracting("type").containsExactly("int", "decimal(1)", "decimal(2)", "varchar(1)", "varchar(2)"); + } + + @Test + void sortColumns_varbinary() { + List cols = postgresDdl.sortColumns(columns("decimal(1)", "varbinary(1)", "varbinary(2)", "int", "decimal(2)")); + assertThat(cols).extracting("type").containsExactly("int", "decimal(1)", "decimal(2)", "varbinary(1)", "varbinary(2)"); + } + + @Test + void sortColumns_clobs() { + List cols = postgresDdl.sortColumns(columns("clob", "blob", "longvarchar(1)", "int", "longvarbinary(2)")); + assertThat(cols).extracting("type").containsExactly("int", "clob", "blob", "longvarchar(1)", "longvarbinary(2)"); + } + + private List columns(String... types) { + int counter = 0; + List cols = new ArrayList<>(); + for (String s : types) { + Column col = new Column(); + col.setName("c" + counter++); + col.setType(s); + cols.add(col); + } + return cols; + } } diff --git a/ebean-ddl-generator/src/test/java/io/ebeaninternal/dbmigration/model/build/ModelBuild_explicitSequencesTest.java b/ebean-ddl-generator/src/test/java/io/ebeaninternal/dbmigration/model/build/ModelBuild_explicitSequencesTest.java index f25567956..8a678aae3 100644 --- a/ebean-ddl-generator/src/test/java/io/ebeaninternal/dbmigration/model/build/ModelBuild_explicitSequencesTest.java +++ b/ebean-ddl-generator/src/test/java/io/ebeaninternal/dbmigration/model/build/ModelBuild_explicitSequencesTest.java @@ -16,7 +16,7 @@ import java.io.IOException; import static org.assertj.core.api.Assertions.assertThat; -public class ModelBuild_explicitSequencesTest extends BaseTestCase { +class ModelBuild_explicitSequencesTest extends BaseTestCase { private SpiEbeanServer createServer(boolean postgres) { @@ -38,7 +38,7 @@ public class ModelBuild_explicitSequencesTest extends BaseTestCase { } @Test - public void test() throws IOException { + void test() throws IOException { SpiEbeanServer ebeanServer = createServer(false); try { CurrentModel currentModel = new CurrentModel(ebeanServer); @@ -54,7 +54,7 @@ public class ModelBuild_explicitSequencesTest extends BaseTestCase { } @Test - public void test_asPostgres() throws IOException { + void test_asPostgres() throws IOException { SpiEbeanServer ebeanServer = createServer(true); try { CurrentModel currentModel = new CurrentModel(ebeanServer); diff --git a/ebean-ddl-generator/src/test/resources/assert/ModelBuild_explicitSequencesTest/pg-apply.sql b/ebean-ddl-generator/src/test/resources/assert/ModelBuild_explicitSequencesTest/pg-apply.sql index 5978992d3..0f4382365 100644 --- a/ebean-ddl-generator/src/test/resources/assert/ModelBuild_explicitSequencesTest/pg-apply.sql +++ b/ebean-ddl-generator/src/test/resources/assert/ModelBuild_explicitSequencesTest/pg-apply.sql @@ -7,8 +7,8 @@ create table PERSONS ( create table PHONES ( id bigint generated by default as identity not null, - phone_number varchar(7) not null, person_id bigint not null, + phone_number varchar(7) not null, constraint uq_phones_phone_number unique (phone_number), constraint pk_phones primary key (id) ); diff --git a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/1.0__initial.sql b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/1.0__initial.sql index 25f82c76d..1fc2b0f70 100644 --- a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/1.0__initial.sql +++ b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/1.0__initial.sql @@ -14,9 +14,9 @@ create table migtest_ckey_detail ( create table migtest_ckey_parent ( one_key integer not null, + version integer not null, two_key varchar(127) not null, name varchar(255), - version integer not null, constraint pk_migtest_ckey_parent primary key (one_key,two_key) ); @@ -56,10 +56,6 @@ create table migtest_fk_set_null ( create table migtest_e_basic ( id integer generated by default as identity not null, - status varchar(1), - status2 varchar(1) default 'N' not null, - name varchar(127), - description varchar(127), description_file bytea, json_list json, a_lob varchar(255) default 'X' not null, @@ -67,13 +63,17 @@ create table migtest_e_basic ( old_boolean boolean default false not null, old_boolean2 boolean, eref_id integer, + user_id integer not null, + status varchar(1), + status2 varchar(1) default 'N' not null, + name varchar(127), + description varchar(127), indextest1 varchar(127), indextest2 varchar(127), indextest3 varchar(127), indextest4 varchar(127), indextest5 varchar(127), indextest6 varchar(127), - user_id integer not null, constraint ck_migtest_e_basic_status check ( status in ('N','A','I')), constraint ck_migtest_e_basic_status2 check ( status2 in ('N','A','I')), constraint uq_migtest_e_basic_indextest2 unique (indextest2), diff --git a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/idx_postgres.migrations b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/idx_postgres.migrations index 7f9a10350..4639f4b3b 100644 --- a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/idx_postgres.migrations +++ b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres/idx_postgres.migrations @@ -1,4 +1,4 @@ -1014378814, 1.0__initial.sql +2094204208, 1.0__initial.sql -2047324426, 1.1.sql -261111052, 1.2__dropsFor_1.1.sql 1335621453, 1.3.sql diff --git a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/1.0__initial.sql b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/1.0__initial.sql index e319a8737..a9869f95b 100644 --- a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/1.0__initial.sql +++ b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/1.0__initial.sql @@ -14,9 +14,9 @@ create table migtest_ckey_detail ( create table migtest_ckey_parent ( one_key integer not null, + version integer not null, two_key varchar(127) not null, name varchar(255), - version integer not null, constraint pk_migtest_ckey_parent primary key (one_key,two_key) ); @@ -56,10 +56,6 @@ create table migtest_fk_set_null ( create table migtest_e_basic ( id serial not null, - status varchar(1), - status2 varchar(1) default 'N' not null, - name varchar(127), - description varchar(127), description_file bytea, json_list json, a_lob varchar(255) default 'X' not null, @@ -67,13 +63,17 @@ create table migtest_e_basic ( old_boolean boolean default false not null, old_boolean2 boolean, eref_id integer, + user_id integer not null, + status varchar(1), + status2 varchar(1) default 'N' not null, + name varchar(127), + description varchar(127), indextest1 varchar(127), indextest2 varchar(127), indextest3 varchar(127), indextest4 varchar(127), indextest5 varchar(127), indextest6 varchar(127), - user_id integer not null, constraint ck_migtest_e_basic_status check ( status in ('N','A','I')), constraint ck_migtest_e_basic_status2 check ( status2 in ('N','A','I')), constraint uq_migtest_e_basic_indextest2 unique (indextest2), diff --git a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/idx_postgres.migrations b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/idx_postgres.migrations index 3ed7fcf1d..fd06b52c1 100644 --- a/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/idx_postgres.migrations +++ b/ebean-test/src/test/resources/migrationtest/dbmigration/postgres9/idx_postgres.migrations @@ -1,4 +1,4 @@ -382349675, 1.0__initial.sql +-231429368, 1.0__initial.sql -606251140, 1.1.sql -261111052, 1.2__dropsFor_1.1.sql -893728811, 1.3.sql