Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ljHKV-0001nC-49 for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 May 2021 08:13:23 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ljHKT-0005Vs-Es for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 May 2021 08:13:21 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ljHKS-0005Va-EN for pgadmin-hackers@lists.postgresql.org; Wed, 19 May 2021 08:13:21 +0000 Received: from mail-ed1-x531.google.com ([2a00:1450:4864:20::531]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1ljHKO-00056m-G1 for pgadmin-hackers@postgresql.org; Wed, 19 May 2021 08:13:19 +0000 Received: by mail-ed1-x531.google.com with SMTP id i13so14281189edb.9 for ; Wed, 19 May 2021 01:13:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=nYSlIMluhfh1chYc6thHuFdhUfg5+w5H5uGFrvt/zL0=; b=DisuxJRGoGhPicE5YTQlRCSKeLuJ9GoCxRdImO2DOu4yF4+Ko1NiHM8WBZjy+AyY48 Q1SoZCMFGyOyB/FpWm3TcjfNn0ZtTBNogdviAwYRoBWtzyitKjBGcapZMzhZa3Bz9fRJ iO3MUWXNDEf6ITV5PPGBPfR2sq71cR+Jbwydj6RKLNGBIsS6rPbC5YozFC1dGyKijtH3 A8XYwO1NzBXQ478sHPsBe6p4lJ82fkanzCJwC6hMNOYjs8oKQPA3x65qIk0Qwv8lu9th ajGV5kdCnnNWPt0A9Oo6f/pGGuExg4Tlt0d4nYy+1KVVa3vZegQ6B6b07NsT0YLwoj2l RGLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=nYSlIMluhfh1chYc6thHuFdhUfg5+w5H5uGFrvt/zL0=; b=CHO642+iAoZKx5x2bUR+F8pTRKUzGJjH2VAYIKcmXH1Br81SURWQdHhvnbbMeE5hdF 4ZNYVHMJBjqX4H6O8f4Ub4o+jA8u8ew2sbOx7ojmmnwGdrrjp7VklDL4tv7VtyBWM2Ow 9+H6fup4i8lpwBAE73HGRjVLujiaIXZ7SKtV3aGy72NM8oWojBlxYRnAb2GwOt8Y7pAa IYnsbGAekvYAB1V+zBXztXzC5ZexhACMi9nIebJBTyG2lHMXOCd+y87ICoP6aSRd0HMo h/5w/Op29cfkxMwsFxaE58eVsx9Qn57q+0NDgoKJDb85fmOMRNNr9aFX8pat14gsJV4g pTNg== X-Gm-Message-State: AOAM531yaT0PwkLXbSixI0OoAnnVdHnOVTPcVo7FQqi35jyzcEHMR96m RnNvDTTwxnbS708xCLVDAIeq0+21ERB1PM1YLpOIbQ== X-Google-Smtp-Source: ABdhPJzvAJEgX2s6hWjzIzcK9zyS1n28IhuT+4U40tJZg3vSKAKLRRtE8YT+eKO/oz1LKXkFt9Af2zaPpp8Ew7qJTuo= X-Received: by 2002:a05:6402:20f:: with SMTP id t15mr12880894edv.370.1621411994661; Wed, 19 May 2021 01:13:14 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Dave Page Date: Wed, 19 May 2021 09:13:03 +0100 Message-ID: Subject: Re: [pgAdmin][RM-2341]: Add menu option for starting PSQL To: Nikhil Mohite Cc: Akshay Joshi , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000fa06a905c2aa65e9" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000fa06a905c2aa65e9 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite < nikhil.mohite@enterprisedb.com> wrote: > Hi Dave/ Team, > > On Tue, May 18, 2021 at 8:41 PM Dave Page wrote: > >> Hi >> >> On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite < > nikhil.mohite@enterprisedb.com> wrote: > >> Hi Dave/Team, >> >> >> On Mon, May 17, 2021 at 6:47 PM Dave Page wrote: >> >>> Hi >>> >>> On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite < >>> nikhil.mohite@enterprisedb.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.htm= l >>> 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 cod= e > 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=E2=80= =99t able > to apply the theme properly as some style get applied as in-line style fo= r > 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? > > >> 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? >> > >> >>> >>>> A couple of other things I noticed: >>>> >>>> - The button is enabled if the treeview has a Server selected. It coul= d >>>> be argued that the query tool should do the same (defaulting to the >>>> maintenance database), however, that would be a separate change, and p= sql >>>> 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=3D# select * from pg_class; >>>> WARNING: terminal is not fully functional >>>> >>> I am not able to reproduce the warning for the terminal (I am working o= n >>> 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 warni= ng. >>> but found one limitation: >>> >> >> It looks like that can be fixed by adding: >> >> env=3D{'TERM': 'xterm'} >> >> 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, U= I >>> gets very slow. >>> >> >> Is xtermjs discarding the older buffer contents when it fills up? Can yo= u >> 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 n= o >>>> 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=3D# select '\!'; >> ERROR: Shell commands are disabled in psql for security >> > >> > >>>> >>>>> >>>>> >>>>> On Tue, May 11, 2021 at 3:40 PM Dave Page wrote: >>>>> >>>>>> Hi >>>>>> >>>>>> On Tue, May 11, 2021 at 9:02 AM Akshay Joshi < >>>>>> akshay.joshi@enterprisedb.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 dif= ficult 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 pgAdm= in4.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=3D# 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=3Dxxx' in conn_attr: >>>>>> conn_attr =3D conn_attr.replace('password=3Dxxx', '') >>>>>> >>>>>> - 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 conf= ig.py >>>>>> is appropriately worded. >>>>>> >>>>>> This is shaping up nicely! Good work. >>>>>> >>>>>> >>>>>>> >>>>>>> On Mon, May 10, 2021 at 7:32 PM Nikhil Mohite < >>>>>>> nikhil.mohite@enterprisedb.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 relat= ed to >>>>>>>> them. >>>>>>>> >>>>>>>> On Mon, May 10, 2021 at 7:10 PM Dave Page >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Hi >>>>>>>>> >>>>>>>>> On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite < >>>>>>>>> nikhil.mohite@enterprisedb.com> wrote: >>>>>>>>> >>>>>>>>>> Hi Hackers, >>>>>>>>>> >>>>>>>>>> Please find the attached patch for RM-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 gue= ssing 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 configurati= on >>>>>>>>> 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_m= odules/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_m= odules/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_m= odules/local-echo.jsx >>>>>>>>> doesn't exist >>>>>>>>> as directory >>>>>>>>> >>>>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_m= odules/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 6004= 1 >>>>>>>>> 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 * >>>>>>> >>>>>>> *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 > >> >> >> -- > *Thanks & Regards,* > *Nikhil Mohite* > *Software Engineer.* > *EDB Postgres* > *Mob.No: +91-7798364578.* > --=20 Dave Page Blog: https://pgsnake.blogspot.com Twitter: @pgsnake EDB: https://www.enterprisedb.com --000000000000fa06a905c2aa65e9 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <<= a href=3D"mailto:nikhil.mohite@enterprisedb.com">nikhil.mohite@enterprisedb= .com> wrote:
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41= PM Dave Page <dp= age@pgadmin.org> wrote:
Hi

=
On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <nikhil.mohite@enterprisedb.com= > wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dav= e Page <dpage@pga= dmin.org> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <nikhil.mohite@= enterprisedb.com> wrote:
=
Hi Akshay/ Team,

Please find the attach= ed updated patch for the psql tool.

=
Hmm, this version is also broken. There's a typo in editor_t= emplate.html on line 138 - it splits a string across two lines which throws= an error. Having fixed that...

I also note there&= #39;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 t= hem into the css for the themes. You=C2=A0have a mix of style, layout and c= ode in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme an= d other settings, earlier I tried with CSS to override the theme but couldn= =E2=80=99t 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 styl= es 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?
=C2=A0
=


Speaking of themes, the background colour f= or selected text doesn't seem right (it's barely visible) in the da= rk theme. Can you fix that to match the colouring in the SQL text boxes ple= ase?=C2=A0
=
=C2=A0

A cou= ple of other things I noticed:

- The button is ena= bled if the treeview has a Server selected. It could be argued that the que= ry tool should do the same (defaulting to the maintenance database), howeve= r, that would be a separate change, and psql should be consistent with the = query tool.
It is now consistent w= ith the query tool.

- If I do a "select * = from pg_class;" I still get:

postgres=3D= # select * from pg_class;
WARNING: terminal is not fully function= al
I am not able to reproduc= e the warning for the terminal (I am working on Catalina 10.15.7), I checke= d on browsers (chrome, firefox, Safari) and also checked on local nwjs runt= ime but still not able to reproduce the warning. but found one limitation:<= /div>

It looks like that can be= fixed by adding:

env=3D{'TERM': 'xter= m'}

to the subprocess.Popen() call.
<= div>
I noticed while I was playing with that, that you are pa= ssing 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 p= sql using the PGPASSWORD environment variable.
=C2=A0
if=C2=A0we try= to load data from the table containing millions of records, UI gets=C2=A0v= ery slow.

Is xtermjs=C2= =A0discarding the older buffer contents when it fills up? Can you tell wher= e the memory usage is?
<= /div>
I checked the psql memory consumption in terminal and pgAdmin psq= l tool memory consumption is the similar. Also tested the performance and q= uery execution timing is also =C2=A0similar.
<= br>
OK, so there's probably not much we can do here.
=C2=A0

<= div>=C2=A0

- I'm sure using \q in the previous version di= splayed a message saying the session exited (the one on line=C2=A0138 of ed= itor_template.html). It no longer seems to do so.=C2=A0=C2=A0

In addi= tion to the issue above, it looks like the \! blocking may have lost it'= ;s ability to ignore quoted strings:

pgweb=3D# sel= ect '\!';
ERROR: Shell commands are disabled in psql for = security=C2=A0
=C2=A0
=C2=A0


On Tue, May 11, 2021 at 3= :40 PM Dave Page <dpage@pgadmin.org> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <akshay.joshi@enterprisedb.com= > wrote:
Hi Nikhil=C2=A0
=
Following are the review comments:

= GUI specific:
  • We need a panel icon for PSQL like q= uery tool, we can also add that on the browser tree toolbar.
  • PSQL T= ool menu should be visible for all the child nodes of the database node. Fo= llow 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 i= s still open even if we disconnect the database server from the browser tre= e.
Code specific:
  • Remove an ex= tra space from requirements.txt and package.json
  • Documentation need= s 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 s= o many unused imports please remove them.
  • We are not using cheroot = so it should be removed from requirements.txt and also remove the import st= atement from pgAdmin4.py=C2=A0
  • Test cases are showing=C2=A0succ= essful but actually, there are some routing errors please check.
<= /div>
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 b= e an issue with terminal compatibility (which I didn't have on my proto= type):

ml=3D# select * from pg_class;
WARNING: terminal is not fully functional
-[ RECORD 1 ]-------= +----------------------------------------------
oid =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | 79354
relname =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | housing
...

- The panel should honour the styleguide. I'm running i= n dark mode, and see a jet black background. I would expect to see the same= background/foreground colours as the treeview.
- I spotted at le= ast one print() statement that shouldn't be there (debug output should = go through the logger) - psql/__init__.py:235
- This seems suspec= t - why would there be a password in a connection string we've built? A= nd why would it be xxx?

=C2=A0 =C2=A0 if '= ;password=3Dxxx' in conn_attr:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 co= nn_attr =3D conn_attr.replace('password=3Dxxx', '')

- There's a thick white line at the bottom of t= he panel, where a horizontal scrollbar might be if there was one.
- The trailing semi-colon=C2=A0should 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.
=C2=A0

On Mon, May 10, 2021 at 7:32= PM Nikhil Mohite <nikhil.mohite@enterprisedb.com> wrote:
Hi Dave/ Team,

P= FA updated patch, sorry for the inconvenience, while cleanup I removed the = unwanted libraries but forgot to remove the code related to them.

=
On Mon, Ma= y 10, 2021 at 7:10 PM Dave Page <dpage@pgadmin.org> wrote:
=
Hi

On Mon, May 10, 2021= at 1:45 PM Nikhil Mohite <nikhil.mohite@enterprisedb.com> wrote:
Hi Hackers,

Please = find the attached patch for RM-2341:=C2=A0Add Menu option for starting PS= QL.
1. Added new Option PSQL Tool in Tools menu.
2. Add= ed the same option for Server and Database nodes from the tree view.
<= /div>

Unfortunately=C2=A0there's a trai= ling 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/to= ols/psql/static/js/psql_module.js 23:50-82
Module not found: Erro= r: Can't resolve 'local-echo-controller' in '/Users/dpage/g= it/pgadmin4/web/pgadmin/tools/psql/static/js'
resolve 'lo= cal-echo-controller' in '/Users/dpage/git/pgadmin4/web/pgadmin/tool= s/psql/static/js'
=C2=A0 Parsed request is a module
=C2=A0 using description file: /Users/dpage/git/pgadmin4/web/package.json = (relative path: ./pgadmin/tools/psql/static/js)
=C2=A0 =C2=A0 ali= ased with mapping 'local-echo-controller': '/Users/dpage/git/pg= admin4/web/node_modules/local-echo' to '/Users/dpage/git/pgadmin4/w= eb/node_modules/local-echo'
=C2=A0 =C2=A0 =C2=A0 using descri= ption file: /Users/dpage/git/pgadmin4/web/package.json (relative path: ./pg= admin/tools/psql/static/js)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 Field = 9;browser' doesn't contain a valid alias configuration
= =C2=A0 =C2=A0 =C2=A0 =C2=A0 root path /Users/dpage/git/pgadmin4/web
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 using description file: /Users/dpage/= git/pgadmin4/web/package.json (relative path: ./Users/dpage/git/pgadmin4/we= b/node_modules/local-echo)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 no extension
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= Field 'browser' doesn't contain a valid alias configuration
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /Users/dpage/git/p= gadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn'= t exist
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .js
= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Field 'browser' do= esn't contain a valid alias configuration
=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /Users/dpage/git/pgadmin4/web/Users/dpage/g= it/pgadmin4/web/node_modules/local-echo.js doesn't exist
=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 .jsx
=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 Field 'browser' doesn't contain a v= alid alias configuration
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_= modules/local-echo.jsx doesn't exist
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 as directory
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/n= ode_modules/local-echo doesn't exist
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 using description file: /Users/dpage/git/pgadmin4/web/package.json (rel= ative path: ./node_modules/local-echo)
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 no extension
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= Field 'browser' doesn't contain a valid alias configuration
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /Users/dpage/git/pgadmin4= /web/node_modules/local-echo doesn't exist
=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 .js
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 F= ield 'browser' doesn't contain a valid alias configuration
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /Users/dpage/git/pgadmin4/w= eb/node_modules/local-echo.js doesn't exist
=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 .jsx
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = Field 'browser' doesn't contain a valid alias configuration
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /Users/dpage/git/pgadmin4/= web/node_modules/local-echo.jsx doesn't exist
=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 as directory
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 /Users/dpage/git/pgadmin4/web/node_modules/local-echo doesn'= t exist
=C2=A0@ ./pgadmin/tools/psql/static/js/index.js 17:19-43<= /div>

2021-05-10 14:38:37: webpack 5.21.2 compiled with = 1 error in 60041 ms
=C2=A0
--
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnak= e

EDB: ht= tps://www.enterprisedb.com
Regards,
Nikhil Mohite=C2=A0


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software ArchitectE= DB Postgres
Mobile: +91 976-788-8246



--
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake<= br>
EDB: http= s://www.enterprisedb.com

Regards,
Nikhil Mohite=C2=A0


--
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake<= br>
EDB: http= s://www.enterprisedb.com

<= /div>
Regards,
Nikhil Mohite=C2=A0


--
Dave Page
Blog: https://pgsnake.blogspot.com
Twitter: @pgsnake<= br>
EDB: http= s://www.enterprisedb.com


Regards,
Nikhil Mohite
=


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob= .No: +91-7798364578.


--
<= /div> --000000000000fa06a905c2aa65e9--