[ovirt-devel] Writing SQL queries in DAO code

Yevgeny Zaspitsky yzaspits at redhat.com
Mon Mar 27 09:01:03 UTC 2017 

> 1. DAO files might become very long.

They would not be much longer than the current alternative, the .sql files.

> 2. An inflation of business entities to represent partial populated
business entity or inflation of mappers inflation (this will be required
for SP as well).

This isn't a problem of the DAO layer but bad design of business objects
that do not verify their own state. That could happen in any layer and
happens in existing DAO that uses stored procedures already.

> 3. An inflation of business entities to represent partial populated
business entity or inflation of mappers inflation (this will be required
for SP as well).

Look at my patches: one of them returns list of ids and another one returns
whole Cluster object and uses the existing RowMapper that populates the
whole Cluster object, so in both cases no much effort is needed in order to
keep the current state in terms of BE completness. IMHO having to create a
new class for a specific query would be the limit that will stop people
from creating queries that return something that isn't an existing type.
Anyhow I do not see how the proposed is changes the current state, can't we
do the same with a stored procedure?

> 4.1 Beside of the fact that a multi-line concatenated string that cannot
be easily copied and run with psql, it means that we should compile the
code in order to test the change (vs building with DEV_REBUILD=0 which only
package the SQL file).

Running a unit-test in your IDE after updating the in-code SQL is even
faster than the current flow.

> 4.2 No syntax highlighting when performing code review. i.e. I don't
think reviewing a patch such as https://gerrit.ovirt.org/#/c/
66729/10/packaging/dbscripts/network_sp.sql would be more clear inside a
java file.

If you review the DAO Java code in your IDE (I'd recommend that for any
Java change) you get the hiighlighting too. Beside that a code is
written/reviewed only once but is read many times and here my proposal is
much easier.

> 4.3 The user permissions management is implemented on DB level. That
means that SQL will be more complex (more concatenated java lines).

We can keep those long queries in SPs if you like. But I'm in favor to
enable the shorter one to be in-lined.

> 5. Stored procedure are cached by project's code. See
SimpleJdbcCallsHandler.getCall(), while the NamedParameterJdbcTemplate are
cached by spring's code which seems less optimal (sync all calls using
synchronized vs using ConcurrentHashMap as in SP cache).

I'm not sure that this caching code is needed. Has this proven impact/need?
Are we pioneers in JDBC area? How other projects do that?

> 6. With the NamedParametersJdbcTemplate there is no use of the
DbEngineDialect. What's the impact of it ?

In my patches I do use it through getCustomMapSqlParameterSource.


On Mon, Mar 27, 2017 at 1:56 AM, Moti Asayag <masayag at redhat.com> wrote:

>
>
> On Sun, Mar 26, 2017 at 5:12 PM, Yevgeny Zaspitsky <yzaspits at redhat.com>
> wrote:
>
>> Hi All,
>>
>> Recently I had a task of performance improvement in one of our network
>> related flows and had some hard time following our DAL code and one of the
>> outcomes of the task was defining a couple of new quite simple, but neat
>> queries.
>> When I came to coding those new queries I remembered how hard was
>> following the existing DAL code, I decided to make my own new methods
>> clearer. So I created [1] and [2] patches.
>>
>> Everything is quite standard there beside the fact that they do not use
>> any stored procedure, but use SQL directly, IMHO by that they save time
>> that I spent in trying to follow what a DAO method does. Looking into the
>> method code you get the understanding of what this method is all about:
>>
>>    - no looking for a stored procedure name that is buried in the DAO
>>    class hierarchy
>>    - no looking for the SP definition
>>
>>
> There are additional pros and cons for the suggestion to consider:
>
> Pros:
>
>    1. No need to run engine-setup after changing DB related code (in case
>    of SQL inside Java).
>
> Cons:
>
>    1. DAO files might become very long.
>    2. If you decide to return the business entity associated with the DAO
>    as a returned object, you won't know as a caller which fields to expect to
>    be populated, which lead to 3:
>    3. An inflation of business entities to represent partial populated
>    business entity or inflation of mappers inflation (this will be required
>    for SP as well).
>    4. SQL code inside of Java:
>    1. Beside of the fact that a multi-line concatenated string that
>       cannot be easily copied and run with psql, it means that we should compile
>       the code in order to test the change (vs building with DEV_REBUILD=0 which
>       only package the SQL file).
>       2. No syntax highlighting when performing code review. i.e. I don't
>       think reviewing a patch such as https://gerrit.ovirt.org/#/c/
>       66729/10/packaging/dbscripts/network_sp.sql
>       <https://gerrit.ovirt.org/#/c/66729/10/packaging/dbscripts/network_sp.sql>
>       would be more clear inside a java file.
>       3. The user permissions management is implemented on DB level. That
>       means that SQL will be more complex (more concatenated java lines).
>    5. Stored procedure are cached by project's code. See
>    SimpleJdbcCallsHandler.getCall(), while the NamedParameterJdbcTemplate
>    are cached by spring's code which seems less optimal (sync all calls using
>    synchronized vs using ConcurrentHashMap as in SP cache).
>    6. With the NamedParametersJdbcTemplate there is no use of the
>    DbEngineDialect. What's the impact of it ?
>
> So besides 5 and 6, the rest is a matter of style. I'd like to hear more
> opinions from other members.
>
> Regards,
> Moti
>
> So I'd like to propose moving towards this approach in general in all
>> cases when nothing beyond a simple SQL is needed (no stored procedure
>> programming language is needed).
>> From my experience with the performance improvement task it looks like
>> people avoid adding new queries for a specific need of a flow, instead they
>> use the existing general ones (e.g. dao.getAllForX()) and do the actual
>> join in the bll code.
>> IMHO the proposed approach would simplify adding new specific queries and
>> by that would prevent a decent part of performance issues in the future.
>>
>> I do not propose changing all existing SP's to inline queries in a once,
>> but to allow adding new queries this way. Also we might consider converting
>> relatively simple SP's to inline SQL statements later in a graduate way.
>>
>> [1] - https://gerrit.ovirt.org/#/c/74456
>> [2] - https://gerrit.ovirt.org/#/c/74458
>>
>> Regards,
>> Yevgeny
>>
>> _______________________________________________
>> Devel mailing list
>> Devel at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/devel
>>
>
>
>
> --
> Regards,
> Moti
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ovirt.org/pipermail/devel/attachments/20170327/7d7aceee/attachment.html>

More information about the Devel mailing list