Re: [pgAdmin][RM-2341]: Add menu option for starting PSQL

Hi Akshay/ Team

On Mon, May 24, 2021 at 9:19 PM Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
wrote:

> Hi Nikhil
>
> Following are the review comments:
>
> - Set "ENABLE_PSQL = False", PSQL button from browser tree and context
> menu option should not be visible.
> - Documentation screenshot should be in standard theme for
> consistency, and check the size it's very large. Take the screenshot with
> the new PSQL button on the browser tree.
> - Update 'menu_bar.rst' and 'toolbar.rst' with new changes.
> - Remove commented code (if any)
> - Check SonarQube (I haven't run)
>
> Please find the updated patch, resolve all the review comments, and update
the code to resolve the SonarQube issues.

>
> On Thu, May 20, 2021 at 2:52 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> Thanks Nikhil. Can someone else review this version please?
>>
>> On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <
>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Dave/ Team,
>>>
>>> On Wed, May 19, 2021 at 1:43 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>
>>>> Hi
>>>>
>>>> On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <
>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Dave/ Team,
>>>>>
>>>>> On Tue, May 18, 2021 at 8:41 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <
>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi Dave/Team,
>>>>>>
>>>>>>
>>>>>> On Mon, May 17, 2021 at 6:47 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <
>>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Akshay/ Team,
>>>>>>>>
>>>>>>>> Please find the attached updated patch for the psql tool.
>>>>>>>>
>>>>>>>
>>>>>>> Hmm, this version is also broken. There's a typo in
>>>>>>> editor_template.html on line 138 - it splits a string across two lines
>>>>>>> which throws an error. Having fixed that...
>>>>>>>
>>>>>>> I also note there's a lot of Javascript in that HTML file. That
>>>>>>> should be pushed into the webpacked bundle I think, and not included inline
>>>>>>> in HTML.
>>>>>>>
>>>>>> I have moved most of the code in the js file, few things are still in
>>>>>> HTML.
>>>>>>
>>>>>
>>>>> Hmm, yes - in particular, colours for the different themes. Please
>>>>> move them into the css for the themes. You have a mix of style, layout and
>>>>> code in this file which needs to be cleaned up.
>>>>> xterm V3 onwards they have provided the API to set the theme and other
>>>>> settings, earlier I tried with CSS to override the theme but couldn’t able
>>>>> to apply the theme properly as some style get applied as in-line style for
>>>>> the HTML, so used the API to set the theme.
>>>>>
>>>>
>>>> OK, but either way we can't hard-code styles from themes in HTML
>>>> templates for individual features; that way leads to madness.
>>>>
>>>> Perhaps Aditya or one of the other team members can give some
>>>> assistance?
>>>>
>>> I have moved the color settings to the respective theme files. Aditya
>>> helped in this.
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>> Speaking of themes, the background colour for selected text doesn't
>>>>>> seem right (it's barely visible) in the dark theme. Can you fix that to
>>>>>> match the colouring in the SQL text boxes please?
>>>>>>
>>>>> I tried the default selection color from SQL for the dark and standard
>>> themes but still, it was not readable so just updated the color code with
>>> another color as follows.
>>> 1. Dark Theme:
>>> [image: Screenshot 2021-05-19 at 6.29.43 PM.png]
>>> 2. High Contrast: (using default SQL selection color)
>>> [image: Screenshot 2021-05-19 at 6.59.52 PM.png]
>>> 3. Standard:
>>> [image: image.png]
>>> can we go with the colors or should we update it?
>>>
>>>>
>>>>>>
>>>>>>>
>>>>>>>> A couple of other things I noticed:
>>>>>>>>
>>>>>>>> - The button is enabled if the treeview has a Server selected. It
>>>>>>>> could be argued that the query tool should do the same (defaulting to the
>>>>>>>> maintenance database), however, that would be a separate change, and psql
>>>>>>>> should be consistent with the query tool.
>>>>>>>>
>>>>>>> It is now consistent with the query tool.
>>>>>>>
>>>>>>>>
>>>>>>>> - If I do a "select * from pg_class;" I still get:
>>>>>>>>
>>>>>>>> postgres=# select * from pg_class;
>>>>>>>> WARNING: terminal is not fully functional
>>>>>>>>
>>>>>>> I am not able to reproduce the warning for the terminal (I am
>>>>>>> working on Catalina 10.15.7), I checked on browsers (chrome, firefox,
>>>>>>> Safari) and also checked on local nwjs runtime but still not able to
>>>>>>> reproduce the warning. but found one limitation:
>>>>>>>
>>>>>>
>>>>>> It looks like that can be fixed by adding:
>>>>>>
>>>>>> env={'TERM': 'xterm'}
>>>>>>
>>>>> Added this in the environment when opening the psql panel.
>>>
>>>>
>>>>>> to the subprocess.Popen() call.
>>>>>>
>>>>>> I noticed while I was playing with that, that you are passing the
>>>>>> password as part of the connection string. As I've mentioned in the past,
>>>>>> that is absolutely not acceptable; it will expose the password to all
>>>>>> manner of tools (such as ps -ef). You *must* pass the password to psql
>>>>>> using the PGPASSWORD environment variable.
>>>>>>
>>>>>>
>>>>>>> if we try to load data from the table containing millions of
>>>>>>> records, UI gets very slow.
>>>>>>>
>>>>>> Removed the password from the connection string and added
>>> 'PGPASSWORD' in the environment.
>>>
>>>>
>>>>>> Is xtermjs discarding the older buffer contents when it fills up? Can
>>>>>> you tell where the memory usage is?
>>>>>>
>>>>> I checked the psql memory consumption in terminal and pgAdmin psql
>>>>> tool memory consumption is the similar. Also tested the performance and
>>>>> query execution timing is also similar.
>>>>>
>>>>
>>>> OK, so there's probably not much we can do here.
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>> - I'm sure using \q in the previous version displayed a message
>>>>>>>> saying the session exited (the one on line 138 of editor_template.html). It
>>>>>>>> no longer seems to do so.
>>>>>>>>
>>>>>>>
>>>>>> In addition to the issue above, it looks like the \! blocking may
>>>>>> have lost it's ability to ignore quoted strings:
>>>>>>
>>>>>> pgweb=# select '\!';
>>>>>> ERROR: Shell commands are disabled in psql for security
>>>>>>
>>>>> Fixed the issue now it is consistent with the psql terminal.
>>>
>>>>
>>>>>>
>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, May 11, 2021 at 3:40 PM Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hi
>>>>>>>>>>
>>>>>>>>>> On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <
>>>>>>>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi Nikhil
>>>>>>>>>>>
>>>>>>>>>>> Following are the review comments:
>>>>>>>>>>>
>>>>>>>>>>> *GUI specific*:
>>>>>>>>>>>
>>>>>>>>>>> - We need a panel icon for PSQL like query tool, we can also
>>>>>>>>>>> add that on the browser tree toolbar.
>>>>>>>>>>> - PSQL Tool menu should be visible for all the child nodes
>>>>>>>>>>> of the database node. Follow the same as Query Tool.
>>>>>>>>>>> - PSQL tab title should be only database server name as the
>>>>>>>>>>> user can change the database/user from PSQL command, so it's been difficult
>>>>>>>>>>> to update the tab title.
>>>>>>>>>>> - PSQL connection is still open even if we disconnect the
>>>>>>>>>>> database server from the browser tree.
>>>>>>>>>>>
>>>>>>>>>>> *Code specific:*
>>>>>>>>>>>
>>>>>>>>>>> - Remove an extra space from requirements.txt and
>>>>>>>>>>> package.json
>>>>>>>>>>> - Documentation needs to be updated to let the user know
>>>>>>>>>>> from where the PSQL tool will open and on which node it is applicable.
>>>>>>>>>>> - psql/__init__.py check there are so many unused imports
>>>>>>>>>>> please remove them.
>>>>>>>>>>> - We are not using cheroot so it should be removed from
>>>>>>>>>>> requirements.txt and also remove the import statement from pgAdmin4.py
>>>>>>>>>>> - Test cases are showing successful but actually, there are
>>>>>>>>>>> some routing errors please check.
>>>>>>>>>>>
>>>>>>>>>>> A few other things I noticed:
>>>>>>>>>>
>>>>>>>>>> - I was prompted to enter a password. This should be passed in
>>>>>>>>>> the environment to psql as it is for pg_dump etc.
>>>>>>>>>> - There seems to be an issue with terminal compatibility (which I
>>>>>>>>>> didn't have on my prototype):
>>>>>>>>>>
>>>>>>>>>> ml=# select * from pg_class;
>>>>>>>>>> WARNING: terminal is not fully functional
>>>>>>>>>> -[ RECORD 1
>>>>>>>>>> ]-------+----------------------------------------------
>>>>>>>>>> oid | 79354
>>>>>>>>>> relname | housing
>>>>>>>>>> ...
>>>>>>>>>>
>>>>>>>>>> - The panel should honour the styleguide. I'm running in dark
>>>>>>>>>> mode, and see a jet black background. I would expect to see the same
>>>>>>>>>> background/foreground colours as the treeview.
>>>>>>>>>> - I spotted at least one print() statement that shouldn't be
>>>>>>>>>> there (debug output should go through the logger) - psql/__init__.py:235
>>>>>>>>>> - This seems suspect - why would there be a password in a
>>>>>>>>>> connection string we've built? And why would it be xxx?
>>>>>>>>>>
>>>>>>>>>> if 'password=xxx' in conn_attr:
>>>>>>>>>> conn_attr = conn_attr.replace('password=xxx', '')
>>>>>>>>>>
>>>>>>>>>> - There's a thick white line at the bottom of the panel, where a
>>>>>>>>>> horizontal scrollbar might be if there was one.
>>>>>>>>>> - The trailing semi-colon should be removed from: "ERROR: Shell
>>>>>>>>>> commands are disabled in psql for security;"
>>>>>>>>>>
>>>>>>>>>> Once we're happy with the patch in general, I'll do a string
>>>>>>>>>> review before committing. In particular, I want to be sure the text in
>>>>>>>>>> config.py is appropriately worded.
>>>>>>>>>>
>>>>>>>>>> This is shaping up nicely! Good work.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Mon, May 10, 2021 at 7:32 PM Nikhil Mohite <
>>>>>>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi Dave/ Team,
>>>>>>>>>>>>
>>>>>>>>>>>> PFA updated patch, sorry for the inconvenience, while cleanup I
>>>>>>>>>>>> removed the unwanted libraries but forgot to remove the code related to
>>>>>>>>>>>> them.
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, May 10, 2021 at 7:10 PM Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <
>>>>>>>>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Hackers,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Please find the attached patch for RM-2341
>>>>>>>>>>>>>> <https://redmine.postgresql.org/issues/2341>: Add Menu
>>>>>>>>>>>>>> option for starting PSQL.
>>>>>>>>>>>>>> 1. Added new Option PSQL Tool in Tools menu.
>>>>>>>>>>>>>> 2. Added the same option for Server and Database nodes from
>>>>>>>>>>>>>> the tree view.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Unfortunately there's a trailing comma in package.json that
>>>>>>>>>>>>> makes it invalid. If I fix that, then I get the error below, so I'm
>>>>>>>>>>>>> guessing the intention was to actually include another package there?
>>>>>>>>>>>>>
>>>>>>>>>>>>> ERROR in ./pgadmin/tools/psql/static/js/psql_module.js 23:50-82
>>>>>>>>>>>>> Module not found: Error: Can't resolve 'local-echo-controller'
>>>>>>>>>>>>> in '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js'
>>>>>>>>>>>>> resolve 'local-echo-controller' in
>>>>>>>>>>>>> '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js'
>>>>>>>>>>>>> Parsed request is a module
>>>>>>>>>>>>> using description file:
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>>>>>>>>>>>>> ./pgadmin/tools/psql/static/js)
>>>>>>>>>>>>> aliased with mapping 'local-echo-controller':
>>>>>>>>>>>>> '/Users/dpage/git/pgadmin4/web/node_modules/local-echo' to
>>>>>>>>>>>>> '/Users/dpage/git/pgadmin4/web/node_modules/local-echo'
>>>>>>>>>>>>> using description file:
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>>>>>>>>>>>>> ./pgadmin/tools/psql/static/js)
>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>> configuration
>>>>>>>>>>>>> root path /Users/dpage/git/pgadmin4/web
>>>>>>>>>>>>> using description file:
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>>>>>>>>>>>>> ./Users/dpage/git/pgadmin4/web/node_modules/local-echo)
>>>>>>>>>>>>> no extension
>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo
>>>>>>>>>>>>> doesn't exist
>>>>>>>>>>>>> .js
>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.js
>>>>>>>>>>>>> doesn't exist
>>>>>>>>>>>>> .jsx
>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx
>>>>>>>>>>>>> doesn't exist
>>>>>>>>>>>>> as directory
>>>>>>>>>>>>>
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo
>>>>>>>>>>>>> doesn't exist
>>>>>>>>>>>>> using description file:
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>>>>>>>>>>>>> ./node_modules/local-echo)
>>>>>>>>>>>>> no extension
>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn't exist
>>>>>>>>>>>>> .js
>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo.js doesn't exist
>>>>>>>>>>>>> .jsx
>>>>>>>>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx doesn't exist
>>>>>>>>>>>>> as directory
>>>>>>>>>>>>>
>>>>>>>>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn't exist
>>>>>>>>>>>>> @ ./pgadmin/tools/psql/static/js/index.js 17:19-43
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2021-05-10 14:38:37: webpack 5.21.2 compiled with 1 error in
>>>>>>>>>>>>> 60041 ms
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Dave Page
>>>>>>>>>>>>> Blog: https://pgsnake.blogspot.com
>>>>>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>>>>>
>>>>>>>>>>>>> EDB: https://www.enterprisedb.com
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Nikhil Mohite
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> *Thanks & Regards*
>>>>>>>>>>> *Akshay Joshi*
>>>>>>>>>>> *pgAdmin Hacker | Principal Software Architect*
>>>>>>>>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>>>>>>>>
>>>>>>>>>>> *Mobile: +91 976-788-8246*
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Dave Page
>>>>>>>>>> Blog: https://pgsnake.blogspot.com
>>>>>>>>>> Twitter: @pgsnake
>>>>>>>>>>
>>>>>>>>>> EDB: https://www.enterprisedb.com
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>> Nikhil Mohite
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Dave Page
>>>>>>>> Blog: https://pgsnake.blogspot.com
>>>>>>>> Twitter: @pgsnake
>>>>>>>>
>>>>>>>> EDB: https://www.enterprisedb.com
>>>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> Nikhil Mohite
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Dave Page
>>>>>> Blog: https://pgsnake.blogspot.com
>>>>>> Twitter: @pgsnake
>>>>>>
>>>>>> EDB: https://www.enterprisedb.com
>>>>>>
>>>>>
>>>>>
>>>>> Regards,
>>>>> Nikhil Mohite
>>>>>
>>>>>> <https://www.enterprisedb.com>
>>>>>>
>>>>>> --
>>>>> *Thanks & Regards,*
>>>>> *Nikhil Mohite*
>>>>> *Software Engineer.*
>>>>> *EDB Postgres* <https://www.enterprisedb.com/>
>>>>> *Mob.No: +91-7798364578.*
>>>>>
>>>>
>>>>
>>>> --
>>>> Dave Page
>>>> Blog: https://pgsnake.blogspot.com
>>>> Twitter: @pgsnake
>>>>
>>>> EDB: https://www.enterprisedb.com
>>>>
>>>> Regards,
>>> Nikhil Mohite
>>>
>>
>>
>> --
>> Dave Page
>> Blog: https://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EDB: https://www.enterprisedb.com
>>
>>
>
> --
> *Thanks & Regards*
> *Akshay Joshi*
> *pgAdmin Hacker | Principal Software Architect*
> *EDB Postgres <http://edbpostgres.com>*
>
> *Mobile: +91 976-788-8246*
>