[Patch] Added compatibility with PostgreSQL

[s.noack@…]

Based on #3290, I have modified formdb.py to work with PostgreSQL. The changes done by #3290 basically worked except the creation of tables and indexes. Following changes were required to make it work with PostgreSQL:

- The syntax for auto incrementing fields of PostgreSQL differs from sqlite and mysql.
- Primary keys must be defined in the line of the column, instead of at the end of the CREATE TABLE statement.
- Use DROP INDEX ... instead of ALTER TABLE ... DROP INDEX ...
- Omit DESC keyword when creating indexes.

Maybe you can switch for the database backend when creating the tables and indexes. It would be nice if it would work out of the box with PostgreSQL. TracFormsPlugin is the only of 5 plugins I am using which didn't worked out of the box after migrating to MySQL.

[Ryan J Ollos]

See also #5608.

[Steffen Hoffmann]

Thanks for the ground work.

Patch looks good so far (not tested by now), so you made fixing that issue really easy as far as I can see. So this should be resolved soon.

[Steffen Hoffmann]

Uh, this is not a patch but a whole new file.

I may find some time to diff it against a current version and add similar syntax to the newer upgrades as well. Still in general a true diff (diff -u upstream_file modified_file) plus mentioning the code revision used as base for the modifications would be appreciated.

[s.noack@…]

I don't remember why I don't have created a patch. However as far as i remeber it was based on TracForms 0.2.

[Steffen Hoffmann]

Replying to s.noack@placement24.com:

I don't remember why I don't have created a patch. However as far as i remeber it was based on TracForms 0.2.

Ok, thanks for clarification.

I'm very busy integrating new features these days, so you could largely reduce the time between now and PostgreSQL of the current db schema by updating your patch to trunk. Interested? I'm ensure, that I'm willing to integrate changes that look reasonable and come pre-tested, only I still can't do testing of alternative db engines on my own due to time constraints.

[Steffen Hoffmann]

patch derived from previous contribution, last db schema changes adapted too

[Steffen Hoffmann]

Anyone able and willing to test the updated changes​?

Your feedback would significantly speedup that work. Thanks in advance.

[Nelso G. Jost]

Replying to hasienda:

Anyone able and willing to test the updated changes​?

Your feedback would significantly speedup that work. Thanks in advance.

I've been following the development of TracFormsPlugin, and I was happy to know that it is progressing. My system is: Trac 0.12 + PostgreSQL 8.4 + Python 2.6.

I applied the referred​ patch in 0.3 and 0.4dev releases, and when running trac-admin <...> upgrade I got some sintaxe error messages, which I correct manually in the code. Here is the little diff:

382d381
<                 );
558d556
<                 )
765d762
<                 )

But still I got another error message:

ProgrammingError: type "tracform_id" does not exist
LINE 4:                      tracform_id 'id', updated_on 'time',
                             ^

This SQL appear in the line 630 from formdb.py, and I don't know what changes need to be make.

PS: I have tested before the patch formdb.py​ sended by s.noack@placement24.com, on tracforms 0.2, and it parcially works: at least, the enviroment upgrade was succeded. The macro in deed generates the form components, but the submit buttons just don't work, i.e., changes was not recorded in database.

[Steffen Hoffmann]

revised compatibility patch

[Steffen Hoffmann]

Nice to hear about interest in the development.

Replying to nelsojost:

![...] I got some sintaxe error messages, which I correct manually in the code. Here is the

Ok, this is easy. Just missed correct matching brackets. I think that the semicolon is not needed in this statements as it appears inconsistently. Please check, if this is clean now.

But still I got another error message: ![...] This SQL appear in the line 630 from formdb.py, and I don't know what changes need to be make.

I've checked the PostgreSQL docs and ​this is most probably related:

The AS Key Word

In the SQL standard, the optional key word AS is just noise and can be omitted without affecting the meaning. The PostgreSQL parser requires this key word when renaming output columns because ...

I've added a specific, alternative statement that could work. Only I'm not entirely convinced regarding the quoted and unquoted column names.

PS: I have tested before the patch formdb.py​ sended by s.noack@placement24.com, on tracforms 0.2, and it parcially works: at least, the enviroment upgrade was succeded. The macro in deed generates the form components, but the submit buttons just don't work, i.e., changes was not recorded in database.

Don't worry. I think we could get it working in no time, if you stay with me for resolving this ticket. 0.2 is history, actually focussing on 0.4, but I deferred 0.3.1 until now to include the PostgreSQL compatibility code there as well.

Because you mentioned submit buttons: Be sure to grant required permission (FORM_EDIT_VAL) as needed to re-enable them for TracForms >= 0.3 .

[Nelso G. Jost]

Thanks for the response. I tried the new patch​ (TracForms-0.4dev_r10152). Here is the log:

	SQL ERROR: syntax error at or near "'id'"
LINE 4:                      tracform_id AS 'id', updated_on AS 'tim...
                                            ^

Although I use PostgreSQL, I don't know much about databases - barely know something about SQL itself. Learn more about this is in my TO-DO list.

I also tried the following:

1. 'id' AS tracform_id, which fix sintax error, but generates a new one (which is not so surprising):

   ProgrammingError: column "id" does not exist
   LINE 1: SELECT id,time,old_state FROM forms_history
   

2. 'tracform_id' AS id.

   ERROR: EXECUTING SQL:
   
               CREATE INDEX forms_history_id_idx
                   ON forms_history(id)
               
   	()
   	SQL ERROR: data type unknown has no default 
   operator class for access method "btree"
   HINT:  You must specify an operator class for the 
   index or define a default operator class for the 
   data type.
   

This was me fooling around, little blindly. Hope to be more helpful next time.

[Steffen Hoffmann]

unquoted column names for PostgreSQL

[Steffen Hoffmann]

Replying to nelsojost:

Thanks for the response. I tried the new patch​ (TracForms-0.4dev_r10152). Here is the log:

Thank you for the quick response. So it was at least a partial fix.

This was me fooling around, little blindly. Hope to be more helpful next time.

Never mind. I've attached another small variation, that could get it done. I'm looking forward to your findings. Good luck.

[Nelso G. Jost]

Moving foward! I've tested the new patch​. In deed, the right sintax is tracform_id AS id - without single quotes. SQL for table forms_history did run OK, but there was another one for table forms_fields (line 703) - I adapted by creating a postgre option and correcting sintaxe just like in forms_history instructions. Finally, the all the db update did run OK.

Now, testing the plugin in action following the "Shopping List" tutorial (nice documentation, by the way), the form are generated fine - macro successfully processed. Unfortunately, "Got 'em!" button still doesn't work.

Trac[tracdb] ERROR: EXECUTING SQL:
SELECT CURRVAL('id_id_seq')
	()
	SQL ERROR: relation "id_id_seq" does not exist
LINE 1: SELECT CURRVAL('id_id_seq')
                       ^

PS: This time I've make sure to grant FORM_EDIT_VAL permission.

[Steffen Hoffmann]

Replying to nelsojost:

Moving foward! I've tested the new patch​. In deed, the right sintax is tracform_id AS id - without single quotes. SQL for table forms_history did run OK, but there was another one for table forms_fields (line 703) - I adapted by creating a postgre option and correcting sintaxe just like in forms_history instructions. Finally, the all the db update did run OK.

Got it, so this should be fine. Thank you.

Now, testing the plugin in action following the "Shopping List" tutorial (nice documentation, by the way), the form are generated fine - macro successfully processed. Unfortunately, "Got 'em!" button still doesn't work.

Trac[tracdb] ERROR: EXECUTING SQL:
SELECT CURRVAL('id_id_seq')
	()
	SQL ERROR: relation "id_id_seq" does not exist
LINE 1: SELECT CURRVAL('id_id_seq')
                       ^

PS: This time I've make sure to grant FORM_EDIT_VAL permission.

If you didn't grant that permission, it simply wouldn't be there. "Doesn't work" is a functional problem. This time it's about the db cursor implementation (see tracdb.py too).

The error is raised from executing formdb.py line 131-138:

                    form_id = cursor("""
                        INSERT INTO forms
                            (realm, resource_id, subcontext,
                            state, author, time)
                            VALUES (%s, %s, %s, %s, %s, %s)
                        """, realm, resource_id, subcontext,
                        state, author, updated_on) \
                        .last_id('forms', 'id')

and the logic behind the last_id property is in tracdb.py line 103-104:

    def last_id(self, cursor, table, column='id'):
        return self.db.get_last_id(self, table, column)

inherited from the logic in Trac itself (see trac.db.postgres_backend):

    def get_last_id(self, cursor, table, column='id'):
        cursor.execute("SELECT CURRVAL('%s_%s_seq')" % (table, column))
        return cursor.fetchone()[0]

Information explaining what's going on here could be found i.e. at ​http://www.postgresql.org/docs/8.2/interactive/functions-sequence.html

Especially from the last two definitions you can see that last_id in formdb.py line 138 is missing the cursor attribute. I've tested with SQLite, and it does no harm. This is included in the next patch version for you. I suspect, that we've found a hidden bug due to stricter parser logic of PostrgreSQL. Looks like digging into the new SQL flavor has already payed back.

Summary of late changes:

tracformsplugin/trunk/0.11/tracforms/formdb.py

@@ -135 +135 @@
                             VALUES (%s, %s, %s, %s, %s, %s)
                         """, realm, resource_id, subcontext,
                         state, author, updated_on) \
-                        .last_id('forms', 'id')
+                        .last_id(cursor, 'forms', 'id')
                 else:
                     cursor("""
                         UPDATE  forms
@@ -706 +706 @@
                      tracform_id 'id', field, 
                      updater 'author', updated_on 'time'
                 FROM tracform_fields
+            """,
+            postgres = """
+            CREATE TABLE forms_fields
+                AS SELECT
+                     tracform_id AS id, field,
+                     updater AS author, updated_on AS time
+                FROM tracform_fields
             """)
         cursor("""
             DROP INDEX tracform_fields_tracform_id_field

Since this has been a travel to the outer perimeters of my programming and SQL knowledge, I'm eagerly awaiting your findings. Good luck.

[Steffen Hoffmann]

compatibility patch updated with second create table and -last_id fix

[Nelso G. Jost]

Hi. I put the missing cursor attribute like in the diff. Something curious happen: the handler /form/update sends the following message (that I coudn't find in debug log):

'DBCursor' object has no attribute 'fetchone'

When trying refresh the page (resending /form/update), I got Conflict message - values already set in db? Going back to the wiki page: the values was recorded in deed. I've try new submits: all works fine!

I also try with new pages: this error really just happen in the first submit. And with tickets, I have to use the macro [[subpage()]] because [[Include()]] doesn't work - also here, the error occours only in the first submit.

[Steffen Hoffmann]

Replying to nelsojost:

Hi. I put the missing cursor attribute like in the diff. Something curious happen: the handler /form/update sends the following message (that I coudn't find in debug log):

'DBCursor' object has no attribute 'fetchone'

Last thing first: I often encounter such a situation myself, that there's nothing in the logs, if the error is displayed in the browser window. Maybe I'll find some time to improve direction/duplication of output for such events. But this is more of a long-term goal.

Another research is needed for the error regarding the fetchone attribute. I'll look into it and comment later. Thank you again for your testing effort. But it looks encouraging, that we've obviously moved beyond the point, where we encounter fatal errors.

When trying refresh the page (resending /form/update), I got Conflict message - values already set in db? Going back to the wiki page: the values was recorded in deed. I've try new submits: all works fine!

Expected. You can't send the same revision twice. You need to go back to the parent page URL. I could only try to add a redirect-back-to-referrer-on-error logic and show the failure on top of the page. What do you think?

I also try with new pages: this error really just happen in the first submit.

This is a very helpful observation indeed.

And with tickets, I have to use the macro [[subpage()]] because [[Include()]] doesn't work - also here, the error occours only in the first submit.

Really? I did use the IncludeMacro hundreds of times in my tickets with Trac 0.13dev without problems. But regarding TracFormsPlugin this is slightly off-topic here. Better mention this inside a ticket directly assigned to IncludeMacro, please.

[Steffen Hoffmann]

enable debug traceback for all calls to fetchone

[Steffen Hoffmann]

Patch above should enable you to see more inside the log. You may choose to send output attached to an email directly to me.

[Steffen Hoffmann]

(In [10168]) TracFormsPlugin: Add compatibility SQL statements for PostgreSQL, refs #5608 and #5667.

This has been developed from a formdb.py of tracforms-0.2 modified by

19. Noack, further testing done by Nelso. Thanks for your contribution.

Despite remarkable progress this is only nearly working. After some hours of try-n-error I decided to not follow this path any longer. As a matter of fact the whole db code of this plugin is broken by design. I've already straightened it a bit, but the custom db cursor implementation seems evil. Reinventing connection wrapper code by own compatibility was never smart and is bound to be finally brocken after upcoming switch from env.get_db_cnx to the new with_transaction, that has much more intelligent transaction handling in general.

[Steffen Hoffmann]

major SQL rewrite following best practice for Trac < 0.12, see ​Trac db API docs

[Steffen Hoffmann]

Attached latest changes, that should lead to code for the next major TracForms version:

 formdb.py |  717 +++++++++++++++++++++++---------------------------------------
 model.py  |    6 
 tracdb.py |  212 +++++++-----------
 web_ui.py |    1 
 4 files changed, 356 insertions(+), 580 deletions(-)

As you'll see this is a big code-cleanup resulting in totally different db handling. By design this will release db connections faster i.e. optimized for concurrent db access. And largely based on proved Trac core code we'll be able to support upcoming Trac versions without major headache.

I'd be happy, if you could patch this on-top of current trunk, test and report back your findings.

Hint: I've introduced a second, direct install method that is executed, if no previous TracForms version is detected. So you're no longer jumping through all the loops of TracForms db schema history but will be lifted up to the top in an instant. This should improve first install experience a lot. Give it a try inside of a (fresh) test environment with PostgreSQL db configured as well, please.

[Steffen Hoffmann]

Side-note: We'll need compatibility code or a dedicated 0.13 branch to cope with the current with_transaction db handling since 0.12 - again slightly modified in 0.13, where the old env.get_db_cnx is already strongly depreciated now. Obviously it will no longer be available in future Trac versions.

[Nelso G. Jost]

Sorry about the delay. Today I've tested the newest trunk, with both a "fresh" enviroment, and with the old one (with tables already created). Neither of them did work. I mean, the error 'DBCursor' object has no attribute 'fetchone' still happens only in the first submit.

I put the traceback calls like in the debug patch​ and did sent the debug log to your email (hoff.st@web.de, as in setup.py). I hope it can be useful.

Nice work with de plugin and the dedication to make it right.

[Steffen Hoffmann]

Replying to nelsojost:

Sorry about the delay.

Hey, np.

Today I've tested the newest trunk, [...]

Well, the last patch has not been committed 'til now, so this was most probably not the correct code for the test. Since it's such a big change, I intended to test more before actually committing it. Sorry for the irritation caused that way at your side. See your mail for my direct reply/next step.

[Steffen Hoffmann]

Just a quick catch-up for public tracking of some private test releases and corresponding testing.

After a request at trac-dev and kindly response by Remy Blank with some valuable hints on the right way to use Trac db cursors I was able to improve the code to a nearly working state by now:

Commenting an email by nelsojost on 05/17/2011 20:27:

I've tested "20110514_tracforms_sql-fixup.tgz" first with the existing db. All works really well! Not a single error message on a debug log. I also took a look into database records: all seems ok.

But when testing in a fresh new enviroment, an error occours in upgrade attempt: IntegrityError: duplicate key value violates unique constraint "system_pkey"

So a final commit is near, hopefully this week considering the current pace of development. The shortcut saved us some more useless iterations here.

More testers for MySQL and PostgreSQL still very welcome. Please contact me, if you're interested in the early (pre-release) beta for TracForms 0.4 - before the next commit. Soon it'll be all public again anyway.

[Steffen Hoffmann]

(In [10211]): TracFormsPlugin: Anwser a long standing question from inside formdb.py, refs #5667.

# Please, please tell me there's a better way to do this...

Yes, it is: Don't try to re-invent the Trac db cursor but just go and use the native one, and i.e. MySQL/PostgreSQL compatibility comes for free by itself.

SQL rewrite details

• introduce native Trac db type selector
• drop DBCursor class with a lot of proprietary, unused special methods
• remove 'DROP' SQL statements before dropping entire tables anyway
• separate index creation SQL statements obsoleted and common suffix for Trac db indexes to new TracForms indexes created automagicly using db_connector.to_sql()

Thanks to osimons, who made this finally clear to me and again encoraged me to do things in a cleaner fashion. Thanks to rblank for valuable hints on proper use of Trac db cursors. And many thanks to Nelso G. Jost for patiently testing published code as well as many more private iterations of code changes in Trac environments using the PostgreSQL db connector.

[Steffen Hoffmann]

(In [10216]) TracFormsPlugin: Releasing version 0.4, pushing development to 0.5, closes #5318, #5608, #5667.

Codename 'Nelso'. ;-) This is the first release supporting PostgreSQL for storing forms data, effectively working equally well with all db backends, that are currently supported by Trac itself. Another big step towards a stable plugin. Thanks to all the kind people, who helped with their contributions in many ways. It was another rewarding experience to find the kinks and iron them out. Now everybody enjoy the new Trac power under the hood.