[jdbc] Fix `tableCaseSensitiveItemNames` for PostgreSQL/TimescaleDB #17587

JonathanvdGHU · 2024-10-18T15:08:09Z

Case-sensitive tables working for TimescaleDB database via JDBC persistence service

Description

The following issue is resolved. My implementation is maybe not the best one because I didn't take the time to understand everything. So I don't mind if someone makes some small adjustments and makes a new pull request :)

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

jlaur · 2024-10-18T15:47:11Z

Thanks for the fix. I'll review this in the weekend.

JonathanvdGHU · 2024-10-18T15:51:27Z

Thanks for the fix. I'll review this in the weekend.

Good to hear. Is it a big problem I made a mess with the commits? I am trying to do a rebase but it's not really working.

jlaur · 2024-10-18T15:58:02Z

Good to hear. Is it a big problem I made a mess with the commits?

It seems some commits are incorrectly signed. If there is at least one correctly signed commit, I can use that when squash-merging. Otherwise you will need to rebase to fix it.

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

JonathanvdGHU · 2024-10-18T16:56:24Z

Good to hear. Is it a big problem I made a mess with the commits?

It seems some commits are incorrectly signed. If there is at least one correctly signed commit, I can use that when squash-merging. Otherwise you will need to rebase to fix it.

I fixed it. This was my first rebase so I am also a little proud about myself xD

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

jlaur

The fix looks valid, but I have provided some feedback on the approach. I think it would be preferable to keep the DTO's unchanged and instead directly fix the queries which only concern PostgreSQL/TimescaleDB.

...rsistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcPostgresqlDAO.java

...sistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcTimescaledbDAO.java

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

JonathanvdGHU · 2024-10-19T14:30:30Z

The fix looks valid, but I have provided some feedback on the approach. I think it would be preferable to keep the DTO's unchanged and instead directly fix the queries which only concern PostgreSQL/TimescaleDB.

I implemented your requested changes :)

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

jlaur · 2024-10-19T14:58:58Z

I implemented your requested changes :)

Thanks! I'll take one more look. In the meantime, I see there are formatting changes for comments, why is that? I just want to avoid having this changed back and forth depending on some individual IDE settings, if that's the case. It also disturbs diff'ing. 😉

jlaur

Last batch of comments, I hope. 🙂

jlaur · 2024-10-19T15:13:36Z

...rsistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcPostgresqlDAO.java

+ @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);
+ }
+ }
+


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

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

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.

I didn't know that.

jlaur · 2024-10-19T15:14:46Z

...rsistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcPostgresqlDAO.java

+ @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);
+ }
+ }
+


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.

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

jlaur · 2024-10-19T15:17:02Z

...rsistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcPostgresqlDAO.java

@@ -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,


Please revert:

Suggested change

String sql = StringUtilsExt.replaceArrayMerge(this.sqlInsertItemValue,

String sql = StringUtilsExt.replaceArrayMerge(sqlInsertItemValue,

We generally only use this. prefixes when necessary.

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

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

jlaur · 2024-10-19T15:18:05Z

...rsistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcPostgresqlDAO.java

@@ -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,


Suggested change

String sql = StringUtilsExt.replaceArrayMerge(this.sqlInsertItemValue,

String sql = StringUtilsExt.replaceArrayMerge(sqlInsertItemValue,

jlaur · 2024-10-19T15:18:41Z

...rsistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcPostgresqlDAO.java

+ @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);
+ }
+ }
+


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

jlaur · 2024-10-19T15:21:10Z

...sistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcTimescaledbDAO.java

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


Suggested change

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

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

jlaur · 2024-10-19T15:23:22Z

...rsistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcPostgresqlDAO.java

+ // 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


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

jlaur · 2024-10-19T15:40:19Z

I think it would be preferable to keep the DTO's unchanged and instead directly fix the queries which only concern PostgreSQL/TimescaleDB.

I can see now after the latest changes the dilemma here when overridden queries differ only in table name being quoted. With the approach I suggested, those queries are now overridden, which feels like duplication. For example:

sqlDropTable = "DROP TABLE \"#tableName#\"";

overriding:

    protected String sqlDropTable = "DROP TABLE #tableName#";

With your original approach, instead we would have had to overload the methods only to call a different DTO method, which would have been even worse.

So it seems we would need to do something different to fully avoid duplication. What would you think about introducing a protected method getTableName in JdbcBaseDAO, something like this:

    protected String getTableName(ItemsVO vo) {
        return vo.getTableName();
    }

which could then be used from all query methods, i.e. getTableName(vo) rather than vo.getTableName()?

This would allow for overriding this method for PostgreSQL to return the quoted table name, but without changing any query methods or queries when they are not otherwise needed to be adapted for PostgreSQL?

This is just from the top of my head and all optional, and if you prefer to stick with your current implementation, that's fine as well.

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

JonathanvdGHU · 2024-10-19T15:46:25Z

I implemented your requested changes :)

Thanks! I'll take one more look. In the meantime, I see there are formatting changes for comments, why is that? I just want to avoid having this changed back and forth depending on some individual IDE settings, if that's the case. It also disturbs diff'ing. 😉

It was the IDE 😅.

JonathanvdGHU · 2024-10-19T16:09:24Z

I think it would be preferable to keep the DTO's unchanged and instead directly fix the queries which only concern PostgreSQL/TimescaleDB.

I can see now after the latest changes the dilemma here when overridden queries differ only in table name being quoted. With the approach I suggested, those queries are now overridden, which feels like duplication. For example:
sqlDropTable = "DROP TABLE \"#tableName#\"";
overriding:
    protected String sqlDropTable = "DROP TABLE #tableName#";
With your original approach, instead we would have had to overload the methods only to call a different DTO method, which would have been even worse.

So it seems we would need to do something different to fully avoid duplication. What would you think about introducing a protected method getTableName in JdbcBaseDAO, something like this:
    protected String getTableName(ItemsVO vo) {
        return vo.getTableName();
    }
which could then be used from all query methods, i.e. getTableName(vo) rather than vo.getTableName()?

This would allow for overriding this method for PostgreSQL to return the quoted table name, but without changing any query methods or queries when they are not otherwise needed to be adapted for PostgreSQL?

This is just from the top of my head and all optional, and if you prefer to stick with your current implementation, that's fine as well.

I also thought it was duplication so that's why I added the function getQuotedTableName to the DTO's. I think it's a good idea but to be honest, I don't have much time to do it. So maybe someone can change it later to improve the code.

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

JonathanvdGHU · 2024-10-19T16:15:35Z

Last batch of comments, I hope. 🙂

I think it's now done.

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl> re-added deleted changed from rebase Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl> removed not related code to the bug Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl> implemented feedback Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl> changes also implied on itemsmanagetable Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl> reverted notes Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl> reverted missed notes removed unnecessary methods Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

…GHU/openhab-addons into timescaledb-table-bug

...rsistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcPostgresqlDAO.java

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

jlaur

LGTM - thanks! I'll wait for confirmation that everything still works correctly after the latest changes. 🙂

jlaur · 2024-10-19T17:04:45Z

I also thought it was duplication so that's why I added the function getQuotedTableName to the DTO's.

The problem with that is that you would need to overload all methods in order to call the alternative DTO method, so that would generate even more duplication - it would just be duplicated methods rather than duplicated query strings.

I think it's a good idea but to be honest, I don't have much time to do it. So maybe someone can change it later to improve the code.

That's fine, I'll see if I can find some time to refactor this later.

JonathanvdGHU · 2024-10-19T17:06:59Z

LGTM - thanks! I'll wait for confirmation that everything still works correctly after the latest changes. 🙂

Thanks for the patience for all this 😌

jlaur · 2024-10-19T17:26:00Z

Thanks for the patience for all this 😌

Likewise. 😉 Before merging, besides test confirmation, there is one more thing needed: When checking the failed DCO, I noticed that your real (full) name is missing in your signoffs. You can fix both issues by rebasing your commits. If you click "Details" right to "DCO", you can see how to do that. If you are not comfortable with that, I can fix the signoff when squash-merging the PR. In that case I'd just need your full name. Thanks!

JonathanvdGHU · 2024-10-19T17:34:30Z

Thanks for the patience for all this 😌

Likewise. 😉 Before merging, besides test confirmation, there is one more thing needed: When checking the failed DCO, I noticed that your real (full) name is missing in your signoffs. You can fix both issues by rebasing your commits. If you click "Details" right to "DCO", you can see how to do that. If you are not comfortable with that, I can fix the signoff when squash-merging the PR. In that case I'd just need your full name. Thanks!

My full name is included in my email, that you can see in the signoff. I'll update it if I return for any future bug fixes! 😅. So you can use that name for squash-merging. I tested on another branch how to rebase successfully (there are a lot of merge conflicts) but I think it's the easiest way if you do it.

JonathanvdGHU requested a review from a team as a code owner October 18, 2024 15:08

jlaur linked an issue Oct 18, 2024 that may be closed by this pull request

[jdbc] TimescaleDB persistence didn’t create correct tables #14671

Open

small changes for uppercase

cee4012

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

jlaur added the bug An unexpected problem or unintended behavior of an add-on label Oct 18, 2024

table bug fixed

563add6

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

JonathanvdGHU force-pushed the timescaledb-table-bug branch from 81f6e1e to 563add6 Compare October 18, 2024 16:48

re-added deleted changed from rebase

8447a0d

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

removed not related code to the bug

160f720

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

jlaur changed the title ~~Case-sensitive tables working for TimescaleDB database via JDBC persistence service~~ [jdbc] Case-sensitive tables working for TimescaleDB database via JDBC persistence service Oct 18, 2024

jlaur requested changes Oct 19, 2024

View reviewed changes

jlaur changed the title ~~[jdbc] Case-sensitive tables working for TimescaleDB database via JDBC persistence service~~ [jdbc] Fix tableCaseSensitiveItemNames for PostgreSQL/TimescaleDB Oct 19, 2024

implemented feedback

45cc154

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

changes also implied on itemsmanagetable

53be80c

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

jlaur requested changes Oct 19, 2024

View reviewed changes

reverted notes

2874b51

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

reverted missed notes

0c147d4

removed unnecessary methods

83e6209

Signed-off-by: JonathanvdGHU <jonathan.vandegiessen@student.hu.nl>

JonathanvdGHU added 2 commits October 19, 2024 18:24

Merge branch 'timescaledb-table-bug' of https://github.com/Jonathanvd…

1da3b7b

…GHU/openhab-addons into timescaledb-table-bug

jlaur reviewed Oct 19, 2024

View reviewed changes

...rsistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcPostgresqlDAO.java Outdated Show resolved Hide resolved

Remove excess line

7d1b4ab

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>

jlaur approved these changes Oct 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jdbc] Fix `tableCaseSensitiveItemNames` for PostgreSQL/TimescaleDB #17587

[jdbc] Fix `tableCaseSensitiveItemNames` for PostgreSQL/TimescaleDB #17587

JonathanvdGHU commented Oct 18, 2024

jlaur commented Oct 18, 2024

JonathanvdGHU commented Oct 18, 2024

jlaur commented Oct 18, 2024

JonathanvdGHU commented Oct 18, 2024

jlaur left a comment

JonathanvdGHU commented Oct 19, 2024

jlaur commented Oct 19, 2024 •

edited

Loading

jlaur left a comment

jlaur Oct 19, 2024

JonathanvdGHU Oct 19, 2024

jlaur Oct 19, 2024

JonathanvdGHU Oct 19, 2024

jlaur Oct 19, 2024

JonathanvdGHU Oct 19, 2024

jlaur Oct 19, 2024

JonathanvdGHU Oct 19, 2024

jlaur Oct 19, 2024

jlaur Oct 19, 2024

jlaur Oct 19, 2024

jlaur Oct 19, 2024

jlaur Oct 19, 2024

jlaur commented Oct 19, 2024

JonathanvdGHU commented Oct 19, 2024

JonathanvdGHU commented Oct 19, 2024 •

edited

Loading

JonathanvdGHU commented Oct 19, 2024

jlaur left a comment

jlaur commented Oct 19, 2024

JonathanvdGHU commented Oct 19, 2024

jlaur commented Oct 19, 2024

JonathanvdGHU commented Oct 19, 2024 •

edited

Loading

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

[jdbc] Fix tableCaseSensitiveItemNames for PostgreSQL/TimescaleDB #17587

Are you sure you want to change the base?

[jdbc] Fix tableCaseSensitiveItemNames for PostgreSQL/TimescaleDB #17587

Conversation

JonathanvdGHU commented Oct 18, 2024

Case-sensitive tables working for TimescaleDB database via JDBC persistence service

Description

jlaur commented Oct 18, 2024

JonathanvdGHU commented Oct 18, 2024

jlaur commented Oct 18, 2024

JonathanvdGHU commented Oct 18, 2024

jlaur left a comment

Choose a reason for hiding this comment

JonathanvdGHU commented Oct 19, 2024

jlaur commented Oct 19, 2024 • edited Loading

jlaur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlaur commented Oct 19, 2024

JonathanvdGHU commented Oct 19, 2024

JonathanvdGHU commented Oct 19, 2024 • edited Loading

JonathanvdGHU commented Oct 19, 2024

jlaur left a comment

Choose a reason for hiding this comment

jlaur commented Oct 19, 2024

JonathanvdGHU commented Oct 19, 2024

jlaur commented Oct 19, 2024

JonathanvdGHU commented Oct 19, 2024 • edited Loading

[jdbc] Fix `tableCaseSensitiveItemNames` for PostgreSQL/TimescaleDB #17587

[jdbc] Fix `tableCaseSensitiveItemNames` for PostgreSQL/TimescaleDB #17587

jlaur commented Oct 19, 2024 •

edited

Loading

JonathanvdGHU commented Oct 19, 2024 •

edited

Loading

JonathanvdGHU commented Oct 19, 2024 •

edited

Loading