Skip to content
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

[jdbc] Fix tableCaseSensitiveItemNames for PostgreSQL/TimescaleDB #17587

Merged
merged 12 commits into from
Oct 19, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.util.List;
import java.util.Objects;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.knowm.yank.Yank;
import org.knowm.yank.exceptions.YankSQLException;
import org.openhab.core.items.Item;
Expand All @@ -35,7 +37,8 @@
/**
* Extended Database Configuration class. Class represents
* the extended database-specific configuration. Overrides and supplements the
* default settings from JdbcBaseDAO. Enter only the differences to JdbcBaseDAO here.
* default settings from JdbcBaseDAO. Enter only the differences to JdbcBaseDAO
* here.
*
* @author Helmut Lehmeyer - Initial contribution
*/
Expand All @@ -58,48 +61,60 @@ public JdbcPostgresqlDAO() {

private void initSqlQueries() {
logger.debug("JDBC::initSqlQueries: '{}'", this.getClass().getSimpleName());
// System Information Functions: https://www.postgresql.org/docs/9.2/static/functions-info.html
// System Information Functions:
// https://www.postgresql.org/docs/9.2/static/functions-info.html
sqlGetDB = "SELECT CURRENT_DATABASE()";
sqlIfTableExists = "SELECT * FROM PG_TABLES WHERE TABLENAME='#searchTable#'";
sqlCreateItemsTableIfNot = "CREATE TABLE IF NOT EXISTS #itemsManageTable# (itemid SERIAL NOT NULL, #colname# #coltype# NOT NULL, CONSTRAINT #itemsManageTable#_pkey PRIMARY KEY (itemid))";
sqlCreateNewEntryInItemsTable = "INSERT INTO items (itemname) SELECT itemname FROM #itemsManageTable# UNION VALUES ('#itemname#') EXCEPT SELECT itemname FROM items";
sqlIfTableExists = "SELECT * FROM PG_TABLES WHERE TABLENAME='\"#searchTable#\"'";
sqlDropTable = "DROP TABLE \"#tableName#\"";
sqlCreateItemsTableIfNot = "CREATE TABLE IF NOT EXISTS \"#itemsManageTable#\" (itemid SERIAL NOT NULL, #colname# #coltype# NOT NULL, CONSTRAINT #itemsManageTable#_pkey PRIMARY KEY (itemid))";
sqlCreateNewEntryInItemsTable = "INSERT INTO items (itemname) SELECT itemname FROM \"#itemsManageTable#\" UNION VALUES ('#itemname#') EXCEPT SELECT itemname FROM items";
sqlGetItemTables = """
SELECT table_name FROM information_schema.tables WHERE table_type='BASE TABLE' AND table_schema=(SELECT table_schema \
FROM information_schema.tables WHERE table_type='BASE TABLE' AND table_name='#itemsManageTable#') AND NOT table_name='#itemsManageTable#'\
FROM information_schema.tables WHERE table_type='BASE TABLE' AND table_name='\"#itemsManageTable#\"') AND NOT table_name='\"#itemsManageTable#\"'\
""";
// The PostgreSQL equivalent to MySQL columns.column_type is data_type (e.g. "timestamp with time zone") and
// udt_name which contains a shorter alias (e.g. "timestamptz"). We alias data_type as "column_type" and
// udt_name as "column_type_alias" to be compatible with the 'Column' class used in Yank.queryBeanList
// The PostgreSQL equivalent to MySQL columns.column_type is data_type (e.g.
// "timestamp with time zone") and
// udt_name which contains a shorter alias (e.g. "timestamptz"). We alias
// data_type as "column_type" and
// udt_name as "column_type_alias" to be compatible with the 'Column' class used
// in Yank.queryBeanList
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to my other comment, this new line wrapping clearly does not improve formatting since not fully reformatted. Can you please revert all?

sqlGetTableColumnTypes = """
SELECT column_name, data_type as column_type, udt_name as column_type_alias, is_nullable FROM information_schema.columns \
WHERE table_name='#tableName#' AND table_catalog='#jdbcUriDatabaseName#' AND table_schema=(SELECT table_schema FROM information_schema.tables WHERE table_type='BASE TABLE' \
AND table_name='#itemsManageTable#')\
WHERE table_name='\"#tableName#\"' AND table_catalog='#jdbcUriDatabaseName#' AND table_schema=(SELECT table_schema FROM information_schema.tables WHERE table_type='BASE TABLE' \
AND table_name='\"#itemsManageTable#\"')\
""";
// NOTICE: on PostgreSql >= 9.5, sqlInsertItemValue query template is modified to do an "upsert" (overwrite
// existing value). The version check and query change is performed at initAfterFirstDbConnection()
sqlInsertItemValue = "INSERT INTO #tableName# (TIME, VALUE) VALUES( #tablePrimaryValue#, CAST( ? as #dbType#) )";
sqlAlterTableColumn = "ALTER TABLE #tableName# ALTER COLUMN #columnName# TYPE #columnType#";
// NOTICE: on PostgreSql >= 9.5, sqlInsertItemValue query template is modified
// to do an "upsert" (overwrite
// existing value). The version check and query change is performed at
// initAfterFirstDbConnection()
sqlInsertItemValue = "INSERT INTO \"#tableName#\" (TIME, VALUE) VALUES( #tablePrimaryValue#, CAST( ? as #dbType#) )";
sqlCreateItemTable = "CREATE TABLE IF NOT EXISTS \"#tableName#\" (time #tablePrimaryKey# NOT NULL, value #dbType#, PRIMARY KEY(time))";
sqlAlterTableColumn = "ALTER TABLE \"#tableName#\" ALTER COLUMN #columnName# TYPE #columnType#";
sqlGetRowCount = "SELECT COUNT(*) FROM \"#tableName#\"";
}

@Override
public void initAfterFirstDbConnection() {
logger.debug("JDBC::initAfterFirstDbConnection: Initializing step, after db is connected.");
DbMetaData dbMeta = new DbMetaData();
this.dbMeta = dbMeta;
// Perform "upsert" (on PostgreSql >= 9.5): Overwrite previous VALUE if same TIME (Primary Key) is provided
// This is the default at JdbcBaseDAO and is equivalent to MySQL: ON DUPLICATE KEY UPDATE VALUE
// Perform "upsert" (on PostgreSql >= 9.5): Overwrite previous VALUE if same
// TIME (Primary Key) is provided
// This is the default at JdbcBaseDAO and is equivalent to MySQL: ON DUPLICATE
// KEY UPDATE VALUE
// see: https://www.postgresql.org/docs/9.5/sql-insert.html
if (dbMeta.isDbVersionGreater(9, 4)) {
logger.debug("JDBC::initAfterFirstDbConnection: Values with the same time will be upserted (Pg >= 9.5)");
sqlInsertItemValue = """
INSERT INTO #tableName# (TIME, VALUE) VALUES( #tablePrimaryValue#, CAST( ? as #dbType#) )\
INSERT INTO \"#tableName#\" (TIME, VALUE) VALUES( #tablePrimaryValue#, CAST( ? as #dbType#) )\
ON CONFLICT (TIME) DO UPDATE SET VALUE=EXCLUDED.VALUE\
""";
}
}

/**
* INFO: http://www.java2s.com/Code/Java/Database-SQL-JDBC/StandardSQLDataTypeswithTheirJavaEquivalents.htm
* INFO:
* http://www.java2s.com/Code/Java/Database-SQL-JDBC/StandardSQLDataTypeswithTheirJavaEquivalents.htm
*/
private void initSqlTypes() {
// Initialize the type array
Expand Down Expand Up @@ -153,6 +168,18 @@ public ItemsVO doCreateItemsTableIfNot(ItemsVO vo) throws JdbcSQLException {
return vo;
}

@Override
public void doDropTable(String tableName) throws JdbcSQLException {
String sql = StringUtilsExt.replaceArrayMerge(this.sqlDropTable, new String[] { "#tableName#" },
new String[] { tableName });
logger.debug("JDBC::doDropTable sql={}", sql);
try {
Yank.execute(sql, null);
} catch (YankSQLException e) {
throw new JdbcSQLException(e);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this method doesn't differ from the base implementation, so there should be no reason to override it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method doesn't differ but the query it uses does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overridden query will still be used by the base implementation. See for example query sqlIfTableExists which is overridden in JdbcPostgresqlDAO (and other subclasses), but doIfTableExists is only implemented in JdbcBaseDAO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know that.

@Override
public Long doCreateNewEntryInItemsTable(ItemsVO vo) throws JdbcSQLException {
String sql = StringUtilsExt.replaceArrayMerge(sqlCreateNewEntryInItemsTable,
Expand Down Expand Up @@ -180,7 +207,8 @@ public List<ItemsVO> doGetItemTables(ItemsVO vo) throws JdbcSQLException {
}

/*
* Override because for PostgreSQL a different query is required with a 3rd argument (itemsManageTable)
* Override because for PostgreSQL a different query is required with a 3rd
* argument (itemsManageTable)
*/
@Override
public List<Column> doGetTableColumns(ItemsVO vo) throws JdbcSQLException {
Expand All @@ -200,8 +228,23 @@ public List<Column> doGetTableColumns(ItemsVO vo) throws JdbcSQLException {
*************/

/*
* Override since PostgreSQL does not support setting NOT NULL in the same clause as ALTER COLUMN .. TYPE
* Override since PostgreSQL does not support setting NOT NULL in the same
* clause as ALTER COLUMN .. TYPE
*/

jlaur marked this conversation as resolved.
Show resolved Hide resolved
@Override
public void doCreateItemTable(ItemVO vo) throws JdbcSQLException {
String sql = StringUtilsExt.replaceArrayMerge(this.sqlCreateItemTable,
new String[] { "#tableName#", "#dbType#", "#tablePrimaryKey#" },
new String[] { vo.getTableName(), vo.getDbType(), sqlTypes.get("tablePrimaryKey") });
logger.debug("JDBC::doCreateItemTable sql={}", sql);
try {
Yank.execute(sql, null);
} catch (YankSQLException e) {
throw new JdbcSQLException(e);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this method doesn't differ from the base implementation, so there should be no reason to override it?

The comment above also refers to doAlterTableColumn, which is now below this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same case here, I added the override because the query it uses needed to be changed.

@Override
public void doAlterTableColumn(String tableName, String columnName, String columnType, boolean nullable)
throws JdbcSQLException {
Expand All @@ -213,7 +256,7 @@ public void doAlterTableColumn(String tableName, String columnName, String colum
Yank.execute(sql, null);
if (!nullable) {
String sql2 = StringUtilsExt.replaceArrayMerge(
"ALTER TABLE #tableName# ALTER COLUMN #columnName# SET NOT NULL",
"ALTER TABLE \"#tableName#\" ALTER COLUMN #columnName# SET NOT NULL",
new String[] { "#tableName#", "#columnName#" }, new String[] { tableName, columnName });
logger.info("JDBC::doAlterTableColumn sql={}", sql2);
Yank.execute(sql2, null);
Expand All @@ -226,7 +269,7 @@ public void doAlterTableColumn(String tableName, String columnName, String colum
@Override
public void doStoreItemValue(Item item, State itemState, ItemVO vo) throws JdbcSQLException {
ItemVO storedVO = storeItemValueProvider(item, itemState, vo);
String sql = StringUtilsExt.replaceArrayMerge(sqlInsertItemValue,
String sql = StringUtilsExt.replaceArrayMerge(this.sqlInsertItemValue,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert:

Suggested change
String sql = StringUtilsExt.replaceArrayMerge(this.sqlInsertItemValue,
String sql = StringUtilsExt.replaceArrayMerge(sqlInsertItemValue,

We generally only use this. prefixes when necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the this. because otherwise it uses the default implementation query.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be implicitly considered as this.sqlInsertItemValue. Otherwise you would have to reference it by super.sqlInsertItemValue.

new String[] { "#tableName#", "#dbType#", "#tablePrimaryValue#" },
new String[] { storedVO.getTableName(), storedVO.getDbType(), sqlTypes.get("tablePrimaryValue") });
Object[] params = { storedVO.getValue() };
Expand All @@ -241,7 +284,7 @@ public void doStoreItemValue(Item item, State itemState, ItemVO vo) throws JdbcS
@Override
public void doStoreItemValue(Item item, State itemState, ItemVO vo, ZonedDateTime date) throws JdbcSQLException {
ItemVO storedVO = storeItemValueProvider(item, itemState, vo);
String sql = StringUtilsExt.replaceArrayMerge(sqlInsertItemValue,
String sql = StringUtilsExt.replaceArrayMerge(this.sqlInsertItemValue,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
String sql = StringUtilsExt.replaceArrayMerge(this.sqlInsertItemValue,
String sql = StringUtilsExt.replaceArrayMerge(sqlInsertItemValue,

new String[] { "#tableName#", "#dbType#", "#tablePrimaryValue#" },
new String[] { storedVO.getTableName(), storedVO.getDbType(), "?" });
java.sql.Timestamp timestamp = new java.sql.Timestamp(date.toInstant().toEpochMilli());
Expand All @@ -254,6 +297,19 @@ public void doStoreItemValue(Item item, State itemState, ItemVO vo, ZonedDateTim
}
}

@Override
public long doGetRowCount(String tableName) throws JdbcSQLException {
final String sql = StringUtilsExt.replaceArrayMerge(this.sqlGetRowCount, new String[] { "#tableName#" },
new String[] { tableName });
logger.debug("JDBC::doGetRowCount sql={}", sql);
try {
final @Nullable Long result = Yank.queryScalar(sql, Long.class, null);
return Objects.requireNonNullElse(result, 0L);
} catch (YankSQLException e) {
throw new JdbcSQLException(e);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this method doesn't differ from the base implementation, so there should be no reason to override it?

/****************************
* SQL generation Providers *
****************************/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,22 @@
*/
package org.openhab.persistence.jdbc.internal.db;

import java.util.List;
import java.util.Properties;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.knowm.yank.Yank;
import org.knowm.yank.exceptions.YankSQLException;
import org.openhab.persistence.jdbc.internal.dto.ItemVO;
import org.openhab.persistence.jdbc.internal.dto.ItemsVO;
import org.openhab.persistence.jdbc.internal.exceptions.JdbcSQLException;
import org.openhab.persistence.jdbc.internal.utils.StringUtilsExt;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Extended Database Configuration class. Class represents the extended database-specific configuration. Overrides and
* Extended Database Configuration class. Class represents the extended
* database-specific configuration. Overrides and
* supplements the default settings from JdbcBaseDAO and JdbcPostgresqlDAO.
*
* @author Riccardo Nimser-Joseph - Initial contribution
Expand All @@ -34,18 +37,24 @@
public class JdbcTimescaledbDAO extends JdbcPostgresqlDAO {
private final Logger logger = LoggerFactory.getLogger(JdbcTimescaledbDAO.class);

private final String sqlCreateHypertable = "SELECT created from create_hypertable('#tableName#', 'time')";
private final String sqlCreateHypertable = "SELECT created FROM create_hypertable('\"#tableName#\"', 'time')";
private final String sqlGetItemTables = "SELECT hypertable_name as table_name FROM timescaledb_information.hypertables WHERE hypertable_name != '\"#itemsManageTable#\"'";

@Override
public Properties getConnectionProperties() {
Properties properties = (Properties) this.databaseProps.clone();
// Adjust the jdbc url since the service name 'timescaledb' is only used to differentiate the DAOs
// Adjust the jdbc url since the service name 'timescaledb' is only used to
// differentiate the DAOs
if (properties.containsKey("jdbcUrl")) {
properties.put("jdbcUrl", properties.getProperty("jdbcUrl").replace("jdbc:timescaledb", "jdbc:postgresql"));
}
return properties;
}

/*************
* ITEM DAOs *
*************/

@Override
public void doCreateItemTable(ItemVO vo) throws JdbcSQLException {
super.doCreateItemTable(vo);
Expand All @@ -58,4 +67,16 @@ public void doCreateItemTable(ItemVO vo) throws JdbcSQLException {
throw new JdbcSQLException(e);
}
}

@Override
public List<ItemsVO> doGetItemTables(ItemsVO vo) throws JdbcSQLException {
String sql = StringUtilsExt.replaceArrayMerge(this.sqlGetItemTables, new String[] { "#itemsManageTable#" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
String sql = StringUtilsExt.replaceArrayMerge(this.sqlGetItemTables, new String[] { "#itemsManageTable#" },
String sql = StringUtilsExt.replaceArrayMerge(sqlGetItemTables, new String[] { "#itemsManageTable#" },

new String[] { vo.getItemsManageTable() });
this.logger.debug("JDBC::doGetItemTables sql={}", sql);
try {
return Yank.queryBeanList(sql, ItemsVO.class, null);
} catch (YankSQLException e) {
throw new JdbcSQLException(e);
}
}
}