MIME-Version: 1.0
In-Reply-To: 
 <CAKKotZSda6+wgyzckDJPXKK3dc8U-UXho9hmaaBp+LwZzdnhaQ@mail.gmail.com>
References: 
 <CAKKotZSda6+wgyzckDJPXKK3dc8U-UXho9hmaaBp+LwZzdnhaQ@mail.gmail.com>
Date: Fri, 11 Mar 2016 16:11:30 +0000
Message-ID: 
 <CA+OCxoxNx3ghQZ+YnWeNpyVs_BaJE+yL7C6H-PUFh6ddpr061Q@mail.gmail.com>
Subject: Re: PATCH: Added Node Type & Catalog objects [pgAdmin4]
From: Dave Page <dpage@pgadmin.org>
To: Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com>
Cc: pgadmin-hackers <pgadmin-hackers@postgresql.org>
Content-Type: text/plain; charset=UTF-8
Precedence: bulk
Sender: pgadmin-hackers-owner@postgresql.org

On Tue, Mar 8, 2016 at 1:38 PM, Murtuza Zabuawala
<murtuza.zabuawala@enterprisedb.com> wrote:
> 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)

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


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