Does not work on pgsql

spydmobile - December 1, 2008 - 18:46
Project:Backup and Migrate
Version:6.x-1.2
Component:Code
Category:feature request
Priority:normal
Assigned:Unassigned
Status:needs work
Description

I have to agree with other comments, for a module to be prime time, it should support both official databases. I notice that someone has offered to help you with this issue, I sincerely hope you take the offer, I am excited to try your module on my pg-drupal systems. Lastly, since I had to install your module, find that it fails to work correctly, come and submit a bug only to find that it never did support PGSQL, I would suggest that you add that fact to the description of the module in bold. Pehaps "This module does not support PostgreSQL (yet)". Thanks for contributing and keep up the good work, I look forward to your module when it supports PGSQL.

For the record, this is the error returned when this module is installed and run on PGSQL

    * warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error at or near "table" LINE 1: show table status ^ in C:\Program Files\Apache Software Foundation\Apache2.2\htdocs\cms\includes\database.pgsql.inc on line 138.
    * user warning: query: show table status in C:\Program Files\Apache Software Foundation\Apache2.2\htdocs\cms\sites\all\modules\backup_migrate\backup_migrate.module on line 669.
    * warning: Invalid argument supplied for foreach() in C:\Program Files\Apache Software Foundation\Apache2.2\htdocs\cms\includes\form.inc on line 1417.
    * warning: Invalid argument supplied for foreach() in C:\Program Files\Apache Software Foundation\Apache2.2\htdocs\cms\includes\form.inc on line 1417.

#1

Charles Oertel - January 15, 2009 - 12:23

I am working on adding postgres and multiple databases to this module.

Some functionality to do this is already in db_maintenance module, and I am creating a dependency between the two modules. Give me a few days and we will see where we get to.

#2

Charles Oertel - January 16, 2009 - 14:27

This module now seems to work with postgres and mysql (I didn't test mysql since I am using postgres). It also works if you have one or more than one database defined, and should even work if some of the databases are postgres and others are mysql.

Unfortunately, the magic for this intelligence comes from the db_maintenance module, and I am not going to cut-n-paste. So I have added that module as a dependency to this one. You need to use the patched version of that module, here:
http://drupal.org/node/306872#comment-1204027

AttachmentSize
backup_migrate-6.x-1.0.module.patch 30.03 KB
backup_migrate-6.x-1.0.info_.patch 341 bytes

#3

ronan - January 18, 2009 - 03:31
Category:bug report» feature request
Priority:critical» normal
Status:active» needs work

Thanks for the hard work. I'm not going to commit a patch that introduces a module dependency to a patched module as it would raise the barrier of entry a little high, but this could be very useful for posgres users who want this functionality.

I'd love to hear PGSQL users' take on whether this works and if it's a good solution, as there maybe a way forward using some sort of conditional dependency.

Thanks again.
r

#4

ronan - January 18, 2009 - 03:32
Status:needs work» needs review

#5

aaron1234nz - January 18, 2009 - 07:48
Status:needs review» needs work

Hi Charles/Ronan.

I've just given the patch a test. It did not quite apply cleanly against the latest dev, but it was close. I've also applied the patch to db_maintainance, and that went smoothly.

I've just attempted to do a backup of my Postgres database. However when I do I get a 0 byte file back. So something is not right. I had compression turned off and it made no difference if I downloaded the file or have it save to my files directory.

I'm running postgres 8.3 on windows

#6

Charles Oertel - January 19, 2009 - 11:12

Hi Aaron

I'm getting the empty backup now too - even though the module worked perfectly on Friday. It may have something to do with the latest dev, because I applied it to my version before using the module this morning. Will let you know.

In one manual test, I got a timeout on a very large database, so that might be one of the issues.

Ronan, I understand the dependency issue and suspected I might be chastised for doing such a thing. One workaround would be to copy the functions across into this module, but we know that refactoring is the proper solution in the long term. Or maybe we can pool resources and have this module supercede db_maintenance? Especially since this one has more functionality and now (theoretically) supports all the databases.

I'll do a new patch when I find the problem.

#7

ronan - January 19, 2009 - 15:59

I made some changes to the latest dev on friday, so that might be the issue. I am trying to keep the 1.x branch stable, but I applied a patch that helps fix some fairly serious issues with temp files not being deleted. That'll hopefully be last big change to the 1.x branch so that won't get in your way.

As to the dependency, I certainly didn't mean to chastise. I'm very happy that you're taking the time and the initiative to work on this issue. I don't mind the dependency either as long as it doesn't complicate life for MySQL users. A conditional warning that checks for the db_maintenance module if the current db is postgres should work. That way, pgsql users would get info on how to enable the module for their situation but MySQL users would be none the wiser, and would not have to install an unnecessary module just to satisfy a dependency.

Once you get something working and stable and vetted by the pgsql community (I don't use postgres it much at all, and am extremely unfamiliar with it's dump formats, so I won't be much use) then I'll either commit it to the 1.x branch with the dependency warning, or refactor it for inclusion in the 2.x branch.

Thanks again for your hard work.

#8

Charles Oertel - January 20, 2009 - 12:15
Version:6.x-1.0» 6.x-1.2

Fixed the problem where the database is created empty. Two problems, one a bug, the other the fact that pg_dump does not accept passwords in the parameter list. In situations where the db connection requires a password, this is what now happens:

  1. the call to db_maintenance to get the postgres options, will result in a file called .pgpass being created, populated with the logon credentials for the database as set in settings.php, and place the file in the parent of the drupal directory with appropriate permissions.
  2. the call to pg_dump sets the PGPASSWORDFILE environment variable to point to that file so that pg_dump can use it.

This patch also includes the latest of Ronan's releases, so temp files are being deleted as intended.

Remember to satisfy the dependency of this module with db_maintenance, patched to http://drupal.org/node/306872#comment-1210575

AttachmentSize
backup_migrate-6.x-1.2.info_.patch 342 bytes
backup_migrate-6.x-1.2.module.patch 33.34 KB

#9

aaron1234nz - January 24, 2009 - 09:42

Hi Charles,

Results of testing:

  • I've applied your new patches against the latest stable release but the patch did not apply cleanly (the backup_migrate_cron part failed ... there seems to be an if statement in the place of a foreach missing in the surrounding cod).
  • The settings for choosing the paths to pgdump etc. are hard coded in. As I use Windows I had to modify the code to get it to do anything
  • I don't have a password on the database I was testing with, but setting the environment variable does not float well on Windows (you can't execute two commands separated with a semicolon for a start) See http://www.mail-archive.com/pgsql-patches@postgresql.org/msg08305.html
    • However the files generated were good and contained (at first glace at least) the right stuff.

      Thanks for the work your putting into this!

      Aaron

#10

Charles Oertel - February 10, 2009 - 08:49

Aaron

My apologies. Firstly for not monitoring this thread over the last two weeks, and secondly for having completely forgotten the issues with running on Windows. I switched to linux 6 years ago and can no longer even imagine the difficulty of getting anything to work there. I propose two fixes to your problems:

  • Put the .passwd file path into the pgdump command line with a parameter, instead of passing it as a session variable, and
  • Not specifying any paths to executables, those executables must be on the system path

Our system is going into testing soon, and only then will I be likely to work on this code again. If I do I will submit a new patch, or, if the module allows, integrate my changes into the new version.

#11

aaron1234nz - February 10, 2009 - 19:07

Thanks Charles,

I've got a couple of pressing projects on the go at the moment, but once I'm through those I'll get a patch together. I should switch to linux but haven't had the time/inclination to get used to a new environment yet.

#12

vinzentt - February 13, 2009 - 17:11

Hi,

first of all thanks for this patch, but i had to modify several things to get it work in backup and migrate and in db_maintenance :

- environment variable is neither DBPASSWORDFILE nor PGPASSWORDFILE but PGPASSFILE according to libpq documentation: http://www.postgresql.org/docs/8.1/static/libpq-pgpass.html
- the .pgpass should be in the user home directory (apache user, so normally /var/www) with 600 for permissions

i don't post a patch 'cause i modified more than that and it concerns both modules.

but if needed i can clean my code and post it.

--
Vincent

#13

spydmobile - February 27, 2009 - 18:24

good call vinzentt, but this is a better URL becuase 8.1 has fundamental differences in syntax than recent versions, so this is a better URL so as to not introduce accidental bugs becuase of old syntax...

http://www.postgresql.org/docs/8.3/static/libpq-pgpass.html

#14

spydmobile - March 4, 2009 - 16:13

Sorry I am not a Patcher.... I have run the latest release version and this is the result upon trying to config the module:

    * warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error at or near "table" LINE 1: show table status ^ in /var/www/sparcs/includes/database.pgsql.inc on line 139.
    * user warning: query: _backup_migrate_get_table_names show table status in /var/www/sparcs/sites/all/modules/backup_migrate/backup_migrate.module on line 682.
    * warning: Invalid argument supplied for foreach() in /var/www/sparcs/includes/form.inc on line 1420.
    * warning: Invalid argument supplied for foreach() in /var/www/sparcs/includes/form.inc on line 1420.

Dont know if this helps or not..... If you folks release something I can use (a dev or a release) I would be happy to feed back...

#15

andypost - April 8, 2009 - 14:44

Last patch still wrond against 6-1.2 if someone already have a working copy please make a new patch

#16

xlyz - October 1, 2009 - 23:55

subscribing

#17

Sailtexas - November 13, 2009 - 00:29

Hi and thanks for your work,

I'm running a fresh install of Drupal 6.14 on pgsql and I'm also running poormanscron.

My error msg:
* warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error at or near "table" at character 6 in /var/www/vhosts/lordnelsonyachts.org/httpdocs/includes/database.pgsql.inc on line 139.
* user warning: query: show table status in /var/www/vhosts/lordnelsonyachts.org/httpdocs/sites/all/modules/backup_migrate/backup_migrate.module on line 682.
* warning: Invalid argument supplied for foreach() in /var/www/vhosts/lordnelsonyachts.org/httpdocs/includes/form.inc on line 1428.
* warning: Invalid argument supplied for foreach() in /var/www/vhosts/lordnelsonyachts.org/httpdocs/includes/form.inc on line 1428.

I'll be happy to assist in any way possible!

 
 

Drupal is a registered trademark of Dries Buytaert.