the offending query: db_query('INSERT INTO {batch} (timestamp) VALUES (%d)', time());

this violates a not null constraint on token, so the query fails.

attached patch corrects by inserting an empty string for token -- this is replaced by the actual token in the update query that follows.

CommentFileSizeAuthor
#11 form-batch-insert-152383-11.patch934 bytesbjaspan
#5 form_batch.patch1.08 KBax
#2 not_null_defaults.patch18.61 KBhunmonk
batch_query.patch970 byteshunmonk

Comments

hunmonk’s picture

hunmonk commented

btw, the patch has been tested as working on postgres.

hunmonk’s picture

hunmonk commented
Title: batch update broken in postgres » not null columns should have a default value
StatusFileSize
new not_null_defaults.patch18.61 KB

after further discussion w/ chx on this, it seems a good long term fix to always include a default value if you're specifying a column as not null -- this avoids the kinds of headaches produced by the above reported bug. attached patch updates all core .schema files as such, and also the d5 -> d6 updates.

not sure if we need a db update for this as well or not -- if so it should be pretty trivial to add.

bjaspan’s picture

bjaspan commented

I just talked about this with hunmonk. I started off with:

I'm not aware of any weirdness. If you try to insert without providing a value for a not-null column with no default, the query will (and should) fail on any SQL db. So, don't do that. If code is written to assume a default value (e.g. your INSERT INTO batch example), then either the schema should have a default value or the code should be fixed not to assume one. Either is perfectly fine. Since blob/text columns cannot have a default, a schema structure that specifies one is a bug and any INSERT statement that does not provide an explicit value for the column is a bug.

"Always provide a default value for not-null columns" is incorrect though possibly not harmful. Default vlaues and INSERT queries just need to be synchronized.

He then pointed out that MySQL has the (IMHO, really broken) behavior of "implicit default values" in this situation. Actually, I found, only MySQL pre-5.0.2 or in non-strict mode behaves this way. Because MySQL is broken in this way, lots of Drupal code has also evolved to depend on the brokenness. I ended up summarizing my thoughts as follows:

0. This explains why, pre-schema API, so many columns had default values in pgsql but not mysql.

1. All the queries that depend on this behavior are wrong and ought to be fixed either by (a) declaring a default value or (b) fixing the query not to assume one, whichever is most appropriate for that query.

2. "Fixing" the problem by adding default values to (almost) all not-null columns will work. It is kinda sloppy to just make our system retroactively declare as correct lazy behavior that depended on mysql brokenness, but it is also much eaiser than checking every query in core.

4. All queries using blob or text columns just have to be fixed to specify a value because MySQL does not allow default values to be specified for them.

DO NOT commit a fix for this issue until schema.module's Comparison report shows 0 inconsistencies on both pgsql and mysql both after a new install and an update from Drupal 5. If only we had an automated testing framework in place...

hunmonk’s picture

hunmonk commented
Status: Needs review » Needs work
ax’s picture

ax commented
Status: Needs work » Needs review
StatusFileSize
new form_batch.patch1.08 KB

why first inserting a row with only the timestamp (causing the error, because the NOT NULL token is missing), and than updating the very same row with token etc.? just do it in one step. this - attached - fixes the original issue.

bjaspan’s picture

bjaspan commented

The patch in #5 is wrong. db_last_insert_id() returns the auto-incremented value from the most recent INSERT. The reason this patch uses INSERT then UPDATE is because a value in the row depends on the auto-incremented value which isn't known until the INSERT is complete.

ax’s picture

ax commented

sorry, my wrong. i was assuming http://api.drupal.org/api/HEAD/function/db_last_insert_id was correct, ie. really returning the last insert id of $table and $field.

ax’s picture

ax commented

btw: "a value in the row depends on the auto-incremented value [of another column in the row]" : smells like a problem with the database table design, doesn't it?

ax’s picture

ax commented
Status: Needs review » Needs work
hunmonk’s picture

hunmonk commented
Assigned: hunmonk » Unassigned
bjaspan’s picture

bjaspan commented
Title: not null columns should have a default value » update.php fails on pgsql
Status: Needs work » Needs review
StatusFileSize
new form-batch-insert-152383-11.patch934 bytes

I think we should fix these not-null/default conflicts as they arise, not by adding blanket null values all over the place. To that end, I have fixed the specific problem that hunmonk reported and which I have now also encountered. This bug makes update.php completely fail on pgsql, so I've marked it as critical.

My fix is to insert a temporary value explicitly for the 'token' column. Adding a default value would work as well, but my thinking is that a default value should be one that is actually correct most of the time unless a specific value is provided. In this case, a default value will never be right and will always be immediately overriden, so it makes more sense to demonstrate that explicitly in the code.

bjaspan’s picture

bjaspan commented

I just realized that my patch is essentially identical to hunmonk's original patch. Either one is fine.

yched’s picture

yched commented
Status: Needs review » Closed (duplicate)

I merged that patch with the related patch in http://drupal.org/node/149593, which fixes other issues introduced by the db_last_insert_id() patch. Don't hesitate to tell me if you think these should stay separate patches.