Home > mailing lists

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

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

From

Khushboo Vashi

Date:

06 June 2018, 12:42:28

Hi,

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

Thanks,

Khushboo

Attachment

RM_3362.patch

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

From

Khushboo Vashi

Date:

07 June 2018, 16:27:50

Hi,

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

Thanks,

Khushboo

On Wed, Jun 6, 2018 at 12:12 PM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> 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

Attachment

RM_3362_ver1.patch

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

From

Dave Page

Date:

08 June 2018, 14:51:34

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> 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

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

From

Khushboo Vashi

Date:

12 June 2018, 13:09:48

Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> 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

Attachment

RM_3362_ver2.patch

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

From

Victoria Henry

Date:

12 June 2018, 22:06:19

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 <khushboo.vashi@enterprisedb.com> wrote:

Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <dpage@pgadmin.org> wrote:
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> 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

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

From

Khushboo Vashi

Date:

13 June 2018, 10:02:48

Hi Victoria,

The updated patch is attached.

On Tue, Jun 12, 2018 at 9:36 PM, Victoria Henry <vhenry@pivotal.io> 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 <khushboo.vashi@enterprisedb.com> wrote:
Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <dpage@pgadmin.org> wrote:
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> 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

Attachment

RM_3362_ver3.patch

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

From

Khushboo Vashi

Date:

13 June 2018, 17:43:48

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

On Wed, Jun 13, 2018 at 9:32 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Hi Victoria,
The updated patch is attached.
On Tue, Jun 12, 2018 at 9:36 PM, Victoria Henry <vhenry@pivotal.io> 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 <khushboo.vashi@enterprisedb.com> wrote:
Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <dpage@pgadmin.org> wrote:
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> 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

Attachment

RM_3362_ver4.patch

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

From

Dave Page

Date:

13 June 2018, 20:03:32

Thanks, patch applied.

On Wed, Jun 13, 2018 at 12:43 PM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:

Please ignore my previous patch. Find the attached updated patch.
On Wed, Jun 13, 2018 at 9:32 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi Victoria,
The updated patch is attached.
On Tue, Jun 12, 2018 at 9:36 PM, Victoria Henry <vhenry@pivotal.io> 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 <khushboo.vashi@enterprisedb.com> wrote:
Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <dpage@pgadmin.org> wrote:
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> 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

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

From

Victoria Henry

Date:

13 June 2018, 21:00:41

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 <dpage@pgadmin.org> wrote:

Thanks, patch applied.
On Wed, Jun 13, 2018 at 12:43 PM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Please ignore my previous patch. Find the attached updated patch.
On Wed, Jun 13, 2018 at 9:32 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi Victoria,
The updated patch is attached.
On Tue, Jun 12, 2018 at 9:36 PM, Victoria Henry <vhenry@pivotal.io> 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 <khushboo.vashi@enterprisedb.com> wrote:
Hi,

Please find the attached updated patch.

On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <dpage@pgadmin.org> wrote:
Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> 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

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

From

Dave Page

Date:

13 June 2018, 21:25:15

On Wed, Jun 13, 2018 at 4:00 PM, Victoria Henry <vhenry@pivotal.io> 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