Re: Stalled post to pgadmin-hackers

Hi,

PFA patch with resolved review comments.

On Tue, May 17, 2016 at 6:46 PM, Murtuza Zabuawala <
murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:

> Hi Surinder,
>
> Please find my comments as below for latest v2. patch,
>
>
> 1) In CREATE mode, Removing Owner & Schema fields causes SQL to break
>
> CREATE OR REPLACE VIEW "<Response 159 bytes [500 INTERNAL SERVER
> ERROR]>"."test view" AS
> select * from table1;
>
> ALTER TABLE "<Response 159 bytes [500 INTERNAL SERVER ERROR]>"."test view"
> OWNER TO pem_agent;
>
> Steps to re-produce,
> - enter name
> - enter definition
> - remove owner & schema from select2 list
> go to SQL tab
>
Fixed.

>
>
> 2) Renaming view do not reflect on browser tree (We need to refresh
> manually)
>
> Steps to re-produce,
> - enter new name
>
> Click on Save
>
Fixed.

>
> 3) Please maintain one line spacing between SQL queries In the SQL Tab.
>
> SQL generated (current patch):
> --------------
> CREATE OR REPLACE VIEW pem.dsfasfa
> WITH (
> check_option=local,
> security_barrier=true
> ) AS
> select * from agent;
>
> ALTER TABLE pem.dsfasfa
> OWNER TO postgres;
> COMMENT ON VIEW pem.dsfasfa
> IS 'my comments...';
>
> SECURITY LABEL FOR pro ON VIEW dsfasfa IS 'lbl1';GRANT ALL ON TABLE
> pem.dsfasfa TO pem_admin;
>
Fixed.

>
> 4) EDIT Mode, Changing schema generates wrong SQL, eg: Schema changed to
> "Public" generated below sql,
>
> ALTER VIEW pem."Test_View"
> SET SCHEMA "2200";
>
Fixed.

>
> 5) EDIT Mode, If i remove check option, it do not generate SQL for that
> operation
>
Fixed.

>
> 6) EDIT Mode, If i modified definition, it creates new view but what about
> my current setting like privileges, security labels, security barrier etc
> Those should be preserved.
>
Fixed.

>
> 7) Security barrier is not being displayed on properties node
>
Fixed.

>
> 8) Rule node, Edit mode, I am not allowed to rename Rule, but as per
> postgres doc we can only rename it rest options should be disabled in edit
> mode.
> http://www.postgresql.org/docs/9.5/static/sql-alterrule.html
>
I will send separate patch for it once done.

>
> 9) Column node, 'Primary key' switch should be hidden under view node,
> refer column node visible condition for table node
>
I will send a add-on patch for this issue.

>
> 10) Same issue in MATERIALIZED VIEW as mentioned in Issue-1
>
Fixed.

>
> 11) Same issue in MATERIALIZED VIEW as mentioned in Issue-3
>
Fixed.

>
> 12) VaccumSetting options are not Sync with Table node, as per Postgres
> Docs it should use same settings as CREATE TABLE storage settings options.
>
> In Table: When we only set
> - Custom auto-vacuum? (table)
> - Custom auto-vacuum? (toast table)
>
> it generates SQL like below but in MATERIALIZED VIEW it does not,
>
> WITH (
> autovacuum_enabled = FALSE,
> toast.autovacuum_enabled = FALSE
> )
>
Fixed.

>
> 13) MATERIALIZED VIEW, Edit mode, remove one Privilege from MV and see
> there is no sql generated in SQL tab
>
Fixed.

>
> 14) MATERIALIZED VIEW, Edit mode, Security label do not work, try adding
> new security label
>
Not an issue. it wasn't working due to issue # 14.

>
> 15) MATERIALIZED VIEW, Edit mode, Changing the definition does nothing, it
> should drop & re-create MV with all previous options & along with new
> definition
>
Fixed.

>
> 16) SQL alignment,
>
> eg: generated sql
> ALTER MATERIALIZED VIEW pem."test_MV1"
> SET(FILLFACTOR = 25);
>
> Expected:
> ALTER MATERIALIZED VIEW pem."test_MV1"
> SET(FILLFACTOR = 25);
>
> Fixed.

> 17) MATERIALIZED VIEW, Edit mode, we should not allow to TableSpace
>
It is fixed. it is reproduced due to select control, its option
'allow_clear' should be set default to 'false'.

>
> 18) Same issue in MATERIALIZED VIEW as mentioned in Issue-4
>
Fixed.

>
> 19) Same issue in MATERIALIZED VIEW as mentioned in Issue-2
>
Fixed.

>
> 20) We can not Fill factore values in MV
>
Fixed.

>
> Regards,
> Murtuza
>
>
>
>
> ---------- Forwarded message ----------
> 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>
> Date: Tue, 17 May 2016 11:52:35 +0530
> Subject: Re: [pgadmin-hackers] [pgAdmin4][Patch]: View and Materialised
> View Nodes
> 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*
>>
>
>
>
> <view_and_mat_view_nodes_v2.patch>
>
>
>