search

azerothcore / azerothcore-wotlk

Complete Open Source and Modular solution for MMO
http://www.azerothcore.org
GNU Affero General Public License v3.0
6.4k stars 2.56k forks source link

Create a Semantic layer for the DB so that changes to the DB can be insulated from the App #3682

Open DrtyByt opened 3 years ago

DrtyByt commented 3 years ago

Is your feature request related to a problem? Please describe. There is no direct issue, this is more an optimization and cleanup task

Describe the solution you'd like Creating a layer of views/sproc to retrieve and update data from the database. This would be done to all methods of get/set to the database for core, and modules could follow. For backwards compat concerns any breaking changes can be surfaced with views to make things look as they did before. See Additional context for examples.

Describe alternatives you've considered While the current method works this does make any database structure changes breaking. It would also allow the db engine to use cached plans for the procedures.

Additional context Example1: (simple example of just data retrieval) Current call: (From CharacterDatabase.cpp)

PrepareStatement(CHAR_SEL_DATA_BY_GUID, "SELECT guid, account, name, gender, race, class, level FROM characters WHERE deleteDate IS NULL AND guid = ?", CONNECTION_BOTH);
Change: (I'm not really a cpp dev, so this may not be the best way to do the cpp part of this)
PrepareStatement(CHAR_SEL_DATA_BY_GUID, "Call sp_GetCharByGuid(?)", CONNECTION_BOTH;

In the DB:

Delimiter //

CREATE PROCEDURE sp_GetCharByGuid(chGuid INT)
BEGIN 
    SELECT guid
        , account
        , name
        , gender
        , race
        , class
        , level
    FROM acore_characters.characters
    WHERE deleteDate IS NULL
    AND guid = chGuid;
END //

de
limiter ;

Example2: (insulation vs db change Current call: (From CharacterDatabase.cpp)

    PrepareStatement(CHAR_SEL_CHARACTER, "SELECT guid, account, name, race, class, gender, level, xp, money, skin, face, hairStyle, hairColor, facialStyle, bankSlots, restState, playerFlags, "
                     "position_x, position_y, position_z, map, orientation, taximask, cinematic, totaltime, leveltime, rest_bonus, logout_time, is_logout_resting, resettalents_cost, "
                     "resettalents_time, trans_x, trans_y, trans_z, trans_o, transguid, extra_flags, stable_slots, at_login, zone, online, death_expire_time, taxi_path, instance_mode_mask, "
                     "arenaPoints, totalHonorPoints, todayHonorPoints, yesterdayHonorPoints, totalKills, todayKills, yesterdayKills, chosenTitle, knownCurrencies, watchedFaction, drunk, "
                     "health, power1, power2, power3, power4, power5, power6, power7, instance_id, talentGroupsCount, activeTalentGroup, exploredZones, equipmentCache, ammoId, knownTitles, actionBars, grantableLevels "
                     "FROM characters WHERE guid = ?", CONNECTION_ASYNC);
Change: (I'm not really a cpp dev, so this may not be the best way to do the cpp part of this)
PrepareStatement(CHAR_SEL_CHARACTER, "Call sp_GetCharForSelByGuid(?)", CONNECTION_ASYNC);
In the DB: (change of character table to break out powers to a separate table, app doesn't change from the SP call with the DB change.)
Delimiter //

CREATE PROCEDURE sp_GetCharForSelByGuid(chGuid INT)
BEGIN 
    SELECT c.guid
        , account
        , name
        , race
        , class
        , gender
        , level
        , xp
        , money
        , skin
        , face
        , hairStyle
        , hairColor
        , facialStyle
        , bankSlots
        , restState
        , playerFlags
        , position_x
        , position_y
        , position_z
        , map
        , orientation
        , taximask
        , cinematic
        , totaltime
        , leveltime
        , rest_bonus
        , logout_time
        , is_logout_resting
        , resettalents_cost
        , resettalents_time
        , trans_x
        , trans_y
        , trans_z
        , trans_o
        , transguid
        , extra_flags
        , stable_slots
        , at_login
        , zone
        , online
        , death_expire_time
        , taxi_path
        , instance_mode_mask
        , arenaPoints
        , totalHonorPoints
        , todayHonorPoints
        , yesterdayHonorPoints
        , totalKills
        , todayKills
        , yesterdayKills
        , chosenTitle
        , knownCurrencies
        , watchedFaction
        , drunk
        , health
        , POW1
        , POW2
        , POW3
        , POW4
        , POW5
        , POW6
        , POW7
        , instance_id
        , talentGroupsCount
        , activeTalentGroup
        , exploredZones
        , equipmentCache
        , ammoId
        , knownTitles
        , actionBars
        , grantableLevels 
    FROM acore_characters.characters c
    LEFT JOIN (
        SELECT guid
            , MAX(case when rownum = 1 then powid END) POW1
            , MAX(case when rownum = 2 then powid END) POW2
            , MAX(case when rownum = 3 then powid END) POW3
            , MAX(case when rownum = 4 then powid END) POW4
            , MAX(case when rownum = 5 then powid END) POW5
            , MAX(case when rownum = 6 then powid END) POW6
            , MAX(case when rownum = 7 then powid END) POW7
        FROM
        (
            SELECT guid
                , powid
                , @ROW:=if(@prev=guid, @ROW, 0) + 1 AS rownum
                , @prev:=guid
            FROM acore_characters.characters_powers, (SELECT @ROW:=0, @prev:=NULL) r
            ORDER BY guid, powid
        ) s
        GROUP BY guid
        ORDER BY guid
    ) p
        ON c.guid = p.guid
    WHERE c.guid = chGuid;
END //

delimiter ;
--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/94282642-create-a-semantic-layer-for-the-db-so-that-changes-to-the-db-can-be-insulated-from-the-app?utm_campaign=plugin&utm_content=tracker%2F40032087&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F40032087&utm_medium=issues&utm_source=github).
BarbzYHOOL commented 3 years ago

you can use triple backticks to format

and this suggestion is very good, i back it up

FALL1N1 commented 3 years ago

What happens if we need a custom column in that table for some reason, we have to redo the procedure?

Also, are there any benefits from this? e.g better performance..etc, or just cosmetic?

DrtyByt commented 3 years ago

Lets changes to the DB and App be insulated from each other. Performance tuning is one aspect as you can make changes to the DB for performance without requiring changes to the App. If you need a custom column on its own then no changes needed for the procedure, if you wanted to return the new column then yes. This does mean that you can do things like remove unused columns from the DB and just stub them in the procedure for columns that always return the same value or Null, this cleans up the DB. You can also rename the tables/columns in the DB itself to whatever you want to make them more descriptive without affecting how the App needs the data/names returned.

https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=icon Virus-free. www.avast.com https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=link <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

On Tue, Dec 8, 2020 at 12:27 PM FALL1N1 notifications@github.com wrote:

What happens if we need a custom column in that table for some reason, we have to redo the procedure?

Also, are there any benefits from this? e.g better performance..etc, or just cosmetic?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/azerothcore/azerothcore-wotlk/issues/3682#issuecomment-740980526, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBPUFJZ2FT4U4GJBVRUIBDST2D45ANCNFSM4TSNJVNQ .

FALL1N1 commented 3 years ago

Not really, the table's columns are returned as an array to the core and the keys of this array will be the values, for example if you swap the name/level entries in the procedure or the table - the worldserver will never boot up. I personally think that we should leave it as-is, none of the leaked cores has procedures neither does Trinity/Mangos.

Yes - for web apps or other kind of queries it'd be awesome - logs for example, but not for major queries within the core.

DrtyByt commented 3 years ago

That's the point, you never have to change what gets sent back to the worldserver, the output back from the procedure is always the same. The procedure's output stays the same, if you change the columns or tables that have the information you still send the data back to the worldserver as it's expecting to get it.

https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=icon Virus-free. www.avast.com https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=link <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

On Tue, Dec 8, 2020 at 3:34 PM FALL1N1 notifications@github.com wrote:

Not really, the table's columns are returned as an array to the core and the keys of this array will be the values, for example if you swap the name/level entries in the procedure or the table - the worldserver will never boot up. I personally think that we should leave it as-is, none of the leaked cores has procedures neither does Trinity/Mangos.

Yes - for web apps or other kind of queries it'd be awesome - logs for example, but not for major queries within the core.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/azerothcore/azerothcore-wotlk/issues/3682#issuecomment-741233574, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBPUFNF75DTKHY2RQYC3HDST2ZXRANCNFSM4TSNJVNQ .

FrancescoBorzi commented 3 years ago

@DrtyByt thanks for posting this suggestion

While the current method works this does make any database structure changes breaking

you meant this doesn't right?

DrtyByt commented 3 years ago

meant that the current method of direct queries does, but using a semantic layer makes it so they are not.

https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=icon Virus-free. www.avast.com https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=link <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

On Thu, Dec 10, 2020 at 12:50 PM Francesco Borzì notifications@github.com wrote:

@DrtyByt https://github.com/DrtyByt thanks for posting this suggestion

While the current method works this does make any database structure changes breaking

you meant this doesn't right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/azerothcore/azerothcore-wotlk/issues/3682#issuecomment-742790504, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBPUFJPXQAWS53X6EQB47LSUEYA7ANCNFSM4TSNJVNQ .

FrancescoBorzi commented 3 years ago

@DrtyByt ok, got it. I like your idea in general, however I'm not sure if this could bring unwanted side effects but definitely worth it a try.

Would you like to provide a proof of concept of this? i.e. a PR that implements and uses one view or stored procedure in the core so we can give it a try

DrtyByt commented 3 years ago

There is a draft PR out there showing the idea on the auth db. There is an issue though with it because of some of the code in how it calls for information in a fetch queue, then calls again with stuff still in the fetch queue. Here is the Stack question for it. https://stackoverflow.com/questions/65191535/how-to-clear-the-db-fetch-queue-before-next-query-is-called

Draft PR https://github.com/azerothcore/azerothcore-wotlk/pull/3768/files

https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=icon Virus-free. www.avast.com https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=link <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

On Thu, Dec 10, 2020 at 2:09 PM Francesco Borzì notifications@github.com wrote:

@DrtyByt https://github.com/DrtyByt ok, got it. I like your idea in general, however I'm not sure if this could bring unwanted side effects but definitely worth it a try.

Would you like to provide a proof of concept of this? i.e. a PR that implements and uses one view or stored procedure in the core so we can give it a try

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/azerothcore/azerothcore-wotlk/issues/3682#issuecomment-742831185, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBPUFPZQPNCPD5PZKDCNWLSUFBJTANCNFSM4TSNJVNQ .

FrancescoBorzi commented 3 years ago

@DrtyByt I see, have you tested it in game already?

DrtyByt commented 3 years ago

tried, but the server won't stay online because of the error. I've been trying to resolve the error but no luck so far. The easiest way I know to fix the error is to just clear the fetch queue before it goes to the next query, but I'm not familiar with the library being used to connect to MySQL so I don't know how to do that.

https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=icon Virus-free. www.avast.com https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=link <#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>

On Thu, Dec 10, 2020 at 2:24 PM Francesco Borzì notifications@github.com wrote:

@DrtyByt https://github.com/DrtyByt I see, have you tested it in game already?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/azerothcore/azerothcore-wotlk/issues/3682#issuecomment-742838417, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBPUFO5ZPP5FEUJK2OJT7TSUFDDTANCNFSM4TSNJVNQ .