#2505 closed defect (fixed)

Magic Folder: consider types of (size, ctime, mtime) more carefully

Reported by: daira Owned by: daira
Priority: normal Milestone: undecided
Component: code-frontend-magic-folder Version: 1.10.1
Keywords: blocks-magic-folder-merge Cc:
Launchpad Bug:

Description

These need to be compared, so it's important that the magic folder db be able to represent them precisely enough.

Change History (11)

comment:2 Changed at 2015-12-03T19:29:21Z by daira

  • Keywords blocks-magic-folder-merge added; otf-magic-folder-objective4 removed
  • Status changed from new to assigned

comment:3 Changed at 2016-01-18T20:39:25Z by daira

The issue here is, for all possible floats f that can be returned by time.time(), does storing f in a database field and then retrieving it result in a float == to f? Since we compare timestamps using ==, if this were not the case, then we'd detect spurious timestamp mismatches.

comment:4 Changed at 2016-01-18T20:42:56Z by daira

I don't know where the type NUMBER came from in the current schema. We probably want REAL.

comment:5 follow-up: Changed at 2016-01-19T14:42:43Z by dawuud

  • Status changed from assigned to new

ok here's my solution: https://github.com/david415/tahoe-lafs/tree/2505.db-schema-types.0

i've replaced NUMBER and TIMESTAMP with REAL. According to this link, TIMESTAMP is useful if you sometimes want to have a default value of the current time; we do not.

https://androidcookbook.com/Recipe.seam?recipeId=413

All tests pass. Review needed.

comment:6 Changed at 2016-01-19T17:51:20Z by meejah

dawuud's changes look good to me; certainly an improvement over what's there now.

I do share the == concern about floats -- *especially* because Python's internal representation isn't necessarily an 8-byte IEEE as SQLite's is.

Also, although time.time() does return a float, we really only care about seconds so wouldn't lose anything by storing an INTEGER in the database and always doing int(time.time()) when comparing timestamps (that is, throwing away fractional seconds). Also, within Twisted, it's far better to use IReactorTime.seconds() anyway. So whenever it's convenient (e.g. whenever we have a reactor instance to call .seconds() on) we should probably prefer that. As a side-effect, this makes testing things that use that interface properly way easier.

Personally, I think the integer based approach is far less likely to have weird errors or rounding issues etc. and is thus overall way safer.

comment:7 in reply to: ↑ 5 Changed at 2016-01-21T15:36:57Z by daira

Replying to dawuud:

ok here's my solution: https://github.com/david415/tahoe-lafs/tree/2505.db-schema-types.0

i've replaced NUMBER and TIMESTAMP with REAL. According to this link, TIMESTAMP is useful if you sometimes want to have a default value of the current time; we do not.

https://androidcookbook.com/Recipe.seam?recipeId=413

All tests pass. Review needed.

Reviewed, LGTM.

Last edited at 2016-01-21T15:37:27Z by daira (previous) (diff)

comment:9 Changed at 2016-01-21T17:13:09Z by dawuud

  • Resolution set to fixed
  • Status changed from new to closed

comment:10 Changed at 2016-03-29T14:53:48Z by daira

  • Resolution fixed deleted
  • Status changed from closed to reopened

This is causing failures on Windows exactly because of the problem in comment 3. On Linux the problem is hidden because the events caused by writing the downloaded file are different (see #2737).

comment:11 Changed at 2016-04-04T14:25:12Z by daira

  • Resolution set to fixed
  • Status changed from reopened to closed

We changed the types to integer nanoseconds (currently this is only on the 1431.windows-fixes.3 branch).

Note: See TracTickets for help on using tickets.