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

EnterpriseDB INDIA:   http://www.enterprisedb.com

On Thu, Dec 18, 2008 at 12:25 PM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> Hi All,
>
> As per my discussion with Dave Page:

[for those that haven't realised, Ashesh is one of EDB's crack
programmers and is very kindly helping me out with some of the pgAdmin
work I don't currently have time for].

> - Add support for execution of jobs on a remote database in pgAgent.
>   (primarily for hot standby support). This essentially means allowing a
>   full connection string to be specified in place of the current database
>   name option.

<snip comprehensive description>

Hi Ashesh,

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. I do
have a few comments though - mostly minor:

- 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.

- The database icon in dlgSelectDatabase seems to be missing (I just
get a blank space where it should be).

- 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!)

- In pgAgent, why is connInfo declared as a struct and not 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:

  * 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).

  * 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.

That should be fairly simple to implement, and nice and robust.

Thoughts?

--
Dave Page
EnterpriseDB UK:   http://www.enterprisedb.com

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.

Thanks

- 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.

- The database icon in dlgSelectDatabase seems to be missing (I just
get a blank space where it should be).

- 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!)

- In pgAgent, why is connInfo declared as a struct and not 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:

  * 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).

 * 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.

EnterpriseDB INDIA:   http://www.enterprisedb.com

On Fri, Dec 19, 2008 at 9:31 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:

>   * 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?

I think we should tie the schema version to the major version number -
so if we change the schema, we also bump the major version. That way
we just need to represent the schema version with a single integer.

I suppose we could tie it to the major.minor version - so 3.0.x ==
300, 3.1.x == 301, 4.0 == 400 and so on. That at least means we could
change the schema in a minor release (which in pgAdmin/PostgreSQL
aren't actually that minor usually!). The downside of this scheme is
that it will be harder to set the macro in cmake.

> This might be useful in future release too.

Thats the idea :-)


--
Dave Page
EnterpriseDB UK:   http://www.enterprisedb.com

On Fri, Dec 19, 2008 at 9:31 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:

  * 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?

I think we should tie the schema version to the major version number -
so if we change the schema, we also bump the major version. That way
we just need to represent the schema version with a single integer.

I suppose we could tie it to the major.minor version - so 3.0.x ==
300, 3.1.x == 301, 4.0 == 400 and so on. That at least means we could
change the schema in a minor release (which in pgAdmin/PostgreSQL
aren't actually that minor usually!). The downside of this scheme is
that it will be harder to set the macro in cmake.

EnterpriseDB INDIA:   http://www.enterprisedb.com

On Fri, Dec 19, 2008 at 10:22 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> Dave Page wrote:
>
> On Fri, Dec 19, 2008 at 9:31 AM, Ashesh Vashi
> <ashesh.vashi@enterprisedb.com> wrote:
>
>   * 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?
>
> I think we should tie the schema version to the major version number -
> so if we change the schema, we also bump the major version. That way
> we just need to represent the schema version with a single integer.
>
> My worries for this are:
> * When user upgrades from one pgagent to other, he/she will have to
> replicate
>   all the jobs in the new version.
> * We will have to code in the pgAdmin III for all version of pgAgent(s).
> * What if, user have two version of pgagent installed (as schema name is
> different
>   for each version, it can be possible), then what should be the behavior of
> the
>   pgAdmin III?
>
> I guess, it will be difficult to maintain in this case :(.
> What do you say?

I don't mean change the schema name, I mean change the design of the
schema (ie. add a column, or remove a table or something). The schema
name should always be pgagent so there should be no migration issues.

> I suppose we could tie it to the major.minor version - so 3.0.x ==
> 300, 3.1.x == 301, 4.0 == 400 and so on. That at least means we could
> change the schema in a minor release (which in pgAdmin/PostgreSQL
> aren't actually that minor usually!). The downside of this scheme is
> that it will be harder to set the macro in cmake.
>
> Agree.
> Only in case of schema change, we will have a change in major version.

OK.

--
Dave Page
EnterpriseDB UK:   http://www.enterprisedb.com

I don't mean change the schema name, I mean change the design of the
schema (ie. add a column, or remove a table or something). The schema
name should always be pgagent so there should be no migration issues. 

EnterpriseDB INDIA:   http://www.enterprisedb.com

- 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:
 * 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).

Introduced a new function pgagent_schema_version() in the pgagent.sql
Also, written into the pgagent_update.sql for unreadability.

- Add support for execution of jobs on a remote database in pgAgent. (primarily for hot standby support). This essentially means allowing a full connection string to be specified in place of the current database name option.

<snip comprehensive description>

Hi Ashesh,

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. I do
have a few comments though - mostly minor:

- 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.

- The database icon in dlgSelectDatabase seems to be missing (I just
get a blank space where it should be).

- 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!)

- In pgAgent, why is connInfo declared as a struct and not 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:
 * 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).

  * 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.

Thanks Ashesh. I've applied your patches with the following minor changes:

- Use 3.0.0 for the pgAgent version number.
- Remove the function to get the pgAgent schema version, and change
the rest of the code so we only use the major version.
- Modified dlgStep so that all sizing is done in the XRC code, not the
C code. Note that to get the default size for an object (ie. a
textboxes height, you can use the special value -1 - for example,
200,-1d)

Regards, Dave.


On Fri, Dec 26, 2008 at 9:57 AM, Ashesh Vashi
<ashesh.vashi@enterprisedb.com> wrote:
> Hi All,
>
> Please find the updated patches for the same as per the instructions from
> Dave Page.
>
> - Add support for execution of jobs on a remote database in pgAgent.
>   (primarily for hot standby support). This essentially means allowing a
>   full connection string to be specified in place of the current database
>   name option.
>
> <snip comprehensive description>
>
> Hi Ashesh,
>
> 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. I do
> have a few comments though - mostly minor:
>
> - 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.
>
> Done.
>
> - The database icon in dlgSelectDatabase seems to be missing (I just
> get a blank space where it should be).
>
> Done.
>
> - 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.
>
> Done.
>
> - Any new headers should be added to include/precomp.h (we all forget
> to do that!)
>
> Done.
>
> - In pgAgent, why is connInfo declared as a struct and not a class?
>
> Done.
>
> - 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:
>
>   * 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).
>
> Introduced a new function pgagent_schema_version() in the pgagent.sql
> Also, written into the pgagent_update.sql for unreadability.
>
>   * 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.
>
> Done.
> Checking for pgagent.pgagent_schema_version exists or not and also check if
> return value of this function matches with the major version of pgagent.
>
>   * 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.
>
> Done.
> Made changes in CMakeList.txt for adding definition of
> PGAGENT_VERSION_MAJOR,
> PGAGENT_VERSION_MINOR, PGAGENT_VERSION_PATCH in the Makefile.
>
> BTW, changed the version of pgagent to 3.0.1 in the CMakeLists.txt
>
> --
> Regards,
> Ashesh Vashi
>
> EnterpriseDB INDIA: http://www.enterprisedb.com



--
Dave Page
EnterpriseDB UK:   http://www.enterprisedb.com