Re: [pgAdmin4][Patch]: View and Materialised View Nodes

From: Surinder Kumar <surinder(dot)kumar(at)enterprisedb(dot)com>
To: Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
Cc: pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: [pgAdmin4][Patch]: View and Materialised View Nodes
Date: 2016-05-17 06:22:35
Message-ID: CAM5-9D-gLAX+396BaQMC83F00gQREEMqrTuOMfKVx4dSY96VKw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

Hi,

PFA updated patch.
Please find inline comments.

On Mon, May 16, 2016 at 1:56 PM, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com
> wrote:

> Hi
>
> Below are my review comments:
>
> - Definition box on the View dialogue not expanded as needed. It
> should expand on resize.
>
> removed the fixed height of definition box, but its height is dependent on
the number of sql code lines in it.

>
> -
> - "Do Instead" on Rule node under View/M-View node not working. Unable
> to generate proper SQL.
>
> Done

>
> - I am still able to drop columns under view/mview node.
>
> Done

>
> - "Save" button is enable by default when user opens Materialised View
> dialog.
>
> I checked it is not reproducible.

>
> On Fri, May 13, 2016 at 11:44 PM, Surinder Kumar <
> surinder(dot)kumar(at)enterprisedb(dot)com> wrote:
>
>> Hi,
>>
>> Please find updated patch with fixed review comments:
>>
>> Most of the issues occurred because some code was missing in tables
>> subnodes patch.
>> Now I have shared the code related to table subnodes with harshal to
>> integrate in tables patch.
>>
>> This patch has dependency on tables patch.
>>
>> On Tue, May 10, 2016 at 7:46 PM, Akshay Joshi <
>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Surinder
>>>
>>> On Fri, Apr 29, 2016 at 8:07 PM, Surinder Kumar <surinder.kumar@
>>> enterprisedb.com> wrote:
>>>
>>>> Hi,
>>>>
>>>>
>>>> PFA patch for View and Materialised View Nodes.
>>>> This patch is dependent on *tables node* and its subnodes patch.
>>>> Please test this patch once latest tables node patch is submitted.
>>>>
>>>> I have merged one other patch:
>>>> *"Don't show Security group of node if it is under Catalogs"*
>>>>
>>>> http://www.postgresql.org/message-id/CAM5-9D8RNZXRG0ygvuRqf2wQAEnye-rqBHSrwcr34dnLCemy3w@mail.gmail.com
>>>>
>>>> Below are the fix for the review comments given by Dave:
>>>>
>>>> *Views*:
>>>>
>>>> - Please add sqlCreateHelp and sqlAlterHelp properties to all nodes.
>>>> *I have added these in View and Materialised View. *
>>>>
>>>> - Some of the SQL templates have inconsistent indenting (i.e. not 4
>>>> chars), e.g.
>>>>
>>>> CREATE OR REPLACE VIEW pem.avail_agents
>>>> AS
>>>> ...
>>>> *Fixed*
>>>>
>>>> - When creating any object, the Owner, Schema and Tablespace should
>>>> have default values.
>>>> *Fixed*
>>>>
>>>> - Property labels should only have the first word capitalised, e.g.
>>>>
>>>> Security barrier
>>>> Check options
>>>> *Fixed*
>>>>
>>>> - The Definition box on the View dialogue should start at a single
>>>> line and expand as needed - see the Function dialogue
>>>> *Fixed*
>>>>
>>>
>>> Still reproducible.
>>>
>>>>
>>>>
>>>> - The Cancel button doesn't work on the View dialog. Please check them
>>>> all.
>>>> *In latest code pull, It **seems to be** fixed. not reproducible at my
>>>> end.*
>>>>
>>>> - The Save button doesn't close the View dialog, nor does it add the
>>>> tree node.
>>>> *In latest code pull, It **seems to be** fixed. not reproducible at my
>>>> end.*
>>>>
>>>> - Dependencies and Dependents don't show icons, and have very tall
>>>> rows. This issue likely comes from elsewhere as I'm seeing it on other
>>>> object types as well.
>>>> Yes, it is general, previously images were visible. It is regression of
>>>> some commit. I will check it.
>>>>
>>>> - Reverse engineered SQL for a column on a view is shown like:
>>>>
>>>> <html><head></head><body>-- Column: id -- ALTER TABLE pem.avail_agents
>>>> DROP COLUMN id; ALTER TABLE pem.avail_agents ADD COLUMN id
>>>> integer(4);</body></html>
>>>>
>>>> (yes, it's displaying the HTML tags)
>>>> *I pulled the latest code, but it is not reproducible.*
>>>>
>>>> - The ACL property should be called Privileges and be in the Security
>>>> group (see functions, sequences etc).
>>>> *Fixed*
>>>>
>>>> - The Schema should not be shown in "Properties" view.
>>>> *Fixed*
>>>>
>>>> - Fields in the General section should be in the order: Name, OID,
>>>> Owner, System xxx? (where appropriate), Comment
>>>> *I have checked that In view Fields (Name, Owner, Schema & Comment) are
>>>> in this order.*
>>>> *I didn't got your point. Can you please give and example, if possible.*
>>>>
>>>
>>> Not Fixed yet.
>>>
>> Now it is fixed.
>>
>>>
>>>>
>>>>
>>>> *Materialised Views:*
>>>> - Selecting an MV results in:
>>>>
>>>> 2016-04-14 09:34:58,863: ERROR pgadmin: Exception on
>>>> /browser/materialized_view/obj/1/1/24587/27424/107612 [GET]
>>>> Traceback (most recent call last):
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/flask/app.py",
>>>> line 1817, in wsgi_app
>>>> response = self.full_dispatch_request()
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/flask/app.py",
>>>> line 1477, in full_dispatch_request
>>>> rv = self.handle_user_exception(e)
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/flask/app.py",
>>>> line 1381, in handle_user_exception
>>>> reraise(exc_type, exc_value, tb)
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/flask/app.py",
>>>> line 1475, in full_dispatch_request
>>>> rv = self.dispatch_request()
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/flask/app.py",
>>>> line 1461, in dispatch_request
>>>> return self.view_functions[rule.endpoint](**req.view_args)
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/flask/views.py",
>>>> line 84, in view
>>>> return self.dispatch_request(*args, **kwargs)
>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/browser/utils.py", line
>>>> 233, in dispatch_request
>>>> return method(*args, **kwargs)
>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/browser/server_
>>>> groups/servers/databases/schemas/views/__init__.py",
>>>> line 185, in wrap
>>>> return f(*args, **kwargs)
>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/browser/server_
>>>> groups/servers/databases/schemas/views/__init__.py",
>>>> line 1226, in properties
>>>> self.conn, result, 'table')
>>>> File "/Users/dpage/git/pgadmin4/web/pgadmin/browser/server_
>>>> groups/servers/databases/schemas/utils.py",
>>>> line 357, in parse_vacuum_data
>>>> vacuum_fields = render_template("vacuum_
>>>> settings/vacuum_fields.json")
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/flask/templating.py",
>>>> line 127, in render_template
>>>> return _render(ctx.app.jinja_env.get_or_select_template(template_
>>>> name_or_list),
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/jinja2/environment.py",
>>>> line 830, in get_or_select_template
>>>> return self.get_template(template_name_or_list, parent, globals)
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/jinja2/environment.py",
>>>> line 791, in get_template
>>>> return self._load_template(name, self.make_globals(globals))
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/jinja2/environment.py",
>>>> line 765, in _load_template
>>>> template = self.loader.load(self, name, globals)
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/jinja2/loaders.py",
>>>> line 113, in load
>>>> source, filename, uptodate = self.get_source(environment, name)
>>>> File "/Users/dpage/.virtualenvs/pgadmin4/lib/python2.7/site-
>>>> packages/flask/templating.py",
>>>> line 64, in get_source
>>>> raise TemplateNotFound(template)
>>>> TemplateNotFound: vacuum_settings/vacuum_fields.json
>>>>
>>>> *It seems this patch*
>>>>
>>>> http://www.postgresql.org/message-id/CAM5-9D-4DYgMMHFc5mzDDdK++h7XNvkiA5ojBbXi5sTHmecGCA@mail.gmail.com
>>>> *wasn't applied properly. Now this patch is sent with tables patch.*
>>>>
>>>> - When creating an MV, an error is immediately shown telling me to
>>>> specify a name. We do not normally show such errors until the field
>>>> loses focus.
>>>> *Now its fixed.*
>>>>
>>>
>>> Not fixed. Issue is reproducible.
>>>
>>>>
>>>>
>>>> - No default owner is shown in create mode.
>>>> *Fixed.*
>>>>
>>>> - "Please enter function definition." is shown as an error on the
>>>> definition field.
>>>> *It triggers this error message as it is mandatory.*
>>>>
>>>> - The Definition field is missing a label. If relying on the tab title
>>>> to be the label, the control should fill the tab (as with Security
>>>> options etc).
>>>> *Fixed.*
>>>>
>>>> - No default tablespace is shown.
>>>> *Fixed*
>>>>
>>>> - If I enable custom auto-vacuum, there is no way to add a row to
>>>> specify options.
>>>> *You can enable custom auto vacuum field and add new values in the auto
>>>> vacuum settings.*
>>>>
>>>> - If I specify a comment on an MV, I get "ERROR: "mv_pg_tables" is not
>>>> a view"
>>>> *For some reason tablespace name was missing in generated sql.*
>>>>
>>>
>>> Not Fixed. Wrong sql generated "COMMENT ON VIEW ..." instead of
>>> "COMMENT ON MATERIALIZED VIEW ..."
>>>
>> Fixed
>>
>>>
>>>>
>>>> - Save/Cancel buttons not working as expected (like other objects)
>>>> *In latest code pull, It **seems to be** fixed. not reproducible at my
>>>> end.*
>>>>
>>>
>>>
>>> Apart from above below are my review comments
>>>
>>> *Views*:-
>>>
>>> - As per pgAdmin3 "indexes" node should not be listed under Views.
>>>
>>> Done
>>
>>>
>>> - SQL not generated when changing the value of "Event" and "Do
>>> Instead" for Rule node under View node.
>>>
>>> Done
>>
>>>
>>> - Changing the "Event" in Rule node not working.
>>>
>>> Done
>>
>>>
>>> - As per pgAdmin3 user can't be able to create columns inside View
>>> node.
>>>
>>> Done
>>
>>>
>>> - User can't be able to delete/drop columns and system generated
>>> Rule's, Trigger's etc..
>>>
>>>
>> Done
>>
>>>
>>> - Found one issue when changing value of "Security Barrier" from
>>> "Yes" to "No" it is not reflected on GUI when we open the dialog
>>> again while in backend value is updated, but on GUI it is showing
>>> "Yes".
>>>
>>> Done
>>
>>>
>>> - Create -> Trigger menu is missing when any view node is selected.
>>>
>>> Done
>>
>>>
>>> *Materialized View:-*
>>>
>>> - *As per pgAdmin3 user can't be able to create columns inside
>>> Materialized View node.*
>>>
>>> Done
>>
>>>
>>> -
>>> -
>>> *User can't be able to delete/drop columns and system generated Rule's,
>>> Trigger's etc. *
>>>
>>> I have checked in pgAdmin3 that delete/drop option specific to rules has
>> 2 cases:
>> 1. If rule is system rule, user can't delete/drop it.
>> 2. If not system rule, it can drop dropped.
>> Done
>>
>>>
>>> -
>>> - Create -> Trigger/Rule/Index menu is missing when any materialized
>>> view node is selected.
>>>
>>> Done
>>
>>>
>>> - In pgAdmin3 we have two more refresh options "Refresh data" and
>>> "Refresh data concurrently" which is missing.
>>>
>>> Done
>>
>>>
>>> - "Custom Auto Vaccum" for Table and Toast Table not working.
>>>
>>> Done
>>
>>>
>>>
>>>>
>>>> Thanks,
>>>> Surinder Kumar
>>>>
>>>>
>>>>
>>>> --
>>>> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>>>> To make changes to your subscription:
>>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>>>
>>>>
>>>
>>>
>>> --
>>> *Akshay Joshi*
>>> *Principal Software Engineer *
>>>
>>>
>>>
>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>>>
>>
>>
>
>
> --
> *Akshay Joshi*
> *Principal Software Engineer *
>
>
>
> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
>

Attachment Content-Type Size
view_and_mat_view_nodes_v2.patch application/octet-stream 214.4 KB

In response to

Browse pgadmin-hackers by date

  From Date Subject
Next Message Akshay Joshi 2016-05-17 07:02:02 pgAdmin 4 commit: Fixed issue change 'Auto Commit' option from preferen
Previous Message Surinder Kumar 2016-05-17 06:18:13 Re: [PATCH] Tables node (pgAdmin4)