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

On Thu, Jan 2, 2020 at 2:43 PM Aditya Toshniwal <
aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:

> Hi Khushboo,
>
> On Thu, Jan 2, 2020 at 12:04 PM Khushboo Vashi <
> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> 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 <
>> aditya(dot)toshniwal(at)enterprisedb(dot)com> 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)
>>>
>>> Done
>>
>>>
>>> 1. The variables return_ajax_response, only_sql, json_resp as far as
>>> I understood are similar. Can we have same var name everywhere ?
>>>
>>> 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 -
>>> web/pgadmin/browser/server_groups/servers/databases/schemas/tables/__init__.py
>>> -> get_sql_from_table_diff
>>>
>>> Done
>>
>>>
>>> 1. 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 ?
>>>
>>> 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
>>> -> check_version_compatibility has hardcoded version numbers. Can we
>>> use get_version_mapping_directories from
>>> web/pgadmin/utils/versioned_template_loader.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.
>>
> 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.

>
>>> 1. Rename .reallyHidden to .really-hidden
>>>
>>> Done
>>
>>>
>>> 1. 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.
>>>
>>> Done
>>
>>>
>>> 1.
>>> 2. .slick-group-toggle.collapsed, .slick-group-toggle.expanded -
>>> svgs are not required. Font awesome has the icons. refer - .obj_properties
>>> .collapsed .caret::before.
>>>
>>> Done
>>
>>>
>>> 1.
>>> 2. 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));
>>> + }
>>> +};
>>>
>>> Any harm in this approach?
>>
> Obviously the extra append operations, which can be avoided.
>
Yeah, right but I don't think it is going to be a performance issue.

>
>>> 1.
>>> 2. In
>>> web/pgadmin/tools/schema_diff/static/js/schema_diff.backform.js
>>> -> fetchData - We should not use async = false.
>>> + $.ajax({
>>> + async: false,
>>> + url: url,
>>> + })
>>>
>>> This is pending.
>
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.

>
>>> 1. In web/pgadmin/tools/schema_diff/static/js/schema_diff_ui.js -
>>> Use 'sources/window' - pgWindow.
>>>
>>>
>>> 1. + 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');
>>>
>>> Done
>>
>>>
>>> 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.
>>> + 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;
>>>
>>> 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.
>>
> 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.
>
At the end, it will return the new array according to the condition in both
the cases.

>
>>> 1.
>>> 2. 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
>>> + });
>>> +
>>> + }
>>>
>>> Forgot to handle, now added.
>>
>>>
>>> 1.
>>> 2. 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() {
>>>
>>> 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 are used by tools like Katacoda.
>>> [image: Screenshot 2019-12-27 at 16.40.47.png]
>>>
>>> Fixed, good catch.
>>
>>>
>>> 1.
>>> 2. Comparing objects loader is not attached to DDL Comparison panel.
>>> [image: compare_overlay.png]
>>>
>>> Fixed.
>>
>>>
>>> 1.
>>> 2. Filter icon and Generate script icon size are different. Also
>>> change icons CSS to use font-icon. You can refer icons from sqleditor.
>>> [image: Screenshot 2019-12-27 at 12.18.00.png]
>>>
>>> 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 <chethana(dot)kumar(at)enterprisedb(dot)com> , 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.
>>>
>>> *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.*
>>>
>>
>> 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.
>
Sure, will look into it in the second phase.

>
>> Thanks,
>> Khushboo
>>
>>>
>>> On Fri, Dec 20, 2019 at 6:59 PM Akshay Joshi <
>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>
>>>> 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*
>>>>
>>>
>>>
>>> --
>>> 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"
>