Skip site navigation (1) Skip section navigation (2)

Re: Patch: Add support for execution of jobs on a remote database

From: Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: Patch: Add support for execution of jobs on a remote database
Date: 2008-12-19 09:31:13
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgadmin-hackers
Hi Dave Page,

Dave Page wrote:
> I haven't tested fully at this stage, just eyeball reviewed the code,
> made sure it builds (on Windows) and checked out the UI. First
> impression is that it's well thought out and nicely implemented.

> - The pgTable::HasColumn() static function seems like an unusual fit
> in that class. Yes, it's a table object, but that class is primarily
> concerned with representing a table in the treeview. It seems to me
> that a non-static pgConn::TableHasColumn() function might make more
> sense, and would probably look cleaner at the call sites.
make sense
> - The database icon in dlgSelectDatabase seems to be missing (I just
> get a blank space where it should be).
It is.
I will do the needful.
> - The height of the connection string textbox is inconsistent with all
> other textboxes. It should be sized as per the other one line text
> boxes, and the button altered to match it.
> - Any new headers should be added to include/precomp.h (we all forget
> to do that!)
Oops I did not know about that :(
I will certainly do it from now onwards.
> - In pgAgent, why is connInfo declared as a struct and not a class?
Earlier, I wanted to use it as a storage for data like user, dbname, 
host, etc...
And hence declared it as a struct instead of class.
But later while implementation, I have introduced some function in it.

I will convert it to a class.
> - pgAdmin seems to be able to handle connecting to an old-style schema
> (which is good), but I didn't see any code in pgAgent to do similar. I
> would suggest we do something like the following, which should allow
> for future changes:
In fact in pgagent when you try to get the value from the given pgSet, 
if that column does not exist in the set, it will return a empty string.
Hence, I did not make any checks for columns exists or not.

If you say, I can go ahead and make the change for checking for column 
(jstconnstr) exists or not.
>   * Add an SQL function pgagent_schema_version() which returns an int
> (with a value of 3 for this version - we'll bump the package to
> 3.0.0).
I thought of this options too. :)
Shouldn't we return a text instead of integer x.x.x support?

This might be useful in future release too.
What everybody has to say?
>   * Add a check to pgAgent in (MainLoop()) - upon setup of the primary
> connection, if pgagent_schema_version() does not exist, then exit with
> an error asking the user to upgrade the schema.
>   * If it does exist, check the value it returns against MAJOR_VERSION
> - a pre-processor macro we can set in cmake from
> CPACK_PACKAGE_VERSION_MAJOR - if it doesn't match, exit with an error
> telling the user there is a schema version mismatch.
ok. sounds good.

Thanks & Regards,
Ashesh Vashi

EnterpriseDB INDIA:

In response to


pgadmin-hackers by date

Next:From: Dave PageDate: 2008-12-19 09:59:15
Subject: Re: Patch: Add support for execution of jobs on a remote database
Previous:From: svnDate: 2008-12-18 18:20:17
Subject: SVN Commit by guillaume: r7515 - trunk/pgadmin3/i18n/de_DE

Privacy Policy | About PostgreSQL
Copyright © 1996-2017 The PostgreSQL Global Development Group