Subject: PATCH: Added Node Type & Catalog objects [pgAdmin4]

From: Murtuza Zabuawala <[email-masked]>
Date: Tue, 8 Mar 2016 19:08:34 +0530

Hi,

PFA patch to add new nodes in pgAdmin4.
1) Type node
2) Catalog objects

*Note:* Both above nodes depended on schema/catalog node, Please apply them
after latest patch of schema/catalog from Ashesh.
- Type node also depends on parse_priv_function_templates.patch (which I
sent in separate email today)

--
Regards,
Murtuza Zabuawala
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgadmin-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

---

From: Ashesh Vashi <[email-masked]>
Date: Fri, 11 Mar 2016 01:11:27 +0530

Committed the catalog_objects with few changes:

- Renamed to catalog_objects
- columns under it were renamed to catalog_object_column
- Some of the query were specific to PPAS moved them to specific SQL
templates.

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company
<http://www.enterprisedb.com>

*http://www.linkedin.com/in/asheshvashi*
<http://www.linkedin.com/in/asheshvashi>

On Tue, Mar 8, 2016 at 7:08 PM, Murtuza Zabuawala <
[email-masked]> wrote:

> *Note:* Both above nodes depended on schema/catalog node, Please apply
> them after latest patch of schema/catalog from Ashesh.

---

From: Dave Page <[email-masked]>
Date: Fri, 11 Mar 2016 16:11:30 +0000

On Tue, Mar 8, 2016 at 1:38 PM, Murtuza Zabuawala
<[email-masked]> wrote:
> Note: Both above nodes depended on schema/catalog node, Please apply them

The type node seems to need quite a bit of work - please review and
test it carefully before re-submitting. There's an updated patch
attached, and some review info below:

Changed in the attached patch:

- s/Oid/OID
- Set defaults for schema and owner
- Rename the Type Defintion group to Definition.
- Moved some properties (acl, members) into the appropriate group
- s/Enumration/Enumeration

To be fixed:

- This module is derived from SchemaChildModule, so why does it still
have the backend support SQL?

- I cleaned up most of the SQL, which was improperly indented in many
places. However I have not event tried to fix up the create.sql
scripts. These need reformatting to:
  - Use 4 character indents
  - Not start a line with a comma - e.g. "  ,FOO=bar", which should be
"  FOO=bar," (obviously the commas need to trail from the line
before).

- The "Show System Objects" option is not honoured.

- The members list should be delimited with ", " not ","

- The ACL columns don't match other objects on the subnode panel -
Grantee/Granter/Privileges should be Grantee/Privileges/Granter

- Attempting to add a security label with empty values gives: SECURITY
LABEL FOR None ON TYPE foo_enum IS NULL;

- The schema is omitted from GRANT statements when creating an object, e.g.

CREATE TYPE pem.foo_enum AS ENUM
    ('foo', 'bar', 'whizz');

ALTER TYPE pem.foo_enum    OWNER TO postgres;

COMMENT ON TYPE pem.foo_enum IS 'This is the foo enum';

GRANT ALL ON TYPE foo_enum TO pem_admin;

- Move remaining Properties display properties to the appropriate
group - e.g. ENUM labels should be in the Definition group. Only Name,
OID, Owner, Alias, System Type and Comment should be under General.

- The schema name is omitted when dropping a type, leading to: type
"foo_enum" does not exist

- The dependencies and dependents tabs don't display any data.

Thanks.

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

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

---

From: Dave Page <[email-masked]>
Date: Fri, 11 Mar 2016 16:14:05 +0000

And this time with the patch.

---

From: Murtuza Zabuawala <[email-masked]>
Date: Mon, 14 Mar 2016 17:51:42 +0530

Hi Dave,

Thank you for reviewing my patch.

I have made changes according to your feedback.

Please review updated patch.

Regards,
Murtuza

> > have the backend support SQL?
Done (Apologies I forgot to delete them)

> > "  FOO=bar," (obviously the commas need to trail from the line
Done

> > - The "Show System Objects" option is not honoured.
Done

> > - The members list should be delimited with ", " not ","
Done

> > Grantee/Granter/Privileges should be Grantee/Privileges/Granter
Done

> > GRANT ALL ON TYPE foo_enum TO pem_admin;
Done

> > OID, Owner, Alias, System Type and Comment should be under General.
Done

> > "foo_enum" does not exist
Done

> > - The dependencies and dependents tabs don't display any data.
Done

---

From: Dave Page <[email-masked]>
Date: Wed, 16 Mar 2016 13:02:00 +0000

Hi

> Please review updated patch.

OK, I think it's in pretty good shape now. Here's what I hope are the
last of the review comments, mostly minor things :-)

- I'm allowed to edit ENUM label names, but PostgreSQL doesn't support that.

- Security label fields resize in Edit mode. See recent changes by
Arun to fix that elsewhere.

- When editing a type, I cannot select a Grantee on the Privileges tab.

- When adding a composite type, I cannot type/select directly in the
grid, I have to expand the row.

- If I click ADD to add a new member to a composite type whilst the
previous row is expanded, the new row is added, but the expanded panel
remains linked to the first row.

- If I then click on the Edit button on the new row, I see two
expanded panels, neither of which is attached to their row (and
they're in the wrong order). I would only expect to see one panel at a
time.

- When creating an External type, the Input, Output, Send, Receive and
Analyze function lists are all blank. Is this expected? I would have
thought there would be system functions listed as there are for Typmod
in/out.

- Labels for Yes/No fields should typically end in a '?', e.g.
"Variable?", "Preferred?"

- s/Prefered/Preferred

- Why does "Range" type have a frame around it?

- If a field is there to select/display a function, it should have
"function" at the end of its label, e.g. "Canonical function"

- As above, but with type. E.g. "Element type"

- Some of the combo boxes have hint text of "Select from the list",
whilst others don't. This should be consistent.

- The colouring of "No" on the switches should be blue (see "System Schema?")

- s/Subtype/Sub-type/

- s/SubType diff/Sub-type diff function/

- Please remove the pre-9.1 support.

- In def create(self, gid, sid, did, scid), ensure error messages are
properly pluralised, e.g. 'Composite types require at least two
members' or 'External types require both Input & Output conversion
functions.'

- additional_features.sql is poorly formatted (lack of proper indentation)

Thanks!

---

From: Ashesh Vashi <[email-masked]>
Date: Wed, 16 Mar 2016 18:48:27 +0530

> - When editing a type, I cannot select a Grantee on the Privileges tab.
Is the grantor same as the current user?

If yes - then it is a bug, otherwise it is by design.
Because - you can not change privileges of a grantee, when it was given by
some other grantor.

But - if you delete the privileges for its grantor, it will remove
privileges for the privileges given by it too. (which is though, we have
not yet take care, and we will do it in later.)

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company
<http://www.enterprisedb.com/>

---

From: Dave Page <[email-masked]>
Date: Wed, 16 Mar 2016 14:52:50 +0000

On Wed, Mar 16, 2016 at 1:18 PM, Ashesh Vashi <[email-masked]
> wrote:

> Because - you can not change privileges of a grantee, when it was given by

This is when adding new privileges - which basically doesn't work as the
form won't validate because there is no grantee selected.

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 17 Mar 2016 15:38:20 +0530

Hi Dave,

Please find updated patch.

On 16-Mar-2016, at 6:32 pm, Dave Page <[email-masked]> wrote:

Hi

Done

Done

This is general issue with privilege control.
[We will add it in TODO list]

Because currently in Backgrid we do not have functionality like if I edit
one cell second cell re-renders according the value of first cell,
If we expand and select the type the other two fields (length &
precision)are dependent on the value of type for composite types they
become enable/disable accordingly.

This is general with sub-node collection control.
[We will add it in TODO list]

Dave, I was not able to test External type functionality completely because
it requires external type which is not inbuilt in postgres.
Once you create that custom type using C/C++ code, it will be listed in
those combobox by sql we are using to fetch external function types added
by user in postgres.

- Labels for Yes/No fields should typically end in a '?', e.g.
"Variable?", "Preferred?”

Done

- s/Prefered/Preferred

Done

So that we can make visible/hide all the control/elements related to Range
type as whole instead having separate code for each control/element.
That frame is because we used ‘FieldsetControl’ of backform to group all
the elements of Range type.

- If a field is there to select/display a function, it should have
"function" at the end of its label, e.g. "Canonical function”

Done

- As above, but with type. E.g. "Element type”

Done

Done

- The colouring of "No" on the switches should be blue (see "System
Schema?")

Done

- s/Subtype/Sub-type/

Done

Done

Done

functions.’

Done

Done

Thanks!

---

From: Dave Page <[email-masked]>
Date: Thu, 17 Mar 2016 10:43:01 +0000

Hi

> [We will add it in TODO list]

OK.

> precision)are dependent on the value of type for composite types they become
> enable/disable accordingly.

That's must-have functionality. Please add a TODO for it.

> [We will add it in TODO list]

OK.

> those combobox by sql we are using to fetch external function types added by
> user in postgres.

I wonder if installing PostGIS would allow that testing to be done.

> That frame is because we used ‘FieldsetControl’ of backform to group all the
> elements of Range type.

Can you make it a plain div with no border/title? That would fit in
better with the existing styling, assuming the columns line up
properly.

Thanks.

---

From: Harshal Dhumal <[email-masked]>
Date: Thu, 17 Mar 2016 16:50:19 +0530

Hi Dave,

-- 
*Harshal Dhumal*
*Software Engineer *

EenterpriseDB <http://www.enterprisedb.com>

> > [We will add it in TODO list]

I just sent a patch for this issue.

---

From: Murtuza Zabuawala <[email-masked]>
Date: Fri, 18 Mar 2016 12:45:47 +0530

Hi Dave,

Even after installing PosGIS I’m not able to create new custom external
type because the SQL does not return any value (Same behaviour as is in
pgAdmin3) , But I’m able to list all of it’s properties of external types
same as pgAdmin3 (PFA screenshots).

I have also attached patch for plain fields control (without lable &
border) for backform. (Please apply it before testing new type patch)

Thanks,

---

From: Murtuza Zabuawala <[email-masked]>
Date: Fri, 18 Mar 2016 12:56:41 +0530

Hi,

Compressed everything in Zip file and sending it again, Because got below
email for my last sent email.

*"Your message to pgadmin-hackers has been delayed, and requires the
approvalof the moderators, for the following reason(s):The message body is
too long (481753 > 204800)"*

*Hi Dave,*

*Even after installing PosGIS I’m not able to create new custom external
type because the SQL does not return any value (Same behaviour as is in
pgAdmin3) , But I’m able to list all of it’s properties of external types
same as pgAdmin3 (PFA screenshots).*

*I have also attached patch for plain fields control (without lable &
border) for backform. (Please apply it before testing new type patch)*

*Thanks,*

*--*
*Regards,*

*Murtuza ZabuawalaEnterpriseDB: http://www.enterprisedb.com
<http://www.enterprisedb.com/>The Enterprise PostgreSQL Company*

---

From: Dave Page <[email-masked]>
Date: Fri, 18 Mar 2016 16:52:45 +0000

Hi

On Fri, Mar 18, 2016 at 7:15 AM, Murtuza Zabuawala
<[email-masked]> wrote:
> Hi Dave,
>
> Even after installing PosGIS I’m not able to create new custom external type
> because the SQL does not return any value (Same behaviour as is in pgAdmin3)
> , But I’m able to list all of it’s properties of external types same as
> pgAdmin3 (PFA screenshots).

Hmm, I wonder if that's a pgAdmin 3 bug. Please check the CREATE TYPE
docs, and make sure the queries we're using are appropriate to find
suitable functions. It's possible we've messed that up.

> I have also attached patch for plain fields control (without lable & border)
> for backform. (Please apply it before testing new type patch)

That looks better. I'll commit that patch.

> - I'm allowed to edit ENUM label names, but PostgreSQL doesn't support that.

This issue still exists.

I also noticed some weirdness with the error message display - please
see the attached screenshots. The one complaining about the missing
name is as I'd expect. The second one is showing the error message an
additional time. Please look at that.

I've attached an updated patch - I cleaned up some strings and
localisation tags in this version.

Thanks.

---

From: Murtuza Zabuawala <[email-masked]>
Date: Tue, 22 Mar 2016 13:44:46 +0530

Hi Dave,

We can create new external type using below method, By running all of below
queries at the same time , we can not create separate external type by only
using create type statement.

So as per my discussion with Ashesh, We should not allow user to create
external type in pgAdmin4 but only show definition in edit mode.

CREATE OR REPLACE FUNCTION geometry_1_in(cstring)
        RETURNS geometry_1
        AS '$libdir/postgis-2.2','LWGEOM_in'
        LANGUAGE 'c' IMMUTABLE STRICT;

CREATE OR REPLACE FUNCTION geometry_1_out(geometry_1)
        RETURNS cstring
        AS '$libdir/postgis-2.2','LWGEOM_out'
        LANGUAGE 'c' IMMUTABLE STRICT;

CREATE OR REPLACE FUNCTION geometry_1_typmod_in(cstring[])
        RETURNS integer
        AS '$libdir/postgis-2.2','geometry_typmod_in'
        LANGUAGE 'c' IMMUTABLE STRICT;

CREATE OR REPLACE FUNCTION geometry_1_typmod_out(integer)
        RETURNS cstring
        AS '$libdir/postgis-2.2','postgis_typmod_out'
        LANGUAGE 'c' IMMUTABLE STRICT;

CREATE OR REPLACE FUNCTION geometry_1_analyze(internal)
        RETURNS bool
        AS '$libdir/postgis-2.2', 'gserialized_analyze_nd'
        LANGUAGE 'c' VOLATILE STRICT;

CREATE OR REPLACE FUNCTION geometry_1_recv(internal)
        RETURNS geometry_1
        AS '$libdir/postgis-2.2','LWGEOM_recv'
        LANGUAGE 'c' IMMUTABLE STRICT;

CREATE OR REPLACE FUNCTION geometry_1_send(geometry_1)
        RETURNS bytea
        AS '$libdir/postgis-2.2','LWGEOM_send'
        LANGUAGE 'c' IMMUTABLE STRICT;

CREATE TYPE public.geometry_1
   (INPUT=geometry_1_in,
       OUTPUT=geometry_1_out,
       RECEIVE=geometry_1_recv,
       SEND=geometry_1_send,
       TYPMOD_IN=geometry_1_typmod_in,
       TYPMOD_OUT=geometry_1_typmod_out,
       ANALYZE=geometry_1_analyze,
       CATEGORY='U', DEFAULT='',
       INTERNALLENGTH=-1, ALIGNMENT=double, STORAGE=MAIN);

Please find inline comment for other two issues & updated patch attached.

Regards,
Murtuza

On Friday 18 March 2016 10:22 PM, Dave Page wrote:

Hi

Hi Dave,

This issue still exists.

This is working fine for me with same patch, labels coming from server are
not editable but we can add new labels (PFA screenshot)

Done
(PFA patch for sub node control also, there was an issue with subnode
control error handling which I've fixed it, please apply patch then test
Type node)

Thanks.

---

From: Dave Page <[email-masked]>
Date: Tue, 22 Mar 2016 09:22:29 +0000

Hi

> external type in pgAdmin4 but only show definition in edit mode.

Hmm, would it not make sense to allow the user to create the shell
type as well (perhaps, with a new type of "SHELL")? Then they could do
what is needed (and that should be easy, as it's just CREATE TYPE
foo;)

For example:

CREATE TYPE box;

CREATE FUNCTION my_box_in_function(cstring) RETURNS box AS ... ;
CREATE FUNCTION my_box_out_function(box) RETURNS cstring AS ... ;

CREATE TYPE box (
    INTERNALLENGTH = 16,
    INPUT = my_box_in_function,
    OUTPUT = my_box_out_function
);

CREATE TABLE myboxes (
    id integer,
    description box
);

---

From: Dave Page <[email-masked]>
Date: Thu, 7 Apr 2016 12:36:41 +0100

Hi Murtuza

>     OUTPUT = my_box_out_function

In the interests of making progress, I've committed the most recent
patch, with a number of minor changes most significantly, the Postgres
docs and system catalogs seem to have different ideas about what to
call length, precision and scale. pgAdmin 3 followed the catalogs and
used length and precision, however I've updated pgAdmin 4 to use
"Length/precision" and "Scale" which is inline with the Postgres docs.
That's only in the UI though - the code follows the catalogs.

There are still a couple of issues - please provide fixes ASAP:

1) If you create a composite type that contains a sized type (e.g.
numeric(5, 4), the precision and scale are not shown if you later open
the properties dialogue, or in the reverse engineered SQL.

E.g. what pgAdmin3 shows as:

CREATE TYPE pem.blergh AS
   (c1 text COLLATE pg_catalog."C",
    c2 numeric(5));

Is shown by pgAdmin4 as:

CREATE TYPE pem.blergh AS
    (c1 text COLLATE pg_catalog."C", c2 numeric);

(adding the \n's would be good too).

2) If you select a different type of type in create mode, the new
options are shown below those for the previously selected type,
instead of replacing them. Please see the attached screenshot.

3) I would still like us to support External types. I believe the
simple option here is to re-add the code you had previously, and to
add a new type of type called "SHELL" as discussed in my previous
email above. The user would then be able to create a SHELL type, add
the required functions, then come back and create the EXTERNAL type.

I'll add cards to our internal kanban chart for these issues.

Thanks.

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 7 Apr 2016 17:11:58 +0530

Thanks Dave.

Sure will look in it ASAP.

Regards,
Murtuza

> On 07-Apr-2016, at 5:06 pm, Dave Page <[email-masked]> wrote:
> 
> Hi Murtuza
> 
> <Screen Shot 2016-04-07 at 12.33.11.png>

---

From: Murtuza Zabuawala <[email-masked]>
Date: Fri, 8 Apr 2016 19:22:37 +0530

Hi Dave,

Please find updated patches to fix all the mentioned issues.

There are three patches,
1) One for fixing rendering issue when selecting type
2) Added utility function to fetch full type name with length & precision
3) Added new Shell type for External types.

Regards,
Murtuza

---

From: Dave Page <[email-masked]>
Date: Wed, 13 Apr 2016 13:48:46 +0100

Hi

I'm seeing some more issues now, some related to new functionality:

- Please don't include the descriptive header for shell types in create.sql.

- Shell types can (and should) have owner/comments set if specified.

- The Type dropdown should just say "Shell" for shell types (no
additional text).

- Privileges and labels grids should be disabled for shell types.

- When I select External type, I see the following error (and a 500
error in the browser console):

2016-04-13 13:44:49,953: ERROR pgadmin: Failed to execute query
(execute_2darray) for the server #1 - DB:pem (Query-id: 4020898):
Error Message:ERROR:  syntax error at or near "and"
LINE 2:     CASE WHEN length(nspname) > 0 AND and length(nspname) !=...

- Length/Precision and scale are not shown in the dialogue for existing types.

Thanks.

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 14 Apr 2016 13:06:00 +0530

Hi Dave,

PFA updated patch which will fix mentioned issues.

Hopefully the last one :-)

Done

> Done (PFA screenshot)

> >> >> queries at the same time , we can not create separate external type
> by
> >> >> only

---

From: Dave Page <[email-masked]>
Date: Thu, 14 Apr 2016 12:41:06 +0100

Thanks - patch applied. Unfortunately there are 3 more minor issues
for you to fix:

- Renaming or changing the schema for a shell type should not be allowed.

- I'm allowed to try to add ACL entries or security labels to an
existing shell type. This should be disallowed.

- Changing the schema on a (non-shell) type doesn't work - the type
name is omitted, e.g.

ALTER TYPE pem
    SET SCHEMA pemhistory;

Which should be:

ALTER TYPE pem.foo
    SET SCHEMA pemhistory;

On Thu, Apr 14, 2016 at 8:36 AM, Murtuza Zabuawala
<[email-masked]> wrote:
>> >> >> We can create new external type using below method, By running all
>> >> >> of
>> >> >> below

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 14 Apr 2016 18:27:28 +0530

Hi Dave,

PFA patch to fixed given issues.

---

From: Dave Page <[email-masked]>
Date: Thu, 14 Apr 2016 14:14:10 +0100

Hi,

The first issue is only partially fixed - it'll still let me try to
rename a shell type.

Also, I notice when hitting Save on my changes, they're applied (if
they're valid), but the dialogue doesn't close. I see the following in
the browser console:

Uncaught TypeError: Cannot read property 'sessChanged' of
null(anonymous function) @ datamodel.js:333
_.each._.forEach @ underscore.js:153
pgBrowser.DataModel.Backbone.Model.extend.toJSON @ datamodel.js:322
(anonymous function) @ node.js:850
jQuery.event.dispatch @ jquery-1.11.2.js:4665
elemData.handle @ jquery-1.11.2.js:4333

On Thu, Apr 14, 2016 at 1:57 PM, Murtuza Zabuawala
<[email-masked]> wrote:
>> >> >> > type as well (perhaps, with a new type of "SHELL")? Then they
>> >> >> > could
>> >> >> > do

---

From: Murtuza Zabuawala <[email-masked]>
Date: Thu, 14 Apr 2016 19:01:39 +0530

Hi Dave,

Sorry my bad I misinterpreted, I thought you meant rename/changing schema
only.

PFA updated patch.

And For Dialog not closing issue is generic and I have already created
internal ticket for it.

On Thu, Apr 14, 2016 at 6:44 PM, Dave Page <[email-masked]> wrote:

> >> >> - Length/Precision and scale are not shown in the dialogue for
> existing
> >> >> types.
> >> >>
> >> > Done (PFA screenshot)
> >> >
> >> >>
> >> >> Thanks.
> >> >>
> >> >>
> >> >> >> >> So as per my discussion with Ashesh, We should not allow user
> to
> >> >> >> >> create

---

From: Dave Page <[email-masked]>
Date: Thu, 14 Apr 2016 15:46:40 +0100

Thanks - patch applied!