public inbox for [email protected]
help / color / mirror / Atom feedFrom: Dave Page <[email protected]>
To: Nikhil Mohite <[email protected]>
Cc: Akshay Joshi <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin][RM-2341]: Add menu option for starting PSQL
Date: Thu, 20 May 2021 10:22:15 +0100
Message-ID: <CA+OCxoxpyfJabnycSGmtUK1-QEndNfZV3q2d1xP-oftDn-pGDw@mail.gmail.com> (raw)
In-Reply-To: <CAOBg0AO6A10OHiQAOOguagW1mNzfK1oVPHG9W+JSCkYjR+xQkA@mail.gmail.com>
References: <CAOBg0AO6Tjksb+EOA_-O13yRzdL_2d3y6G74m=GGQS+Ymjv=0g@mail.gmail.com>
<CA+OCxow9+UDMrsBBvJKW2AeAYaN4cO3w7kBGNKtsWhSM6_D7gQ@mail.gmail.com>
<CAOBg0AMVyLBma8wsbZ-VQWF4q3OAhqWeRLpmrxVcM_YOKH+MDQ@mail.gmail.com>
<CANxoLDd2xMu7ym9PFdhYUSy4ucNuPoZ2fvaRkuwyB+EqowLXwA@mail.gmail.com>
<CA+OCxowoz1oFE=eMKEnfFtf=oDrkPKHGfZpUGRW-sG_tpfMBTw@mail.gmail.com>
<CAOBg0ANFNvtD-LoFxXcLY5JJ4BVH5zNyrmdBED6B++-nKf012g@mail.gmail.com>
<CA+OCxowW7BEZzULbEpn73ygqWtWkUwdCAB3PMcHocUeQXcM3ow@mail.gmail.com>
<CAOBg0APGPTA4qNNryAXMwKP63uFLxQdQwnFdDXMjSr4OeH-HAg@mail.gmail.com>
<CA+OCxozYus58i3BhP37E-2Xc6Nq5zrJegcq+eANWpvN++_LtXw@mail.gmail.com>
<CAOBg0AO7NfJjLLFD8g5NkkbbnKiCSR5g9yshxc-65sWk2sFyBA@mail.gmail.com>
<CA+OCxoxdaBmh6e4Fo1F_+vZExq8SPUyGAByRzOPy2kz_vgRiSA@mail.gmail.com>
<CAOBg0AO6A10OHiQAOOguagW1mNzfK1oVPHG9W+JSCkYjR+xQkA@mail.gmail.com>
Thanks Nikhil. Can someone else review this version please?
On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <
[email protected]> wrote:
> Hi Dave/ Team,
>
> On Wed, May 19, 2021 at 1:43 PM Dave Page <[email protected]> wrote:
>
>> Hi
>>
>> On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <
>> [email protected]> wrote:
>>
>>> Hi Dave/ Team,
>>>
>>> On Tue, May 18, 2021 at 8:41 PM Dave Page <[email protected]> wrote:
>>>
>>>> Hi
>>>>
>>>> On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <
>>> [email protected]> wrote:
>>>
>>>> Hi Dave/Team,
>>>>
>>>>
>>>> On Mon, May 17, 2021 at 6:47 PM Dave Page <[email protected]> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <
>>>>> [email protected]> 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 <[email protected]> wrote:
>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <
>>>>>>>> [email protected]> 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 <
>>>>>>>>> [email protected]> 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 <[email protected]>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi
>>>>>>>>>>>
>>>>>>>>>>> On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <
>>>>>>>>>>> [email protected]> 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
Attachments:
[image/png] Screenshot 2021-05-19 at 6.29.43 PM.png (81.5K, 3-Screenshot%202021-05-19%20at%206.29.43%20PM.png)
download | view image
[image/png] Screenshot 2021-05-19 at 6.59.52 PM.png (76.3K, 4-Screenshot%202021-05-19%20at%206.59.52%20PM.png)
download | view image
[image/png] image.png (52.6K, 5-image.png)
download | view image
view thread (54+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [pgAdmin][RM-2341]: Add menu option for starting PSQL
In-Reply-To: <CA+OCxoxpyfJabnycSGmtUK1-QEndNfZV3q2d1xP-oftDn-pGDw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox