FWD » Core Development » Database

This is mostly I think about multiplex ID being inlined instead of using an argument. The cases I found are:

TemporaryBuffer.removeRecords
FastCopyHelper.copyTable
FastCopyHelper.restartSequence
TemporaryBuffer.count

The point here is that inlining an argument makes that SQL string 'unique' and using a prepared statement (and the psCache in TempTableDataSourceProvider.DataSourceImpl) useless. This also requires the re-parse of the SQL on each invocation.

The task title mentions SELECT, but all kind of queries need to be checked/fixed: INSERT, UPDATE, DELETE.

• Out of 26 possible spots, 16 were modified with no further problems.
• P2JSQLServer2008Dialect.getDropIndexString and P2JSQLServer2012Dialect.getDropSequenceString are used just for generating DDLs so there is no point in using prepared statements.
• For ReportApi.persistSearchHistory (in all three places), I am not sure if H2 still doesn't allow parameter substitutions in Subselects. If it does allow it, I can proceed to modifying this method as well.
• For FastCopyHelper.updatePeerRecords (in both places), I am not sure if parameter substitution is supported by the method executeSQLBatch.
• In TemporaryBuffer.removeRecords (in all three places), the business logic is pretty complicated. It would be difficult to add prepared statements and the code readability would certainly decrease.

Next, I will test the corectness of these changes in order to make sure that it doesn't break something.

• Out of 16 modifications, 14 of them are working fine.
• I could not test MariaDbDialect.getSequenceSetValString because I cannot access sequences in MariaDb (more in #6868).
• Considering that sequences are only for persistent databases, I am not sure if P2JH2Dialect.getSequenceSetValString should be tested. (Also, I have a small problem with persistent H2 which prevents me from testing this at the moment.)

Should I commit these changes to 3821c?

Radu Apetrii wrote:

• I could not test MariaDbDialect.getSequenceSetValString because I cannot access sequences in MariaDb (more in #6868).

I posted you there a temporary patch which solves the issue. Please do not commit the changes to 3821c. They will be merged naturally with that branch.

• Considering that sequences are only for persistent databases, I am not sure if P2JH2Dialect.getSequenceSetValString should be tested. (Also, I have a small problem with persistent H2 which prevents me from testing this at the moment.)

The H2 is not limited to temporary databases. FWD supports persistent databases backed by H2. In fact this is a solution for a quick setups of project without the hassle of installing and configuring the a SQL server like PostgreSQL. To use this, just update the database section of your directory.xml. For an example, see directory_db_h2.xml.template in the hotel test application.

Radu, please post a patch here for review, and mention the branch and revision it was built on.

I reviewed the 6830.patch. Nice job!

• in FastCopyHelper.java I see we use an inlined alter sequence statement. Even if we know that the dialect will always be H2, I think we should use the buffer's dialect-specific getSequenceSetValString() method instead. This way we can reuse the code and manageability (benefit of any improvement we do in the dialect specific updates this place too.
• you added some new APIs in Dialog class. I think the idea is good. I would like to go one step ahead and use the sequence name as parameter, too, but I guess this is not possible for all dialects since some of them (if not the majority) require the name directly. Also, the overloaded method is still necessary because it is used in DDL generation.

Greg, is this a good moment to add the patch to 3821c?

Greg, is this a good moment to add the patch to 3821c?

How safe are the changes? Have we tested them in one of the large customer applications?

Greg Shah wrote:

Greg, is this a good moment to add the patch to 3821c?

How safe are the changes? Have we tested them in one of the large customer applications?

I've made tests for every change and debugged them. The syntax is not a problem, but I could not test that each one of them gives the correct result (only for some of them I was able to check that).
In a large application almost half of the changes won't even be noticeable, because they appear in reports or in really specific cases. Still, I will test them as you said, in a large customer application, and I will provide the result obtained.

Radu, what is the state of this task? Was anything committed to 3821c?

Constantin Asofiei wrote:

Radu, what is the state of this task? Was anything committed to 3821c?

No, I did not commit these changes to 3821c. The only thing I did was posting the patch here.

Radu Apetrii wrote:

Constantin Asofiei wrote:

Radu, what is the state of this task? Was anything committed to 3821c?

No, I did not commit these changes to 3821c. The only thing I did was posting the patch here.

Please port it to 6129b and post the patch here. If you do this today, that would be great. Thank you.

Radu, the changes are OK with 6129b and a customer app (performance is improved). Please commit the 6830-6129b.patch to 6129b.

Constantin Asofiei wrote:

Radu, the changes are OK with 6129b and a customer app (performance is improved). Please commit the 6830-6129b.patch to 6129b.

Done. Committed to 6129b, rev. 14346.