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

Hi,

On Thu, Jan 2, 2020 at 3:08 PM Khushboo Vashi <
khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:

>
>
> 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.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.
>
Neither am I suggesting to change get_version_mapping_directories, but to
use the return value as mentioned.

>
>>>> 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.
>
Well, up to you. There are lot many small things in pgAdmin which do not
affect performance individually, but collectively they do.

>
>>>> 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.
>
OK.

>
>>>> 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.
>
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.
>>>> + 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"
>>
>

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