Обсуждение: pgAdmin III commit: Install adminpack on user request in frmStatus
Install adminpack on user request in frmStatus Now that we can install an extension quite easily, it's simpler to ask the user if he wants pgAdmin to install it, rather than displaying him a hint. We still do that if it's not an 9.1 server or if the server doesn't have the adminpack extension installed. Branch ------ master Details ------- http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=bd13fc270aaf7fdf007c7ea94969c1fbfeb4f9fe Modified Files -------------- pgadmin/db/pgConn.cpp | 4 ++-- pgadmin/frm/frmStatus.cpp | 30 ++++++++++++++++++++++++++---- pgadmin/include/db/pgConn.h | 2 +- 3 files changed, 29 insertions(+), 7 deletions(-)
Hi Guillaume, I've tweaked the wording on the message in this patch as I wasn't entirely happy with the phrasing, but whilst doing so I noticed there doesn't seem to be any error checking to handle the case where the extension cannot be installed (for example, because the user is using a hosting site which hasn't installed the contrib modules). There's also no way to permanently suppress the message box, which could become very annoying. Final thought - we also have a guru hint about the admin pack (instrumentation.html). Shouldn't this patch touch the same places as that hint (as well as anywhere else it makes sense)? On Mon, Apr 18, 2011 at 9:23 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Install adminpack on user request in frmStatus > > Now that we can install an extension quite easily, it's simpler to ask the > user if he wants pgAdmin to install it, rather than displaying him a hint. We > still do that if it's not an 9.1 server or if the server doesn't have the > adminpack extension installed. > > Branch > ------ > master > > Details > ------- > http://git.postgresql.org/gitweb?p=pgadmin3.git;a=commitdiff;h=bd13fc270aaf7fdf007c7ea94969c1fbfeb4f9fe > > Modified Files > -------------- > pgadmin/db/pgConn.cpp | 4 ++-- > pgadmin/frm/frmStatus.cpp | 30 ++++++++++++++++++++++++++---- > pgadmin/include/db/pgConn.h | 2 +- > 3 files changed, 29 insertions(+), 7 deletions(-) > > > -- > Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgadmin-hackers > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, Le 19/04/2011 10:40, Dave Page a écrit : > [...] > I've tweaked the wording on the message in this patch as I wasn't > entirely happy with the phrasing, Good idea. > but whilst doing so I noticed there > doesn't seem to be any error checking to handle the case where the > extension cannot be installed (for example, because the user is using > a hosting site which hasn't installed the contrib modules). > We first check if the extension is available (query on pg_available_extensions). bUT I should probably add a check on the CREATE EXTENSION query because it may fail (if you're not superuser for example). > There's also no way to permanently suppress the message box, which > could become very annoying. > Yeah, I didn't think about it yesterday. That would be good to add. > Final thought - we also have a guru hint about the admin pack > (instrumentation.html). Shouldn't this patch touch the same places as > that hint (as well as anywhere else it makes sense)? > I've been thinking about it. I was wondering if we should have a specific hint for 8.1 till 9.0 and another one for 9.1. And one other issue: we may be connected to another database than the postgres one. I should probably check that too. -- Guillaume http://www.postgresql.fr http://dalibo.com
On Tue, Apr 19, 2011 at 10:48 AM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Hi, > > Le 19/04/2011 10:40, Dave Page a écrit : >> [...] >> I've tweaked the wording on the message in this patch as I wasn't >> entirely happy with the phrasing, > > Good idea. > >> but whilst doing so I noticed there >> doesn't seem to be any error checking to handle the case where the >> extension cannot be installed (for example, because the user is using >> a hosting site which hasn't installed the contrib modules). >> > > We first check if the extension is available (query on > pg_available_extensions). bUT I should probably add a check on the > CREATE EXTENSION query because it may fail (if you're not superuser for > example). > >> There's also no way to permanently suppress the message box, which >> could become very annoying. >> > > Yeah, I didn't think about it yesterday. That would be good to add. > >> Final thought - we also have a guru hint about the admin pack >> (instrumentation.html). Shouldn't this patch touch the same places as >> that hint (as well as anywhere else it makes sense)? >> > > I've been thinking about it. I was wondering if we should have a > specific hint for 8.1 till 9.0 and another one for 9.1. > > And one other issue: we may be connected to another database than the > postgres one. I should probably check that too. I was thinking more that we shouldn't have a hint in one place, and offer to fix it in another. We should have the hint, and offer to fix the problem in all places (frmHint can handle "fix" options iirc - for example, it can vacuum for you). -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Le 19/04/2011 12:47, Dave Page a écrit : > On Tue, Apr 19, 2011 at 10:48 AM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: >> Hi, >> >> Le 19/04/2011 10:40, Dave Page a écrit : >>> [...] >>> I've tweaked the wording on the message in this patch as I wasn't >>> entirely happy with the phrasing, >> >> Good idea. >> >>> but whilst doing so I noticed there >>> doesn't seem to be any error checking to handle the case where the >>> extension cannot be installed (for example, because the user is using >>> a hosting site which hasn't installed the contrib modules). >>> >> >> We first check if the extension is available (query on >> pg_available_extensions). bUT I should probably add a check on the >> CREATE EXTENSION query because it may fail (if you're not superuser for >> example). >> >>> There's also no way to permanently suppress the message box, which >>> could become very annoying. >>> >> >> Yeah, I didn't think about it yesterday. That would be good to add. >> >>> Final thought - we also have a guru hint about the admin pack >>> (instrumentation.html). Shouldn't this patch touch the same places as >>> that hint (as well as anywhere else it makes sense)? >>> >> >> I've been thinking about it. I was wondering if we should have a >> specific hint for 8.1 till 9.0 and another one for 9.1. >> >> And one other issue: we may be connected to another database than the >> postgres one. I should probably check that too. > > I was thinking more that we shouldn't have a hint in one place, and > offer to fix it in another. We should have the hint, and offer to fix > the problem in all places (frmHint can handle "fix" options iirc - for > example, it can vacuum for you). > Got it :) This patch should fix all the remaining issues, shouldn't it? -- Guillaume http://www.postgresql.fr http://dalibo.com
On Tue, Apr 19, 2011 at 9:58 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Le 19/04/2011 12:47, Dave Page a écrit : >> >> I was thinking more that we shouldn't have a hint in one place, and >> offer to fix it in another. We should have the hint, and offer to fix >> the problem in all places (frmHint can handle "fix" options iirc - for >> example, it can vacuum for you). >> > > Got it :) > > This patch should fix all the remaining issues, shouldn't it? Yes, that's more along the lines of what I was expecting. I did spot a typo: +<p>One your extension is installed, you only need to click on the "Fix" s/One/Once/ -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Le 20/04/2011 11:30, Dave Page a écrit : > On Tue, Apr 19, 2011 at 9:58 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: >> Le 19/04/2011 12:47, Dave Page a écrit : >>> >>> I was thinking more that we shouldn't have a hint in one place, and >>> offer to fix it in another. We should have the hint, and offer to fix >>> the problem in all places (frmHint can handle "fix" options iirc - for >>> example, it can vacuum for you). >>> >> >> Got it :) >> >> This patch should fix all the remaining issues, shouldn't it? > > Yes, that's more along the lines of what I was expecting. I did spot a typo: > > +<p>One your extension is installed, you only need to click on the "Fix" > > s/One/Once/ > Fixed and pushed. Thanks. -- Guillaume http://www.postgresql.fr http://dalibo.com