Subject: [Feature-3452] Schema diff tool with Table, View, Materialized View, Functions and Procedures

From: Akshay Joshi <[email-masked]>
Date: Fri, 20 Dec 2019 18:59:37 +0530

Hi Hackers,

Attached is the implementation of the new feature Schema Diff Tool.
Initial work(backend code to compare the objects) has been done by me and
then most of the task has been completed by *Khushboo Vashi. *Sending the
patch on behalf of her*.*

Currently, this tool only supports Tables, Views, Materialized Views,
Functions and Procedures node.

Please review and test it thoroughly. Suggestions are welcome to improve
the tool.

-- 
*Thanks & Regards*
*Akshay Joshi*

*Sr. Software Architect*
*EnterpriseDB Software India Private Limited*
*Mobile: +91 976-788-8246*

---

From: Aditya Toshniwal <[email-masked]>
Date: Fri, 27 Dec 2019 17:26:05 +0530

Hi Akshay/Khushboo,

I have few suggestions/questions for the attached patch:

   1. Code like SchemaDiffRegistry('server', ServerNode) should be replaced
   with SSchemaDiffRegistry(ServerModule.NODE_TYPE, ServerNode)
   2. The variables return_ajax_response, only_sql, json_resp as far as I
   understood are similar. Can we have same var name everywhere ?
   3. Remove commented code -
   web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py
   -> get_sql_from_table_diff
   4. In
   web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py
   -> fetch_tables - keys_to_remove is passed. How is it different from
   keys_to_ignore used at other places ?
   5. web/pgadmin/tools/schema_diff/__init__.py
   -> check_version_compatibility has hardcoded version numbers. Can we
   use get_version_mapping_directories from
   web/pgadmin/utils/versioned_template_loader.py ?
   6. Rename .reallyHidden to .really-hidden
   7. CSS class #schema-diff-grid  -> background: white; - hardcoded color
   can be changed to use $color-bg instead. Also use rem or px for -
   font-size: 9pt.
   8. .slick-group-toggle.collapsed, .slick-group-toggle.expanded - svgs
   are not required. Font awesome has the icons. refer - .obj_properties
   .collapsed .caret::before.
   9. In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js ->
   formatNode - Appends can be avoided and formed in a single statement.
   +  } else {
   +    return $('<span></span>').append(
   +      $('<span></span>', {
   +        class: 'wcTabIcon ' + optimage,
   +      })
   +    ).append($('<span></span>').text(opt.text));
   +  }
   +};
   10. In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
   -> fetchData - We should not use async = false.
   +        $.ajax({
   +          async: false,
   +          url: url,
   +        })
   11. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -
   Use 'sources/window' - pgWindow.
   +            let preferences = (window.opener !== null) ?
   window.opener.pgAdmin.Browser.get_preferences_for_module('schema_diff') :
   window.top.pgAdmin.Browser.get_preferences_for_module('schema_diff');
   12. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js - Use
   map instead of for loop. It will also remove the sel_rows_data.push. will
   be helpfull in large data.
   +      for (var row = 0; row < sel_rows.length; row++) {
   +        let data = self.grid.getData().getItem(sel_rows[row]);
   +
   +        if (data.type) {
   +          let tmp_data = {
   +            'node_type': data.type,
   +            'source_oid': parseInt(data.oid, 10),
   +            'target_oid': parseInt(data.oid, 10),
   +            'comp_status': data.status,
   +          };
   +
   +          if(data.status && (data.status.toLowerCase() == 'different'
   || data.status.toLowerCase() == 'identical')) {
   +            tmp_data['target_oid'] = data.target_oid;
   +          }
   +          sel_rows_data.push(tmp_data);
   +        }
   +      }
   +
   +      url_params['sel_rows'] = sel_rows_data;
   13. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -Are we
   doing anything to handle failure.
   +  connect_database(server_id, db_id, callback) {
   +    var url = url_for('schema_diff.connect_database', {'sid':
   server_id, 'did': db_id});
   +    $.post(url)
   +      .done(function(res) {
   +        if (res.success && res.data) {
   +          callback(res.data);
   +        }
   +      })
   +      .fail(function() {
   +        // Fail
   +      });
   +
   +  }
   14. As you've added a completely different function for connect_server,
   I would suggest to rename dlgServerPass to a different name to avoid
   conflict with existing dlgServerPass in server.js
   +    if (!Alertify.dlgServerPass) {
   +      Alertify.dialog('dlgServerPass', function factory() {
   15. Generate script does not work if pgAdmin opened in iframe. Iframes
   are used by tools like Katacoda.
   16. Comparing objects loader is not attached to DDL Comparison panel.
   17. Filter icon and Generate script icon size are different. Also change
   icons CSS to use font-icon. You can refer icons from sqleditor.

*The fetch_objects_to_compare function used in each node uses loop to fetch
data. Although it is working for now, but I would suggest using bulk fetch
nodes instead of looping through all the nodes one by one.*

On Fri, Dec 27, 2019 at 4:55 PM Aditya Toshniwal <
[email-masked]> wrote:

> Hi Akshay/Khushboo,
>
> I have few suggestions/questions for the attached patch:
>
>    1. Code like SchemaDiffRegistry('server', ServerNode) should be
>    replaced with SSchemaDiffRegistry(ServerModule.NODE_TYPE, ServerNode)
>    2. The variables return_ajax_response, only_sql, json_resp as far as I
>    understood are similar. Can we have same var name everywhere ?
>    3. Remove commented code -
>    web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py
>    -> get_sql_from_table_diff
>    4. In
>    web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py
>    -> fetch_tables - keys_to_remove is passed. How is it different from
>    keys_to_ignore used at other places ?
>    5. web/pgadmin/tools/schema_diff/__init__.py
>    -> check_version_compatibility has hardcoded version numbers. Can we
>    use get_version_mapping_directories from
>    web/pgadmin/utils/versioned_template_loader.py ?
>    6. Rename .reallyHidden to .really-hidden
>    7. CSS class #schema-diff-grid  -> background: white; - hardcoded
>    color can be changed to use $color-bg instead. Also use rem or px for -
>    font-size: 9pt.
>    8. .slick-group-toggle.collapsed, .slick-group-toggle.expanded - svgs
>    are not required. Font awesome has the icons. refer - .obj_properties
>    .collapsed .caret::before.
>    9. In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
>    -> formatNode - Appends can be avoided and formed in a single statement.
>    +  } else {
>    +    return $('<span></span>').append(
>    +      $('<span></span>', {
>    +        class: 'wcTabIcon ' + optimage,
>    +      })
>    +    ).append($('<span></span>').text(opt.text));
>    +  }
>    +};
>    10. In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
>    -> fetchData - We should not use async = false.
>    +        $.ajax({
>    +          async: false,
>    +          url: url,
>    +        })
>    11. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -
>    Use 'sources/window' - pgWindow.
>    +            let preferences = (window.opener !== null) ?
>    window.opener.pgAdmin.Browser.get_preferences_for_module('schema_diff') :
>    window.top.pgAdmin.Browser.get_preferences_for_module('schema_diff');
>    12. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js - Use
>    map instead of for loop. It will also remove the sel_rows_data.push. will
>    be helpfull in large data.
>    +      for (var row = 0; row < sel_rows.length; row++) {
>    +        let data = self.grid.getData().getItem(sel_rows[row]);
>    +
>    +        if (data.type) {
>    +          let tmp_data = {
>    +            'node_type': data.type,
>    +            'source_oid': parseInt(data.oid, 10),
>    +            'target_oid': parseInt(data.oid, 10),
>    +            'comp_status': data.status,
>    +          };
>    +
>    +          if(data.status && (data.status.toLowerCase() == 'different'
>    || data.status.toLowerCase() == 'identical')) {
>    +            tmp_data['target_oid'] = data.target_oid;
>    +          }
>    +          sel_rows_data.push(tmp_data);
>    +        }
>    +      }
>    +
>    +      url_params['sel_rows'] = sel_rows_data;
>    13. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -Are
>    we doing anything to handle failure.
>    +  connect_database(server_id, db_id, callback) {
>    +    var url = url_for('schema_diff.connect_database', {'sid':
>    server_id, 'did': db_id});
>    +    $.post(url)
>    +      .done(function(res) {
>    +        if (res.success && res.data) {
>    +          callback(res.data);
>    +        }
>    +      })
>    +      .fail(function() {
>    +        // Fail
>    +      });
>    +
>    +  }
>    14. As you've added a completely different function for
>    connect_server, I would suggest to rename dlgServerPass to a different name
>    to avoid conflict with existing dlgServerPass in server.js
>    +    if (!Alertify.dlgServerPass) {
>    +      Alertify.dialog('dlgServerPass', function factory() {
>    15. Generate script does not work if pgAdmin opened in iframe. Iframes
>    are used by tools like Katacoda.
>    16. Comparing objects loader is not attached to DDL Comparison panel.
>    17. Filter icon and Generate script icon size are different. Also
>    change icons CSS to use font-icon. You can refer icons from sqleditor.
>
> *The fetch_objects_to_compare function used in each node uses loop to
> fetch data. Although it is working for now, but I would suggest using bulk
> fetch nodes instead of looping through all the nodes one by one.*
>
> On Fri, Dec 20, 2019 at 6:59 PM Akshay Joshi <
> [email-masked]> wrote:
>
>> Hi Hackers,
>>
>> then most of the task has been completed by *Khushboo Vashi. *Sending
>> the patch on behalf of her*.*
>>
> Thanks and Regards,
> Aditya Toshniwal
> pgAdmin Hacker | Sr. Software Engineer | EnterpriseDB India | Pune
> "Don't Complain about Heat, Plant a TREE"
>

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

---

From: Khushboo Vashi <[email-masked]>
Date: Thu, 2 Jan 2020 12:04:13 +0530

Hi Aditya,

Thanks for the review. Please find the inline response.
Also, the updated patch attached.

On Fri, Dec 27, 2019 at 4:55 PM Aditya Toshniwal <
[email-masked]> wrote:

> Done

>
>    1. The variables return_ajax_response, only_sql, json_resp as far as I
>  only_sql is used when I need only SQL but not to be executed in the
backend. json_resp is used to have the plain text/json response.

>
>    1. Remove commented code -
> Done

>    1. In
> keys_to_remove used to remove the table properties before comparison as we
combine all child nodes of the table while comparing and keys_to_ignore is
to ignore the keys while comparing. We have also used the keys_to_ignore in
the table node itself.

>
>    1. web/pgadmin/tools/schema_diff/__init__.py
> I have checked the possibilities before using it in the schema diff. The *
purpose and the return values * are different for both the files.

>
>    1. Rename .reallyHidden to .really-hidden
>
> Done

>
>    1. CSS class #schema-diff-grid  -> background: white; - hardcoded
> Done

>
>    1.
>    2. .slick-group-toggle.collapsed, .slick-group-toggle.expanded - svgs
> Done

>
>    1.
>    2. In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
> Any harm in this approach?

>
>    1.
>    2. In web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
>    3. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -
>    1. +            let preferences = (window.opener !== null) ?
> Done

>
>    1. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js - Use
> This is a debatable topic as there are pros and cons of both map and for
loop. Like, it's more readable if we use map and in case of for loop,
chrome and firefox will be more happy in terms of performance.

>
>    1.
>    2. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -Are
> Forgot to handle, now added.

>
>    1.
>    2. As you've added a completely different function for connect_server,
> As we open the schema diff in different frame, I think, this should not be
the issue.

>
>    1.
>    2. Generate script does not work if pgAdmin opened in iframe. Iframes
> Fixed, good catch.

>
>    1.
>    2. Comparing objects loader is not attached to DDL Comparison panel.
>
> Fixed.

>
>    1.
>    2. Filter icon and Generate script icon size are different. Also
> The problem is, for the generate script icon, I have used the svg (as no
similar icon in font-awesome) whereas for Filter, font-awesome is used. I
can replace the Generate Script icon from font-awesome (can search for some
similar icon) if Chetana agrees.
@Chethana Kumar <[email-masked]> , please have a look.

https://fontawesome.com/v4.7.0/icon/file-code-o
https://fontawesome.com/v4.7.0/icon/file-text-o

>    1.

Can you please elaborate the approach which you are suggesting?

Thanks,
Khushboo

---

From: Chethana Kumar <[email-masked]>
Date: Thu, 2 Jan 2020 14:04:26 +0530

Hi Khushboo,

On Thu, Jan 2, 2020 at 12:04 PM Khushboo Vashi <
[email-masked]> wrote:

>>    1. The variables return_ajax_response, only_sql, json_resp as far as
>>    I understood are similar. Can we have same var name everywhere ?
>>
>> keys_to_remove used to remove the table properties before comparison as
> we combine all child nodes of the table while comparing and keys_to_ignore
> is to ignore the keys while comparing. We have also used the keys_to_ignore
> in the table node itself.
>
>>
>> I have checked the possibilities before using it in the schema diff. The
> * purpose and the return values * are different for both the files.
>
>>
>>    +          if(data.status && (data.status.toLowerCase() ==
>>    'different' || data.status.toLowerCase() == 'identical')) {
>>    2. As you've added a completely different function for
>> As we open the schema diff in different frame, I think, this should not
> be the issue.
>
>>
>>    1.

We can use the above one as it looks appropriate to me.

Thanks,
Chethana kumar

-- 
Chethana Kumar
Principal UI/UX Designer
EnterpriseDB Corporation

The Postgres Database Company

P: +91 86981 57146
www.enterprisedb.com

---

From: Khushboo Vashi <[email-masked]>
Date: Thu, 2 Jan 2020 14:33:49 +0530

On Thu, Jan 2, 2020 at 2:04 PM Chethana Kumar <
[email-masked]> wrote:

>>>    2. .slick-group-toggle.collapsed, .slick-group-toggle.expanded -
>>>    svgs are not required. Font awesome has the icons. refer - .obj_properties
>>>    2. In
>>>    web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js ->
>>>    2. In
>>>    web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
>>>    1. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -
>>>    Use map instead of for loop. It will also remove the sel_rows_data.push.
>>>    will be helpfull in large data.
>>>    2. Generate script does not work if pgAdmin opened in iframe.
>>>    Iframes are used by tools like Katacoda.
>>>
>>> Fixed, good catch.
>>
>>>
>>>    1.
Thanks, I will change it.

>>>> Please review and test it thoroughly. Suggestions are welcome to
>>>> improve the tool.
>>>>
>>>> --
>>>> *Thanks & Regards*
>>>> *Akshay Joshi*
>>>>
>>>> *Sr. Software Architect*

---

From: Aditya Toshniwal <[email-masked]>
Date: Thu, 2 Jan 2020 14:43:16 +0530

Hi Khushboo,

> * purpose and the return values * are different for both the files.
The return value can be taken and tweaked to fit your purpose. If let's say
a new version of Postgres is released then that also need to be added in
schema diff along with get_version_mapping_directories. That would not be
the case if you use get_version_mapping_directories.

>> Any harm in this approach?
Obviously the extra append operations, which can be avoided.

>> This is pending.

> chrome and firefox will be more happy in terms of performance.
I'm not comparing "map" and "for" here. I'm trying to avoid
 sel_rows_data.push statement here which is directly proportional to the
number of selected rows.

> Can you please elaborate the approach which you are suggesting?
The fetch_objects_to_compare function fetches all the nodes first, and then
in a loop the properties for each node is fetched. Instead of that, all the
nodes along with their properties can be fetched in one go from the
database. Although this is no stopper, but it can be an improvement done in
future.

---

From: Khushboo Vashi <[email-masked]>
Date: Thu, 2 Jan 2020 15:08:08 +0530

On Thu, Jan 2, 2020 at 2:43 PM Aditya Toshniwal <
[email-masked]> wrote:

> The return value can be taken and tweaked to fit your purpose. If let's
> say a new version of Postgres is released then that also need to be added
> in schema diff along with get_version_mapping_directories. That would not
> be the case if you use get_version_mapping_directories.
>
I can do one thing, will take the versions dict at one place and both the
functions  get_version_mapping_directories and schema_diff can use that
version dict.
As, I am not in favour of tweaking get_version_mapping_directories function
as the name suggests something else.

> Obviously the extra append operations, which can be avoided.
Yeah, right but I don't think it is going to be a performance issue.

>>>    -> fetchData - We should not use async = false.
It's not pending, I left it as it is,  as I have copied this code from
backform.pgadmin.js and tweaked it accordingly, as we already cleaned up
the async: false in the past but this has not been changed. So, before
changing it, we need to analyse why we have not changed it.

>  sel_rows_data.push statement here which is directly proportional to the
At the end, it will return the new array according to the condition in both
the cases.

> The fetch_objects_to_compare function fetches all the nodes first, and
> then in a loop the properties for each node is fetched. Instead of that,
> all the nodes along with their properties can be fetched in one go from the
Sure, will look into it in the second phase.

---

From: Aditya Toshniwal <[email-masked]>
Date: Thu, 2 Jan 2020 15:18:37 +0530

Hi,

On Thu, Jan 2, 2020 at 3:08 PM Khushboo Vashi <
[email-masked]> wrote:

>>>>    1. The variables return_ajax_response, only_sql, json_resp as far
>>>>    as I understood are similar. Can we have same var name everywhere ?
>>>>
>>> in the table node itself.web/pgadmin/tools/schema_diff/__init__.py
>>> use get_version_mapping_directories from web/pgadmin/utils/versioned_template_loader.py
>>> ?
>>>
>> in schema diff along with get_version_mapping_directories. That would
>> not be the case if you use get_version_mapping_directories.
>>
> As, I am not in favour of tweaking get_version_mapping_directories
> function as the name suggests something else.
>
Neither am I suggesting to change  get_version_mapping_directories,  but to
use the return value as mentioned.

> Yeah, right but I don't think it is going to be a performance issue.
Well, up to you. There are lot many small things in pgAdmin which do not
affect performance individually, but collectively they do.

> changing it, we need to analyse why we have not changed it.
OK.

>>>> This is a debatable topic as there are pros and cons of both map and
>>> for loop. Like, it's more readable if we use map and in case of for loop,
> At the end, it will return the new array according to the condition in
> both the cases.
>
OK. I'm still not in favor of having "for" with extra "push". May be I'll
send a patch later.

>
>>>>    1.
>>>>    2. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js
>>>>    -Are we doing anything to handle failure.
>>>> The problem is, for the generate script icon, I have used the svg (as
>>> no similar icon in font-awesome) whereas for Filter, font-awesome is used.
>>> I can replace the Generate Script icon from font-awesome (can search for
>>> some similar icon) if Chetana agrees.
>>>

---

From: Khushboo Vashi <[email-masked]>
Date: Wed, 8 Jan 2020 11:36:51 +0530

Hi,

Please find the attached updated patch.

- The Generate Icon has been changed to
https://fontawesome.com/v4.7.0/icon/file-code-o .

Thanks,
Khushboo

On Thu, Jan 2, 2020 at 3:08 PM Khushboo Vashi <
[email-masked]> wrote:

>>>> I have checked the possibilities before using it in the schema diff.
>>> The * purpose and the return values * are different for both the files.
>>>
I have changed the logic for the version comparison, so now no need to hard
code the version values and no need to change the existing code.

---

From: Akshay Joshi <[email-masked]>
Date: Fri, 10 Jan 2020 15:43:49 +0530

Thanks, patch applied.

On Wed, Jan 8, 2020 at 11:37 AM Khushboo Vashi <
[email-masked]> wrote:

>>>>> keys_to_remove used to remove the table properties before comparison
>>>> as we combine all child nodes of the table while comparing and
>>>> keys_to_ignore is to ignore the keys while comparing. We have also used the
>>>> keys_to_ignore in the table node itself.
>>>>
>>>>>
> I have changed the logic for the version comparison, so now no need to
> hard code the version values and no need to change the existing code.
>
>>
>>>  sel_rows_data.push statement here which is directly proportional to
>>> the number of selected rows.
>>>
>>>>> As we open the schema diff in different frame, I think, this should
>>>> not be the issue.
>>>>
>>>>>
>>>>>    1.

-- 
*Thanks & Regards*
*Akshay Joshi*

*Sr. Software Architect*