[pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

Khushboo Vashi
Hi,

Please find the attached patch to fix #3362 - Fix the functions for PG v11, and add support procedure for PG v11.

Thanks,
Khushboo


RM_3362.patch (133K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

Khushboo Vashi
Hi,

Please ignore my previous patch, find the attached updated one.

Thanks,
Khushboo

On Wed, Jun 6, 2018 at 12:12 PM, Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached patch to fix #3362 - Fix the functions for PG v11, and add support procedure for PG v11.

Thanks,
Khushboo



RM_3362_ver1.patch (133K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

Dave Page-7
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <[hidden email]> wrote:
Hi,

Please ignore my previous patch, find the attached updated one.

I found a couple of issues with this:

- Clicking the + button on the Parameters tab does nothing in either Create or Edit modes

- The debugger fails to start (though, perhaps that's because the plugin doesn't have Ashesh's latest patches in it).

FYI, I was trying to test the debugger with:

-- PROCEDURE: public.dummy_proc(integer)

-- DROP PROCEDURE public.dummy_proc(integer);

CREATE OR REPLACE  PROCEDURE public.dummy_proc(
id integer)
LANGUAGE 'plpgsql'

AS $BODY$BEGIN
  raise notice 'id is %', id;
END;$BODY$;

Thanks!

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Reply | Threaded
Open this post in threaded view
|

Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

Khushboo Vashi
Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <[hidden email]> wrote:
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <[hidden email]> wrote:
Hi,

Please ignore my previous patch, find the attached updated one.

I found a couple of issues with this:

- Clicking the + button on the Parameters tab does nothing in either Create or Edit modes

Fixed 
- The debugger fails to start (though, perhaps that's because the plugin doesn't have Ashesh's latest patches in it).

FYI, I was trying to test the debugger with:

-- PROCEDURE: public.dummy_proc(integer)

-- DROP PROCEDURE public.dummy_proc(integer);

CREATE OR REPLACE  PROCEDURE public.dummy_proc(
id integer)
LANGUAGE 'plpgsql'

AS $BODY$BEGIN
  raise notice 'id is %', id;
END;$BODY$;

Fixed. Tested with the latest code of the plugin.
Thanks!

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

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


RM_3362_ver2.patch (142K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

Victoria Henry

Hi Khushboo,

The following change is allowing the creation of procedures in postgresql versions less then 11 and also GreenPlum

--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
@@ -71,11 +71,7 @@ define('pgadmin.node.procedure', [
         if ('catalog' in node_hierarchy)
           return false;

-        // Procedures supported only in PPAS
-        return (
-          'server' in node_hierarchy &&
-            node_hierarchy['server'].server_type == 'ppas'
-        );
+        return true;

Now that the Procedures are a thing in Postgresql maybe they should live in their own module.
In the tests for trigger functions we are not consistent on the naming of the utils , in some places we call it funcs_utils in others trigger_funcs_utils.

Thanks

Victoria & Joao

On Tue, Jun 12, 2018 at 3:10 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <[hidden email]> wrote:
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <[hidden email]> wrote:
Hi,

Please ignore my previous patch, find the attached updated one.

I found a couple of issues with this:

- Clicking the + button on the Parameters tab does nothing in either Create or Edit modes

Fixed 
- The debugger fails to start (though, perhaps that's because the plugin doesn't have Ashesh's latest patches in it).

FYI, I was trying to test the debugger with:

-- PROCEDURE: public.dummy_proc(integer)

-- DROP PROCEDURE public.dummy_proc(integer);

CREATE OR REPLACE  PROCEDURE public.dummy_proc(
id integer)
LANGUAGE 'plpgsql'

AS $BODY$BEGIN
  raise notice 'id is %', id;
END;$BODY$;

Fixed. Tested with the latest code of the plugin.
Thanks!

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

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

Reply | Threaded
Open this post in threaded view
|

Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

Khushboo Vashi
Hi Victoria,

The updated patch is attached.

On Tue, Jun 12, 2018 at 9:36 PM, Victoria Henry <[hidden email]> wrote:

Hi Khushboo,

The following change is allowing the creation of procedures in postgresql versions less then 11 and also GreenPlum

--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
@@ -71,11 +71,7 @@ define('pgadmin.node.procedure', [
         if ('catalog' in node_hierarchy)
           return false;

-        // Procedures supported only in PPAS
-        return (
-          'server' in node_hierarchy &&
-            node_hierarchy['server'].server_type == 'ppas'
-        );
+        return true;

Fixed. 

Now that the Procedures are a thing in Postgresql maybe they should live in their own module. 

The main functionalities of the functions and procedures are almost same and we have inherited most of the things from function itself. 
So, as per me they should live in one module.

In the tests for trigger functions we are not consistent on the naming of the utils , in some places we call it funcs_utils in others trigger_funcs_utils.

Fixed. 

Thanks

Victoria & Joao

Thanks,
Khushboo 
On Tue, Jun 12, 2018 at 3:10 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <[hidden email]> wrote:
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <[hidden email]> wrote:
Hi,

Please ignore my previous patch, find the attached updated one.

I found a couple of issues with this:

- Clicking the + button on the Parameters tab does nothing in either Create or Edit modes

Fixed 
- The debugger fails to start (though, perhaps that's because the plugin doesn't have Ashesh's latest patches in it).

FYI, I was trying to test the debugger with:

-- PROCEDURE: public.dummy_proc(integer)

-- DROP PROCEDURE public.dummy_proc(integer);

CREATE OR REPLACE  PROCEDURE public.dummy_proc(
id integer)
LANGUAGE 'plpgsql'

AS $BODY$BEGIN
  raise notice 'id is %', id;
END;$BODY$;

Fixed. Tested with the latest code of the plugin.
Thanks!

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

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



RM_3362_ver3.patch (143K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

Khushboo Vashi
Please ignore my previous patch. Find the attached updated patch.

On Wed, Jun 13, 2018 at 9:32 AM, Khushboo Vashi <[hidden email]> wrote:
Hi Victoria,

The updated patch is attached.

On Tue, Jun 12, 2018 at 9:36 PM, Victoria Henry <[hidden email]> wrote:

Hi Khushboo,

The following change is allowing the creation of procedures in postgresql versions less then 11 and also GreenPlum

--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
@@ -71,11 +71,7 @@ define('pgadmin.node.procedure', [
         if ('catalog' in node_hierarchy)
           return false;

-        // Procedures supported only in PPAS
-        return (
-          'server' in node_hierarchy &&
-            node_hierarchy['server'].server_type == 'ppas'
-        );
+        return true;

Fixed. 

Now that the Procedures are a thing in Postgresql maybe they should live in their own module. 

The main functionalities of the functions and procedures are almost same and we have inherited most of the things from function itself. 
So, as per me they should live in one module.

In the tests for trigger functions we are not consistent on the naming of the utils , in some places we call it funcs_utils in others trigger_funcs_utils.

Fixed. 

Thanks

Victoria & Joao

Thanks,
Khushboo 
On Tue, Jun 12, 2018 at 3:10 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <[hidden email]> wrote:
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <[hidden email]> wrote:
Hi,

Please ignore my previous patch, find the attached updated one.

I found a couple of issues with this:

- Clicking the + button on the Parameters tab does nothing in either Create or Edit modes

Fixed 
- The debugger fails to start (though, perhaps that's because the plugin doesn't have Ashesh's latest patches in it).

FYI, I was trying to test the debugger with:

-- PROCEDURE: public.dummy_proc(integer)

-- DROP PROCEDURE public.dummy_proc(integer);

CREATE OR REPLACE  PROCEDURE public.dummy_proc(
id integer)
LANGUAGE 'plpgsql'

AS $BODY$BEGIN
  raise notice 'id is %', id;
END;$BODY$;

Fixed. Tested with the latest code of the plugin.
Thanks!

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

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




RM_3362_ver4.patch (143K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

Dave Page-7
Thanks, patch applied.

On Wed, Jun 13, 2018 at 12:43 PM, Khushboo Vashi <[hidden email]> wrote:
Please ignore my previous patch. Find the attached updated patch.

On Wed, Jun 13, 2018 at 9:32 AM, Khushboo Vashi <[hidden email]> wrote:
Hi Victoria,

The updated patch is attached.

On Tue, Jun 12, 2018 at 9:36 PM, Victoria Henry <[hidden email]> wrote:

Hi Khushboo,

The following change is allowing the creation of procedures in postgresql versions less then 11 and also GreenPlum

--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
@@ -71,11 +71,7 @@ define('pgadmin.node.procedure', [
         if ('catalog' in node_hierarchy)
           return false;

-        // Procedures supported only in PPAS
-        return (
-          'server' in node_hierarchy &&
-            node_hierarchy['server'].server_type == 'ppas'
-        );
+        return true;

Fixed. 

Now that the Procedures are a thing in Postgresql maybe they should live in their own module. 

The main functionalities of the functions and procedures are almost same and we have inherited most of the things from function itself. 
So, as per me they should live in one module.

In the tests for trigger functions we are not consistent on the naming of the utils , in some places we call it funcs_utils in others trigger_funcs_utils.

Fixed. 

Thanks

Victoria & Joao

Thanks,
Khushboo 
On Tue, Jun 12, 2018 at 3:10 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <[hidden email]> wrote:
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <[hidden email]> wrote:
Hi,

Please ignore my previous patch, find the attached updated one.

I found a couple of issues with this:

- Clicking the + button on the Parameters tab does nothing in either Create or Edit modes

Fixed 
- The debugger fails to start (though, perhaps that's because the plugin doesn't have Ashesh's latest patches in it).

FYI, I was trying to test the debugger with:

-- PROCEDURE: public.dummy_proc(integer)

-- DROP PROCEDURE public.dummy_proc(integer);

CREATE OR REPLACE  PROCEDURE public.dummy_proc(
id integer)
LANGUAGE 'plpgsql'

AS $BODY$BEGIN
  raise notice 'id is %', id;
END;$BODY$;

Fixed. Tested with the latest code of the plugin.
Thanks!

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

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






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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Reply | Threaded
Open this post in threaded view
|

Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

Victoria Henry
Hi,

Khushboo, thanks for making some of the changes we mentioned in our previous email.
But when you say that they share logic and the code is

if self.node_type == 'procedure':
50+ lines of code
else:
50+ lines of code

This tells us that they share some logic but there is a huge difference between them. This breaks the Single Responsibility Principle, that says that each class should have only one responsibility on the context of the application. Even if we completely ignore SOLID principles the clear opportunity for a refactor that would make the code much easier to read was lost, when last Monday we agreed that we should not lose these opportunities.

Dave, we don't understand why this change was committed into master, when there was a clear disagreement. 


Sincerely,

Victoria && Joao

On Wed, Jun 13, 2018 at 10:03 AM Dave Page <[hidden email]> wrote:
Thanks, patch applied.

On Wed, Jun 13, 2018 at 12:43 PM, Khushboo Vashi <[hidden email]> wrote:
Please ignore my previous patch. Find the attached updated patch.

On Wed, Jun 13, 2018 at 9:32 AM, Khushboo Vashi <[hidden email]> wrote:
Hi Victoria,

The updated patch is attached.

On Tue, Jun 12, 2018 at 9:36 PM, Victoria Henry <[hidden email]> wrote:

Hi Khushboo,

The following change is allowing the creation of procedures in postgresql versions less then 11 and also GreenPlum

--- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/procedure.js
@@ -71,11 +71,7 @@ define('pgadmin.node.procedure', [
         if ('catalog' in node_hierarchy)
           return false;

-        // Procedures supported only in PPAS
-        return (
-          'server' in node_hierarchy &&
-            node_hierarchy['server'].server_type == 'ppas'
-        );
+        return true;

Fixed. 

Now that the Procedures are a thing in Postgresql maybe they should live in their own module. 

The main functionalities of the functions and procedures are almost same and we have inherited most of the things from function itself. 
So, as per me they should live in one module.

In the tests for trigger functions we are not consistent on the naming of the utils , in some places we call it funcs_utils in others trigger_funcs_utils.

Fixed. 

Thanks

Victoria & Joao

Thanks,
Khushboo 
On Tue, Jun 12, 2018 at 3:10 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <[hidden email]> wrote:
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <[hidden email]> wrote:
Hi,

Please ignore my previous patch, find the attached updated one.

I found a couple of issues with this:

- Clicking the + button on the Parameters tab does nothing in either Create or Edit modes

Fixed 
- The debugger fails to start (though, perhaps that's because the plugin doesn't have Ashesh's latest patches in it).

FYI, I was trying to test the debugger with:

-- PROCEDURE: public.dummy_proc(integer)

-- DROP PROCEDURE public.dummy_proc(integer);

CREATE OR REPLACE  PROCEDURE public.dummy_proc(
id integer)
LANGUAGE 'plpgsql'

AS $BODY$BEGIN
  raise notice 'id is %', id;
END;$BODY$;

Fixed. Tested with the latest code of the plugin.
Thanks!

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

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






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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Reply | Threaded
Open this post in threaded view
|

Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

Dave Page-7


On Wed, Jun 13, 2018 at 4:00 PM, Victoria Henry <[hidden email]> wrote:
Hi,

Khushboo, thanks for making some of the changes we mentioned in our previous email.
But when you say that they share logic and the code is

if self.node_type == 'procedure':
50+ lines of code
else:
50+ lines of code

This tells us that they share some logic but there is a huge difference between them. This breaks the Single Responsibility Principle, that says that each class should have only one responsibility on the context of the application.

The responsibility here is to handle the objects from pg_proc which are fundamentally almost identical bar a couple of nuances in the SQL spec. Looking at the code you quote (or at least that I think you're quoting), I see there's maybe duplication of 50% - 75% or so of the code in each branch which probably could be cleaned up.
 
Even if we completely ignore SOLID principles the clear opportunity for a refactor that would make the code much easier to read was lost, when last Monday we agreed that we should not lose these opportunities.

Don't mistake not spotting an opportunity with intentionally ignoring it.
 

Dave, we don't understand why this change was committed into master, when there was a clear disagreement. 

I must be missing an email then. I only saw one from you previously, to which Khushboo addressed all the concerns you raised. 

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

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