1198556 - PostgreSQL crashes when deleting a custom field of type INTEGER

Status: NEW

(Bugzilla :: Database, defect)

Description

[Matt Tyson]

When Bugzilla creates an integer custom field, it is created as 'NOT NULL DEFAULT 0'

When you mark the field as obsolete and attempt to delete it, it will attempt the following query:

SELECT COUNT(*) FROM bugs WHERE $name IS NOT NULL AND $name != ''

This works on MySQL as it will implicitly cast '' to 0.
Postgres doesn't do this kind of typecasting implicitly and so the process blows up.

Should we allow the deletion if all fields are set to zero?
The other approach I can think of is to make the field DEFAULT NULL and allow the deletion if all fields are NULL.

Does anyone have a preference for either approach?

Comment 1

[Simon Green]

(In reply to Matt Tyson from comment #0)
> The other approach I can think of is to make the field DEFAULT NULL and
> allow the deletion if all fields are NULL.
> 
> Does anyone have a preference for either approach?

This isn't the correct approach. NULL is different to 0, and leads to confusion when doing a search.

Comment 2

[Matt Tyson]

(In reply to Simon Green from comment #1)
> This isn't the correct approach. NULL is different to 0, and leads to
> confusion when doing a search.

The flipside is that zero is a valid value that the user could have entered.  It's not a reliable indicator that the field is 'empty'.

However MySQL will delete fields with zero in them, so explicitly testing for zero and deleting if all matched would be consistent with existing behaviour.

Comment 3

[Matt Tyson]

Attachment: delete an integer field if all values are equal to zero.

This patch will allow the custom field deletion to proceed if all integer type fields are equal to zero.

Comment 4

[Frédéric Buclin]

(In reply to Simon Green from comment #1)
> This isn't the correct approach. NULL is different to 0, and leads to
> confusion when doing a search.

You are correct that NULL is different to 0. But 0 is a valid integer. NULL means that the field is empty. I agree with Matt that if an integer field has no value, it should be NULL, not 0, because you have no way to make the difference between "0 as no value" and "0 as the value entered by the user".

For 5.0, such a change is not possible because DB schema changes are not allowed on stable branches. For 6.0, I would prefer empty integer fields to be NULL by default.

Comment 5

[Gervase Markham [:gerv]]

(In reply to Frédéric Buclin from comment #4)
> For 5.0, such a change is not possible because DB schema changes are not
> allowed on stable branches. For 6.0, I would prefer empty integer fields to
> be NULL by default.

What would that mean for old values? Updating existing fields to be DEFAULT NULL? How do you migrate the data - assume all 0 values should be NULL? Or leave them at 0?

Gerv

Comment 6

[Gervase Markham [:gerv]]

Comment on attachment 8652679 [details] [diff] [review]
delete an integer field if all values are equal to zero.

Review of attachment 8652679 [details] [diff] [review]:
-----------------------------------------------------------------

Removing review request pending discussion of right way forward.

Gerv

Comment 7

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #5)
> What would that mean for old values? Updating existing fields to be DEFAULT
> NULL? How do you migrate the data - assume all 0 values should be NULL? Or
> leave them at 0?

That's the problem. We have no way to know if the user entered 0 himself, or if it was just the default value because the user didn't set it. For 5.0, I think Matt's patch makes sense. That's unfortunate, but that's the best we can do. We cannot migrate old data for this reason. If the user really meant 0, it would be a bad assumption to convert it to NULL.

What we can do for master, though, is to change the default value from 0 to NULL, so that this problem doesn't happen again for new values.

Comment 8

[Gervase Markham [:gerv]]

So it seems like we want Matt's current patch on trunk and 5.0, except that we only want it to work for FIELD_TYPE_INTEGER columns which are DEFAULT 0, not any new ones which are DEFAULT NULL. Matt: are you able to produce such a patch?

We also want another patch to change the default type for FIELD_TYPE_INTEGER to DEFAULT NULL, on trunk only. Matt: care to whip that one up too? :-)

Gerv

Comment 9

[Matt Tyson]

Attachment: 1198556_trunk

Sure thing, here's one for trunk that will make the field default to NULL.

Comment 10

[Frédéric Buclin]

Comment on attachment 8653817 [details] [diff] [review]
1198556_trunk

>         if ($self->type != FIELD_TYPE_BUG_ID
>             && $self->type != FIELD_TYPE_DATE
>-            && $self->type != FIELD_TYPE_DATETIME)
>+            && $self->type != FIELD_TYPE_DATETIME
>+            && $self->type != FIELD_TYPE_INTEGER)
>         {
>             $bugs_query .= " AND $name != ''";
>         }

I didn't look at the current code, but why looking for != '' for field types such as INTEGER or BUG_ID? '' is not a valid integer nor a valid bug ID. Is the empty string really stored in the DB? I'm surprised PostgreSQL doesn't complain about that. Even an empty date should be stored as NULL, not as ''.

Comment 11

[Gervase Markham [:gerv]]

(In reply to Frédéric Buclin from comment #10) 
> I didn't look at the current code, but why looking for != '' for field types
> such as INTEGER or BUG_ID?

That's != not ==. He is excluding the FIELD_TYPE_INTEGER from the fields where we also check for ''. So all you get is:
$bugs_query = "SELECT COUNT(*) FROM bugs WHERE $name IS NOT NULL";
which is what you want.

Gerv

Comment 12

[Gervase Markham [:gerv]]

LpSolit: this patch (attachment 8653817 [details] [diff] [review]) is surely only for trunk, isn't it?

Comment 13

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #12)
> LpSolit: this patch is surely only for trunk, isn't it?

That's a good question. The original implementation has been done in 5.0 (bug 446178), so I think we should fix the bad DB schema definition in 5.0 before too much people start creating custom fields of type INTEGER (Matt's patch). For those who already created such custom fields, I would even be tempted to say that they should be converted to the right format (NOT NULL -> NULL), but leave 0's alone (comment 7). This would give a chance to admins/power users to convert 0 to undef.

While testing Matt's patch, I realized that the fix is not complete. _check_integer_field() in Bug.pm must be fixed to stop returning 0 when the custom field is left empty. Could the patch be updated accordingly?

Another bug I found is that the bug history doesn't record undef -> 0 and 0 -> undef changes. But this problem could be fixed in a separate bug.

Comment 14

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #13)
> That's a good question. The original implementation has been done in 5.0
> (bug 446178)

I meat bug 466178, as specified in the dependency tree.

Comment 15

[Matt Tyson]

Attachment: 1198556_trunk_2

Updated to fix _check_integer_field().

Let me know what you'd like to fix this bug for 5.0 and I can submit a patch for that.

> Another bug I found is that the bug history doesn't record undef -> 0 and 0 -> undef changes.

It looks like this is caused by the following line in LogActivityEntry()

> while ($removed || $added) {

since 0 and '' are both false, nothing happens.
Is this if() statement checking for definedness or truth?

Comment 16

[Gervase Markham [:gerv]]

(In reply to Matt Tyson from comment #15)
> Created attachment 8658001 [details] [diff] [review]
> 1198556_trunk_2
> 
> Updated to fix _check_integer_field().
> 
> Let me know what you'd like to fix this bug for 5.0 and I can submit a patch
> for that.

I think we should fix the field definitions in 5.0 but leave the data alone. We can add a release note informing admins so they can manually fix it if they like. In practice, I doubt anyone will care. It may well be no-one has created an INTEGER custom field yet!

> > Another bug I found is that the bug history doesn't record undef -> 0 and 0 -> undef changes.
> 
> It looks like this is caused by the following line in LogActivityEntry()
> 
> > while ($removed || $added) {
> 
> since 0 and '' are both false, nothing happens.
> Is this if() statement checking for definedness or truth?

I think it should be checking for definedness.

If you can produce updated patches for trunk and 5.0, I will review them.

Gerv

Comment 17

[Gervase Markham [:gerv]]

Comment on attachment 8658001 [details] [diff] [review]
1198556_trunk_2

Review of attachment 8658001 [details] [diff] [review]:
-----------------------------------------------------------------

See previous comment.

Gerv