Subject: [GSoC][Patch] Automatic Mode Detection V1

From: Yosry Muhammad <[email-masked]>
Date: Sat, 15 Jun 2019 08:48:20 +0200

Dear all,

This is my first patch of my GSoC project, query tool automatic mode
detection.

In this patch, the initial (basic) version of the project is implemented.
In this version, query resultsets are updatable if and only if:
- All the columns belong to a single table
- No duplicate columns are available
- All the primary keys of the table are available

Inserts, updates and deletes work automatically when the resultset is
updatable.

The 'save' button in the query tool works automatically to save the changes
in the resultset if the query is the updatable, and saves the query to a
file otherwise. The 'save as' button stays as is.

I will work on improving and adding features to this version throughout my
work during the summer according to what has the highest priorities
(supporting duplicate columns or columns produced by functions or
aggregations as read-only columns in the results seems like a good next
move).

Please give me your feedback of the changes I made, and any hints or
comments that will improve my code in any aspect.

I also have a couple of questions,
- Should the save button in the query tool work the way I am using it now?
or should there be a new dedicated button for saving the query to a file?

- What documentations or unit tests should I write? any guidelines here
would be appreciated.

Thanks a lot!

-- 
*Yosry Muhammad Yosry*

Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.
https://www.linkedin.com/in/yosrym93/
diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py
index 16f7133f..22050451 100644
--- a/web/pgadmin/tools/sqleditor/__init__.py
+++ b/web/pgadmin/tools/sqleditor/__init__.py
@@ -384,6 +384,8 @@ def poll(trans_id):
     rset = None
     has_oids = False
     oids = None
+    additional_messages = None
+    notifies = None
 
     # Check the transaction and connection status
     status, error_msg, conn, trans_obj, session_obj = \
@@ -422,6 +424,22 @@ def poll(trans_id):
 
             st, result = conn.async_fetchmany_2darray(ON_DEMAND_RECORD_COUNT)
 
+            # There may be additional messages even if result is present
+            # eg: Function can provide result as well as RAISE messages
+            messages = conn.messages()
+            if messages:
+                additional_messages = ''.join(messages)
+            notifies = conn.get_notifies()
+
+            # Procedure/Function output may comes in the form of Notices
+            # from the database server, so we need to append those outputs
+            # with the original result.
+            if result is None:
+                result = conn.status_message()
+                if (result != 'SELECT 1' or result != 'SELECT 0') and \
+                    result is not None and additional_messages:
+                    result = additional_messages + result
+
             if st:
                 if 'primary_keys' in session_obj:
                     primary_keys = session_obj['primary_keys']
@@ -438,10 +456,22 @@ def poll(trans_id):
                 )
                 session_obj['client_primary_key'] = client_primary_key
 
-                if columns_info is not None:
+                # If trans_obj is a QueryToolCommand then check for updatable
+                # resultsets and primary keys
+                if isinstance(trans_obj, QueryToolCommand):
+                    trans_obj.check_for_updatable_resultset_and_primary_keys()
+                    pk_names, primary_keys = trans_obj.get_primary_keys()
+                    # If primary_keys exist, add them to the session_obj to
+                    # allow for saving any changes to the data
+                    if primary_keys is not None:
+                        session_obj['primary_keys'] = primary_keys
 
-                    command_obj = pickle.loads(session_obj['command_obj'])
-                    if hasattr(command_obj, 'obj_id'):
+                if columns_info is not None:
+                    # If it is a QueryToolCommand that has obj_id attribute
+                    # then it should also be editable
+                    if hasattr(trans_obj, 'obj_id') and \
+                        (not isinstance(trans_obj, QueryToolCommand) or
+                         trans_obj.can_edit()):
                         # Get the template path for the column
                         template_path = 'columns/sql/#{0}#'.format(
                             conn.manager.version
@@ -449,7 +479,7 @@ def poll(trans_id):
 
                         SQL = render_template(
                             "/".join([template_path, 'nodes.sql']),
-                            tid=command_obj.obj_id,
+                            tid=trans_obj.obj_id,
                             has_oids=True
                         )
                         # rows with attribute not_null
@@ -520,26 +550,8 @@ def poll(trans_id):
         status = 'NotConnected'
         result = error_msg
 
-    # There may be additional messages even if result is present
-    # eg: Function can provide result as well as RAISE messages
-    additional_messages = None
-    notifies = None
-    if status == 'Success':
-        messages = conn.messages()
-        if messages:
-            additional_messages = ''.join(messages)
-        notifies = conn.get_notifies()
-
-    # Procedure/Function output may comes in the form of Notices from the
-    # database server, so we need to append those outputs with the
-    # original result.
-    if status == 'Success' and result is None:
-        result = conn.status_message()
-        if (result != 'SELECT 1' or result != 'SELECT 0') and \
-           result is not None and additional_messages:
-            result = additional_messages + result
-
     transaction_status = conn.transaction_status()
+
     return make_json_response(
         data={
             'status': status, 'result': result,
@@ -750,7 +762,6 @@ def save(trans_id):
                 return make_json_response(
                     data={'status': status, 'result': u"{}".format(msg)}
                 )
-
         status, res, query_res, _rowid = trans_obj.save(
             changed_data,
             session_obj['columns_info'],
diff --git a/web/pgadmin/tools/sqleditor/command.py b/web/pgadmin/tools/sqleditor/command.py
index d4b0700f..bccb6b38 100644
--- a/web/pgadmin/tools/sqleditor/command.py
+++ b/web/pgadmin/tools/sqleditor/command.py
@@ -19,6 +19,10 @@ from flask import render_template
 from flask_babelex import gettext
 from pgadmin.utils.ajax import forbidden
 from pgadmin.utils.driver import get_driver
+from pgadmin.tools.sqleditor.utils.constant_definition import ASYNC_OK
+from pgadmin.tools.sqleditor.utils.is_query_resultset_updatable \
+    import is_query_resultset_updatable
+from pgadmin.tools.sqleditor.utils.save_changed_data import save_changed_data
 
 from config import PG_DEFAULT_DRIVER
 
@@ -668,244 +672,11 @@ class TableCommand(GridCommand):
         else:
             conn = default_conn
 
-        status = False
-        res = None
-        query_res = dict()
-        count = 0
-        list_of_rowid = []
-        operations = ('added', 'updated', 'deleted')
-        list_of_sql = {}
-        _rowid = None
-
-        if conn.connected():
-
-            # Start the transaction
-            conn.execute_void('BEGIN;')
-
-            # Iterate total number of records to be updated/inserted
-            for of_type in changed_data:
-                # No need to go further if its not add/update/delete operation
-                if of_type not in operations:
-                    continue
-                # if no data to be save then continue
-                if len(changed_data[of_type]) < 1:
-                    continue
-
-                column_type = {}
-                column_data = {}
-                for each_col in columns_info:
-                    if (
-                        columns_info[each_col]['not_null'] and
-                        not columns_info[each_col]['has_default_val']
-                    ):
-                        column_data[each_col] = None
-                        column_type[each_col] =\
-                            columns_info[each_col]['type_name']
-                    else:
-                        column_type[each_col] = \
-                            columns_info[each_col]['type_name']
-
-                # For newly added rows
-                if of_type == 'added':
-                    # Python dict does not honour the inserted item order
-                    # So to insert data in the order, we need to make ordered
-                    # list of added index We don't need this mechanism in
-                    # updated/deleted rows as it does not matter in
-                    # those operations
-                    added_index = OrderedDict(
-                        sorted(
-                            changed_data['added_index'].items(),
-                            key=lambda x: int(x[0])
-                        )
-                    )
-                    list_of_sql[of_type] = []
-
-                    # When new rows are added, only changed columns data is
-                    # sent from client side. But if column is not_null and has
-                    # no_default_value, set column to blank, instead
-                    # of not null which is set by default.
-                    column_data = {}
-                    pk_names, primary_keys = self.get_primary_keys()
-                    has_oids = 'oid' in column_type
-
-                    for each_row in added_index:
-                        # Get the row index to match with the added rows
-                        # dict key
-                        tmp_row_index = added_index[each_row]
-                        data = changed_data[of_type][tmp_row_index]['data']
-                        # Remove our unique tracking key
-                        data.pop(client_primary_key, None)
-                        data.pop('is_row_copied', None)
-                        list_of_rowid.append(data.get(client_primary_key))
-
-                        # Update columns value with columns having
-                        # not_null=False and has no default value
-                        column_data.update(data)
-
-                        sql = render_template(
-                            "/".join([self.sql_path, 'insert.sql']),
-                            data_to_be_saved=column_data,
-                            primary_keys=None,
-                            object_name=self.object_name,
-                            nsp_name=self.nsp_name,
-                            data_type=column_type,
-                            pk_names=pk_names,
-                            has_oids=has_oids
-                        )
-
-                        select_sql = render_template(
-                            "/".join([self.sql_path, 'select.sql']),
-                            object_name=self.object_name,
-                            nsp_name=self.nsp_name,
-                            primary_keys=primary_keys,
-                            has_oids=has_oids
-                        )
-
-                        list_of_sql[of_type].append({
-                            'sql': sql, 'data': data,
-                            'client_row': tmp_row_index,
-                            'select_sql': select_sql
-                        })
-                        # Reset column data
-                        column_data = {}
-
-                # For updated rows
-                elif of_type == 'updated':
-                    list_of_sql[of_type] = []
-                    for each_row in changed_data[of_type]:
-                        data = changed_data[of_type][each_row]['data']
-                        pk = changed_data[of_type][each_row]['primary_keys']
-                        sql = render_template(
-                            "/".join([self.sql_path, 'update.sql']),
-                            data_to_be_saved=data,
-                            primary_keys=pk,
-                            object_name=self.object_name,
-                            nsp_name=self.nsp_name,
-                            data_type=column_type
-                        )
-                        list_of_sql[of_type].append({'sql': sql, 'data': data})
-                        list_of_rowid.append(data.get(client_primary_key))
-
-                # For deleted rows
-                elif of_type == 'deleted':
-                    list_of_sql[of_type] = []
-                    is_first = True
-                    rows_to_delete = []
-                    keys = None
-                    no_of_keys = None
-                    for each_row in changed_data[of_type]:
-                        rows_to_delete.append(changed_data[of_type][each_row])
-                        # Fetch the keys for SQL generation
-                        if is_first:
-                            # We need to covert dict_keys to normal list in
-                            # Python3
-                            # In Python2, it's already a list & We will also
-                            # fetch column names using index
-                            keys = list(
-                                changed_data[of_type][each_row].keys()
-                            )
-                            no_of_keys = len(keys)
-                            is_first = False
-                    # Map index with column name for each row
-                    for row in rows_to_delete:
-                        for k, v in row.items():
-                            # Set primary key with label & delete index based
-                            # mapped key
-                            try:
-                                row[changed_data['columns']
-                                    [int(k)]['name']] = v
-                            except ValueError:
-                                continue
-                            del row[k]
-
-                    sql = render_template(
-                        "/".join([self.sql_path, 'delete.sql']),
-                        data=rows_to_delete,
-                        primary_key_labels=keys,
-                        no_of_keys=no_of_keys,
-                        object_name=self.object_name,
-                        nsp_name=self.nsp_name
-                    )
-                    list_of_sql[of_type].append({'sql': sql, 'data': {}})
-
-            for opr, sqls in list_of_sql.items():
-                for item in sqls:
-                    if item['sql']:
-                        row_added = None
-
-                        # Fetch oids/primary keys
-                        if 'select_sql' in item and item['select_sql']:
-                            status, res = conn.execute_dict(
-                                item['sql'], item['data'])
-                        else:
-                            status, res = conn.execute_void(
-                                item['sql'], item['data'])
-
-                        if not status:
-                            conn.execute_void('ROLLBACK;')
-                            # If we roll backed every thing then update the
-                            # message for each sql query.
-                            for val in query_res:
-                                if query_res[val]['status']:
-                                    query_res[val]['result'] = \
-                                        'Transaction ROLLBACK'
-
-                            # If list is empty set rowid to 1
-                            try:
-                                if list_of_rowid:
-                                    _rowid = list_of_rowid[count]
-                                else:
-                                    _rowid = 1
-                            except Exception:
-                                _rowid = 0
-
-                            return status, res, query_res, _rowid
-
-                        # Select added row from the table
-                        if 'select_sql' in item:
-                            status, sel_res = conn.execute_dict(
-                                item['select_sql'], res['rows'][0])
-
-                            if not status:
-                                conn.execute_void('ROLLBACK;')
-                                # If we roll backed every thing then update
-                                # the message for each sql query.
-                                for val in query_res:
-                                    if query_res[val]['status']:
-                                        query_res[val]['result'] = \
-                                            'Transaction ROLLBACK'
-
-                                # If list is empty set rowid to 1
-                                try:
-                                    if list_of_rowid:
-                                        _rowid = list_of_rowid[count]
-                                    else:
-                                        _rowid = 1
-                                except Exception:
-                                    _rowid = 0
-
-                                return status, sel_res, query_res, _rowid
-
-                            if 'rows' in sel_res and len(sel_res['rows']) > 0:
-                                row_added = {
-                                    item['client_row']: sel_res['rows'][0]}
-
-                        rows_affected = conn.rows_affected()
-
-                        # store the result of each query in dictionary
-                        query_res[count] = {
-                            'status': status,
-                            'result': None if row_added else res,
-                            'sql': sql, 'rows_affected': rows_affected,
-                            'row_added': row_added
-                        }
-
-                        count += 1
-
-            # Commit the transaction if there is no error found
-            conn.execute_void('COMMIT;')
-
-        return status, res, query_res, _rowid
+        return save_changed_data(changed_data=changed_data,
+                                 columns_info=columns_info,
+                                 command_obj=self,
+                                 client_primary_key=client_primary_key,
+                                 conn=conn)

 class ViewCommand(GridCommand):
@@ -1089,18 +860,87 @@ class QueryToolCommand(BaseCommand, FetchedRowTracker):
         self.auto_rollback = False
         self.auto_commit = True
 
+        # Attributes needed to be able to edit updatable resultselts
+        self.is_updatable_resultset = False
+        self.primary_keys = None
+        self.pk_names = None
+
     def get_sql(self, default_conn=None):
         return None
 
     def get_all_columns_with_order(self, default_conn=None):
         return None
 
+    def get_primary_keys(self):
+        return self.pk_names, self.primary_keys
+
     def can_edit(self):
-        return False
+        return self.is_updatable_resultset
 
     def can_filter(self):
         return False
 
+    def check_for_updatable_resultset_and_primary_keys(self):
+        """
+            This function is used to check whether the last successful query
+            produced updatable results and sets the necessary flags and
+            attributes accordingly
+        """
+        # Fetch the connection object
+        driver = get_driver(PG_DEFAULT_DRIVER)
+        manager = driver.connection_manager(self.sid)
+        conn = manager.connection(did=self.did, conn_id=self.conn_id)
+
+        # Check that the query results are ready first
+        status, result = conn.poll(
+            formatted_exception_msg=True, no_result=True)
+        if status != ASYNC_OK:
+            return
+
+        # Get the path to the sql templates
+        sql_path = 'sqleditor/sql/#{0}#'.format(manager.version)
+
+        self.is_updatable_resultset, self.primary_keys, pk_names, table_oid = \
+            is_query_resultset_updatable(conn, sql_path)
+
+        # Create pk_names attribute in the required format
+        if pk_names is not None:
+            self.pk_names = ''
+
+            for pk_name in pk_names:
+                self.pk_names += driver.qtIdent(conn, pk_name) + ','
+
+            if self.pk_names != '':
+                # Remove last character from the string
+                self.pk_names = self.pk_names[:-1]
+
+        # Add attributes required to be able to update table data
+        if self.is_updatable_resultset:
+            self.__set_updatable_resultset_attributes(sql_path=sql_path,
+                                                      table_oid=table_oid,
+                                                      conn=conn)
+
+    def save(self,
+             changed_data,
+             columns_info,
+             client_primary_key='__temp_PK',
+             default_conn=None):
+        if not self.is_updatable_resultset:
+            return False, gettext('The resultset is not updatable.'), None, None
+        else:
+            driver = get_driver(PG_DEFAULT_DRIVER)
+            if default_conn is None:
+                manager = driver.connection_manager(self.sid)
+                conn = manager.connection(did=self.did, conn_id=self.conn_id)
+            else:
+                conn = default_conn
+
+            return save_changed_data(changed_data=changed_data,
+                                     columns_info=columns_info,
+                                     conn=conn,
+                                     command_obj=self,
+                                     client_primary_key=client_primary_key)
+
     def set_connection_id(self, conn_id):
         self.conn_id = conn_id
 
@@ -1109,3 +949,29 @@ class QueryToolCommand(BaseCommand, FetchedRowTracker):
 
     def set_auto_commit(self, auto_commit):
         self.auto_commit = auto_commit
+
+    def __set_updatable_resultset_attributes(self, sql_path,
+                                             table_oid, conn):
+        # Set template path for sql scripts and the table object id
+        self.sql_path = sql_path
+        self.obj_id = table_oid
+
+        if conn.connected():
+            # Fetch the Namespace Name and object Name
+            query = render_template(
+                "/".join([self.sql_path, 'objectname.sql']),
+                obj_id=self.obj_id
+            )
+
+            status, result = conn.execute_dict(query)
+            if not status:
+                raise Exception(result)
+
+            self.nsp_name = result['rows'][0]['nspname']
+            self.object_name = result['rows'][0]['relname']
+        else:
+            raise Exception(gettext(
+                'Not connected to server or connection with the server '
+                'has been closed.')
+            )
+
diff --git a/web/pgadmin/tools/sqleditor/static/js/sqleditor.js b/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
index 6af098b4..3bccd447 100644
--- a/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
+++ b/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
@@ -2376,6 +2376,18 @@ define('tools.querytool', [
         else
           self.can_edit = true;
 
+        /* If the query results are updatable then keep track of newly added
+         * rows
+         */
+        if (self.is_query_tool && self.can_edit) {
+          // keep track of newly added rows
+          self.rows_to_disable = new Array();
+          // Temporarily hold new rows added
+          self.temp_new_rows = new Array();
+          self.has_more_rows = false;
+          self.fetching_rows = false;
+        }
+
         /* If user can filter the data then we should enabled
          * Filter and Limit buttons.
          */
@@ -2818,12 +2830,15 @@ define('tools.querytool', [
        * the ajax call to save the data into the database server.
        * and will open save file dialog conditionally.
        */
-      _save: function(view, controller, save_as) {
+      _save: function(view, controller, save_as=false) {
         var self = this,
           save_data = true;
 
-        // Open save file dialog if query tool
-        if (self.is_query_tool) {
+        // Open save file dialog if query tool and:
+        // - results are not editable
+        // or
+        // - 'save as' is pressed instead of 'save'
+        if (self.is_query_tool && (!self.can_edit || save_as)) {
           var current_file = self.gridView.current_file;
           if (!_.isUndefined(current_file) && !save_as) {
             self._save_file_handler(current_file);
diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/primary_keys.sql b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/primary_keys.sql
index 60d0e56f..a96c928f 100644
--- a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/primary_keys.sql
+++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/primary_keys.sql
@@ -1,8 +1,8 @@
 {# ============= Fetch the primary keys for given object id ============= #}
 {% if obj_id %}
-SELECT at.attname, ty.typname
+SELECT at.attname, at.attnum, ty.typname
 FROM pg_attribute at LEFT JOIN pg_type ty ON (ty.oid = at.atttypid)
 WHERE attrelid={{obj_id}}::oid AND attnum = ANY (
     (SELECT con.conkey FROM pg_class rel LEFT OUTER JOIN pg_constraint con ON con.conrelid=rel.oid
     AND con.contype='p' WHERE rel.relkind IN ('r','s','t') AND rel.oid = {{obj_id}}::oid)::oid[])
-{% endif %}
\ No newline at end of file
+{% endif %}
diff --git a/web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py b/web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py
new file mode 100644
index 00000000..ed60f1e9
--- /dev/null
+++ b/web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py
@@ -0,0 +1,79 @@
+##########################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2019, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+
+"""
+    Check if the result-set of a query is updatable, A resultset is
+    updatable (as of this version) if:
+        - All columns belong to the same table.
+        - All the primary key columns of the table are present in the resultset
+        - No duplicate columns
+"""
+from flask import render_template
+try:
+    from collections import OrderedDict
+except ImportError:
+    from ordereddict import OrderedDict
+
+
+def is_query_resultset_updatable(conn, sql_path):
+    """
+        This function is used to check whether the last successful query
+        produced updatable results.
+
+        Args:
+            conn: Connection object.
+            sql_path: the path to the sql templates.
+    """
+    columns_info = conn.get_column_info()
+    # Fetch the column info
+    if len(columns_info) < 1:
+        return False, None, None, None
+
+    # First check that all the columns belong to a single table
+    table_oid = columns_info[0]['table_oid']
+    column_numbers = []
+    for column in columns_info:
+        if column['table_oid'] != table_oid:
+            return False, None, None, None
+        else:
+            column_numbers.append(column['table_column'])
+
+    # Check for duplicate columns
+    is_duplicate_columns = len(column_numbers) != len(set(column_numbers))
+    if is_duplicate_columns:
+        return False, None, None, None
+
+    if conn.connected():
+        # Then check that all the primary keys of the table are present
+        query = render_template(
+            "/".join([sql_path, 'primary_keys.sql']),
+            obj_id=table_oid
+        )
+        status, result = conn.execute_dict(query)
+        if not status:
+            return False, None, None, None
+
+        primary_keys_column_numbers = []
+        primary_keys = OrderedDict()
+        pk_names = []
+
+        for row in result['rows']:
+            primary_keys[row['attname']] = row['typname']
+            primary_keys_column_numbers.append(row['attnum'])
+            pk_names.append(row['attname'])
+
+        all_primary_keys_exist = all(elem in column_numbers
+                                     for elem in primary_keys_column_numbers)
+    else:
+        return False, None, None, None
+
+    if all_primary_keys_exist:
+        return True, primary_keys, pk_names, table_oid
+    else:
+        return False, None, None, None
diff --git a/web/pgadmin/tools/sqleditor/utils/save_changed_data.py b/web/pgadmin/tools/sqleditor/utils/save_changed_data.py
new file mode 100644
index 00000000..f22c5da3
--- /dev/null
+++ b/web/pgadmin/tools/sqleditor/utils/save_changed_data.py
@@ -0,0 +1,268 @@
+##########################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2019, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+
+from flask import render_template
+try:
+    from collections import OrderedDict
+except ImportError:
+    from ordereddict import OrderedDict
+
+
+def save_changed_data(changed_data, columns_info, conn, command_obj,
+                      client_primary_key):
+    """
+    This function is used to save the data into the database.
+    Depending on condition it will either update or insert the
+    new row into the database.
+
+    Args:
+        changed_data: Contains data to be saved
+        command_obj: The transaction object (command_obj or trans_obj)
+        conn: The connection object
+        columns_info:
+        client_primary_key:
+    """
+    status = False
+    res = None
+    query_res = dict()
+    count = 0
+    list_of_rowid = []
+    operations = ('added', 'updated', 'deleted')
+    list_of_sql = {}
+    _rowid = None
+
+    if conn.connected():
+
+        # Start the transaction
+        conn.execute_void('BEGIN;')
+
+        # Iterate total number of records to be updated/inserted
+        for of_type in changed_data:
+            # No need to go further if its not add/update/delete operation
+            if of_type not in operations:
+                continue
+            # if no data to be save then continue
+            if len(changed_data[of_type]) < 1:
+                continue
+
+            column_type = {}
+            column_data = {}
+            for each_col in columns_info:
+                if (
+                    columns_info[each_col]['not_null'] and
+                    not columns_info[each_col]['has_default_val']
+                ):
+                    column_data[each_col] = None
+                    column_type[each_col] = \
+                        columns_info[each_col]['type_name']
+                else:
+                    column_type[each_col] = \
+                        columns_info[each_col]['type_name']
+
+            # For newly added rows
+            if of_type == 'added':
+                # Python dict does not honour the inserted item order
+                # So to insert data in the order, we need to make ordered
+                # list of added index We don't need this mechanism in
+                # updated/deleted rows as it does not matter in
+                # those operations
+                added_index = OrderedDict(
+                    sorted(
+                        changed_data['added_index'].items(),
+                        key=lambda x: int(x[0])
+                    )
+                )
+                list_of_sql[of_type] = []
+
+                # When new rows are added, only changed columns data is
+                # sent from client side. But if column is not_null and has
+                # no_default_value, set column to blank, instead
+                # of not null which is set by default.
+                column_data = {}
+                pk_names, primary_keys = command_obj.get_primary_keys()
+                has_oids = 'oid' in column_type
+
+                for each_row in added_index:
+                    # Get the row index to match with the added rows
+                    # dict key
+                    tmp_row_index = added_index[each_row]
+                    data = changed_data[of_type][tmp_row_index]['data']
+                    # Remove our unique tracking key
+                    data.pop(client_primary_key, None)
+                    data.pop('is_row_copied', None)
+                    list_of_rowid.append(data.get(client_primary_key))
+
+                    # Update columns value with columns having
+                    # not_null=False and has no default value
+                    column_data.update(data)
+
+                    sql = render_template(
+                        "/".join([command_obj.sql_path, 'insert.sql']),
+                        data_to_be_saved=column_data,
+                        primary_keys=None,
+                        object_name=command_obj.object_name,
+                        nsp_name=command_obj.nsp_name,
+                        data_type=column_type,
+                        pk_names=pk_names,
+                        has_oids=has_oids
+                    )
+
+                    select_sql = render_template(
+                        "/".join([command_obj.sql_path, 'select.sql']),
+                        object_name=command_obj.object_name,
+                        nsp_name=command_obj.nsp_name,
+                        primary_keys=primary_keys,
+                        has_oids=has_oids
+                    )
+
+                    list_of_sql[of_type].append({
+                        'sql': sql, 'data': data,
+                        'client_row': tmp_row_index,
+                        'select_sql': select_sql
+                    })
+                    # Reset column data
+                    column_data = {}
+
+            # For updated rows
+            elif of_type == 'updated':
+                list_of_sql[of_type] = []
+                for each_row in changed_data[of_type]:
+                    data = changed_data[of_type][each_row]['data']
+                    pk = changed_data[of_type][each_row]['primary_keys']
+                    sql = render_template(
+                        "/".join([command_obj.sql_path, 'update.sql']),
+                        data_to_be_saved=data,
+                        primary_keys=pk,
+                        object_name=command_obj.object_name,
+                        nsp_name=command_obj.nsp_name,
+                        data_type=column_type
+                    )
+                    list_of_sql[of_type].append({'sql': sql, 'data': data})
+                    list_of_rowid.append(data.get(client_primary_key))
+
+            # For deleted rows
+            elif of_type == 'deleted':
+                list_of_sql[of_type] = []
+                is_first = True
+                rows_to_delete = []
+                keys = None
+                no_of_keys = None
+                for each_row in changed_data[of_type]:
+                    rows_to_delete.append(changed_data[of_type][each_row])
+                    # Fetch the keys for SQL generation
+                    if is_first:
+                        # We need to covert dict_keys to normal list in
+                        # Python3
+                        # In Python2, it's already a list & We will also
+                        # fetch column names using index
+                        keys = list(
+                            changed_data[of_type][each_row].keys()
+                        )
+                        no_of_keys = len(keys)
+                        is_first = False
+                # Map index with column name for each row
+                for row in rows_to_delete:
+                    for k, v in row.items():
+                        # Set primary key with label & delete index based
+                        # mapped key
+                        try:
+                            row[changed_data['columns']
+                                            [int(k)]['name']] = v
+                        except ValueError:
+                            continue
+                        del row[k]
+
+                sql = render_template(
+                    "/".join([command_obj.sql_path, 'delete.sql']),
+                    data=rows_to_delete,
+                    primary_key_labels=keys,
+                    no_of_keys=no_of_keys,
+                    object_name=command_obj.object_name,
+                    nsp_name=command_obj.nsp_name
+                )
+                list_of_sql[of_type].append({'sql': sql, 'data': {}})
+
+        for opr, sqls in list_of_sql.items():
+            for item in sqls:
+                if item['sql']:
+                    row_added = None
+
+                    # Fetch oids/primary keys
+                    if 'select_sql' in item and item['select_sql']:
+                        status, res = conn.execute_dict(
+                            item['sql'], item['data'])
+                    else:
+                        status, res = conn.execute_void(
+                            item['sql'], item['data'])
+
+                    if not status:
+                        conn.execute_void('ROLLBACK;')
+                        # If we roll backed every thing then update the
+                        # message for each sql query.
+                        for val in query_res:
+                            if query_res[val]['status']:
+                                query_res[val]['result'] = \
+                                    'Transaction ROLLBACK'
+
+                        # If list is empty set rowid to 1
+                        try:
+                            if list_of_rowid:
+                                _rowid = list_of_rowid[count]
+                            else:
+                                _rowid = 1
+                        except Exception:
+                            _rowid = 0
+
+                        return status, res, query_res, _rowid
+
+                    # Select added row from the table
+                    if 'select_sql' in item:
+                        status, sel_res = conn.execute_dict(
+                            item['select_sql'], res['rows'][0])
+
+                        if not status:
+                            conn.execute_void('ROLLBACK;')
+                            # If we roll backed every thing then update
+                            # the message for each sql query.
+                            for val in query_res:
+                                if query_res[val]['status']:
+                                    query_res[val]['result'] = \
+                                        'Transaction ROLLBACK'
+
+                            # If list is empty set rowid to 1
+                            try:
+                                if list_of_rowid:
+                                    _rowid = list_of_rowid[count]
+                                else:
+                                    _rowid = 1
+                            except Exception:
+                                _rowid = 0
+
+                            return status, sel_res, query_res, _rowid
+
+                        if 'rows' in sel_res and len(sel_res['rows']) > 0:
+                            row_added = {
+                                item['client_row']: sel_res['rows'][0]}
+
+                    rows_affected = conn.rows_affected()
+
+                    # store the result of each query in dictionary
+                    query_res[count] = {
+                        'status': status,
+                        'result': None if row_added else res,
+                        'sql': sql, 'rows_affected': rows_affected,
+                        'row_added': row_added
+                    }
+
+                    count += 1
+
+        # Commit the transaction if there is no error found
+        conn.execute_void('COMMIT;')
+
+    return status, res, query_res, _rowid
diff --git a/web/pgadmin/tools/sqleditor/utils/start_running_query.py b/web/pgadmin/tools/sqleditor/utils/start_running_query.py
index a5399774..ece11f9c 100644
--- a/web/pgadmin/tools/sqleditor/utils/start_running_query.py
+++ b/web/pgadmin/tools/sqleditor/utils/start_running_query.py
@@ -45,6 +45,9 @@ class StartRunningQuery:
         if type(session_obj) is Response:
             return session_obj
 
+        # Remove any existing primary keys in session_obj
+        session_obj.pop('primary_keys', None)
+
         transaction_object = pickle.loads(session_obj['command_obj'])
         can_edit = False
         can_filter = False

---

From: Yosry Muhammad <[email-masked]>
Date: Sun, 16 Jun 2019 16:09:55 +0200

This is a patch fixing a problem with the above patch that happened when:
- primary key columns are renamed.
- other columns are renamed to be like primary key columns.

This problem happened mainly because the primary keys are identified in the
front-end by their names. This can be handled in a better way in a future
update where columns that are primary keys are identified by the backend
and sent to the frontend instead.
Also, renamed columns can be handled better by making them read-only in a
future update (now they are editable but they cannot be updated as a column
with the new name does not exist - it produces an error message to the
user).

Waiting for your feedback. Thanks !

On Sat, Jun 15, 2019 at 8:48 AM Yosry Muhammad <[email-masked]> wrote:

> The 'save' button in the query tool works automatically to save the
> changes in the resultset if the query is the updatable, and saves the query
> to a file otherwise. The 'save as' button stays as is.
>

-- 
*Yosry Muhammad Yosry*

Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.
https://www.linkedin.com/in/yosrym93/
diff --git a/web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py b/web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py
index ed60f1e9..2ff18d83 100644
--- a/web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py
+++ b/web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py
@@ -37,20 +37,25 @@ def is_query_resultset_updatable(conn, sql_path):
 
     # First check that all the columns belong to a single table
     table_oid = columns_info[0]['table_oid']
-    column_numbers = []
+    columns = []
     for column in columns_info:
         if column['table_oid'] != table_oid:
             return False, None, None, None
         else:
-            column_numbers.append(column['table_column'])
+            columns.append({
+                'display_name': column['display_name'],
+                'column_number': column['table_column']
+            })
 
     # Check for duplicate columns
+    column_numbers = [col['column_number'] for col in columns]
     is_duplicate_columns = len(column_numbers) != len(set(column_numbers))
     if is_duplicate_columns:
         return False, None, None, None
 
     if conn.connected():
         # Then check that all the primary keys of the table are present
+        # and no primary keys are renamed (or other columns renamed to be like primary keys)
         query = render_template(
             "/".join([sql_path, 'primary_keys.sql']),
             obj_id=table_oid
@@ -59,21 +64,36 @@ def is_query_resultset_updatable(conn, sql_path):
         if not status:
             return False, None, None, None
 
-        primary_keys_column_numbers = []
+        primary_keys_columns = []
         primary_keys = OrderedDict()
         pk_names = []
 
         for row in result['rows']:
             primary_keys[row['attname']] = row['typname']
-            primary_keys_column_numbers.append(row['attnum'])
+            primary_keys_columns.append({
+                'name': row['attname'],
+                'column_number': row['attnum']
+            })
             pk_names.append(row['attname'])
 
-        all_primary_keys_exist = all(elem in column_numbers
-                                     for elem in primary_keys_column_numbers)
-    else:
-        return False, None, None, None
+        # Check that all primary keys exist and that all of them are not renamed
+        # and other columns are not renamed to primary key names
+        for pk in primary_keys_columns:
+            pk_exists = False
+            for col in columns:
+                if col['column_number'] == pk['column_number']:
+                    pk_exists = True
+                    if col['display_name'] != pk['name']:  # If the primary key column is renamed
+                        return False, None, None, None
+                # If the column is not the primary key but it is renamed to its name
+                elif col['display_name'] == pk['name']:
+                    return False, None, None, None
+
+            if not pk_exists:
+                return False, None, None, None
 
-    if all_primary_keys_exist:
+        # If the for loop exited without returning from the function then
+        # all primary keys exist without being renamed
         return True, primary_keys, pk_names, table_oid
     else:
         return False, None, None, None

---

From: Dave Page <[email-masked]>
Date: Mon, 17 Jun 2019 11:16:56 +0100

Hi

On Sun, Jun 16, 2019 at 3:10 PM Yosry Muhammad <[email-masked]> wrote:

> This problem happened mainly because the primary keys are identified in
> the front-end by their names. This can be handled in a better way in a
> future update where columns that are primary keys are identified by the
> backend and sent to the frontend instead.

Seems like a fairly low-impact problem. Most people probably don't rename
columns whilst they're editing data in the same table. That said, if it's
not overly invasive or complex, I see no reason not to fix it.

>> Inserts, updates and deletes work automatically when the resultset is
Hmm, I wonder if I under-estimated the complexity of this task! There is
more work to do of course, but it almost looks like you've done the hard
part. Still, there are plenty of other related things that can be improved
along the way.

>> to a file otherwise. The 'save as' button stays as is.
Yeah, I think we'll have to have a second Save button. The current one
would save the query text, whilst the new one would save changes to the
data.

Do you want me to ask our design guy for an icon?

>
>> I will work on improving and adding features to this version throughout
>> my work during the summer according to what has the highest priorities
I think handling read-only columns is most important, then duplicates.

>> comments that will improve my code in any aspect.
Well the first problem is that it doesn't actually work for me. This is
what I get when running a simple "select * from pem.probe" (where pem.probe
is a table with primary key and a few columns - see below) on a PG11 system
(with both of your patches applied):

2019-06-17 10:56:44,610: SQL flask.app: Execute (void) for server #5 -
CONN:3976967 (Query-id: 2391511):
BEGIN;
2019-06-17 10:56:44,610: SQL flask.app: Execute (async) for server #5 -
CONN:3976967 (Query-id: 5707996):
select * from pem.probe
2019-06-17 10:56:44,614: INFO werkzeug: 127.0.0.1 - - [17/Jun/2019
10:56:44] "POST /sqleditor/query_tool/start/3781524 HTTP/1.1" 200 -
2019-06-17 10:56:44,631: SQL flask.app: Polling result for (Query-id:
5707996)
2019-06-17 10:56:44,635: SQL flask.app: Polling result for (Query-id:
5707996)
2019-06-17 10:56:44,639: SQL flask.app: Execute (dict) for server #5 -
CONN:3976967 (Query-id: 5976248):
SELECT at.attname, ty.typname
FROM pg_attribute at LEFT JOIN pg_type ty ON (ty.oid = at.atttypid)
WHERE attrelid=17491::oid AND attnum = ANY (
    (SELECT con.conkey FROM pg_class rel LEFT OUTER JOIN pg_constraint con
ON con.conrelid=rel.oid
    AND con.contype='p' WHERE rel.relkind IN ('r','s','t', 'p') AND rel.oid
= 17491::oid)::oid[])

2019-06-17 10:56:44,641: ERROR flask.app: 'attnum'
Traceback (most recent call last):
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python3.7/site-packages/flask/app.py",
line 1813, in full_dispatch_request
    rv = self.dispatch_request()
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python3.7/site-packages/flask/app.py",
line 1799, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File
"/Users/dpage/.virtualenvs/pgadmin4/lib/python3.7/site-packages/flask_login/utils.py",
line 261, in decorated_view
    return func(*args, **kwargs)
  File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/__init__.py",
line 462, in poll
    trans_obj.check_for_updatable_resultset_and_primary_keys()
  File "/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/command.py",
line 904, in check_for_updatable_resultset_and_primary_keys
    is_query_resultset_updatable(conn, sql_path)
  File
"/Users/dpage/git/pgadmin4/web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py",
line 75, in is_query_resultset_updatable
    'column_number': row['attnum']
KeyError: 'attnum'

This is the table definition:

========
-- Table: pem.probe

-- DROP TABLE pem.probe;

CREATE TABLE pem.probe
(
    id integer NOT NULL DEFAULT nextval('pem.probe_id_seq'::regclass),
    display_name text COLLATE pg_catalog."default" NOT NULL,
    internal_name text COLLATE pg_catalog."default" NOT NULL,
    collection_method character(1) COLLATE pg_catalog."default" NOT NULL,
    target_type_id integer NOT NULL,
    applies_to_id integer NOT NULL,
    agent_capability text COLLATE pg_catalog."default",
    probe_code text COLLATE pg_catalog."default" NOT NULL,
    enabled_by_default boolean NOT NULL,
    default_execution_frequency integer NOT NULL,
    default_lifetime integer NOT NULL,
    any_server_version boolean NOT NULL,
    force_enabled boolean NOT NULL DEFAULT false,
    probe_key_list character varying[] COLLATE pg_catalog."default" NOT
NULL DEFAULT '{}'::character varying[],
    discard_history boolean NOT NULL DEFAULT false,
    is_system_probe boolean NOT NULL DEFAULT true,
    deleted boolean NOT NULL DEFAULT false,
    deleted_time timestamp with time zone,
    platform text COLLATE pg_catalog."default",
    is_chartable boolean NOT NULL DEFAULT true,
    jstid integer,
    CONSTRAINT probe_pkey PRIMARY KEY (id),
    CONSTRAINT probe_applies_to_id_fkey FOREIGN KEY (applies_to_id)
        REFERENCES pem.probe_target_type (id) MATCH SIMPLE
        ON UPDATE NO ACTION
        ON DELETE NO ACTION,
    CONSTRAINT probe_purge_jobstep_id_fkey FOREIGN KEY (jstid)
        REFERENCES pem.jobstep (jstid) MATCH SIMPLE
        ON UPDATE NO ACTION
        ON DELETE NO ACTION,
    CONSTRAINT probe_target_type_id_fkey FOREIGN KEY (target_type_id)
        REFERENCES pem.probe_target_type (id) MATCH SIMPLE
        ON UPDATE NO ACTION
        ON DELETE NO ACTION,
    CONSTRAINT probe_collection_method CHECK (collection_method = ANY
(ARRAY['b'::bpchar, 's'::bpchar, 'i'::bpchar, 'w'::bpchar])),
    CONSTRAINT probe_target_type_coherence CHECK (collection_method <>
's'::bpchar OR target_type_id <> 100)
)
WITH (
    OIDS = FALSE
)
TABLESPACE pg_default;

ALTER TABLE pem.probe
    OWNER to postgres;

COMMENT ON COLUMN pem.probe.default_lifetime
    IS 'Default lifetime value in days';

-- Trigger: create_delete_purge_probe_jobstep_trigger

-- DROP TRIGGER create_delete_purge_probe_jobstep_trigger ON pem.probe;

CREATE TRIGGER create_delete_purge_probe_jobstep_trigger
    AFTER INSERT OR DELETE
    ON pem.probe
    FOR EACH ROW
    EXECUTE PROCEDURE pem.create_delete_probe_purge_jobstep();

-- Trigger: pem_custom_probe_deleted

-- DROP TRIGGER pem_custom_probe_deleted ON pem.probe;

CREATE TRIGGER pem_custom_probe_deleted
    BEFORE UPDATE OF deleted
    ON pem.probe
    FOR EACH ROW
    EXECUTE PROCEDURE pem.custom_probe_deleted();

-- Trigger: probe_preupdate

-- DROP TRIGGER probe_preupdate ON pem.probe;

CREATE TRIGGER probe_preupdate
    BEFORE INSERT OR UPDATE
    ON pem.probe
    FOR EACH ROW
    EXECUTE PROCEDURE pem.probe_preupdate();

-- Trigger: update_purge_jobs_on_insert_probe

-- DROP TRIGGER update_purge_jobs_on_insert_probe ON pem.probe;

CREATE TRIGGER update_purge_jobs_on_insert_probe
    AFTER INSERT
    ON pem.probe
    FOR EACH STATEMENT
    EXECUTE PROCEDURE pem.run_job_to_update_probe_objects_combo();

-- Trigger: update_purge_jobs_on_update_probe

-- DROP TRIGGER update_purge_jobs_on_update_probe ON pem.probe;

CREATE TRIGGER update_purge_jobs_on_update_probe
    AFTER UPDATE OF default_lifetime
    ON pem.probe
    FOR EACH ROW
    EXECUTE PROCEDURE pem.run_job_to_update_probe_objects_combo();
========

>> - Should the save button in the query tool work the way I am using it
>> now? or should there be a new dedicated button for saving the query to a
>> file?
>>
>
See above :-)

>> - What documentations or unit tests should I write? any guidelines here
We're aiming to add unit tests to give as much coverage as possible,
focussing on Jasmine, Python/API and then feature tests in that order (fast
-> slow execution, which is important). So we probably want

- one feature test to do basic end-to-end validation
- Python/API tests to exercise is_query_resultset_updatable,
save_changed_data and anything else that seems relevant.
- Jasmine tests to ensure buttons are enabled/disabled as they should be,
and that primary key and updatability data is tracked properly (this may
not be feasible, but I'd still like it to be investigated and justified if
not).

We're also a day or two away from committing a new test suite for
exercising CRUD operations and the resulting reverse engineered SQL; if we
can utilise that to test primary_keys.sql, that'd be good.

Once the in-place editing works, we'll need to rip out all the code related
to the View/Edit data mode of the query tool. For example, there will be no
need to have the Filter/Sort options any more as the user can edit the SQL
directly (that one may be controversial - it's probably worth polling the
users first). Of course, if they don't want it to be removed, we'll need to
re-think how it works as then we'd have a dialogue that tries to edit
arbitrary SQL strings.

When all that's done, the docs will need an overhaul to make them match the
new design. That'll require new screenshots, and some non-trivial changes I
suspect. You'll need to review what's there at the moment, and figure out
what needs to be updated. It's possible we'll need to talk about structural
changes as well, but we can do that nearer the time.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Dave Page <[email-masked]>
Date: Tue, 18 Jun 2019 14:05:34 +0100

Hi

[please keep the maililng list CC'd]

On Mon, Jun 17, 2019 at 3:05 PM Yosry Muhammad <[email-masked]> wrote:

> That would be great to keep things clear and separated for the users.
>

I've asked Chethana (CC'd) to create one.

> Please find attached a patch to fix the problem that happened with you.
> The problem is that I edited the primary_keys.sql file in
> web/tools/sqleditor/templates/sqleditor/sql/default/ only, while there was
> another one in ..../templates/sqleditor/sql/11_plus/. I wonder what happens
> with versions before 11? are the scripts in the default/ folder used if
> they are not found in that version folder?
>
> The patch also removes a few unnecessary lines of code that I found, not
> related to the problem.
>

Ahh, yes - that works :-). I haven't done a detailed code review yet as
you're going to be whacking things around for a bit, but I didn't see any
obvious styling issues except for:

(pgadmin4) dpage@hal:~/git/pgadmin4$ make check-pep8
pycodestyle --config=.pycodestyle docs/
pycodestyle --config=.pycodestyle pkg/
pycodestyle --config=.pycodestyle web/
web/pgadmin/tools/sqleditor/__init__.py:440: [E125] continuation line with
same indent as next logical line
web/pgadmin/tools/sqleditor/command.py:929: [E501] line too long (80 > 79
characters)
web/pgadmin/tools/sqleditor/command.py:977: [W391] blank line at end of file
web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:53:
[E501] line too long (92 > 79 characters)
web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:74:
[E501] line too long (80 > 79 characters)
web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:81:
[E501] line too long (97 > 79 characters)
web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py:83:
[E501] line too long (84 > 79 characters)
1       E125 continuation line with same indent as next logical line
5       E501 line too long (80 > 79 characters)
1       W391 blank line at end of file
7
make: *** [check-pep8] Error 1

All patches need to pass that (and all other) existing tests before they
can be committed. Aside from that:

- When revising patches, please send an updated one for the whole thing,
rather than incremental ones. Incrementals are more work to apply and don't
give us any benefit in return.

- We need to add a "do you want to continue" warning before actions like
Execute or EXPLAIN are run, if there are unsaved changes in the grid.

- I think we should make the text in any cells that has been edited bold
until saved, so the user can see where changes have been made (as they can
with deleted rows).

- If I make two data edits and then delete a row, I get 3 entries in the
History panel, all showing the same delete. I would actually argue that
data edit queries that pgAdmin generates should not go into the History at
all, but maybe they should be added albeit with a flag to say they're
internal queries and an option to hide them. Thoughts?

- We need to think about how data editing fits in with transaction control.
Right now, it seems to happen entirely outside of it - for example, I tend
to work with auto commit turned off, so my connection sits
idle-in-transaction following an initial select, and remains un-affected by
edits. Please think about this and suggest options for us to discuss.

> I am sorry but I don't understand what should be done exactly in those
> tests. Could you tell me where I can look at examples for feature tests,
> Python/API tests and Jasmine tests (preferably for features related to the
> query tool)?
>

They're all over the codebase to be honest. Some examples though:

Varions Jasmine tests: web/regression/javascript (e.g. history, slickgrid,
sqleditor)
Various API tests: web/pgadmin/tools/sqleditor/tests
Feature tests: web/pgadmin/feature_tests (e.g. query_tool_*)

>
>
>> Once the in-place editing works, we'll need to rip out all the code
>> related to the View/Edit data mode of the query tool. For example, there
>> will be no need to have the Filter/Sort options any more as the user can
>> edit the SQL directly (that one may be controversial - it's probably worth
>> polling the users first). Of course, if they don't want it to be removed,
>> we'll need to re-think how it works as then we'd have a dialogue that tries
>> to edit arbitrary SQL strings.
>>
>
> I think it makes more sense for filters to be disabled. I mean since the
> user is already writing SQL it would be more convenient to just edit it
> directly.
>

Well we're not going to just disable them - we'll either remove them, or
try to make them work. I'm leaning strongly towards just removing that code
entirely.

Good work - thanks!

---

From: Yosry Muhammad <[email-masked]>
Date: Wed, 19 Jun 2019 07:18:08 +0200

Hi,
I have been working all day to try to make this patch applicable.

> I've asked Chethana (CC'd) to create one.

Waiting for the icon, will set it up once it is ready.

> can be committed. Aside from that:
I ran pep8 checks and JS tests on this patch, however I could not run
python tests due to a problem with chromedriver (working on it), please let
me know if any tests fail.

> give us any benefit in return.
The attached patch is a single patch including all old and new increments.

> until saved, so the user can see where changes have been made (as they can

Both done, new rows are highlighted too.

> internal queries and an option to hide them. Thoughts?

That was a bug with the existing 'save changes' action of 'View Data', on
which mine is based upon. I fixed the bug, now the queries are shown
correctly. However, the queries are shown in the form in which they are
sent from the backend to the database driver (without parameters - also an
already existing bug in View Data Mode), for example:

INSERT INTO public.kweek (
> media_url, username, text, created_at) VALUES (
> %(media_url)s::character varying, %(username)s::character varying,
> %(text)s::text, %(created_at)s::timestamp without time zone)
>  returning id;
>

I propose two solutions:
1- Hide pgadmin's generated sql from history (in both modes).
2- Show the actual sql query that was executed after the parameters are
plugged in (more understandable and potentially helpful).

> - We need to think about how data editing fits in with transaction
> control. Right now, it seems to happen entirely outside of it - for
> example, I tend to work with auto commit turned off, so my connection sits

I integrated the data editing in the transaction control as you noted. Now
the behavior is as follows:
1- In View Data mode, same existing behavior.
2- In Query Tool mode:
- If auto-commit is on: the modifications are made and commited once save
is pressed.
- If auto-commit is off: the modifications are made as part of the ongoing
transaction (or a new one if no transaction is ongoing), they are not
commited unless the user executes a commit command (or rollback).

>>> - Jasmine tests to ensure buttons are enabled/disabled as they should
>>> be, and that primary key and updatability data is tracked properly (this
>>> may not be feasible, but I'd still like it to be investigated and justified
>>> if not).
>>>
I meant disabling them in the query tool while keeping them in the View
Data mode as the user cannot edit the sql in the View Data mode. Do you
want to remove the feature from both modes completely?

> Good work - thanks!

Looking forward to your feedback. Thanks !

-- 
*Yosry Muhammad Yosry*

---

From: Dave Page <[email-masked]>
Date: Wed, 19 Jun 2019 12:54:43 +0100

Hi

> Waiting for the icon, will set it up once it is ready.

It's underway :-)

> me know if any tests fail.

Take a look in the Makefile (or web/regression/README) and you'll see how
you can run tests selectively - e.g. to avoid the feature tests when
running the Python suite, you can do "python regression/runtests.py
--exclude feature_tests"

As for chromedriver, there's a utility (tools/get_chromedriver.py) you can
use to download and install the correct version. You should save it to
somewhere in your path; I'd suggest the bin/ directory in your virtual
environment.

> The attached patch is a single patch including all old and new increments.

:-)

Aditya; can you do a quick code review please? Bear in mind it's a work in
progress and there are no docs or tests etc. yet.

> Both done, new rows are highlighted too.

Nice! I realise it's most likely not your code, but if you can fix the
wrapping so it doesn't break mid-word, that would be good. See the attached
screenshot to see what I mean.

> plugged in (more understandable and potentially helpful).

I like the idea of doing 2 - but I think we should have a checkbox on the
history panel to show/hide generated queries (and we should include
transaction control - BEGIN, COMMIT etc - in the generated query class).

> commited unless the user executes a commit command (or rollback).

That seems to work. I think we need to make it more obvious that there's a
transaction in progress - especially as that can be the case after the user
hits the Save button and thinks their data is safe (a side-thought is that
perhaps we shouldn't require the Save button to be pressed when auto-commit
is turned off, as that's just odd). We should highlight the transaction
state more clearly to the user, and make sure we prompt for confirmation if
they try to close the tab or the whole window.

> want to remove the feature from both modes completely?

I think you misunderstand - I want to remove the View Data mode entirely.
Your work should replace it.

---

From: Yosry Muhammad <[email-masked]>
Date: Wed, 19 Jun 2019 15:47:04 +0200

Hi,

On Wed, Jun 19, 2019, 1:54 PM Dave Page <[email-masked]> wrote:
.

Will do.

I propose two solutions:

I can work on option 2 now and then work on
the checkbox if/when there is time.

The transaction status can be made more obvious and point out when a
transaction is in progress that changes aren't commited. However, removing
the save button when auto commit is off will cause us to a send a request
and execute a query every time any cell is changed (which can be by
accident or some kind of draft). I also think it will make more sense when
there is a dedicated button, which can be named such that it is clear that
it only executes some queries. Also, the pop up that shows after edits are
succeesful can also state thar these changes are not yet commited.

As a user of pgAdmin I think this might not be the best option. Not all
users of pgAdmin are developers or know SQL. I worked on several projects
before where other people on the team (or frontend developers) would just
want to take a look at some data or do simple edits using the GUI. Also,
other management studios for other DBMSs also allow for this. In addition,
the user can do sorting of data without knowing SQL. What I think can be
done (potentially - maybe in the future) is limit the dependance on SQL
knowledge when doing filters in View Data mode, while disabling filters and
so in the Query Tool.

Thanks !

---

From: Dave Page <[email-masked]>
Date: Wed, 19 Jun 2019 15:10:51 +0100

Hi

> the checkbox if/when there is time.

I'm pretty sure there will be time :-)

> succeesful can also state thar these changes are not yet commited.

Yeah, I agree removing the button for some modes only is weird. Maybe
adding info to the notification, and having a more prominent "Your
transaction is still in progress" notification will be enough.

Another thought - we also need to figure out what happens if the user edits
data in the grid and when saving, an error occurs (e.g. trying to insert
null into a not-null field). How does that tie into transaction control?
For example, auto-rollback may then revert other changes made via SQL
(which should have been atomic, with the data changes) - or having
auto-rollback turned off may then require the user to explicitly start a
new transaction before attempting to save the data again. Perhaps we need
to precede data changes with a savepoint, and then roll back to that if
there's an error?

> knowledge when doing filters in View Data mode, while disabling filters and

Hmm, the point of this project (which has been a goal for maybe 20 years!)
was to remove that mode entirely. There is an argument that users can use
the "SELECT Script" option instead if they don't know SQL, but that would
still require the Sort/Filter options.

What do other folks think?

Oh, and icon attached!

---

From: Yosry Muhammad <[email-masked]>
Date: Wed, 19 Jun 2019 22:49:45 +0200

Hi there !

On Wed, Jun 19, 2019 at 4:11 PM Dave Page <[email-masked]> wrote:

>> I integrated the data editing in the transaction control as you noted.
>> Now the behavior is as follows:
>> - If auto-commit is off: the modifications are made as part of the
>> ongoing transaction (or a new one if no transaction is ongoing), they are
>> not commited unless the user executes a commit command (or rollback).
>>
>>
>> That seems to work. I think we need to make it more obvious that there's
>> a transaction in progress - especially as that can be the case after the
>> user hits the Save button and thinks their data is safe (a side-thought is
>> that perhaps we shouldn't require the Save button to be pressed when
>> auto-commit is turned off, as that's just odd). We should highlight the
>> transaction state more clearly to the user, and make sure we prompt for
>> confirmation if they try to close the tab or the whole window.
>>
>>

Will work on adding a notification and making the transaction status more
prominent.

>
> Another thought - we also need to figure out what happens if the user
> edits data in the grid and when saving, an error occurs (e.g. trying to
> insert null into a not-null field). How does that tie into transaction
> control? For example, auto-rollback may then revert other changes made via
> SQL (which should have been atomic, with the data changes) - or having

I think the savepoint is an adequate solution.If any problems happen then
rollback to the savepoint then release it, else just release the savepoint.

> What do other folks think?

Waiting for other people's opinion on that matter.

Oh, and icon attached!

Will work on adding the new icon and switching the save functionality to
it. I propose using this icon to save data in both View Data and Query Tool
mode, and the existing save button for exclusively saving the query files
(will be disabled in View Data mode).

On Wed, Jun 19, 2019 at 4:11 PM Dave Page <[email-masked]> wrote:

>> I integrated the data editing in the transaction control as you noted.
>> Now the behavior is as follows:
>> - If auto-commit is off: the modifications are made as part of the
>> ongoing transaction (or a new one if no transaction is ongoing), they are
>> not commited unless the user executes a commit command (or rollback).
>>
>>
>> That seems to work. I think we need to make it more obvious that there's
>> a transaction in progress - especially as that can be the case after the
>> user hits the Save button and thinks their data is safe (a side-thought is
>> that perhaps we shouldn't require the Save button to be pressed when
>> auto-commit is turned off, as that's just odd). We should highlight the
>> transaction state more clearly to the user, and make sure we prompt for
>> confirmation if they try to close the tab or the whole window.
>>
>>
> Another thought - we also need to figure out what happens if the user
> edits data in the grid and when saving, an error occurs (e.g. trying to
> insert null into a not-null field). How does that tie into transaction
> control? For example, auto-rollback may then revert other changes made via
> SQL (which should have been atomic, with the data changes) - or having

-- 
*Yosry Muhammad Yosry*

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 20 Jun 2019 10:39:17 +0530

On Thu, Jun 20, 2019 at 2:20 AM Yosry Muhammad <[email-masked]> wrote:

>>> - If auto-commit is on: the modifications are made and commited once
>>> save is pressed.
>>> I think you misunderstand - I want to remove the View Data mode
>>> entirely. Your work should replace it.
>>>
>>>
+1

View data mode is quick & easy way for any novice user who want to interact
with table data.

>> Hmm, the point of this project (which has been a goal for maybe 20
>> years!) was to remove that mode entirely. There is an argument that users
>> can use the "SELECT Script" option instead if they don't know SQL, but that
>> would still require the Sort/Filter options.
>>
>>> That was a bug with the existing 'save changes' action of 'View Data',
>>> on which mine is based upon. I fixed the bug, now the queries are shown
>>> I like the idea of doing 2 - but I think we should have a checkbox on
>>> the history panel to show/hide generated queries (and we should include
>>> - If auto-commit is on: the modifications are made and commited once
>>> save is pressed.
>>> I think you misunderstand - I want to remove the View Data mode
>>> entirely. Your work should replace it.
>>>
>>>
>> Hmm, the point of this project (which has been a goal for maybe 20
>> years!) was to remove that mode entirely. There is an argument that users
>> can use the "SELECT Script" option instead if they don't know SQL, but that
>> would still require the Sort/Filter options.
>>

---

From: Aditya Toshniwal <[email-masked]>
Date: Thu, 20 Jun 2019 11:19:11 +0530

[forked the mail chain for code review]
Hi Yosry,

> progress and there are no docs or tests etc. yet.
Nice work there. :)

I just had look on the code changes, and have few suggestions:
1) I found the code around primary key in the function
check_for_updatable_resultset_and_primary_keys repeating. Try if you cpuld
modify/reuse the get_primary_keys function.
2) The function name check_for_updatable_resultset_and_primary_keys could
be shorter, something like check_updatabale_rset_pkeys. Same for
__set_updatable_resultset_attributes to __set_updatable_rset_attr
3) You've used background: #f4f4f4; for .highlighted_grid_cells class. This
should go to sqleditor.scss with appropriate color from
web/pgadmin/static/scss/resources/_default.variables.scss. Hard coded color
codes are highly discouraged.
Otherwise, looks good (didn't run and check)

-- 
Thanks and Regards,
Aditya Toshniwal
Software Engineer | EnterpriseDB India | Pune
"Don't Complain about Heat, Plant a TREE"

---

From: Yosry Muhammad <[email-masked]>
Date: Thu, 20 Jun 2019 16:54:50 +0200

On Thu, Jun 20, 2019 at 7:49 AM Aditya Toshniwal <
[email-masked]> wrote:

>> Aditya; can you do a quick code review please? Bear in mind it's a work
>> in progress and there are no docs or tests etc. yet.
>>
> Nice work there. :)
>
> check_for_updatable_resultset_and_primary_keys repeating. Try if you
> cpuld modify/reuse the get_primary_keys function.
> 3) You've used background: #f4f4f4; for .highlighted_grid_cells class.
> This should go to sqleditor.scss with appropriate color from
I shortened both function names and fixed the hard-coded color. For #1, in
the QueryToolCommand different processing of the primary keys occur in
is_query_resultset_updatable function, where the attribute number of the
primary keys is compared against columns and so. The only repeated part in
check_for_updatable_resultset_and_primary_keys is the part where pk_names
string is created in the required format (which is only a few lines of
code). I could factor it out to a utility function - takes primary_keys
dict and returns the pk_names string in the required format. What do you
think?

These changes (together with other changes that I am working on) will be
included in the next version of this patch.

Thanks !

-- 
*Yosry Muhammad Yosry*

---

From: Yosry Muhammad <[email-masked]>
Date: Mon, 24 Jun 2019 07:38:03 +0200

Hi all,
Please find attached a patch with all the changes (from the beginning of
the project). What I added in this patch:

1- Fixed #1 and #2 that Mr. Toshniwal pointed out in his code review. Still
waiting for a reply on #3.

2- When data is edited in Query Tool with auto-commit turned off, the
notification message now tells the user that they need to commit these
changes.

3- The new icon is added and the functionality of both icons are now
completely separated as follows:
    a) Save File button: exclusively for saving the query file (disabled in
View Table mode).
    b) Save Data Changes button: for saving changes in data grid in both
modes.
I completely separated the 2 functionalities in all related files and
modules. I also fixed an existing bug that went as follows:
    - The user has unsaved edits (existed in View Table mode).
    - The user tries to close the panel, they are asked if they want to
save the changes.
    - If they choose to save and the save failed (null in a non-null column
for example), the panel closes anyway.
The panel now does not close if the save failed.

Something that is missing with the new button is the shortcut, I don't know
how to modify the Preferences in the configuration database. I could not
find the code responsible for adding data in the Preferences table and so.
Any help?

4- A savepoint is now created before any attempts are made to save data
changes, if the operation fails, the transaction is rolled back only till
the savepoint, keeping the previous SQL in the same transaction unharmed.
The whole transaction is rolled back if none existed in the first place.

5- Fixed a bug with all Alertiy.js confirm dialogs where line break would
break words.

6- I re-implemented the code responsible for handling the panel close event
in following way:
- The event used to handle one of two mutually exclusive events (or
neither): exiting with unsaved changes in the query or exiting with unsaved
changes in the data.
- As both can happen simultaneously now, I re-implemented this code to
check for multiple cases and produce sequential dialogs for different cases
(asynchronously to avoid freezing the page) . I also added a dialog that
asks for user confirmation when exiting with an un-commited transaction (or
data changes save).

I have several questions:
- How can I edit the data in the configuration database (specifically the
preferences), what parts of the code are responsible for this?
- For running python tests, how should I produce an appropriate
test_config.json.in file for my environment?
- After running python and feature tests, changes were made to nearly all
the files (git status shows modifications in a ton of files), is there
something I have done wrong?
- When closing a panel in pgAdmin 4, my browser keeps asking me if I want
to leave the page or stay which I think might be annoying to some users
(specially when closing several tabs at once). We already produce dialogs
if any changes are unsaved, the browsers' ones are unnecessary. Is this
produces by our code or automatically by the browser? any way around it? I
use Firefox.
- What else is missing from this patch to make it applicable ? I would like
to produce a release-ready patch if possible. If so, I can continue working
on the project on following patches, I just want to know what is the
minimum amount of work needed to make this patch release-ready (especially
that changes are being made in the master branch that require me to re-edit
parts of the code that I have written before to keep things in-sync).
- For the bug that I reported before (generated queries in Query History
appear in a distorted way for the user), to get the actual query that is
being executed I can use the mogirfy() function of psycopg2 but I need
access to a cursor. I can get one directly in save_changed_data() function
by using conn.conn.cursor() but then I would be bypassing the wrapper
Connection class. Should I modify the wrapper Connection class and add a
function that can provide a cursor (or a wrapper around cursor.mogrify() )?
Thoughts?

Here are things I think I might be working on next (share your thoughts):
- Make the transaction status more prominent.
- Handle cases where one or more columns can be made read-only for the
remaining of the resultset to be updatable (for example: SELECT col1, col2,
col1 || col2 as concat FROM some_table;). This will require modifying some
of the data that is sent from the backend to the frontend and a lot o
modifications (i think) in the front-end for handling columns individually.

Thanks everyone and sorry for the long email !

-- 
*Yosry Muhammad Yosry*

Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.
https://www.linkedin.com/in/yosrym93/
diff --git a/web/pgadmin/static/js/sqleditor/call_render_after_poll.js b/web/pgadmin/static/js/sqleditor/call_render_after_poll.js
index 3f32d571..51c2302b 100644
--- a/web/pgadmin/static/js/sqleditor/call_render_after_poll.js
+++ b/web/pgadmin/static/js/sqleditor/call_render_after_poll.js
@@ -37,7 +37,7 @@ export function callRenderAfterPoll(sqlEditor, alertify, res) {
     const msg = sprintf(
       gettext('Query returned successfully in %s.'), sqlEditor.total_time);
     res.result += '\n\n' + msg;
-    sqlEditor.update_msg_history(true, res.result, false);
+    sqlEditor.update_msg_history(true, res.result, true);
     if (isNotificationEnabled(sqlEditor)) {
       alertify.success(msg, sqlEditor.info_notifier_timeout);
     }
diff --git a/web/pgadmin/static/js/sqleditor/query_tool_preferences.js b/web/pgadmin/static/js/sqleditor/query_tool_preferences.js
index c3906aaa..595cbb2a 100644
--- a/web/pgadmin/static/js/sqleditor/query_tool_preferences.js
+++ b/web/pgadmin/static/js/sqleditor/query_tool_preferences.js
@@ -97,6 +97,9 @@ function updateUIPreferences(sqlEditor) {
     .attr('title',
       shortcut_title('Download as CSV',preferences.download_csv));
 
+  $el.find('#btn-save-data')
+    .attr('title', 'Save Data Changes'); // TODO: Add shortcut to preferences
+
   $el.find('#btn-commit')
     .attr('title',
       shortcut_title('Commit',preferences.commit_transaction));
diff --git a/web/pgadmin/static/scss/_alertify.overrides.scss b/web/pgadmin/static/scss/_alertify.overrides.scss
index 413e09e7..d43becd5 100644
--- a/web/pgadmin/static/scss/_alertify.overrides.scss
+++ b/web/pgadmin/static/scss/_alertify.overrides.scss
@@ -56,7 +56,7 @@
     bottom: $footer-height-calc !important;
   }
   .ajs-wrap-text {
-    word-break: break-all;
+    word-break: normal;
     word-wrap: break-word;
   }
   /* Removes padding from alertify footer */
diff --git a/web/pgadmin/static/scss/resources/_default.variables.scss b/web/pgadmin/static/scss/resources/_default.variables.scss
index 09aaf8ae..ff8fae6b 100644
--- a/web/pgadmin/static/scss/resources/_default.variables.scss
+++ b/web/pgadmin/static/scss/resources/_default.variables.scss
@@ -44,6 +44,8 @@ $color-editor-number: #964 !default;
 $color-editor-foldmarker: #0000FF !default;
 $color-editor-activeline: #50B0F0 !default;
 
+$color-highlighted-grid-cell: #F4F4F4;
+
 /* Typography */
 $font-family-primary: "Roboto", "Helvetica Neue", -apple-system, BlinkMacSystemFont, "Segoe UI", Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji" !default;
 $font-family-semibold: "Roboto Medium" !default;
diff --git a/web/pgadmin/tools/datagrid/templates/datagrid/index.html b/web/pgadmin/tools/datagrid/templates/datagrid/index.html
index f138b9a4..563f8110 100644
--- a/web/pgadmin/tools/datagrid/templates/datagrid/index.html
+++ b/web/pgadmin/tools/datagrid/templates/datagrid/index.html
@@ -21,7 +21,7 @@
                         tabindex="0">
                     <i class="fa fa-folder-open-o sql-icon-lg" aria-hidden="true"></i>
                 </button>
-                <button id="btn-save" type="button" class="btn btn-sm btn-secondary"
+                <button id="btn-save-file" type="button" class="btn btn-sm btn-secondary"
                         title=""
                         accesskey=""
                         disabled>
@@ -128,6 +128,14 @@
                     <i class="fa fa-trash sql-icon-lg" aria-hidden="true"></i>
                 </button>
             </div>
+            <div class="btn-group mr-1" role="group" aria-label="">
+                <button id="btn-save-data" type="button" class="btn btn-sm btn-secondary"
+                        title=""
+                        accesskey=""
+                        tabindex="0" disabled>
+                    <i class="icon-save-data-changes sql-icon-lg" aria-hidden="true"></i>
+                </button>
+            </div>
             <div class="btn-group mr-1" role="group" aria-label="">
                 <button id="btn-edit-dropdown" type="button" class="btn btn-sm btn-secondary dropdown-toggle"
                         data-toggle="dropdown" aria-haspopup="true" aria-expanded="false"
diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py
index ac923a55..067bff7b 100644
--- a/web/pgadmin/tools/sqleditor/__init__.py
+++ b/web/pgadmin/tools/sqleditor/__init__.py
@@ -385,6 +385,8 @@ def poll(trans_id):
     rset = None
     has_oids = False
     oids = None
+    additional_messages = None
+    notifies = None
 
     # Check the transaction and connection status
     status, error_msg, conn, trans_obj, session_obj = \
@@ -423,6 +425,22 @@ def poll(trans_id):

@@ -439,10 +457,22 @@ def poll(trans_id):
 
+                    trans_obj.check_updatable_results_pkeys()
 
@@ -450,7 +480,7 @@ def poll(trans_id):
 
                         SQL = render_template(
                             "/".join([template_path, 'nodes.sql']),
-                            tid=command_obj.obj_id,
+                            tid=trans_obj.obj_id,
                             has_oids=True
                         )
                         # rows with attribute not_null
@@ -525,26 +555,8 @@ def poll(trans_id):
         status = 'NotConnected'
         result = error_msg
 
@@ -733,7 +745,8 @@ def save(trans_id):
        trans_obj is not None and session_obj is not None:
 
         # If there is no primary key found then return from the function.
-        if (len(session_obj['primary_keys']) <= 0 or
+        if ('primary_keys' not in session_obj or
+           len(session_obj['primary_keys']) <= 0 or
            len(changed_data) <= 0) and \
            'has_oids' not in session_obj:
             return make_json_response(
@@ -746,32 +759,38 @@ def save(trans_id):
 
         manager = get_driver(
             PG_DEFAULT_DRIVER).connection_manager(trans_obj.sid)
-        default_conn = manager.connection(did=trans_obj.did)
+        if hasattr(trans_obj, 'conn_id'):
+            conn = manager.connection(did=trans_obj.did,
+                                      conn_id=trans_obj.conn_id)
+        else:
+            conn = manager.connection(did=trans_obj.did)  # default connection
 
         # Connect to the Server if not connected.
-        if not default_conn.connected():
-            status, msg = default_conn.connect()
+        if not conn.connected():
+            status, msg = conn.connect()
             if not status:
                 return make_json_response(
                     data={'status': status, 'result': u"{}".format(msg)}
                 )
-
-        status, res, query_res, _rowid = trans_obj.save(
+        status, res, query_res, _rowid, is_commit_required = trans_obj.save(
             changed_data,
             session_obj['columns_info'],
             session_obj['client_primary_key'],
-            default_conn)
+            conn)
     else:
         status = False
         res = error_msg
         query_res = None
+        _rowid = None
+        is_commit_required = None
 
     return make_json_response(
         data={
             'status': status,
             'result': res,
             'query_result': query_res,
-            '_rowid': _rowid
+            '_rowid': _rowid,
+            'is_commit_required': is_commit_required
         }
     )
 
index 28b7cc44..08f02310 100644

@@ -668,268 +672,11 @@ class TableCommand(GridCommand):
         else:
             conn = default_conn
 
-        status = False
-        res = None
-        query_res = dict()
-        count = 0
-        list_of_rowid = []
-        pgadmin_alias = {
-            col_name: col_info['pgadmin_alias']
-            for col_name, col_info in columns_info
-            .items()
-                            pgadmin_alias=pgadmin_alias,
-                        pk_escaped = {
-                            pk: pk_val.replace('%', '%%')
-                            for pk, pk_val in
-                            changed_data[of_type][each_row]['primary_keys']
-                            .items()
-                            pgadmin_alias=pgadmin_alias,
-                            primary_keys=pk_escaped,
-                        item['data'] = {
-                            pgadmin_alias[k] if k in pgadmin_alias else k: v
-                            for k, v in item['data'].items()
-                        def failure_handle():
-                        except Exception as _:
-                            failure_handle()
-                            raise
-                            return failure_handle()

 class ViewCommand(GridCommand):
@@ -1113,18 +860,88 @@ class QueryToolCommand(BaseCommand, FetchedRowTracker):
         self.auto_rollback = False
         self.auto_commit = True

 
 
     def can_filter(self):
         return False
 
+    def check_updatable_results_pkeys(self):
+        """
+            self.__set_updatable_results_attrs(sql_path=sql_path,
+            return False, gettext('Resultset is not updatable.'), None, None
+                                     auto_commit=self.auto_commit)
 
@@ -1133,3 +950,28 @@ class QueryToolCommand(BaseCommand, FetchedRowTracker):
 
     def set_auto_commit(self, auto_commit):
         self.auto_commit = auto_commit
+
+    def __set_updatable_results_attrs(self, sql_path,
+                                      table_oid, conn):
+        # Set template path for sql scripts and the table object id
+        self.sql_path = sql_path
+        self.obj_id = table_oid
+
+        if conn.connected():
+            # Fetch the Namespace Name and object Name
+            query = render_template(
+                "/".join([self.sql_path, 'objectname.sql']),
+                obj_id=self.obj_id
+            )
+
+            status, result = conn.execute_dict(query)
+            if not status:
+                raise Exception(result)
+
+            self.nsp_name = result['rows'][0]['nspname']
+            self.object_name = result['rows'][0]['relname']
+        else:
+            raise Exception(gettext(
+                'Not connected to server or connection with the server '
+                'has been closed.')
+            )
diff --git a/web/pgadmin/tools/sqleditor/static/css/sqleditor.css b/web/pgadmin/tools/sqleditor/static/css/sqleditor.css
index 86d3defc..21448327 100644
--- a/web/pgadmin/tools/sqleditor/static/css/sqleditor.css
+++ b/web/pgadmin/tools/sqleditor/static/css/sqleditor.css
@@ -291,7 +291,7 @@ input.editor-checkbox:focus {
   background-image: url('../img/disconnect.svg');
 }
 
-.icon-commit, .icon-rollback {
+.icon-commit, .icon-rollback, .icon-save-data-changes {
   display: inline-block;
   align-content: center;
   vertical-align: middle;
@@ -311,6 +311,10 @@ input.editor-checkbox:focus {
   background-image: url('../img/rollback.svg') !important;
 }
 
+.icon-save-data-changes {
+  background-image: url('../img/save_data_changes.svg') !important;
+}
+
 .ajs-body .warn-header {
   font-size: 13px;
   font-weight: bold;
diff --git a/web/pgadmin/tools/sqleditor/static/img/save_data_changes.svg b/web/pgadmin/tools/sqleditor/static/img/save_data_changes.svg
new file mode 100644
index 00000000..bd2c518e
--- /dev/null
+++ b/web/pgadmin/tools/sqleditor/static/img/save_data_changes.svg
@@ -0,0 +1,11 @@
+<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" style="enable-background:new 0 0 1792 1792;" xml:space="preserve" width="1792" height="1792"><rect id="backgroundrect" width="100%" height="100%" x="0" y="0" fill="none" stroke="none"/>
+<style type="text/css">
+	.st0{fill:#FFFFFF;}
+</style>
+<title>save_data_changes</title>
+
+
+<g class="currentLayer" style=""><title>Layer 1</title><path d="M623.031982421875,1385.1308941841125 v-172.1298828125 c0,-8.337540626525879 -2.5724587440490723,-15.240666389465332 -7.717376232147217,-20.61972427368164 s-11.74756145477295,-8.068588256835938 -19.722183227539062,-8.068588256835938 H321.1979675292969 c-7.9746222496032715,0 -14.577266693115234,2.6895294189453125 -19.722183227539062,8.068588256835938 s-7.717376232147217,12.282183647155762 -7.717376232147217,20.61972427368164 v172.1298828125 c0,8.337540626525879 2.5724587440490723,15.240666389465332 7.717376232147217,20.61972427368164 s11.74756145477295,8.068588256835938 19.722183227539062,8.068588256835938 h274.3955993652344 c7.9746222496032715,0 14.577266693115234,-2.6895294189453125 19.722183227539062,-8.068588256835938 S623.031982421875,1393.468418598175 623.031982421875,1385.1308941841125 zM623.031982421875,1040.869785785675 V868.7395977973938 c0,-8.337540626525879 -2.5724587440490723,-15.240666389465332 -7.717376232147217,-20.61972427368164 s-11.74756145477295,-8.068588256835938 -19.722183227539062,-8.068588256835938 H321.1979675292969 c-7.9746222496032715,0 -14.577266693115234,2.6895294189453125 -19.722183227539062,8.068588256835938 s-7.717376232147217,12.282183647155762 -7.717376232147217,20.61972427368164 v172.1298828125 c0,8.337540626525879 2.5724587440490723,15.240666389465332 7.717376232147217,20.61972427368164 s11.74756145477295,8.068588256835938 19.722183227539062,8.068588256835938 h274.3955993652344 c7.9746222496032715,0 14.577266693115234,-2.6895294189453125 19.722183227539062,-8.068588256835938 S623.031982421875,1049.2075543403625 623.031982421875,1040.869785785675 zM1062.065673828125,1385.1308941841125 v-172.1298828125 c0,-8.337540626525879 -2.5724587440490723,-15.240666389465332 -7.717376232147217,-20.61972427368164 s-11.74756145477295,-8.068588256835938 -19.722183227539062,-8.068588256835938 H760.2296752929688 c-7.9746222496032715,0 -14.577266693115234,2.6895294189453125 -19.722183227539062,8.068588256835938 s-7.717376232147217,12.282183647155762 -7.717376232147217,20.61972427368164 v172.1298828125 c0,8.337540626525879 2.5724587440490723,15.240666389465332 7.717376232147217,20.61972427368164 s11.74756145477295,8.068588256835938 19.722183227539062,8.068588256835938 h274.3955993652344 c7.9746222496032715,0 14.577266693115234,-2.6895294189453125 19.722183227539062,-8.068588256835938 S1062.065673828125,1393.468418598175 1062.065673828125,1385.1308941841125 zM623.031982421875,696.6097149848938 V524.4805035591125 c0,-8.337540626525879 -2.5724587440490723,-15.240666389465332 -7.717376232147217,-20.61972427368164 s-11.74756145477295,-8.068588256835938 -19.722183227539062,-8.068588256835938 H321.1979675292969 c-7.9746222496032715,0 -14.577266693115234,2.6895294189453125 -19.722183227539062,8.068588256835938 s-7.717376232147217,12.282183647155762 -7.717376232147217,20.61972427368164 v172.1298828125 c0,8.337540626525879 2.5724587440490723,15.240666389465332 7.717376232147217,20.61972427368164 s11.74756145477295,8.068588256835938 19.722183227539062,8.068588256835938 h274.3955993652344 c7.9746222496032715,0 14.577266693115234,-2.6895294189453125 19.722183227539062,-8.068588256835938 S623.031982421875,704.9472393989563 623.031982421875,696.6097149848938 zM1062.065673828125,1040.869785785675 V868.7395977973938 c0,-8.337540626525879 -2.5724587440490723,-15.240666389465332 -7.717376232147217,-20.61972427368164 s-11.74756145477295,-8.068588256835938 -19.722183227539062,-8.068588256835938 H760.2296752929688 c-7.9746222496032715,0 -14.577266693115234,2.6895294189453125 -19.722183227539062,8.068588256835938 s-7.717376232147217,12.282183647155762 -7.717376232147217,20.61972427368164 v172.1298828125 c0,8.337540626525879 2.5724587440490723,15.240666389465332 7.717376232147217,20.61972427368164 s11.74756145477295,8.068588256835938 19.722183227539062,8.068588256835938 h274.3955993652344 c7.9746222496032715,0 14.577266693115234,-2.6895294189453125 19.722183227539062,-8.068588256835938 S1062.065673828125,1049.2075543403625 1062.065673828125,1040.869785785675 zM1501.100341796875,1385.1308941841125 v-172.1298828125 c0,-8.337540626525879 -2.5724587440490723,-15.240666389465332 -7.717376232147217,-20.61972427368164 s-11.74756145477295,-8.068588256835938 -19.722183227539062,-8.068588256835938 h-274.3955993652344 c-7.9746222496032715,0 -14.577266693115234,2.6895294189453125 -19.722183227539062,8.068588256835938 s-7.717376232147217,12.282183647155762 -7.717376232147217,20.61972427368164 v172.1298828125 c0,8.337540626525879 2.5724587440490723,15.240666389465332 7.717376232147217,20.61972427368164 s11.74756145477295,8.068588256835938 19.722183227539062,8.068588256835938 h274.3955993652344 c7.9746222496032715,0 14.577266693115234,-2.6895294189453125 19.722183227539062,-8.068588256835938 S1501.100341796875,1393.468418598175 1501.100341796875,1385.1308941841125 zM1062.065673828125,696.6097149848938 V524.4805035591125 c0,-8.337540626525879 -2.5724587440490723,-15.240666389465332 -7.717376232147217,-20.61972427368164 s-11.74756145477295,-8.068588256835938 -19.722183227539062,-8.068588256835938 H760.2296752929688 c-7.9746222496032715,0 -14.577266693115234,2.6895294189453125 -19.722183227539062,8.068588256835938 s-7.717376232147217,12.282183647155762 -7.717376232147217,20.61972427368164 v172.1298828125 c0,8.337540626525879 2.5724587440490723,15.240666389465332 7.717376232147217,20.61972427368164 s11.74756145477295,8.068588256835938 19.722183227539062,8.068588256835938 h274.3955993652344 c7.9746222496032715,0 14.577266693115234,-2.6895294189453125 19.722183227539062,-8.068588256835938 S1062.065673828125,704.9472393989563 1062.065673828125,696.6097149848938 zM1501.100341796875,1040.869785785675 V868.7395977973938 c0,-8.337540626525879 -2.5724587440490723,-15.240666389465332 -7.717376232147217,-20.61972427368164 s-11.74756145477295,-8.068588256835938 -19.722183227539062,-8.068588256835938 h-274.3955993652344 c-7.9746222496032715,0 -14.577266693115234,2.6895294189453125 -19.722183227539062,8.068588256835938 s-7.717376232147217,12.282183647155762 -7.717376232147217,20.61972427368164 v172.1298828125 c0,8.337540626525879 2.5724587440490723,15.240666389465332 7.717376232147217,20.61972427368164 s11.74756145477295,8.068588256835938 19.722183227539062,8.068588256835938 h274.3955993652344 c7.9746222496032715,0 14.577266693115234,-2.6895294189453125 19.722183227539062,-8.068588256835938 S1501.100341796875,1049.2075543403625 1501.100341796875,1040.869785785675 zM1501.100341796875,696.6097149848938 V524.4805035591125 c0,-8.337540626525879 -2.5724587440490723,-15.240666389465332 -7.717376232147217,-20.61972427368164 s-11.74756145477295,-8.068588256835938 -19.722183227539062,-8.068588256835938 h-274.3955993652344 c-7.9746222496032715,0 -14.577266693115234,2.6895294189453125 -19.722183227539062,8.068588256835938 s-7.717376232147217,12.282183647155762 -7.717376232147217,20.61972427368164 v172.1298828125 c0,8.337540626525879 2.5724587440490723,15.240666389465332 7.717376232147217,20.61972427368164 s11.74756145477295,8.068588256835938 19.722183227539062,8.068588256835938 h274.3955993652344 c7.9746222496032715,0 14.577266693115234,-2.6895294189453125 19.722183227539062,-8.068588256835938 S1501.100341796875,704.9472393989563 1501.100341796875,696.6097149848938 zM1610.858642578125,409.7273907661438 v975.4026489257812 c0,39.44643020629883 -13.462533950805664,73.24484252929688 -40.30185317993164,101.30560302734375 s-59.16654968261719,42.13595962524414 -96.89594268798828,42.13595962524414 H321.1979675292969 c-37.729393005371094,0 -70.05662536621094,-14.075202941894531 -96.89594268798828,-42.13595962524414 s-40.30185317993164,-61.85917282104492 -40.30185317993164,-101.30560302734375 V409.7273907661438 c0,-39.44643020629883 13.462533950805664,-73.24484252929688 40.30185317993164,-101.30560302734375 s59.16654968261719,-42.13595962524414 96.89594268798828,-42.13595962524414 h1152.4615478515625 c37.729393005371094,0 70.05662536621094,14.075202941894531 96.89594268798828,42.13595962524414 S1610.858642578125,370.2809796333313 1610.858642578125,409.7273907661438 z" id="svg_1" class=""/><g id="svg_2" class="selected">
+	<path d="M901.1859393119812,1303.7109375 c-22.93654441833496,0 -43.534461975097656,-8.121683120727539 -59.54507064819336,-23.587440490722656 l-292.5084533691406,-281.4076843261719 c-16.55029296875,-15.12015438079834 -25.27517318725586,-34.99235534667969 -25.27517318725586,-57.45659255981445 c0,-22.291427612304688 8.365094184875488,-41.645225524902344 24.915388107299805,-57.542991638183594 l33.19053268432617,-32.313934326171875 c0.17989447712898254,-0.17280176281929016 0.44973617792129517,-0.432004451751709 0.6296306252479553,-0.6048062443733215 c16.640239715576172,-15.12015438079834 37.14820861816406,-23.06903648376465 59.45512390136719,-23.06903648376465 c22.93654441833496,0 43.534461975097656,8.121683120727539 59.54507064819336,23.587440490722656 l86.16944885253906,82.77205657958984 V692.3363647460938 c0,-21.68662452697754 8.634936332702637,-41.4724235534668 25.005332946777344,-57.197391510009766 c16.370399475097656,-15.724961280822754 36.968318939208984,-24.019445419311523 59.54507064819336,-24.019445419311523 h57.56623077392578 c22.576757431030273,0 43.17467498779297,8.294486045837402 59.54507064819336,24.019445419311523 c16.370399475097656,15.724961280822754 25.005332946777344,35.51075744628906 25.005332946777344,57.197391510009766 v241.5768585205078 l86.16944885253906,-82.77205657958984 c16.100557327270508,-15.465757369995117 36.60852813720703,-23.587440490722656 59.54507064819336,-23.587440490722656 c22.306913375854492,0 42.90483474731445,7.94888162612915 59.45512390136719,23.06903648376465 c0.17989447712898254,0.17280176281929016 0.3597889542579651,0.3456035256385803 0.5396835207939148,0.5184053778648376 l33.73021697998047,32.400333404541016 c0.17989447712898254,0.17280176281929016 0.3597889542579651,0.3456035256385803 0.5396835207939148,0.5184053778648376 c15.740767478942871,15.984163284301758 24.015911102294922,35.68356704711914 24.015911102294922,57.11098098754883 c0,22.032228469848633 -8.455039978027344,41.818031311035156 -24.55559539794922,57.197391510009766 l-292.7782287597656,281.66693115234375 c-0.17989447712898254,0.17280176281929016 -0.3597889542579651,0.3456035256385803 -0.5396835207939148,0.5184053778648376 C944.0908465385437,1295.67578125 923.4930682182312,1303.7109375 901.1859393119812,1303.7109375 z" id="svg_3"/>
+	<path class="st0" d="M929.9692034721375,637.0398559570312 c15.560872077941895,0 29.052959442138672,5.44325590133667 40.47625732421875,16.416168212890625 s17.089975357055664,23.933048248291016 17.089975357055664,38.880401611328125 v304.1311340332031 l132.22242736816406,-127.0093002319336 c11.063511848449707,-10.627310752868652 24.55559539794922,-15.984163284301758 40.47625732421875,-15.984163284301758 c15.560872077941895,0 29.23285484313965,5.356855392456055 40.92599868774414,15.984163284301758 l33.73021697998047,32.400333404541016 c11.063511848449707,11.2321138381958 16.640239715576172,24.36505126953125 16.640239715576172,39.31240463256836 c0,15.292959213256836 -5.576728343963623,28.253089904785156 -16.640239715576172,38.880401611328125 l-292.7782287597656,281.66693115234375 c-11.69313907623291,10.627310752868652 -25.365121841430664,15.984163284301758 -40.92599868774414,15.984163284301758 c-15.920661926269531,0 -29.412750244140625,-5.356855392456055 -40.47625732421875,-15.984163284301758 l-292.7782287597656,-281.66693115234375 c-11.423300743103027,-10.3681058883667 -17.089975357055664,-23.32823944091797 -17.089975357055664,-38.880401611328125 c0,-15.292959213256836 5.666676044464111,-28.339488983154297 17.089975357055664,-39.31240463256836 l33.280479431152344,-32.400333404541016 c11.69313907623291,-10.627310752868652 25.365121841430664,-15.984163284301758 40.92599868774414,-15.984163284301758 c15.920661926269531,0 29.412750244140625,5.356855392456055 40.47625732421875,15.984163284301758 l132.22242736816406,127.0093002319336 v-304.1311340332031 c0,-14.94735336303711 5.666676044464111,-27.907485961914062 17.089975357055664,-38.880401611328125 s24.915388107299805,-16.416168212890625 40.47625732421875,-16.416168212890625 H929.9692034721375 M929.9692034721375,585.1992797851562 h-57.56623077392578 c-29.95243263244629,0 -57.11650085449219,10.88651180267334 -78.6138916015625,31.622722625732422 C772.2016863822937,637.5581665039062 760.8683733940125,663.6512451171875 760.8683733940125,692.3363647460938 v178.9362335205078 l-40.11647033691406,-38.534793853759766 c-21.227550506591797,-20.390609741210938 -48.39161682128906,-31.190719604492188 -78.6138916015625,-31.190719604492188 c-29.32280158996582,0 -56.306968688964844,10.454507827758789 -77.98426055908203,30.153913497924805 c-0.44973617792129517,0.432004451751709 -0.8994723558425903,0.864008903503418 -1.3492085933685303,1.209612488746643 l-33.19053268432617,32.313934326171875 c-21.407445907592773,20.649812698364258 -32.74079895019531,46.82927703857422 -32.74079895019531,75.85997772216797 c0,29.63550567626953 11.513246536254883,55.814971923828125 33.370426177978516,75.94638061523438 l292.3285217285156,281.23486328125 c21.227550506591797,20.390609741210938 48.4815673828125,31.190719604492188 78.7038345336914,31.190719604492188 c29.32280158996582,0 56.306968688964844,-10.454507827758789 77.98426055908203,-30.153913497924805 c0.3597889542579651,-0.3456035256385803 0.7195779085159302,-0.6912070512771606 1.0793670415878296,-1.0368107557296753 l292.7782287597656,-281.66693115234375 c21.227550506591797,-20.390609741210938 32.38100051879883,-46.483673095703125 32.38100051879883,-75.5143814086914 c0,-28.166688919067383 -10.883615493774414,-54.08695602416992 -31.39158821105957,-74.9095687866211 c-0.3597889542579651,-0.3456035256385803 -0.7195779085159302,-0.6912070512771606 -1.0793670415878296,-1.0368107557296753 l-33.73021697998047,-32.400333404541016 c-0.3597889542579651,-0.3456035256385803 -0.7195779085159302,-0.6912070512771606 -1.0793670415878296,-1.0368107557296753 c-21.67728614807129,-19.69940185546875 -48.66145706176758,-30.153913497924805 -77.98426055908203,-30.153913497924805 c-30.222272872924805,0 -57.38634490966797,10.800111770629883 -78.6138916015625,31.190719604492188 l-40.11647033691406,38.534793853759766 V692.3363647460938 c0,-28.771495819091797 -11.333352088928223,-54.86456298828125 -32.92068862915039,-75.5143814086914 C987.0856585502625,596.1721801757812 959.9216570854187,585.1992797851562 929.9692034721375,585.1992797851562 L929.9692034721375,585.1992797851562 z" id="svg_4"/>
+</g></g></svg>
\ No newline at end of file
diff --git a/web/pgadmin/tools/sqleditor/static/js/sqleditor.js b/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
index e2d69f78..03e39889 100644
--- a/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
+++ b/web/pgadmin/tools/sqleditor/static/js/sqleditor.js
@@ -85,8 +85,8 @@ define('tools.querytool', [
     // Bind all the events
     events: {
       'click .btn-load-file': 'on_file_load',
-      'click #btn-save': 'on_save',
-      'click #btn-file-menu-save': 'on_save',
+      'click #btn-save-file': 'on_save_file',
+      'click #btn-file-menu-save': 'on_save_file',
       'click #btn-file-menu-save-as': 'on_save_as',
       'click #btn-find': 'on_find',
       'click #btn-find-menu-find': 'on_find',
@@ -97,6 +97,7 @@ define('tools.querytool', [
       'click #btn-find-menu-find-persistent': 'on_find_persistent',
       'click #btn-find-menu-jump': 'on_jump',
       'click #btn-delete-row': 'on_delete',
+      'click #btn-save-data': 'on_save_data',
       'click #btn-filter': 'on_show_filter',
       'click #btn-filter-menu': 'on_show_filter',
       'click #btn-include-filter': 'on_include_filter',
@@ -351,26 +352,7 @@ define('tools.querytool', [
         _.each(window.top.pgAdmin.Browser.docker.findPanels('frm_datagrid'), function(p) {
           if (p.isVisible()) {
             p.on(wcDocker.EVENT.CLOSING, function() {
-              // Only if we can edit data then perform this check
-              var notify = false,
-                msg;
-              if (self.handler.can_edit
-                  && self.preferences.prompt_save_data_changes) {
-                var data_store = self.handler.data_store;
-                if (data_store && (_.size(data_store.added) ||
-                    _.size(data_store.updated))) {
-                  msg = gettext('The data has changed. Do you want to save changes?');
-                  notify = true;
-                }
-              } else if (self.handler.is_query_tool && self.handler.is_query_changed
-                         && self.preferences.prompt_save_query_changes) {
-                msg = gettext('The text has changed. Do you want to save changes?');
-                notify = true;
-              }
-              if (notify) {
-                return self.user_confirmation(p, msg);
-              }
-              return true;
+              return self.handler.check_needed_confirmations_before_closing_panel(true);
             });
 
             // Set focus on query tool of active panel
@@ -615,62 +597,6 @@ define('tools.querytool', [
       }
     },
 
-    /* To prompt user for unsaved changes */
-    user_confirmation: function(panel, msg) {
-      // If there is anything to save then prompt user
-      var that = this;
-
-      alertify.confirmSave || alertify.dialog('confirmSave', function() {
-        return {
-          main: function(title, message) {
-            this.setHeader(title);
-            this.setContent(message);
-          },
-          setup: function() {
-            return {
-              buttons: [{
-                text: gettext('Cancel'),
-                key: 27, // ESC
-                invokeOnClose: true,
-                className: 'btn btn-secondary fa fa-lg fa-times pg-alertify-button',
-              }, {
-                text: gettext('Don\'t save'),
-                className: 'btn btn-secondary fa fa-lg fa-trash-o pg-alertify-button',
-              }, {
-                text: gettext('Save'),
-                className: 'btn btn-primary fa fa-lg fa-save pg-alertify-button',
-              }],
-              focus: {
-                element: 0,
-                select: false,
-              },
-              options: {
-                maximizable: false,
-                resizable: false,
-              },
-            };
-          },
-          callback: function(closeEvent) {
-            switch (closeEvent.index) {
-            case 0: // Cancel
-              //Do nothing.
-              break;
-            case 1: // Don't Save
-              that.handler.close_on_save = false;
-              that.handler.close();
-              break;
-            case 2: //Save
-              that.handler.close_on_save = true;
-              that.handler._save(that, that.handler);
-              break;
-            }
-          },
-        };
-      });
-      alertify.confirmSave(gettext('Save changes?'), msg);
-      return false;
-    },
-
     /* Regarding SlickGrid usage in render_grid function.
 
      SlickGrid Plugins:
@@ -734,6 +660,8 @@ define('tools.querytool', [
     render_grid: function(collection, columns, is_editable, client_primary_key, rows_affected) {
       var self = this;
 
+      self.handler.numberOfModifiedCells = 0;
+
       // This will work as data store and holds all the
       // inserted/updated/deleted data from grid
       self.handler.data_store = {
@@ -1067,6 +995,14 @@ define('tools.querytool', [
           _pk = args.item[self.client_primary_key] || null, // Unique key to identify row
           column_data = {};
 
+        // Highlight the changed cell
+        self.handler.numberOfModifiedCells++;
+        args.grid.addCellCssStyles(self.handler.numberOfModifiedCells, {
+          [args.row] : {
+            [changed_column]: 'highlighted_grid_cells',
+          },
+        });
+
         // Access to row/cell value after a cell is changed.
         // The purpose is to remove row_id from temp_new_row
         // if new row has primary key instead of [default_value]
@@ -1122,7 +1058,7 @@ define('tools.querytool', [
           }
         }
         // Enable save button
-        $('#btn-save').prop('disabled', false);
+        $('#btn-save-data').prop('disabled', false);
       }.bind(editor_data));
 
       // Listener function which will be called when user adds new rows
@@ -1150,6 +1086,7 @@ define('tools.querytool', [
           'data': item,
         };
         self.handler.data_store.added_index[data_length] = _key;
+
         // Fetch data type & add it for the column
         var temp = {};
         temp[column.name] = _.where(this.columns, {
@@ -1158,8 +1095,17 @@ define('tools.querytool', [
         grid.updateRowCount();
         grid.render();
 
+        // Highlight the first added cell of the new row
+        var row = dataView.getRowByItem(item);
+        self.handler.numberOfModifiedCells++;
+        args.grid.addCellCssStyles(self.handler.numberOfModifiedCells, {
+          [row] : {
+            [column.field]: 'highlighted_grid_cells',
+          },
+        });
+
         // Enable save button
-        $('#btn-save').prop('disabled', false);
+        $('#btn-save-data').prop('disabled', false);
       }.bind(editor_data));
 
       // Listen grid viewportChanged event to load next chunk of data.
@@ -1208,9 +1154,11 @@ define('tools.querytool', [
       }
       dataView.setItems(collection, self.client_primary_key);
     },
+
     fetch_next_all: function(cb) {
       this.fetch_next(true, cb);
     },
+
     fetch_next: function(fetch_all, cb) {
       var self = this,
         url = '';
@@ -1394,6 +1342,18 @@ define('tools.querytool', [
       );
     },
 
+    // Callback function for Save Data Changes button click.
+    on_save_data: function() {
+      var self = this;
+
+      self.handler.close_on_save = false;
+      // Trigger the save_data signal to the SqlEditorController class
+      self.handler.trigger(
+        'pgadmin-sqleditor:button:save_data',
+        self
+      );
+    },
+
     _stopEventPropogation: function(ev) {
       ev = ev || window.event;
       ev.cancelBubble = true;
@@ -1410,7 +1370,7 @@ define('tools.querytool', [
     },
 
     // Callback function for Save button click.
-    on_save: function(ev) {
+    on_save_file: function(ev) {
       var self = this;
 
       this._stopEventPropogation(ev);
@@ -1419,9 +1379,7 @@ define('tools.querytool', [
       self.handler.close_on_save = false;
       // Trigger the save signal to the SqlEditorController class
       self.handler.trigger(
-        'pgadmin-sqleditor:button:save',
-        self,
-        self.handler
+        'pgadmin-sqleditor:button:save_file'
       );
     },
 
@@ -1435,7 +1393,7 @@ define('tools.querytool', [
       self.handler.close_on_save = false;
       // Trigger the save signal to the SqlEditorController class
       self.handler.trigger(
-        'pgadmin-sqleditor:button:save',
+        'pgadmin-sqleditor:button:save_file',
         self,
         self.handler,
         true
@@ -2194,8 +2152,9 @@ define('tools.querytool', [
 
         // Listen on events come from SQLEditorView for the button clicked.
         self.on('pgadmin-sqleditor:button:load_file', self._load_file, self);
-        self.on('pgadmin-sqleditor:button:save', self._save, self);
+        self.on('pgadmin-sqleditor:button:save_file', self._save_file, self);
         self.on('pgadmin-sqleditor:button:deleterow', self._delete, self);
+        self.on('pgadmin-sqleditor:button:save_data', self._save_data, self);
         self.on('pgadmin-sqleditor:button:show_filter', self._show_filter, self);
         self.on('pgadmin-sqleditor:button:include_filter', self._include_filter, self);
         self.on('pgadmin-sqleditor:button:exclude_filter', self._exclude_filter, self);
@@ -2309,8 +2268,7 @@ define('tools.querytool', [
                 $('#btn-filter').addClass('btn-secondary');
                 $('#btn-filter-dropdown').addClass('btn-secondary');
               }
-              $('#btn-save').prop('disabled', true);
-              $('#btn-file-menu-dropdown').prop('disabled', true);
+
               $('#btn-copy-row').prop('disabled', true);
               $('#btn-paste-row').prop('disabled', true);
 
@@ -2376,6 +2334,18 @@ define('tools.querytool', [
         else
           self.can_edit = true;
 
@@ -2386,13 +2356,12 @@ define('tools.querytool', [
           $('#btn-filter-dropdown').prop('disabled', false);
         }
 
+        // No data to save initially
+        $('#btn-save-data').prop('disabled', true);
+
         // Initial settings for delete row, copy row and paste row buttons.
         $('#btn-delete-row').prop('disabled', true);
-        // Do not disable save button in query tool
-        if (!self.is_query_tool && !self.can_edit) {
-          $('#btn-save').prop('disabled', true);
-          $('#btn-file-menu-dropdown').prop('disabled', true);
-        }
+
         if (!self.can_edit) {
           $('#btn-delete-row').prop('disabled', true);
           $('#btn-copy-row').prop('disabled', true);
@@ -2776,9 +2745,9 @@ define('tools.querytool', [
           if (_.size(self.data_store.added) || is_updated) {
             // Do not disable save button if there are
             // any other changes present in grid data
-            $('#btn-save').prop('disabled', false);
+            $('#btn-save-data').prop('disabled', false);
           } else {
-            $('#btn-save').prop('disabled', true);
+            $('#btn-save-data').prop('disabled', true);
           }
           alertify.success(gettext('Row(s) deleted.'));
         } else {
@@ -2807,41 +2776,39 @@ define('tools.querytool', [
           if (_.size(self.data_store.added) || is_updated || _.size(self.data_store.deleted)) {
             // Do not disable save button if there are
             // any other changes present in grid data
-            $('#btn-save').prop('disabled', false);
+            $('#btn-save-data').prop('disabled', false);
           } else {
-            $('#btn-save').prop('disabled', true);
+            $('#btn-save-data').prop('disabled', true);
           }
         }
       },
 
+      /// This function will open save file dialog conditionally.
+
+      _save_file: function(save_as=false) {
+        var self = this;
+
+        var current_file = self.gridView.current_file;
+        if (!_.isUndefined(current_file) && !save_as) {
+          self._save_file_handler(current_file);
+        } else {
+          // provide custom option to save file dialog
+          var params = {
+            'supported_types': ['*', 'sql'],
+            'dialog_type': 'create_file',
+            'dialog_title': 'Save File',
+            'btn_primary': 'Save',
+          };
+          pgAdmin.FileManager.init();
+          pgAdmin.FileManager.show_dialog(params);
+        }
+      },
+
       /* This function will fetch the list of changed models and make
        * the ajax call to save the data into the database server.
-       * and will open save file dialog conditionally.
        */
-      _save: function(view, controller, save_as) {
-        var self = this,
-          save_data = true;
-
-        // Open save file dialog if query tool
-        if (self.is_query_tool) {
-          var current_file = self.gridView.current_file;
-          if (!_.isUndefined(current_file) && !save_as) {
-            self._save_file_handler(current_file);
-          } else {
-            // provide custom option to save file dialog
-            var params = {
-              'supported_types': ['*', 'sql'],
-              'dialog_type': 'create_file',
-              'dialog_title': 'Save File',
-              'btn_primary': 'Save',
-            };
-            pgAdmin.FileManager.init();
-            pgAdmin.FileManager.show_dialog(params);
-          }
-          return;
-        }
-        $('#btn-save').prop('disabled', true);
-        $('#btn-file-menu-dropdown').prop('disabled', true);
+      _save_data: function(view) {
+        var self = this;
 
         var is_added = _.size(self.data_store.added),
           is_updated = _.size(self.data_store.updated),
@@ -2851,154 +2818,179 @@ define('tools.querytool', [
           return; // Nothing to save here
         }
 
-        if (save_data) {
+        self.trigger(
+          'pgadmin-sqleditor:loading-icon:show',
+          gettext('Saving the updated data...')
+        );
 
-          self.trigger(
-            'pgadmin-sqleditor:loading-icon:show',
-            gettext('Saving the updated data...')
-          );
+        // Add the columns to the data so the server can remap the data
+        var req_data = self.data_store;
+        req_data.columns = view ? view.handler.columns : self.columns;
 
-          // Add the columns to the data so the server can remap the data
-          var req_data = self.data_store;
-          req_data.columns = view ? view.handler.columns : self.columns;
+        var save_successful = false;
 
-          // Make ajax call to save the data
-          $.ajax({
-            url: url_for('sqleditor.save', {
-              'trans_id': self.transId,
-            }),
-            method: 'POST',
-            contentType: 'application/json',
-            data: JSON.stringify(req_data),
-          })
-            .done(function(res) {
-              var grid = self.slickgrid,
-                dataView = grid.getData(),
-                data_length = dataView.getLength(),
-                data = [];
-
-              if (res.data.status) {
-                if(is_added) {
-                // Update the rows in a grid after addition
-                  dataView.beginUpdate();
-                  _.each(res.data.query_result, function(r) {
-                    if (!_.isNull(r.row_added)) {
-                    // Fetch temp_id returned by server after addition
-                      var row_id = Object.keys(r.row_added)[0];
-                      _.each(req_data.added_index, function(v, k) {
-                        if (v == row_id) {
-                        // Fetch item data through row index
-                          var item = grid.getDataItem(k);
-                          _.extend(item, r.row_added[row_id]);
-                        }
-                      });
-                    }
-                  });
-                  dataView.endUpdate();
-                }
-                // Remove flag is_row_copied from copied rows
-                _.each(data, function(row) {
-                  if (row.is_row_copied) {
-                    delete row.is_row_copied;
+        // Make ajax call to save the data
+        $.ajax({
+          url: url_for('sqleditor.save', {
+            'trans_id': self.transId,
+          }),
+          method: 'POST',
+          contentType: 'application/json',
+          data: JSON.stringify(req_data),
+        })
+          .done(function(res) {
+            var grid = self.slickgrid,
+              dataView = grid.getData(),
+              data_length = dataView.getLength(),
+              data = [];
+
+            if (res.data.status) {
+              // Disable Save Data Changes button
+              $('#btn-save-data').prop('disabled', true);
+
+              save_successful = true;
+
+              // Remove highlighted cells styling
+              for (let i = 1; i <= self.numberOfModifiedCells; i++)
+                grid.removeCellCssStyles(i);
+
+              self.numberOfModifiedCells = 0;
+
+              if(is_added) {
+              // Update the rows in a grid after addition
+                dataView.beginUpdate();
+                _.each(res.data.query_result, function(r) {
+                  if (!_.isNull(r.row_added)) {
+                  // Fetch temp_id returned by server after addition
+                    var row_id = Object.keys(r.row_added)[0];
+                    _.each(req_data.added_index, function(v, k) {
+                      if (v == row_id) {
+                      // Fetch item data through row index
+                        var item = grid.getDataItem(k);
+                        _.extend(item, r.row_added[row_id]);
+                      }
+                    });
                   }
                 });
-
-                // Remove 2d copied_rows array
-                if (grid.copied_rows) {
-                  delete grid.copied_rows;
+                dataView.endUpdate();
+              }
+              // Remove flag is_row_copied from copied rows
+              _.each(data, function(row) {
+                if (row.is_row_copied) {
+                  delete row.is_row_copied;
                 }
+              });
+
+              // Remove 2d copied_rows array
+              if (grid.copied_rows) {
+                delete grid.copied_rows;
+              }
 
-                // Remove deleted rows from client as well
-                if (is_deleted) {
-                  var rows = _.keys(self.data_store.deleted);
-                  if (data_length == rows.length) {
-                  // This means all the rows are selected, clear all data
-                    data = [];
-                    dataView.setItems(data, self.client_primary_key);
-                  } else {
-                    dataView.beginUpdate();
-                    for (var i = 0; i < rows.length; i++) {
-                      var item = grid.getDataItem(rows[i]);
-                      data.push(item);
-                      dataView.deleteItem(item[self.client_primary_key]);
-                    }
-                    dataView.endUpdate();
+              // Remove deleted rows from client as well
+              if (is_deleted) {
+                var rows = _.keys(self.data_store.deleted);
+                if (data_length == rows.length) {
+                // This means all the rows are selected, clear all data
+                  data = [];
+                  dataView.setItems(data, self.client_primary_key);
+                } else {
+                  dataView.beginUpdate();
+                  for (var i = 0; i < rows.length; i++) {
+                    var item = grid.getDataItem(rows[i]);
+                    data.push(item);
+                    dataView.deleteItem(item[self.client_primary_key]);
                   }
-                  self.rows_to_delete.apply(self, [data]);
-                  grid.setSelectedRows([]);
+                  dataView.endUpdate();
                 }
-
+                self.rows_to_delete.apply(self, [data]);
                 grid.setSelectedRows([]);
+              }
 
-                // Reset data store
-                self.data_store = {
-                  'added': {},
-                  'updated': {},
-                  'deleted': {},
-                  'added_index': {},
-                  'updated_index': {},
-                };
+              grid.setSelectedRows([]);
 
-                // Reset old primary key data now
-                self.primary_keys_data = {};
+              // Reset data store
+              self.data_store = {
+                'added': {},
+                'updated': {},
+                'deleted': {},
+                'added_index': {},
+                'updated_index': {},
+              };
 
-                // Clear msgs after successful save
-                self.set_sql_message('');
+              // Reset old primary key data now
+              self.primary_keys_data = {};
 
-                alertify.success(gettext('Data saved successfully.'));
-              } else {
-              // Something went wrong while saving data on the db server
-                $('#btn-flash').prop('disabled', false);
-                $('#btn-download').prop('disabled', false);
-                self.set_sql_message(res.data.result);
-                var err_msg = S(gettext('%s.')).sprintf(res.data.result).value();
-                alertify.error(err_msg, 20);
-                grid.setSelectedRows([]);
-                // To highlight the row at fault
-                if (_.has(res.data, '_rowid') &&
-                (!_.isUndefined(res.data._rowid) || !_.isNull(res.data._rowid))) {
-                  var _row_index = self._find_rowindex(res.data._rowid);
-                  if (_row_index in self.data_store.added_index) {
-                  // Remove new row index from temp_list if save operation
-                  // fails
-                    var index = self.handler.temp_new_rows.indexOf(res.data._rowid);
-                    if (index > -1) {
-                      self.handler.temp_new_rows.splice(index, 1);
-                    }
-                    self.data_store.added[self.data_store.added_index[_row_index]].err = true;
-                  } else if (_row_index in self.data_store.updated_index) {
-                    self.data_store.updated[self.data_store.updated_index[_row_index]].err = true;
+              // Clear msgs after successful save
+              self.set_sql_message('');
+
+              var msg, is_commit_required = _.has(res.data, 'is_commit_required') &&
+                                            !_.isNull(res.data.is_commit_required) &&
+                                            res.data.is_commit_required;
+
+              if(is_commit_required) {
+                msg = gettext('Data saved successfully, you still need to commit changes to the database.');
+                self.disable_transaction_buttons(false);
+              }
+              else
+                msg = gettext('Data saved successfully.');
+
+              alertify.success(msg);
+            } else {
+            // Something went wrong while saving data on the db server
+              self.set_sql_message(res.data.result);
+              var err_msg = S(gettext('%s.')).sprintf(res.data.result).value();
+              alertify.error(err_msg, 20);
+              grid.setSelectedRows([]);
+              // To highlight the row at fault
+              if (_.has(res.data, '_rowid') &&
+              (!_.isUndefined(res.data._rowid) || !_.isNull(res.data._rowid))) {
+                var _row_index = self._find_rowindex(res.data._rowid);
+                if (_row_index in self.data_store.added_index) {
+                // Remove new row index from temp_list if save operation
+                // fails
+                  var index = self.handler.temp_new_rows.indexOf(res.data._rowid);
+                  if (index > -1) {
+                    self.handler.temp_new_rows.splice(index, 1);
                   }
+                  self.data_store.added[self.data_store.added_index[_row_index]].err = true;
+                } else if (_row_index in self.data_store.updated_index) {
+                  self.data_store.updated[self.data_store.updated_index[_row_index]].err = true;
                 }
-                grid.gotoCell(_row_index, 1);
               }
+              grid.gotoCell(_row_index, 1);
+            }
 
-              // Update the sql results in history tab
-              _.each(res.data.query_result, function(r) {
-                self.gridView.history_collection.add({
-                  'status': r.status,
-                  'start_time': self.query_start_time,
-                  'query': r.sql,
-                  'row_affected': r.rows_affected,
-                  'total_time': self.total_time,
-                  'message': r.result,
-                });
+            // Update the sql results in history tab
+            _.each(res.data.query_result, function(r) {
+              self.gridView.history_collection.add({
+                'status': r.status,
+                'start_time': self.query_start_time,
+                'query': r.sql,
+                'row_affected': r.rows_affected,
+                'total_time': self.total_time,
+                'message': r.result,
               });
-              self.trigger('pgadmin-sqleditor:loading-icon:hide');
+            });
+            self.trigger('pgadmin-sqleditor:loading-icon:hide');
 
-              grid.invalidate();
-              if (self.close_on_save) {
-                self.close();
+            grid.invalidate();
+            if (self.close_on_save) {
+              if(save_successful) {
+                // Check for any other needed confirmations before closing
+                self.check_needed_confirmations_before_closing_panel();
               }
-            })
-            .fail(function(e) {
-              let stateParams = [view, controller, save_as];
-              let msg = httpErrorHandler.handleQueryToolAjaxError(
-                pgAdmin, self, e, '_save', stateParams, true
-              );
-              self.update_msg_history(false, msg);
-            });
-        }
+              else {
+                self.close_on_save = false;
+              }
+            }
+          })
+          .fail(function(e) {
+            let stateParams = [view];
+            let msg = httpErrorHandler.handleQueryToolAjaxError(
+              pgAdmin, self, e, '_savdata', stateParams, true
+            );
+            self.update_msg_history(false, msg);
+          });
       },
 
       // Find index of row at fault from grid data
@@ -3042,7 +3034,7 @@ define('tools.querytool', [
 
       // Save as
       _save_as: function() {
-        return this._save(true);
+        return this._save_file(true);
       },
 
       // Set panel title.
@@ -3132,7 +3124,7 @@ define('tools.querytool', [
             $busy_icon_div.removeClass('show_progress');
 
             // disable save button on file save
-            $('#btn-save').prop('disabled', true);
+            $('#btn-save-file').prop('disabled', true);
             $('#btn-file-menu-save').css('display', 'none');
 
             // Update the flag as new content is just loaded.
@@ -3177,7 +3169,7 @@ define('tools.querytool', [
               self.gridView.current_file = e;
               self.setTitle(self.gridView.current_file.replace(/^.*[\\\/]/g, ''), true);
               // disable save button on file save
-              $('#btn-save').prop('disabled', true);
+              $('#btn-save-file').prop('disabled', true);
               $('#btn-file-menu-save').css('display', 'none');
 
               // Update the flag as query is already saved.
@@ -3186,7 +3178,8 @@ define('tools.querytool', [
             }
             self.trigger('pgadmin-sqleditor:loading-icon:hide');
             if (self.close_on_save) {
-              self.close();
+              // Check for any other needed confirmations before closing
+              self.check_needed_confirmations_before_closing_panel();
             }
           })
           .fail(function(e) {
@@ -3226,7 +3219,7 @@ define('tools.querytool', [
             self.setTitle(title);
           }
 
-          $('#btn-save').prop('disabled', false);
+          $('#btn-save-file').prop('disabled', false);
           $('#btn-file-menu-save').css('display', 'block');
           $('#btn-file-menu-dropdown').prop('disabled', false);
         }
@@ -3460,7 +3453,7 @@ define('tools.querytool', [
         if (copied_rows.length > 0) {
           // Enable save button so that user can
           // save newly pasted rows on server
-          $('#btn-save').prop('disabled', false);
+          $('#btn-save-data').prop('disabled', false);
 
           var arr_to_object = function(arr) {
             var obj = {};
@@ -3563,7 +3556,7 @@ define('tools.querytool', [
         $('#btn-explain-options-dropdown').prop('disabled', mode_disabled);
         $('#btn-edit-dropdown').prop('disabled', mode_disabled);
         $('#btn-load-file').prop('disabled', mode_disabled);
-        $('#btn-save').prop('disabled', mode_disabled);
+        $('#btn-save-file').prop('disabled', mode_disabled);
         $('#btn-file-menu-dropdown').prop('disabled', mode_disabled);
         $('#btn-find').prop('disabled', mode_disabled);
         $('#btn-find-menu-dropdown').prop('disabled', mode_disabled);
@@ -3592,8 +3585,36 @@ define('tools.querytool', [
       // This function will fetch the sql query from the text box
       // and execute the query.
       execute: function(explain_prefix, shouldReconnect=false) {
-        var self = this,
-          sql = '';
+        var self = this;
+
+        // Check if the data grid has any changes before running query
+        // Check if the data grid has any changes before running query
+        if (self.can_edit && _.has(self, 'data_store') &&
+          (_.size(self.data_store.added) ||
+            _.size(self.data_store.updated) ||
+            _.size(self.data_store.deleted))
+        ) {
+          alertify.confirm(gettext('Unsaved changes'),
+            gettext('The data has been modified, but not saved. Are you sure you wish to discard the changes?'),
+            function() {
+              // Do nothing as user do not want to save, just continue
+              self._execute_sql_query(explain_prefix, shouldReconnect);
+            },
+            function() {
+              // Stop, User wants to save
+              return true;
+            }
+          ).set('labels', {
+            ok: gettext('Yes'),
+            cancel: gettext('No'),
+          });
+        } else {
+          self._execute_sql_query(explain_prefix, shouldReconnect);
+        }
+      },
+
+      _execute_sql_query: function(explain_prefix, shouldReconnect) {
+        var self = this, sql = '';
 
         self.has_more_rows = false;
         self.fetching_rows = false;
@@ -3602,8 +3623,8 @@ define('tools.querytool', [
           sql = self.special_sql;
         } else {
           /* If code is selected in the code mirror then execute
-           * the selected part else execute the complete code.
-           */
+          * the selected part else execute the complete code.
+          */
           var selected_code = self.gridView.query_tool_obj.getSelection();
           if (selected_code.length > 0)
             sql = selected_code;
@@ -3998,6 +4019,144 @@ define('tools.querytool', [
         is_query_running = value;
       },
 
+      /* Checks if there is any unsaved data changes, unsaved changes in the query
+      or uncommited transactions before closing a panel */
+      check_needed_confirmations_before_closing_panel: function(is_close_event_call = false) {
+        var self = this, msg;
+
+        /*
+         is_close_event_call = true only when the function is called when the
+         close panel event is triggered, otherwise it is false
+        */
+        if(!self.ignore_on_close || is_close_event_call)
+          self.ignore_on_close = {
+            unsaved_data: false,
+            unsaved_query: false,
+            uncommited_transaction: false,
+          };
+
+        var ignore_unsaved_data = self.ignore_on_close.unsaved_data,
+          ignore_unsaved_query = self.ignore_on_close.unsaved_query,
+          ignore_uncommited_transaction = self.ignore_on_close.uncommited_transaction;
+
+        // If there is unsaved data changes in the grid
+        if (!ignore_unsaved_data && self.can_edit
+            && self.preferences.prompt_save_data_changes &&
+            self.data_store &&
+             (_.size(self.data_store.added) ||
+              _.size(self.data_store.updated) ||
+              _.size(self.data_store.deleted))) {
+          msg = gettext('The data has changed. Do you want to save changes?');
+          self.unsaved_changes_user_confirmation(msg, true);
+        } // If there is unsaved query changes in the query editor
+        else if (!ignore_unsaved_query && self.is_query_tool
+                   && self.is_query_changed
+                   && self.preferences.prompt_save_query_changes) {
+          msg = gettext('The text has changed. Do you want to save changes?');
+          self.unsaved_changes_user_confirmation(msg, false);
+        } // If a transaction is currently ongoing
+        else if (!ignore_uncommited_transaction && SqlEditorUtils.previousStatus != 0) {
+          self.uncommited_transaction_user_confirmation();
+        }
+        else {
+          // No other function should call close() except through this function
+          // in order to perform necessary checks
+          self.ignore_on_close = undefined;
+          self.close();
+        }
+        // Return false so that the panel does not close unless close()
+        // is called explicitly (when all needed prompts are issued).
+        return false;
+      },
+
+      /* To prompt the user for uncommited transaction */
+      uncommited_transaction_user_confirmation: function() {
+        var self = this;
+
+        alertify.confirm(
+          gettext('Uncommited transaction'),
+          gettext(`The current transaction was not commited to the database.
+           Are you sure you wish to discard the current transaction?`),
+          function() {
+            // Go back to check for any other needed confirmations before closing
+            self.ignore_on_close.uncommited_transaction = true;
+            self.check_needed_confirmations_before_closing_panel();
+          },
+          function() {
+            return;
+          }
+        ).set('labels', {
+          ok: gettext('Yes'),
+          cancel: gettext('No'),
+        });
+      },
+
+      /* To prompt user for unsaved changes */
+      unsaved_changes_user_confirmation: function(msg, is_unsaved_data) {
+        // If there is anything to save then prompt user
+        var self = this;
+
+        alertify.confirmSave || alertify.dialog('confirmSave', function() {
+          return {
+            main: function(title, message, is_unsaved_data) {
+              this.is_unsaved_data = is_unsaved_data;
+              this.setHeader(title);
+              this.setContent(message);
+            },
+            setup: function() {
+              return {
+                buttons: [{
+                  text: gettext('Cancel'),
+                  key: 27, // ESC
+                  invokeOnClose: true,
+                  className: 'btn btn-secondary fa fa-lg fa-times pg-alertify-button',
+                }, {
+                  text: gettext('Don\'t save'),
+                  className: 'btn btn-secondary fa fa-lg fa-trash-o pg-alertify-button',
+                }, {
+                  text: gettext('Save'),
+                  className: 'btn btn-primary fa fa-lg fa-save pg-alertify-button',
+                }],
+                focus: {
+                  element: 0,
+                  select: false,
+                },
+                options: {
+                  maximizable: false,
+                  resizable: false,
+                },
+              };
+            },
+            callback: function(closeEvent) {
+              switch (closeEvent.index) {
+              case 0: // Cancel
+                //Do nothing.
+                break;
+              case 1: // Don't Save
+                self.close_on_save = false;
+                if(this.is_unsaved_data)
+                  self.ignore_on_close.unsaved_data = true;
+                else
+                  self.ignore_on_close.unsaved_query = true;
+                // Go back to check for any other needed confirmations before closing
+                self.check_needed_confirmations_before_closing_panel();
+                break;
+              case 2: //Save
+                self.close_on_save = true;
+                if(this.is_unsaved_data) {
+                  self._save_data(self.gridView);
+                }
+                else {
+                  self._save_file();
+                }
+                break;
+              }
+            },
+          };
+        });
+        alertify.confirmSave(gettext('Save changes?'), msg, is_unsaved_data);
+      },
+
       close: function() {
         var self = this;
 
diff --git a/web/pgadmin/tools/sqleditor/static/scss/_sqleditor.scss b/web/pgadmin/tools/sqleditor/static/scss/_sqleditor.scss
index ebf5b180..4b256846 100644
--- a/web/pgadmin/tools/sqleditor/static/scss/_sqleditor.scss
+++ b/web/pgadmin/tools/sqleditor/static/scss/_sqleditor.scss
@@ -209,6 +209,13 @@ li.CodeMirror-hint-active {
   color: $text-muted;
 }
 
+/* Highlighted (modified or new) cell */
+.grid-canvas .highlighted_grid_cells {
+  background: $color-highlighted-grid-cell;
+  font-weight: bold;
+}
+
+
 /* Override selected row color */
 #datagrid .slick-row .slick-cell.selected {
     background-color: $table-bg-selected;
diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/11_plus/primary_keys.sql b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/11_plus/primary_keys.sql
index 1dfb094f..459977e9 100644
--- a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/11_plus/primary_keys.sql
+++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/11_plus/primary_keys.sql
@@ -1,6 +1,6 @@
 {# ============= Fetch the primary keys for given object id ============= #}
 {% if obj_id %}
-SELECT at.attname, ty.typname
+SELECT at.attname, at.attnum, ty.typname
 FROM pg_attribute at LEFT JOIN pg_type ty ON (ty.oid = at.atttypid)
 WHERE attrelid={{obj_id}}::oid AND attnum = ANY (
     (SELECT con.conkey FROM pg_class rel LEFT OUTER JOIN pg_constraint con ON con.conrelid=rel.oid
diff --git a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/primary_keys.sql b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/primary_keys.sql
index 60d0e56f..a96c928f 100644
--- a/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/primary_keys.sql
+++ b/web/pgadmin/tools/sqleditor/templates/sqleditor/sql/default/primary_keys.sql
@@ -1,8 +1,8 @@
 {# ============= Fetch the primary keys for given object id ============= #}
 {% if obj_id %}
-SELECT at.attname, ty.typname
+SELECT at.attname, at.attnum, ty.typname
 FROM pg_attribute at LEFT JOIN pg_type ty ON (ty.oid = at.atttypid)
 WHERE attrelid={{obj_id}}::oid AND attnum = ANY (
     (SELECT con.conkey FROM pg_class rel LEFT OUTER JOIN pg_constraint con ON con.conrelid=rel.oid
     AND con.contype='p' WHERE rel.relkind IN ('r','s','t') AND rel.oid = {{obj_id}}::oid)::oid[])
-{% endif %}
\ No newline at end of file
+{% endif %}
diff --git a/web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py b/web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py
new file mode 100644
index 00000000..a89ecc6a
--- /dev/null
+++ b/web/pgadmin/tools/sqleditor/utils/is_query_resultset_updatable.py
@@ -0,0 +1,97 @@
+##########################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2019, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+
+"""
+    Check if the result-set of a query is updatable, A resultset is
+    updatable (as of this version) if:
+        - All columns belong to the same table.
+        - All the primary key columns of the table are present in the resultset
+        - No duplicate columns
+"""
+from flask import render_template
+try:
+    from collections import OrderedDict
+except ImportError:
+    from ordereddict import OrderedDict
+
+
+def is_query_resultset_updatable(conn, sql_path):
+    """
+        This function is used to check whether the last successful query
+        produced updatable results.
+
+        Args:
+            conn: Connection object.
+            sql_path: the path to the sql templates.
+    """
+    columns_info = conn.get_column_info()
+
+    # Fetch the column info
+    if columns_info is None or len(columns_info) < 1:
+        return False, None, None, None
+
+    # First check that all the columns belong to a single table
+    table_oid = columns_info[0]['table_oid']
+    for column in columns_info:
+        if column['table_oid'] != table_oid:
+            return False, None, None, None
+
+    # Check for duplicate columns
+    column_numbers = \
+        [col['table_column'] for col in columns_info]
+    is_duplicate_columns = len(column_numbers) != len(set(column_numbers))
+    if is_duplicate_columns:
+        return False, None, None, None
+
+    if conn.connected():
+        # Then check that all the primary keys of the table are present
+        # and no primary keys are renamed
+        # (or other columns renamed to be like primary keys)
+        query = render_template(
+            "/".join([sql_path, 'primary_keys.sql']),
+            obj_id=table_oid
+        )
+        status, result = conn.execute_dict(query)
+        if not status:
+            return False, None, None, None
+
+        primary_keys_columns = []
+        primary_keys = OrderedDict()
+        pk_names = []
+
+        for row in result['rows']:
+            primary_keys[row['attname']] = row['typname']
+            primary_keys_columns.append({
+                'name': row['attname'],
+                'column_number': row['attnum']
+            })
+            pk_names.append(row['attname'])
+
+        # Check that all primary keys exist and that all of them are not
+        # renamed and other columns are not renamed to primary key names
+        for pk in primary_keys_columns:
+            pk_exists = False
+            for col in columns_info:
+                if col['table_column'] == pk['column_number']:
+                    pk_exists = True
+                    # If the primary key column is renamed
+                    if col['display_name'] != pk['name']:
+                        return False, None, None, None
+                # If a normal column is renamed to a primary key column name
+                elif col['display_name'] == pk['name']:
+                    return False, None, None, None
+
+            if not pk_exists:
+                return False, None, None, None
+
+        # If the for loop exited without returning from the function then
+        # all primary keys exist without being renamed
+        return True, primary_keys, pk_names, table_oid
+    else:
+        return False, None, None, None
diff --git a/web/pgadmin/tools/sqleditor/utils/save_changed_data.py b/web/pgadmin/tools/sqleditor/utils/save_changed_data.py
new file mode 100644
index 00000000..0cf7612e
--- /dev/null
+++ b/web/pgadmin/tools/sqleditor/utils/save_changed_data.py
@@ -0,0 +1,318 @@
+##########################################################################
+#
+# pgAdmin 4 - PostgreSQL Tools
+#
+# Copyright (C) 2013 - 2019, The pgAdmin Development Team
+# This software is released under the PostgreSQL Licence
+#
+##########################################################################
+
+from flask import render_template
+from pgadmin.tools.sqleditor.utils.constant_definition import TX_STATUS_IDLE
+try:
+    from collections import OrderedDict
+except ImportError:
+    from ordereddict import OrderedDict
+
+
+def save_changed_data(changed_data, columns_info, conn, command_obj,
+                      client_primary_key, auto_commit=True):
+    """
+    This function is used to save the data into the database.
+    Depending on condition it will either update or insert the
+    new row into the database.
+
+    Args:
+        changed_data: Contains data to be saved
+        command_obj: The transaction object (command_obj or trans_obj)
+        conn: The connection object
+        columns_info: session_obj['columns_info']
+        client_primary_key: session_obj['client_primary_key']
+        auto_commit: If the changes should be commited automatically.
+    """
+    status = False
+    res = None
+    query_res = dict()
+    count = 0
+    list_of_rowid = []
+    operations = ('added', 'updated', 'deleted')
+    list_of_sql = {}
+    _rowid = None
+    is_commit_required = False
+
+    pgadmin_alias = {
+        col_name: col_info['pgadmin_alias']
+        for col_name, col_info in columns_info
+            .items()
+    }
+
+    if conn.connected():
+        is_savepoint = False
+        # Start the transaction if the session is idle
+        if conn.transaction_status() == TX_STATUS_IDLE:
+            conn.execute_void('BEGIN;')
+        else:
+            conn.execute_void('SAVEPOINT save_data;')
+            is_savepoint = True
+
+        # Iterate total number of records to be updated/inserted
+        for of_type in changed_data:
+            # No need to go further if its not add/update/delete operation
+            if of_type not in operations:
+                continue
+            # if no data to be save then continue
+            if len(changed_data[of_type]) < 1:
+                continue
+
+            column_type = {}
+            column_data = {}
+            for each_col in columns_info:
+                if (
+                    columns_info[each_col]['not_null'] and
+                    not columns_info[each_col]['has_default_val']
+                ):
+                    column_data[each_col] = None
+                    column_type[each_col] = \
+                        columns_info[each_col]['type_name']
+                else:
+                    column_type[each_col] = \
+                        columns_info[each_col]['type_name']
+
+            # For newly added rows
+            if of_type == 'added':
+                # Python dict does not honour the inserted item order
+                # So to insert data in the order, we need to make ordered
+                # list of added index We don't need this mechanism in
+                # updated/deleted rows as it does not matter in
+                # those operations
+                added_index = OrderedDict(
+                    sorted(
+                        changed_data['added_index'].items(),
+                        key=lambda x: int(x[0])
+                    )
+                )
+                list_of_sql[of_type] = []
+
+                # When new rows are added, only changed columns data is
+                # sent from client side. But if column is not_null and has
+                # no_default_value, set column to blank, instead
+                # of not null which is set by default.
+                column_data = {}
+                pk_names, primary_keys = command_obj.get_primary_keys()
+                has_oids = 'oid' in column_type
+
+                for each_row in added_index:
+                    # Get the row index to match with the added rows
+                    # dict key
+                    tmp_row_index = added_index[each_row]
+                    data = changed_data[of_type][tmp_row_index]['data']
+                    # Remove our unique tracking key
+                    data.pop(client_primary_key, None)
+                    data.pop('is_row_copied', None)
+                    list_of_rowid.append(data.get(client_primary_key))
+
+                    # Update columns value with columns having
+                    # not_null=False and has no default value
+                    column_data.update(data)
+
+                    sql = render_template(
+                        "/".join([command_obj.sql_path, 'insert.sql']),
+                        data_to_be_saved=column_data,
+                        pgadmin_alias=pgadmin_alias,
+                        primary_keys=None,
+                        object_name=command_obj.object_name,
+                        nsp_name=command_obj.nsp_name,
+                        data_type=column_type,
+                        pk_names=pk_names,
+                        has_oids=has_oids
+                    )
+
+                    select_sql = render_template(
+                        "/".join([command_obj.sql_path, 'select.sql']),
+                        object_name=command_obj.object_name,
+                        nsp_name=command_obj.nsp_name,
+                        primary_keys=primary_keys,
+                        has_oids=has_oids
+                    )
+
+                    list_of_sql[of_type].append({
+                        'sql': sql, 'data': data,
+                        'client_row': tmp_row_index,
+                        'select_sql': select_sql
+                    })
+                    # Reset column data
+                    column_data = {}
+
+            # For updated rows
+            elif of_type == 'updated':
+                list_of_sql[of_type] = []
+                for each_row in changed_data[of_type]:
+                    data = changed_data[of_type][each_row]['data']
+                    pk_escaped = {
+                        pk: pk_val.replace('%', '%%')
+                        for pk, pk_val in
+                        changed_data[of_type][each_row]['primary_keys']
+                            .items()
+                    }
+                    sql = render_template(
+                        "/".join([command_obj.sql_path, 'update.sql']),
+                        data_to_be_saved=data,
+                        pgadmin_alias=pgadmin_alias,
+                        primary_keys=pk_escaped,
+                        object_name=command_obj.object_name,
+                        nsp_name=command_obj.nsp_name,
+                        data_type=column_type
+                    )
+                    list_of_sql[of_type].append({'sql': sql, 'data': data})
+                    list_of_rowid.append(data.get(client_primary_key))
+
+            # For deleted rows
+            elif of_type == 'deleted':
+                list_of_sql[of_type] = []
+                is_first = True
+                rows_to_delete = []
+                keys = None
+                no_of_keys = None
+                for each_row in changed_data[of_type]:
+                    rows_to_delete.append(changed_data[of_type][each_row])
+                    # Fetch the keys for SQL generation
+                    if is_first:
+                        # We need to covert dict_keys to normal list in
+                        # Python3
+                        # In Python2, it's already a list & We will also
+                        # fetch column names using index
+                        keys = list(
+                            changed_data[of_type][each_row].keys()
+                        )
+                        no_of_keys = len(keys)
+                        is_first = False
+                # Map index with column name for each row
+                for row in rows_to_delete:
+                    for k, v in row.items():
+                        # Set primary key with label & delete index based
+                        # mapped key
+                        try:
+                            row[changed_data['columns']
+                                            [int(k)]['name']] = v
+                        except ValueError:
+                            continue
+                        del row[k]
+
+                sql = render_template(
+                    "/".join([command_obj.sql_path, 'delete.sql']),
+                    data=rows_to_delete,
+                    primary_key_labels=keys,
+                    no_of_keys=no_of_keys,
+                    object_name=command_obj.object_name,
+                    nsp_name=command_obj.nsp_name
+                )
+                list_of_sql[of_type].append({'sql': sql, 'data': {}})
+
+        for opr, sqls in list_of_sql.items():
+            for item in sqls:
+                if item['sql']:
+                    item['data'] = {
+                        pgadmin_alias[k] if k in pgadmin_alias else k: v
+                        for k, v in item['data'].items()
+                    }
+
+                    row_added = None
+
+                    def failure_handle(res):
+                        if is_savepoint:
+                            conn.execute_void('ROLLBACK TO SAVEPOINT '
+                                              'save_data;')
+                            msg = 'Query ROLLBACK, but the current ' \
+                                  'transaction is still ongoing.'
+                            res += ' Saving ROLLBACK, but the current ' \
+                                   'transaction is still ongoing'
+                        else:
+                            conn.execute_void('ROLLBACK;')
+                            msg = 'Transaction ROLLBACK'
+                        # If we roll backed every thing then update the
+                        # message for each sql query.
+                        for val in query_res:
+                            if query_res[val]['status']:
+                                query_res[val]['result'] = msg
+
+                        # If list is empty set rowid to 1
+                        try:
+                            if list_of_rowid:
+                                _rowid = list_of_rowid[count]
+                            else:
+                                _rowid = 1
+                        except Exception:
+                            _rowid = 0
+
+                        return status, res, query_res, _rowid,\
+                            is_commit_required
+
+                    try:
+                        # Fetch oids/primary keys
+                        if 'select_sql' in item and item['select_sql']:
+                            status, res = conn.execute_dict(
+                                item['sql'], item['data'])
+                        else:
+                            status, res = conn.execute_void(
+                                item['sql'], item['data'])
+                    except Exception as _:
+                        failure_handle(res)
+                        raise
+
+                    if not status:
+                        return failure_handle(res)
+
+                    # Select added row from the table
+                    if 'select_sql' in item:
+                        status, sel_res = conn.execute_dict(
+                            item['select_sql'], res['rows'][0])
+
+                        if not status:
+                            if is_savepoint:
+                                conn.execute_void('ROLLBACK TO SAVEPOINT'
+                                                  ' save_data;')
+                                msg = 'Query ROLLBACK, the current' \
+                                      ' transaction is still ongoing.'
+                            else:
+                                conn.execute_void('ROLLBACK;')
+                                msg = 'Transaction ROLLBACK'
+                            # If we roll backed every thing then update
+                            # the message for each sql query.
+                            for val in query_res:
+                                if query_res[val]['status']:
+                                    query_res[val]['result'] = msg
+
+                            # If list is empty set rowid to 1
+                            try:
+                                if list_of_rowid:
+                                    _rowid = list_of_rowid[count]
+                                else:
+                                    _rowid = 1
+                            except Exception:
+                                _rowid = 0
+
+                            return status, sel_res, query_res, _rowid,\
+                                is_commit_required
+
+                        if 'rows' in sel_res and len(sel_res['rows']) > 0:
+                            row_added = {
+                                item['client_row']: sel_res['rows'][0]}
+
+                    rows_affected = conn.rows_affected()
+                    # store the result of each query in dictionary
+                    query_res[count] = {
+                        'status': status,
+                        'result': None if row_added else res,
+                        'sql': item['sql'], 'rows_affected': rows_affected,
+                        'row_added': row_added
+                    }
+
+                    count += 1
+
+        # Commit the transaction if no error is found & autocommit is activated
+        if auto_commit:
+            conn.execute_void('COMMIT;')
+        else:
+            is_commit_required = True
+
+    return status, res, query_res, _rowid, is_commit_required
diff --git a/web/pgadmin/tools/sqleditor/utils/start_running_query.py b/web/pgadmin/tools/sqleditor/utils/start_running_query.py
index a5399774..ece11f9c 100644
--- a/web/pgadmin/tools/sqleditor/utils/start_running_query.py
+++ b/web/pgadmin/tools/sqleditor/utils/start_running_query.py
@@ -45,6 +45,9 @@ class StartRunningQuery:
         if type(session_obj) is Response:
             return session_obj
 
+        # Remove any existing primary keys in session_obj
+        session_obj.pop('primary_keys', None)
+
         transaction_object = pickle.loads(session_obj['command_obj'])
         can_edit = False
         can_filter = False
diff --git a/web/regression/javascript/sqleditor/call_render_after_poll_spec.js b/web/regression/javascript/sqleditor/call_render_after_poll_spec.js
index d68f5ee1..5dad8470 100644
--- a/web/regression/javascript/sqleditor/call_render_after_poll_spec.js
+++ b/web/regression/javascript/sqleditor/call_render_after_poll_spec.js
@@ -37,7 +37,7 @@ describe('#callRenderAfterPoll', () => {
       sqlEditorSpy.is_query_tool = false;
     });
 
-    describe('query was successful but had no result to display', () => {
+    describe('query was successful and have results', () => {
       beforeEach(() => {
         queryResult = {
           rows_affected: 10,
@@ -65,7 +65,7 @@ describe('#callRenderAfterPoll', () => {
       });
     });
 
-    describe('query was successful and have results', () => {
+    describe('query was successful but had no result to display', () => {
       beforeEach(() => {
         queryResult = {
           rows_affected: 10,
@@ -81,7 +81,7 @@ describe('#callRenderAfterPoll', () => {
         expect(sqlEditorSpy.update_msg_history).toHaveBeenCalledWith(
           true,
           'Some result\n\nQuery returned successfully in 0 msec.',
-          false
+          true
         );
       });
 
@@ -116,7 +116,7 @@ describe('#callRenderAfterPoll', () => {
       sqlEditorSpy.is_query_tool = true;
     });
 
-    describe('query was successful but had no result to display', () => {
+    describe('query was successful and have results', () => {
       beforeEach(() => {
         queryResult = {
           rows_affected: 10,
@@ -150,7 +150,7 @@ describe('#callRenderAfterPoll', () => {
       });
     });
 
-    describe('query was successful and have results', () => {
+    describe('query was successful but had no result to display', () => {
       beforeEach(() => {
         queryResult = {
           rows_affected: 10,
@@ -166,7 +166,7 @@ describe('#callRenderAfterPoll', () => {
         expect(sqlEditorSpy.update_msg_history).toHaveBeenCalledWith(
           true,
           'Some result\n\nQuery returned successfully in 0 msec.',
-          false
+          true
         );
       });

---

From: Aditya Toshniwal <[email-masked]>
Date: Mon, 24 Jun 2019 16:33:15 +0530

Hi Yosry,

On Mon, Jun 24, 2019 at 11:08 AM Yosry Muhammad <[email-masked]> wrote:

> 1- Fixed #1 and #2 that Mr. Toshniwal pointed out in his code review.
> Still waiting for a reply on #3.
>
What I meant for the color was to use existing colors defined which are
based on a theme. The best fit for your work is $color-gray-lighter. Please
use - $color-highlighted-grid-cell : $color-gray-lighter. Or may be remove
$color-highlighted-grid-cell completely and use $color-gray-lighter in your
class.

>     a) Save File button: exclusively for saving the query file (disabled
> in View Table mode).
>     - If they choose to save and the save failed (null in a non-null
> column for example), the panel closes anyway.
> Something that is missing with the new button is the shortcut, I don't
> know how to modify the Preferences in the configuration database. I could
> not find the code responsible for adding data in the Preferences table and
> so. Any help?
>
> 6- I re-implemented the code responsible for handling the panel close
> event in following way:
In the sqleditor __init__.py, you'll find a call - RegisterQueryToolPreferences
(web/pgadmin/tools/sqleditor/utils/query_tool_preferences.py). Refer the
code for btn_save_file

> test_config.json.in file for my environment?
Copy the test_config.json.in to test_config.json in the same directory. You
just need to change the server details, sample below. You can add multiple
servers to the list:
...

"server_credentials": [
  {
    "name": "PostgreSQL 9.4",
    "comment": "PostgreSQL 9.4 Server",
    "db_username": "postgres",
    "host": "localhost",
    "db_password": "postgres",
    "db_port": 5432,
    "maintenance_db": "postgres",
    "sslmode": "prefer",
    "tablespace_path": "",
    "enabled": true,
    "default_binary_paths": {
      "pg": "/Library/PostgreSQL/9.4/bin/",
      "ppas": "",
      "gpdb": ""
    }
  }
],

...

> something I have done wrong?
What command did you use, can you share the screenshot of the files changed?

> produces by our code or automatically by the browser? any way around it? I
This can be disabled from Preferences ->  Browser -> Display -> Confirm on
close or refresh ?. Set it to false and you'll not get the browser warning.
This is particularly helpful if you open the query tool in a new browser
tab and close the tab itself.

> - What else is missing from this patch to make it applicable ? I would
> like to produce a release-ready patch if possible. If so, I can continue
> working on the project on following patches, I just want to know what is
> the minimum amount of work needed to make this patch release-ready
> (especially that changes are being made in the master branch that require
> me to re-edit parts of the code that I have written before to keep things
> in-sync).
>
@Dave Page is the right person to answer this.

> function that can provide a cursor (or a wrapper around cursor.mogrify() )?
Could you please share the query/screenshot ? The query history just stores
the SQL text and fetches back to show in CodeMirror. No
modifications/generation of queries is done by Query History.

>>> 2) The function name check_for_updatable_resultset_and_primary_keys
>>> could be shorter, something like check_updatabale_rset_pkeys. Same for
>> I shortened both function names and fixed the hard-coded color. For #1,
>> in the QueryToolCommand different processing of the primary keys occur in

---

From: Yosry Muhammad <[email-masked]>
Date: Mon, 24 Jun 2019 18:43:16 +0200

Hi,

Please find attached a patch file with the following updates (last patch +
updates) attached:
- Changed the color to $color-gray-lighter and added the shortcut for the
new button.
- Added a preferences option to enable/disable prompting on uncommited
transactions on exiting.
- Changed call_render_after_poll_specs test to be in sync with code
changes, also fixed a mix up in the test descriptions in the same file.
- Fixed a bug with a recent patch 'Allow editing of data where a primary
key column includes a % sign in the value.' that occurred when the primary
key was a number.

> What command did you use, can you share the screenshot of the files

I tried it again after a proper test_config.json as you mentioned and
everything worked fine. All tests pass for this patch except for 3 feature
tests that all fail because of a TimeoutException related to selenium.
Please find a log file of the feature tests attached.

> @Dave Page is the right person to answer this.

Waiting for his reply :D

> Could you please share the query/screenshot ? The query history just
> stores the SQL text and fetches back to show in CodeMirror. No
By 'generated queries' I meant the querie that are generated by pgAdmin to
save changes to the data grid to the database. Here is a screenshot from
the released version (not the version I am working on).
Scenario:
- Opened View Data on a table (public.kweek)
- Modified a cell in a column named media_url with a primary key (id = 50)
to 'new link'
- Instead of showing 'new link' in the query %(media_url) is shown.

This can be fixed in save_changed_data() function in my patch but I need
access to a cursor as previously mentioned. Thoughts?

Thanks a lot for your help!

-- 
*Yosry Muhammad Yosry*

Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.
https://www.linkedin.com/in/yosrym93/
diff --git a/web/pgadmin/static/js/keyboard_shortcuts.js b/web/pgadmin/static/js/keyboard_shortcuts.js
index c565b862..5dd2384b 100644
--- a/web/pgadmin/static/js/keyboard_shortcuts.js
+++ b/web/pgadmin/static/js/keyboard_shortcuts.js
@@ -205,6 +205,7 @@ function keyboardShortcutsQueryTool(
   let toggleCaseKeys = sqlEditorController.preferences.toggle_case;
   let commitKeys = sqlEditorController.preferences.commit_transaction;
   let rollbackKeys = sqlEditorController.preferences.rollback_transaction;
+  let saveDataKeys = sqlEditorController.preferences.save_data;
 
   if (this.validateShortcutKeys(executeKeys, event)) {
     this._stopEventPropagation(event);
@@ -233,6 +234,9 @@ function keyboardShortcutsQueryTool(
       this._stopEventPropagation(event);
       queryToolActions.executeRollback(sqlEditorController);
     }
+  } else if (this.validateShortcutKeys(saveDataKeys, event)) {
+    this._stopEventPropagation(event);
+    queryToolActions.saveDataChanges(sqlEditorController);
   } else if ((
     (this.isMac() && event.metaKey) ||
      (!this.isMac() && event.ctrlKey)
diff --git a/web/pgadmin/static/js/sqleditor/call_render_after_poll.js b/web/pgadmin/static/js/sqleditor/call_render_after_poll.js
index 3f32d571..51c2302b 100644
--- a/web/pgadmin/static/js/sqleditor/call_render_after_poll.js
+++ b/web/pgadmin/static/js/sqleditor/call_render_after_poll.js
@@ -37,7 +37,7 @@ export function callRenderAfterPoll(sqlEditor, alertify, res) {
     const msg = sprintf(
       gettext('Query returned successfully in %s.'), sqlEditor.total_time);
     res.result += '\n\n' + msg;
-    sqlEditor.update_msg_history(true, res.result, false);
+    sqlEditor.update_msg_history(true, res.result, true);
     if (isNotificationEnabled(sqlEditor)) {
       alertify.success(msg, sqlEditor.info_notifier_timeout);
     }
diff --git a/web/pgadmin/static/js/sqleditor/query_tool_actions.js b/web/pgadmin/static/js/sqleditor/query_tool_actions.js
index 7739e9b5..e5cf8a36 100644
--- a/web/pgadmin/static/js/sqleditor/query_tool_actions.js
+++ b/web/pgadmin/static/js/sqleditor/query_tool_actions.js
@@ -153,6 +153,11 @@ let queryToolActions = {
     sqlEditorController.special_sql = 'ROLLBACK;';
     self.executeQuery(sqlEditorController);
   },
+
+  saveDataChanges: function (sqlEditorController) {
+    sqlEditorController.close_on_save = false;
+    sqlEditorController.save_data();
+  },
 };
 
 module.exports = queryToolActions;
diff --git a/web/pgadmin/static/js/sqleditor/query_tool_preferences.js b/web/pgadmin/static/js/sqleditor/query_tool_preferences.js
index c3906aaa..abceb099 100644
--- a/web/pgadmin/static/js/sqleditor/query_tool_preferences.js
+++ b/web/pgadmin/static/js/sqleditor/query_tool_preferences.js
@@ -29,7 +29,7 @@ function updateUIPreferences(sqlEditor) {
     .attr('title', shortcut_accesskey_title('Open File',preferences.btn_open_file))
     .attr('accesskey', shortcut_key(preferences.btn_open_file));
 
-  $el.find('#btn-save')
+  $el.find('#btn-save-file')
     .attr('title', shortcut_accesskey_title('Save File',preferences.btn_save_file))
     .attr('accesskey', shortcut_key(preferences.btn_save_file));
 
@@ -97,6 +97,10 @@ function updateUIPreferences(sqlEditor) {
     .attr('title',
       shortcut_title('Download as CSV',preferences.download_csv));
 
+    .attr('title',
+      shortcut_title('Save Data Changes',preferences.save_data));

 

 
                         SQL = render_template(

 
         manager = get_driver(

 

 
-        status = False
-        res = None
-        query_res = dict()
-        count = 0
-        list_of_rowid = []

 

     def can_filter(self):
         return False

 

index e2d69f78..f2d28d86 100644

 
      SlickGrid Plugins:

 

@@ -1410,7 +1358,7 @@ define('tools.querytool', [
     },

@@ -1419,9 +1367,7 @@ define('tools.querytool', [
 
@@ -1435,7 +1381,7 @@ define('tools.querytool', [
       self.handler.close_on_save = false;
       // Trigger the save signal to the SqlEditorController class
       self.handler.trigger(
-        'pgadmin-sqleditor:button:save',
+        'pgadmin-sqleditor:button:save_file',
         self,
         self.handler,
         true
@@ -1608,6 +1554,11 @@ define('tools.querytool', [
       );
     },
 
+    // Callback function for Save Data Changes button click.
+    on_save_data: function() {
+      queryToolActions.saveDataChanges(this.handler);
+    },
+
     // Callback function for the flash button click.
     on_flash: function() {
       queryToolActions.executeQuery(this.handler);
@@ -2194,7 +2145,7 @@ define('tools.querytool', [
 
@@ -2309,8 +2260,7 @@ define('tools.querytool', [
 
@@ -2376,6 +2326,18 @@ define('tools.querytool', [
         else
           self.can_edit = true;
 
@@ -2386,13 +2348,12 @@ define('tools.querytool', [
 
@@ -2776,9 +2737,9 @@ define('tools.querytool', [
@@ -2807,41 +2768,42 @@ define('tools.querytool', [
 
+      save_data: function() {
+        var self = this;
 
+        if(!self.can_edit)
           return;
 
         var is_added = _.size(self.data_store.added),
           is_updated = _.size(self.data_store.updated),
@@ -2851,154 +2813,179 @@ define('tools.querytool', [
           return; // Nothing to save here
         }
 
-        if (save_data) {
+        self.trigger(
 
-          self.trigger(
+        var req_data = self.data_store, view = self.gridView;

 
 
-                // Reset data store
-                self.data_store = {
-                  'added': {},
-                  'updated': {},
-                  'deleted': {},
-                  'added_index': {},
-                  'updated_index': {},
-                };

 

-              grid.invalidate();
 
       // Find index of row at fault from grid data
@@ -3042,7 +3029,7 @@ define('tools.querytool', [
 
       // Save as
       _save_as: function() {
 
       // Set panel title.
@@ -3132,7 +3119,7 @@ define('tools.querytool', [
             $busy_icon_div.removeClass('show_progress');

@@ -3177,7 +3164,7 @@ define('tools.querytool', [
 
               // Update the flag as query is already saved.
@@ -3186,7 +3173,8 @@ define('tools.querytool', [
             }
             self.trigger('pgadmin-sqleditor:loading-icon:hide');
             if (self.close_on_save) {
-              self.close();
+              // Check for any other needed confirmations before closing
+              self.check_needed_confirmations_before_closing_panel();
             }
           })
           .fail(function(e) {
@@ -3226,7 +3214,7 @@ define('tools.querytool', [
             self.setTitle(title);
           }
 
@@ -3460,7 +3448,7 @@ define('tools.querytool', [
 
@@ -3563,7 +3551,7 @@ define('tools.querytool', [
@@ -3592,8 +3580,36 @@ define('tools.querytool', [
 
@@ -3602,8 +3618,8 @@ define('tools.querytool', [
@@ -3998,6 +4014,146 @@ define('tools.querytool', [
 
+        else if (!ignore_uncommited_transaction
+                 && self.preferences.prompt_uncommited_transaction
+                 && SqlEditorUtils.previousStatus != 0) {
+                  self.save_data();
 
index ebf5b180..2742f320 100644
 
+  background: $color-gray-lighter;
diff --git a/web/pgadmin/tools/sqleditor/utils/query_tool_preferences.py b/web/pgadmin/tools/sqleditor/utils/query_tool_preferences.py
index ca09eaec..65e2b23c 100644
--- a/web/pgadmin/tools/sqleditor/utils/query_tool_preferences.py
+++ b/web/pgadmin/tools/sqleditor/utils/query_tool_preferences.py
@@ -163,6 +163,16 @@ def RegisterQueryToolPreferences(self):
         )
     )
 
+    self.show_prompt_uncommited_transaction = self.preference.register(
+        'Options', 'prompt_uncommited_transaction',
+        gettext("Prompt on uncommited transactions?"), 'boolean', True,
+        category_label=gettext('Options'),
+        help_str=gettext(
+            'Specifies whether or not to prompt user when a current '
+            'transaction is uncommited on Query Tool exit.'
+        )
+    )
+
     self.csv_quoting = self.preference.register(
         'CSV_output', 'csv_quoting',
         gettext("CSV quoting"), 'options', 'strings',
@@ -290,6 +300,24 @@ def RegisterQueryToolPreferences(self):
         fields=shortcut_fields
     )
 
+    self.preference.register(
+        'keyboard_shortcuts',
+        'save_data',
+        gettext('Save Data Changes'),
+        'keyboardshortcut',
+        {
+            'alt': False,
+            'shift': False,
+            'control': False,
+            'key': {
+                'key_code': 117,
+                'char': 'F6'
+            }
+        },
+        category_label=gettext('Keyboard shortcuts'),
+        fields=shortcut_fields
+    )
+
     self.preference.register(
         'keyboard_shortcuts',
         'explain_query',
index 00000000..301b8803
@@ -0,0 +1,316 @@
+        for col_name, col_info in columns_info.items()
+                        pk: str(pk_val).replace('%', '%%')
+                        changed_data[of_type][each_row]['primary_keys'].items()

---

From: Aditya Toshniwal <[email-masked]>
Date: Tue, 25 Jun 2019 16:38:48 +0530

Hi,

> - Instead of showing 'new link' in the query %(media_url) is shown.
The update queries fired internally should not go to history. Queries fired
by user only should go. That's what I think.

---

From: Dave Page <[email-masked]>
Date: Tue, 25 Jun 2019 07:09:22 -0400

Hi

> @Dave Page is the right person to answer this.

It needs:

- A code complete feature (or infrastructure/refactoring ready for a
feature), that is acceptable to us. Seems like this is 90% there for an
initial commit.
- Documentation updates.
- Tests for the new feature to ensure it works without needing manual
testing.
- To pass all existing tests (which may be modified if appropriate).

---

From: Dave Page <[email-masked]>
Date: Tue, 25 Jun 2019 07:10:57 -0400

On Tue, Jun 25, 2019 at 7:09 AM Aditya Toshniwal <
[email-masked]> wrote:

>> Please find attached a patch file with the following updates (last patch
>> + updates) attached:
>> By 'generated queries' I meant the querie that are generated by pgAdmin
>> to save changes to the data grid to the database. Here is a screenshot from
>> - Modified a cell in a column named media_url with a primary key (id =
>> 50) to 'new link'
> The update queries fired internally should not go to history. Queries
> fired by user only should go. That's what I think.
>

The conclusion I came to in previous discussion was that both should be
available, with a checkbox (off by default) to include the internal
queries, which would include any BEGIN/COMMITs etc.

---

From: Aditya Toshniwal <[email-masked]>
Date: Tue, 25 Jun 2019 17:45:46 +0530

Hi,

On Tue, Jun 25, 2019 at 4:41 PM Dave Page <[email-masked]> wrote:

>>> - Changed the color to $color-gray-lighter and added the shortcut for
>>> the new button.
>>> - After running python and feature tests, changes were made to nearly
>>>>> all the files (git status shows modifications in a ton of files), is there
OK. Yosry, How about storing the mogirfied query in the cookie/session when
the query is executed and then modifying query history storing logic to use
it when called ? This way you will not need to change any parsing when
query history is displayed.

---

From: Yosry Muhammad <[email-masked]>
Date: Tue, 25 Jun 2019 17:16:32 +0200

Hi,

> query history is displayed.
My problem is not where to store the mogrified query, I can just replace
the sql sent with the response to saving the data with the mogrified one.
In order to produce the mogrified query in the first place I need a
psycopg2.cursor object, but I only have access to a Connection object (the
wrapper, not the actual psycopg2.connection object). Should I modify that
wrapper Connection class to add a mogirfy function? or just get a cursor
using the psycopg2.connection object like this: conn.conn.cursor() - which
doesn't seem right. Thoughts?

Also, could you please help me with the selenium TimeoutException in the
feature tests? 3 tests are failing because of that exception and I am not
sure what is it related to. I am attaching the feature test log file in
this email if you would like to take a look on it.

Thanks !

-- 
*Yosry Muhammad Yosry*

---

From: Yosry Muhammad <[email-masked]>
Date: Tue, 25 Jun 2019 17:19:53 +0200

> - To pass all existing tests (which may be modified if appropriate).

Could you tell me what is missing from this patch (in terms of features -
other than tests) to be acceptable? I will start working on the tests once
the patch is complete. The patch passes all the existing tests except for 3
feature tests that fail due to a TimeoutException in selenium. I do not
know what this is about I hope Aditya will help me with it.

Also, do you mean code documentation or documentation for the users? Could
you point me towards the related parts?

Thanks a lot.

-- 
*Yosry Muhammad Yosry*

---

From: Aditya Toshniwal <[email-masked]>
Date: Wed, 26 Jun 2019 10:35:14 +0530

Hi Yosry,

On Tue, Jun 25, 2019 at 8:46 PM Yosry Muhammad <[email-masked]> wrote:

> Hi,
>
>
> OK. Yosry, How about storing the mogirfied query in the cookie/session
>> when the query is executed and then modifying query history storing logic
>> to use it when called ? This way you will not need to change any parsing
>> when query history is displayed.
>>
>>> --
>>>
>>
That would be the way. But I think
web/pgadmin/utils/driver/psycopg2/cursor.py will be corrrect place to add
the mogrify function. Please note, psycopg2 does not support (,),% in the
parameter names. So if the column names has any of these characters,
mogrify might fail. Although it is handled with pgadmin_alias (line 493 -
web/pgadmin/tools/sqleditor/__init__.py), please test this case as well.

> this email if you would like to take a look on it.
Can you check on latest chrome and chromdriver. You can check the
chromedriver version as below. Your venv should have the chromedriver
binary.

(pypg37) laptop207:pgadmin4 adityatoshniwal$ chromedriver  --version
ChromeDriver 75.0.3770.8
(681f24ea911fe754973dda2fdc6d2a2e159dd300-refs/branch-heads/3770@{#40})

---

From: Aditya Toshniwal <[email-masked]>
Date: Wed, 26 Jun 2019 15:16:04 +0530

Hi,

> web/pgadmin/tools/sqleditor/__init__.py), please test this case as well.
You can also get the last executed SQL in psycopg2 -
http://initd.org/psycopg/docs/cursor.html#cursor.query. May be this can be
used.

---

From: Dave Page <[email-masked]>
Date: Wed, 26 Jun 2019 07:01:40 -0400

Hi

> know what this is about I hope Aditya will help me with it.

Here are the issues I think should be fixed first:

- I think the Save button should be moved to the left of the Find button.
It makes more sense to be near the Save Query button.

- Umm, that's about it, bar the history issue. The quick fix there might be
to hide the internal queries for now as previously discussed, but I do this
the checkbox to include them (in their mogrified state) should be included
as part of the overall project.

Note that I haven't done in-depth testing. Once the patch is committed (and
about now is a good time, as we're at the beginning of the release cycle),
we'll get our QA guy to see if he can find any issues we've missed.

> you point me towards the related parts?

User documentation. I would expect at least one screenshot update due to
the new button on the toolbar (probably more - please check for others that
need updating), as well as updates to at least:

editgrid.rst
query_tool.rst
query_tool_toolbar.rst

Great work!

---

From: Yosry Muhammad <[email-masked]>
Date: Wed, 26 Jun 2019 14:20:03 +0200

Hi,

On Wed, Jun 26, 2019 at 1:01 PM Dave Page <[email-masked]> wrote:

> - Umm, that's about it, bar the history issue. The quick fix there might
> be to hide the internal queries for now as previously discussed, but I do
> this the checkbox to include them (in their mogrified state) should be
> included as part of the overall project.
>
> Note that I haven't done in-depth testing. Once the patch is committed
> (and about now is a good time, as we're at the beginning of the release
> cycle), we'll get our QA guy to see if he can find any issues we've missed.
>
>
>>
>> Also, do you mean code documentation or documentation for the users?
>> Could you point me towards the related parts?
>>
>
I will disable the generated queries for now, then for the next patch I
will work on (optionally) including them (mogrified). Should I send a patch
with the completed work then start working on the tests and documentation
(for it to get reviewed)? or wait until the patch is complete with tests
and documentation?

Thanks all !
-- 
*Yosry Muhammad Yosry*

---

From: Yosry Muhammad <[email-masked]>
Date: Wed, 26 Jun 2019 14:29:21 +0200

On Wed, Jun 26, 2019 at 11:46 AM Aditya Toshniwal <
[email-masked]> wrote:

>> the mogrify function. Please note, psycopg2 does not support (,),% in
>> the parameter names. So if the column names has any of these characters,
>> mogrify might fail. Although it is handled with pgadmin_alias (line 493
>> - web/pgadmin/tools/sqleditor/__init__.py), please test this case as well.
>>
> http://initd.org/psycopg/docs/cursor.html#cursor.query. May be this can
> be used.
>

The cursor wrapper class (DictCursor) is exclusively used by the connection
wrapper class (at web/pgadmin/utils/driver/psycopg2/connection.py) as a
cursor factory. I think the mogrify function will need to be implemented in
both classes, as the Connection class is the one that is used throughout
the code - no code uses cursors directly. What do you think?
The use of cursor.query will not be possible as this is a property of the
cursor, not the connection. I will need to call cursor.query on the exact
cursor that was used to execute the query, which is not accessible in this
case.

Thanks a lot for your help !

>> (681f24ea911fe754973dda2fdc6d2a2e159dd300-refs/branch-heads/3770@{#40})
The version I have is actually 75.0.3770.90. Could the more recent version
be causing the problem ?

Thanks and regards.

-- 
*Yosry Muhammad Yosry*

---

From: Dave Page <[email-masked]>
Date: Wed, 26 Jun 2019 08:32:06 -0400

Hi

On Wed, Jun 26, 2019 at 8:20 AM Yosry Muhammad <[email-masked]> wrote:

>>> Could you tell me what is missing from this patch (in terms of features
>>> - other than tests) to be acceptable? I will start working on the tests
>>> once the patch is complete. The patch passes all the existing tests except
>>> for 3 feature tests that fail due to a TimeoutException in selenium. I do
>>> not know what this is about I hope Aditya will help me with it.
>>>
>>

We always want to commit the docs and tests along with the code so we don't
get in a situation where they later get missed or omitted.

Thanks.