3
\$\begingroup\$

I need suggestions on improving this code, the code is working fine ,but i'm gonna deploy this to production system, so before that would like to ask the community for any improvements/suggestions.

The script insert/update MySQL database using the data from PostgreSQL DB.

I have tested with some sample values and both insert and Updates are working fine.

Note: Using Python 3.5

import psycopg2
import os
import time
import MySQLdb
import sys

from pprint import pprint
from datetime import datetime
from utils.config import Configuration as Config
from utils.postgres_helper import get_connection
from utils.utils import get_global_config

#ef db_connect():

# MySQLdb connection
try:
    source_host = 'magento'
    conf = get_global_config()
    cnx_msql = MySQLdb.connect(host=conf.get(source_host, 'host'),
                               user=conf.get(source_host, 'user'),
                               passwd=conf.get(source_host, 'password'),
                               port=int(conf.get(source_host, 'port')),
                               db=conf.get(source_host, 'db'))
    print('Magento MySQL DB Connected')
except mysql.connector.Error as e:
   print ("MYSQL: Unable to connect!", e.msg)
   sys.exit(1)

# Postgresql connection
try:
   cnx_psql = get_connection(get_global_config(), 'pg_dwh')
   print('DWH PostgreSQL DB Connected')
except psycopg2.Error as e:
   print('PSQL: Unable to connect!\n{0}').format(e)
   sys.exit(1)

# Cursors initializations
cur_msql = cnx_msql.cursor()
cur_msql_1 = cnx_msql.cursor()
cur_msql_2 = cnx_msql.cursor()
cur_psql = cnx_psql.cursor()
cur_psql_1 = cnx_psql.cursor()

now = time.strftime('%Y-%m-%d %H:%M:%S')

##################################################################################
update_sql_base="""select gr.email from unsubscribed gr
                   INNER JOIN nl_subscriber sn on sn.email=gr.email"""

msql_update_1="""UPDATE `nl_subscriber` SET `status`=3, `timestamp`=now() WHERE `email`='%s'"""

msql_update_2="""UPDATE `nl_subscriber` SET `subscriber_status`=3,`change_status_at`=now() WHERE `subscriber_email`='%s';"""

cur_psql.execute(update_sql_base)

for row in cur_psql:
    email=row[0]
    cur_msql.execute(msql_update_1 %email)
    cur_msql_2.execute(msql_update_2 %email)
cnx_msql.commit()

##################################################################################

insert_sql_base="""select gr.email,c.customer_id,'',3,'',CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP),'','',CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP)
                   from unsubscribed gr
                   LEFT JOIN nl_subscriber sn on sn.email=gr.email
                   LEFT JOIN customers c on c.customer_email=gr.email
                   WHERE sn.email IS NULL"""

msql_insert_1="""INSERT INTO `nl_subscriber`(
            `email`,  `customer_id`,  `options`,  `status`,  `confirm_code`,  `timestamp`,  `ip`,  `store_id`,`confirmed_at`) SELECT %s, %s, %s, %s, %s, %s, %s, %s, %s"""

cur_psql_1.execute(insert_sql_base)

for row in cur_psql_1:
    print(msql_insert_1)
    cur_msql_1.execute(msql_insert_1, row)
cnx_msql.commit()


## Closing cursors'
cur_msql.close()
cur_psql.close()
cur_psql_1.close()
cur_msql_1.close()
cur_msql_2.close()
## Committing
#cnx_psql.commit()
## Closing database connections
cnx_msql.close()
cnx_psql.close()


#if __name__ == '__main__':
 #db_connect()
\$\endgroup\$
1
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Mast
    Commented Jun 9, 2020 at 20:01

1 Answer 1

3
\$\begingroup\$

A couple thoughts.

When opening database or other resources, try to use a context manager (the with statement). At the end of your code you are closing a bunch of connections/cursors:

## Closing cursors'
## Closing cursors'
cur_msql.close()
cur_psql.close()
cur_psql_1.close()
cur_msql_1.close()
cur_msql_2.close()

## Closing database connections
cnx_msql.close()

But your code could crash while processing, all it takes is a network error. There is no guarantee this code will be executed.

One thing you can do is wrap the code in a try/finally block, and move those lines to the finally section. You don't have to have an except section. So I think it would be a good idea to use the finally for cleanup tasks.

Some lines (the SQL statements) are way too long and require scrolling eg:

insert_sql_base="""select gr.email,c.customer_id,'',3,'',CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP),'','',CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP)
                   from unsubscribed gr
                   LEFT JOIN nl_subscriber sn on sn.email=gr.email
                   LEFT JOIN customers c on c.customer_email=gr.email
                   WHERE sn.email IS NULL"""

You can reflow the text like this for example:

insert_sql_base = """SELECT gr.email,c.customer_id, '', 3, '',
CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP), '', '',
CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP)
FROM unsubscribed gr
LEFT JOIN nl_subscriber sn ON sn.email = gr.email
LEFT JOIN customers c ON c.customer_email = gr.email
WHERE sn.email IS NULL"""

Added some more spacing too. Have a look at PEP guidelines, for instance PEP8.

Also, one thing I like to do is add aliases to the expressions in SQL queries. You have some blanks ('') or hardcoded values (3) but it's not immediately apparent what they represent.


I think that you could reduce the number of variables. Too many similar-looking variables increases the risk of confusion. Consider this code:

msql_update_1="""UPDATE `nl_subscriber` SET `status`=3, `timestamp`=now() WHERE `email`='%s'"""

msql_update_2="""UPDATE `nl_subscriber` SET `subscriber_status`=3,`change_status_at`=now() WHERE `subscriber_email`='%s';"""

cur_psql.execute(update_sql_base)

for row in cur_psql:
    email=row[0]
    cur_msql.execute(msql_update_1 %email)
    cur_msql_2.execute(msql_update_2 %email)
cnx_msql.commit()

You could have one cursor and reuse it eg:

for row in cur_psql:
    email=row[0]
    msql_update = """UPDATE `nl_subscriber` SET `status`=3, `timestamp`=now() WHERE `email`='%s'"""
    cur_msql.execute(msql_update %email)

    msql_update = """UPDATE `nl_subscriber` SET `subscriber_status`=3,`change_status_at`=now() WHERE `subscriber_email`='%s';"""
    cur_msql.execute(msql_update %email)
cnx_msql.commit()

Although I am not sure why you have two statements instead of just one. Again, use context managers whenever possible to limit scope as much as possible.


There are very few comments in the code, and the flow is not that straightforward because there is overlap. For example:

update_sql_base="""select gr.email from unsubscribed gr
                   INNER JOIN nl_subscriber sn on sn.email=gr.email"""

msql_update_1="""UPDATE `nl_subscriber` SET `status`=3, `timestamp`=now() WHERE `email`='%s'"""

msql_update_2="""UPDATE `nl_subscriber` SET `subscriber_status`=3,`change_status_at`=now() WHERE `subscriber_email`='%s';"""

cur_psql.execute(update_sql_base)

for row in cur_psql:
    email=row[0]
    cur_msql.execute(msql_update_1 %email)
    cur_msql_2.execute(msql_update_2 %email)
cnx_msql.commit()

Note that between the definition of update_sql_base and its execution, you define msql_update_1, msql_update_2. Move these two lines, try to keep related code together.

A very modest change to better separate the two (includes proposed changes above):

update_sql_base = """SELECT gr.email FROM unsubscribed gr
INNER JOIN nl_subscriber sn ON sn.email = gr.email"""
cur_psql.execute(update_sql_base)


for row in cur_psql:
    email=row[0]
    msql_update = """UPDATE `nl_subscriber` SET `status`=3, `timestamp`=now() WHERE `email`='%s'"""
    cur_msql.execute(msql_update %email)

    msql_update = """UPDATE `nl_subscriber` SET `subscriber_status`=3,`change_status_at`=now() WHERE `subscriber_email`='%s';"""
    cur_msql.execute(msql_update %email)
cnx_msql.commit()

But I find that the name update_sql_base is misleading. This is not an UPDATE statement but a SELECT statement. The names are not very helpful overall.


You can declutter the code by moving blocks of code to small functions then call them in sequence eg:

open_mysql_db()
open_postgress_db()
update_mysql_db()
update_postgress_db()
close_postgress_db()
close_mysql_db()

That would make the whole more readable and easier to follow.

Example:

def copy_subscribers(cnx_msql, cnx_psql):
    """Copy subscribers from MySQL to Postgress
    """

    try:
        cur_psql = cnx_psql.cursor()
        cur_msql = cnx_msql.cursor()

        # get subscriber list from MySQL
        insert_sql_base = """SELECT gr.email,c.customer_id, '', 3, '',
        CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP), '', '',
        CAST(TO_CHAR(now(),'YYYY-MM-DD HH24:MI:SS') AS TIMESTAMP)
        FROM unsubscribed gr
        LEFT JOIN nl_subscriber sn ON sn.email = gr.email
        LEFT JOIN customers c ON c.customer_email = gr.email
        WHERE sn.email IS NULL"""
        cur_psql.execute(insert_sql_base)

        # insert susbcriber to Postgress
        msql_insert = """INSERT INTO `nl_subscriber`(`email`,  `customer_id`,
        `options`,  `status`,  `confirm_code`,
        `timestamp`,  `ip`,  `store_id`,`confirmed_at`)
        SELECT %s, %s, %s, %s, %s, %s, %s, %s, %s"""
        for row in cur_psql:
            print(msql_insert)
            cur_msql.execute(msql_insert, row)

    finally:
        cnx_msql.commit()
        cur_psql.close()
        cur_msql.close()

I don't know if the function description is accurate but this is an idea. Note that I am passing connection instances cnx_msql and cnx_psql as arguments to the function (I prefer to avoid global variables). The idea is to better isolate the blocks, also avoid the proliferation of variables and at the same time limit their scope.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot for this detailed explanation! I'm rather new to python, I have corrected fe things as you mentioned, But the function block, when i created it it was saying cursor was not defined, may be the scope is limited.And what i can do with the context manager here? \$\endgroup\$
    – Linu
    Commented Jun 9, 2020 at 19:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.