[Monotone-devel] Re: comments on .sqlite3.binary branch

[Christof Petig]

Nathaniel Smith schrieb:
> I can see you're making an effort to match our standard style more
> closely, which I really appreciate :-).  I did notice a few
> places that were off, though, so I'll mention them again:

Sorry, sometimes I simply fall into old habits (saving space on the screen).

> Are pack/unpack/sqlite3_unpack_fn/sqlite3_unbase64_fn now dead code
> that can be removed?

pack and unpack should be dead code, sqlite3_unpack should get replaced
by a gunzip (or inflate) function. sqlite3_unbase64 should only get used
in schema_migration (which has to ship a copy since the function is
registered too late). So basically all of them can go away.

> The changes to dump_row_cb look wrong -- shouldn't we just be putting
> the sqlite3_stmt into the dump_request structure, instead of rewriting
> dump_table_cb to iterate by hand?  I haven't looked super-closely at
> this, though.

I tried to minimize changes to make the review more easy. Replacing this
function would have been more obvious, in retrospect. You might simply
tell me to do it.

> I feel like the queryarg stuff isn't _quite_ the right API yet; it
> should be possible to make it a bit cleaner.  This might be something
> to defer, though, I will see how much work it would be.

I decided to stay with the most minimalistic class wrapper possible for
now. Since sqlite3 also supports binary stored integers (and floats
since 3.3) there is room for a redesign to cover these, if ever needed.

> (Does inheriting from std::string to add a public bool attribute seem
> a little odd to anyone else?  Obviously it works, but I would have
> used a struct { std::string; bool; } instead.)

It's odd. It tries to look like a string (which it had been unless I
realized that sqlite3 clearly distinguishes between BLOBs and strings
when it comes to equalness).

> Ditto on Tim's comments.

We can kill all the WHERE conditions on migration without changing
anything (I now trust sqlite3 enough to not give us the same row twice,
even if updated), and the H4sI comment can get replaced by "I would love
to use ALTER TABLE COMMENT if it had been available"

> I think that's everything; the cert base64ing stuff is clearly
> suboptimal but I can see how one might want to put off touching cert
> stuff for now -- cert.cc is ooold code...

I might kill it once binary is settled.

> Something we should vaguely consider -- switching to 'deflate' instead
> of 'gzip', since we're rewriting all the packed values anyway.  This
> saves 18 bytes per cell.  'db info' tells me that in my db, it would
> save about half a meg (1%).  Not that big a deal, but it might make
> some code simpler (Matt knows about this).

Can the inflate code handle both variants or do we need to recode the
whole database (and introduce yet another binary incompatibility over
the wire).

   Christof

signature.asc
Description: OpenPGP digital signature