# Design for refactoring PBS database code **URL:** \ **Category:** Developers\ **Created:** [February 13, 2020, 8:34pm UTC](https://community.openpbs.org/t/design-for-refactoring-pbs-database-code/2009 "2020-02-13T20:34:31Z")\ **Posts on this page:** 10\ **Page:** 1
**Author:** ![ashwathraop](https://yyz2.discourse-cdn.com/flex030/user_avatar/community.openpbs.org/ashwathraop/32/79_2.png) [@ashwathraop](https://community.openpbs.org/u/ashwathraop)\ **Post date:** [February 13, 2020, 8:34pm UTC](https://community.openpbs.org/t/design-for-refactoring-pbs-database-code/2009/1 "2020-02-13T20:34:31Z")
Here is the design page for refactoring PBS database code. I am proposing to make databse code to a dynamic library and PBS will talk to this library over bunch of APIs. [https://pbspro.atlassian.net/wiki/spaces/PD/pages/1524563969/DB+Refactor+Design](https://pbspro.atlassian.net/wiki/spaces/PD/pages/1524563969/DB+Refactor+Design). Thanks. ---
**Author:** ![agrawalravi90](https://avatars.discourse-cdn.com/v4/letter/a/848f3c/32.png) [@agrawalravi90](https://community.openpbs.org/u/agrawalravi90)\ **Post date:** [February 13, 2020, 9:42pm UTC](https://community.openpbs.org/t/design-for-refactoring-pbs-database-code/2009/2 "2020-02-13T21:42:49Z")
Thanks for posting this Ashwath. The doc looks good! Some comments: - “PBS\_EXEC/sbin/pbs\_dataservice \”: Can you please mention here what “status” will return? - “PBS\_EXEC/sbin/pbs\_ds\_password -r | -C \”: - Can you please explain what the options -r and -C do? Are there any other options? - Does this script configure the underlying db? If yes, how will it work for different kinds of db? Will one have to modify this file to add support for a new db? - Does “user\_name” need to be an existing user or can the script actually create a new user? - Just a thought, right now it seems like if one wanted to replace the db service from postgres to, say, redis, then they’d need their own version of **pbs\_dataservice** , **pbs\_dataservice** and **pbs\_db\_utility** , along with the libdb.so. Why not instead move the functionality of these additional scripts also to libdb.so and convert these scripts into generic programs instead which link with libdb? That way, one will just have to replace postgres libdb.so with redis libdb.so in order to completely replace postgres with redis. - **PBS\_EXEC/include/pbs\_db.h** - can you please provide details about all the members of these structs? - **pbs\_db\_connect** : - how about renaming **pbs\_data\_service\_host** and **pbs\_data\_service\_port** to pbs\_ds\_host and pbs\_ds\_port instead? - Should this interface take a ‘timeout’ argument? - **pbs\_db\_disconnect** : - You’ve mentioned that this function will also stop the db. Since there’s a separate function for stopping the db, i think you should remove that from here. - this function takes a “char \*\*db\_msg” while connect takes a “char \*err\_msg”, can we make them consistent? If this is a return pointer then I think it will need to be a double pointer anyways which points to a single string. - “db\_msg[out] : Connection logs/messages generated by libdb”: I think any log messages generated by libdb should be logged separately. How about a new “db\_logs” directory similar to other logs in pbs? - **get\_db\_errmsg** : It seems like most interfaces return an error message directly anyways. So, is this really needed? If the interfaces returned an error code instead of an error message then this interface would make more sense. - **pbs\_start\_db, pbs\_stop\_db and pbs\_status\_db** : Now that I think about it, pbs\_dataservice is going to be the way one would start, status and stop the db right? I don’t think we need these functions, what do you think? ---
**Author:** ![ashwathraop](https://yyz2.discourse-cdn.com/flex030/user_avatar/community.openpbs.org/ashwathraop/32/79_2.png) [@ashwathraop](https://community.openpbs.org/u/ashwathraop)\ **Post date:** [February 18, 2020, 4:44am UTC](https://community.openpbs.org/t/design-for-refactoring-pbs-database-code/2009/3 "2020-02-18T04:44:32Z")
Thanks for the comments. > - “PBS\_EXEC/sbin/pbs\_dataservice \”: Can you please mention here what “status” will return? The status option will return the status of the database instance. > - “PBS\_EXEC/sbin/pbs\_ds\_password -r | -C \”: > - Can you please explain what the options -r and -C do? Are there any other options? > - Does this script configure the underlying db? If yes, how will it work for different kinds of db? Will one have to modify this file to add support for a new db? > - Does “user\_name” need to be an existing user or can the script actually create a new user? I updated the page with info on the options. -r is to set a random password to the current database user. -C can create or change the database user to a new one. > - Just a thought, right now it seems like if one wanted to replace the db service from postgres to, say, redis, then they’d need their own version of **pbs\_dataservice** , **pbs\_dataservice** and **pbs\_db\_utility** , along with the libdb.so. Why not instead move the functionality of these additional scripts also to libdb.so and convert these scripts into generic programs instead which link with libdb? That way, one will just have to replace postgres libdb.so with redis libdb.so in order to completely replace postgres with redis. pbs\_ds\_password is already a binary executable. Since pbs\_dataservice and utility scripts will be mostly used by pre-configure scripts of initial pbs startup, it makes sense to retain them as scripts. What do you think? > - **PBS\_EXEC/include/pbs\_db.h** - can you please provide details about all the members of these structs? Added structure info to the page. > - **pbs\_db\_connect** : > - how about renaming **pbs\_data\_service\_host** and **pbs\_data\_service\_port** to pbs\_ds\_host and pbs\_ds\_port instead? Updated. > - Should this interface take a ‘timeout’ argument? Yes, this will make sure PBS server won’t wait indefinitely trying to connect to database. > - **pbs\_db\_disconnect** : > - You’ve mentioned that this function will also stop the db. Since there’s a separate function for stopping the db, i think you should remove that from here. > - this function takes a “char \*\*db\_msg” while connect takes a “char \*err\_msg”, can we make them consistent? If this is a return pointer then I think it will need to be a double pointer anyways which points to a single string. Yes, corrected it now. I removed failcode. I guess we can rely on err\_msg to capture both system and db error messages. > - “db\_msg[out] : Connection logs/messages generated by libdb”: I think any log messages generated by libdb should be logged separately. How about a new “db\_logs” directory similar to other logs in pbs? PBS server is the one who is talking to libdb so it only makes sense we log the messages to server log. No? > - **get\_db\_errmsg** : It seems like most interfaces return an error message directly anyways. So, is this really needed? If the interfaces returned an error code instead of an error message then this interface would make more sense. > - **pbs\_start\_db, pbs\_stop\_db and pbs\_status\_db** : Now that I think about it, pbs\_dataservice is going to be the way one would start, status and stop the db right? I don’t think we need these functions, what do you think? After reading your comment, yes it makes sense. The server internally uses data service script to start/stop the database. Let me know your opinion on the changes I have made. Thanks. ---
**Author:** ![agrawalravi90](https://avatars.discourse-cdn.com/v4/letter/a/848f3c/32.png) [@agrawalravi90](https://community.openpbs.org/u/agrawalravi90)\ **Post date:** [March 5, 2020, 12:55am UTC](https://community.openpbs.org/t/design-for-refactoring-pbs-database-code/2009/4 "2020-03-05T00:55:42Z")
Hey sorry for the late reply. > [@ashwathraop](#): > > The status option will return the status of the database instance. I meant to say can you list down exactly what it can return? “Up”/“Down”/“Running”/“Unknown” etc., and what these states will mean? **“dynamic library will have the functionality for the PBS server to access the database”** Just checking, is server the only daemon that write to the db? mom also has job\_save() calls, so i thought ill ask. **“PBS\_DB\_CONNECT\_STATE\_NOT\_CONNECTED”** that’s quite long, how about replacing “CONNECT\_STATE” with just “CONN” ? so this one will become PBS\_DB\_CONN\_NOT\_CONNECTED? **“PBS\_DB\_DOWN”, “PBS\_DB\_STARTING”, “PBS\_DB\_STARTED”** : “down” and “started” don’t go together, please make it “up” and “down”, or “started” and “ended” **“pbs\_db\_mominfo\_time\_t \*pbs\_db\_mominfo\_tm”** : how about making this a pbs\_db\_mom\_info\_t instead? that’ll make it consistent with the other object types. pbs\_db\_job\_info: **“ji\_svrflags;”** : what is this? **“ji\_un\_type”** : please rename this to be more intuitive **ji\_momaddr; host addr of Server** : the var name says momaddr but comment says addr of server? if it’s mom addr, what will this be if the job runs on multiple moms? **ji\_rteretry;** : what is this? **"char ji\_4jid[8]; /\* extended job save data \*/ \*\* char ji\_4ash[8]; /\* extended job save data \*/** Can these be combined into one? `pbs_db_svr_info`: `sv_jobidnumber`: will this be useful after multi-server? `pbs_db_sched_info`: Would a ’ `partition_name` field make sense here? now that scheds can only serve one partition, it might speed up db queries to find the sched associated with a partition if we add a field for it here. What do you think? Similarly, it might be useful to add a parititon\_name field to queue and node info structures. **pbs\_db\_query\_options\_t** : Will this make sense for a no SQL db ? " **query\_cb\_t:** **Function pointer for call back function to process the data returned by the database.**" That seems like a weird name for a function pointer, how about just calling it “query\_cb” ? Can you also mention what kind of “processing” is this function intended for? (as opposed to the kind of processing that the Server might do after querying data from db) “ **PBS\_DB\_MOMINFO\_TIME** ”: Again, this sticks out, I think we should just call it PBS\_DB\_MOM under pbs\_db\_save\_obj(): **1 - Execution of prepared statement failed.** **0 - Success and \> 0 rows were affected.** **1 - Execution succeeded but the statement did not affect any rows.** did you mean -1 for one of them? Same comment for other interfaces as well. **int pbs\_db\_load\_obj(pbs\_db\_conn\_t \*conn, pbs\_db\_obj\_info\_t \*obj) int pbs\_db\_find\_obj(pbs\_db\_conn\_t \*conn, pbs\_db\_obj\_info\_t \*obj, pbs\_db\_query\_options\_t \*opts, query\_cb\_t query\_cb)** shouldn’t ‘obj’ be a double pointer in these functions? Under pbs\_db\_del\_attr\_obj: " **Returns: Error code 0 - Success 1 - On Failure"** Will it return an error code or 0/1? I don’t think it can return both, it’ll be confusing to the callers. **pbs\_db\_start, pbs\_shutdown\_db and pbs\_status\_db** : Don’t we need char \*pbs\_ds\_host and int pbs\_ds\_port here? or a connection handler for shutdown and status? ---
**Author:** ![ashwathraop](https://yyz2.discourse-cdn.com/flex030/user_avatar/community.openpbs.org/ashwathraop/32/79_2.png) [@ashwathraop](https://community.openpbs.org/u/ashwathraop)\ **Post date:** [March 10, 2020, 6:58pm UTC](https://community.openpbs.org/t/design-for-refactoring-pbs-database-code/2009/5 "2020-03-10T18:58:41Z")
> [@agrawalravi90](#): > > Hey sorry for the late reply. > > > [@ashwathraop](#): > > > > The status option will return the status of the database instance. > > I meant to say can you list down exactly what it can return? “Up”/“Down”/“Running”/“Unknown” etc., and what these states will mean? Updated now > [@agrawalravi90](#): > > **“dynamic library will have the functionality for the PBS server to access the database”** > > Just checking, is server the only daemon that write to the db? mom also has job\_save() calls, so i thought ill ask. Yes, the server is the only one who is talking to the DB. > [@agrawalravi90](#): > > **“PBS\_DB\_CONNECT\_STATE\_NOT\_CONNECTED”** that’s quite long, how about replacing “CONNECT\_STATE” with just “CONN” ? so this one will become PBS\_DB\_CONN\_NOT\_CONNECTED? > > **“PBS\_DB\_DOWN”, “PBS\_DB\_STARTING”, “PBS\_DB\_STARTED”** : “down” and “started” don’t go together, please make it “up” and “down”, or “started” and “ended” I updated the design to not to manage these states anymore. Figured its the application layer which is concerned and nothing to do with libdb itself. > [@agrawalravi90](#): > > **“pbs\_db\_mominfo\_time\_t \*pbs\_db\_mominfo\_tm”** : how about making this a pbs\_db\_mom\_info\_t instead? that’ll make it consistent with the other object types. But the structure itself is used for a specific thing, which in this context mominfo time used to map host to vnodes. So I feel the name makes sense. > [@agrawalravi90](#): > > pbs\_db\_job\_info: > **“ji\_svrflags;”** : what is this? > **“ji\_un\_type”** : please rename this to be more intuitive > **ji\_momaddr; host addr of Server** : the var name says momaddr but comment says addr of server? if it’s mom addr, what will this be if the job runs on multiple moms? > **ji\_rteretry;** : what is this? > **"char ji\_4jid[8]; /\* extended job save data \*/ > \*\* char ji\_4ash[8]; /\* extended job save data \*/** > Can these be combined into one? Thanks for bringing this up. We have some redundant variables in our core component structures which is increasing the technical debt. I can update the design to remove them to some extent but I strongly feel (since its independent of libdb work) we need to have a separate design and PR to refactor these core structures. What do you think? > [@agrawalravi90](#): > > `pbs_db_svr_info`: > `sv_jobidnumber`: will this be useful after multi-server? We might still need it till multi-server is implemented right? > [@agrawalravi90](#): > > `pbs_db_sched_info`: > Would a ’ `partition_name` field make sense here? now that scheds can only serve one partition, it might speed up db queries to find the sched associated with a partition if we add a field for it here. What do you think? Similarly, it might be useful to add a parititon\_name field to queue and node info structures. Again, I feel this can be done independently than db refactoring work. But of-course we can brainstorm more on this. > [@agrawalravi90](#): > > **pbs\_db\_query\_options\_t** : Will this make sense for a no SQL db ? We will still run queries in NoSQL DB as well. This can help to pass clauses to queries. For example, with mutli-server where we would like to query based on timestamp to get delta changes, we can send timestamps using this struct. > [@agrawalravi90](#): > > " **query\_cb\_t:** **Function pointer for call back function to process the data returned by the database.**" > That seems like a weird name for a function pointer, how about just calling it “query\_cb” ? Can you also mention what kind of “processing” is this function intended for? (as opposed to the kind of processing that the Server might do after querying data from db) Its a typedef to function pointer hence the name. I added some more description. > [@agrawalravi90](#): > > “ **PBS\_DB\_MOMINFO\_TIME** ”: Again, this sticks out, I think we should just call it PBS\_DB\_MOM Wouldnt it make it very generic when we are only worried about mominfo\_time. > [@agrawalravi90](#): > > under pbs\_db\_save\_obj(): > **1 - Execution of prepared statement failed.** > **0 - Success and \> 0 rows were affected.** > **1 - Execution succeeded but the statement did not affect any rows.** > > did you mean -1 for one of them? Same comment for other interfaces as well. The page is updated now. > [@agrawalravi90](#): > > **int pbs\_db\_load\_obj(pbs\_db\_conn\_t \*conn, pbs\_db\_obj\_info\_t \*obj) > int pbs\_db\_find\_obj(pbs\_db\_conn\_t \*conn, pbs\_db\_obj\_info\_t \*obj, pbs\_db\_query\_options\_t \*opts, query\_cb\_t query\_cb)** > shouldn’t ‘obj’ be a double pointer in these functions? obj is not carrying out any output, so no. > [@agrawalravi90](#): > > Under pbs\_db\_del\_attr\_obj: > " **Returns: Error code > 0 - Success > 1 - On Failure"** > Will it return an error code or 0/1? I don’t think it can return both, it’ll be confusing to the callers. Updated now. > [@agrawalravi90](#): > > **pbs\_db\_start, pbs\_shutdown\_db and pbs\_status\_db** : Don’t we need char \*pbs\_ds\_host and int pbs\_ds\_port here? or a connection handler for shutdown and status? Updated now. Thanks for your comments. Please check the updates and share more feedback. ---
**Author:** ![agrawalravi90](https://avatars.discourse-cdn.com/v4/letter/a/848f3c/32.png) [@agrawalravi90](https://community.openpbs.org/u/agrawalravi90)\ **Post date:** [March 10, 2020, 7:31pm UTC](https://community.openpbs.org/t/design-for-refactoring-pbs-database-code/2009/6 "2020-03-10T19:31:34Z")
> [@ashwathraop](#): > > I can update the design to remove them to some extent but I strongly feel (since its independent of libdb work) we need to have a separate design and PR to refactor these core structures. What do you think? I’m ok with that > [@ashwathraop](#): > > We might still need it till multi-server is implemented right? Ok, ya this can be removed in the design that talks about multiple servers > [@agrawalravi90](#): > > > [@agrawalravi90](#): > > > > `pbs_db_sched_info` : > > > > > Would a ’ `partition_name` field make sense here? now that scheds can only serve one partition, it might speed up db queries to find the sched associated with a partition if we add a field for it here. What do you think? Similarly, it might be useful to add a parititon\_name field to queue and node info structures. > > Again, I feel this can be done independently than db refactoring work. But of-course we can brainstorm more on this. sure, it was just a thought. @arungrover and @suresht what do you think about this particular field? > [@agrawalravi90](#): > > > [@agrawalravi90](#): > > > > **int pbs\_db\_load\_obj(pbs\_db\_conn\_t \*conn, pbs\_db\_obj\_info\_t \*obj)** > > > > > > > > > **int pbs\_db\_find\_obj(pbs\_db\_conn\_t \*conn, pbs\_db\_obj\_info\_t \*obj, pbs\_db\_query\_options\_t \*opts, query\_cb\_t query\_cb)** > > > shouldn’t ‘obj’ be a double pointer in these functions? > > obj is not carrying out any output, so no. I remember us discussing this and realizing that the find and load functions should probably be combined into 1 as they seem to do similar things, one finds multiple objects while the other finds one specific object. What do you think about that? ---
**Author:** ![arungrover](https://yyz2.discourse-cdn.com/flex030/user_avatar/community.openpbs.org/arungrover/32/40_2.png) [@arungrover](https://community.openpbs.org/u/arungrover)\ **Post date:** [March 10, 2020, 8:43pm UTC](https://community.openpbs.org/t/design-for-refactoring-pbs-database-code/2009/7 "2020-03-10T20:43:23Z")
I think it will be useful to have partition name saved in the sched/queue/node/reservation info structures. Now I don’t know the internals of multi-server but we do try to find server/queue on the basis of partition name so it might be useful to add that. I have another comment about qrank job attribute. Please change it to BIGINT because we have seen integer overflows as qrank is now measured in milliseconds. ---
**Author:** ![nithinj](https://yyz2.discourse-cdn.com/flex030/user_avatar/community.openpbs.org/nithinj/32/45_2.png) [@nithinj](https://community.openpbs.org/u/nithinj)\ **Post date:** [March 11, 2020, 6:02pm UTC](https://community.openpbs.org/t/design-for-refactoring-pbs-database-code/2009/8 "2020-03-11T18:02:47Z")
We do have a partition attribute on queue, and node. The design does not explicitly talks about them as they are handled in a similar way. Most of the object attributes are saves as hstore attribute which is separate from the fixed area. ---
**Author:** ![arungrover](https://yyz2.discourse-cdn.com/flex030/user_avatar/community.openpbs.org/arungrover/32/40_2.png) [@arungrover](https://community.openpbs.org/u/arungrover)\ **Post date:** [March 12, 2020, 6:32pm UTC](https://community.openpbs.org/t/design-for-refactoring-pbs-database-code/2009/9 "2020-03-12T18:32:16Z")
Thanks Nithin! I don’t understand hstore schema that well, but can one query db where primary key is one of the hstore attribute (partition in this case)? If so, then whatever we have is fine. ---
**Author:** ![suresht](https://avatars.discourse-cdn.com/v4/letter/s/ec9cab/32.png) [@suresht](https://community.openpbs.org/u/suresht)\ **Post date:** [March 13, 2020, 3:09am UTC](https://community.openpbs.org/t/design-for-refactoring-pbs-database-code/2009/10 "2020-03-13T03:09:36Z")
> [@agrawalravi90](#): > > sure, it was just a thought. @arungrover and @suresht what do you think about this particular field? Having partition\_name as a field really helps in making the query faster. Having said that we can also achieve the same by doing the following. We can actually create an index on a specific hstore key i.e. partition attribute in this case and use it while querying. This way the query runs faster. For example CREATE UNIQUE INDEX “partition\_idx” ON “pbs.scheduler” ( (attributes → ‘partition’) );