583882 - Need a way to clone an existing connection

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Shawn Wilsher :sdwilsh]

We should have a way to clone an existing connection so consumers don't have to do all the work again.  Additionally, provide an option for it to be a read only connection to support multiple readers and one writer.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

This is basically there, but I think I found a bug in SQLite.  If you open a db with the shared cache, and then clone, it doesn't matter if you make it read only, you can still write to it.  Pinged drh about it, but he's not around atm.  This is a wip, but is likely reviewable.

Comment 2

[Shawn Wilsher :sdwilsh]

OK, so this is intentional in SQLite.  So, I'll add some notes to the idl, add some tests so we know if this ever changes, and then carry on.

Comment 3

[[e-mail]]

Now I'm curious... why?

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> Now I'm curious... why?
Because these are just flags that they pass to open.  The cache is what opens the database, and since the first one is readwrite, they all will be.  They'll look into making it cache-level instead of file level.

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 6

[Shawn Wilsher :sdwilsh]

This patch is missing something important - we should be copying over functions added to the connection too (but busy handlers should not be copied).

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

This ended up being a bit more invasive than I had hoped, but I have full test coverage on the changes, so I'm pretty confidant that it's safe.  This copies over any added functions to the new connection as well.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 464906 [details] [diff] [review]
v2.0

Might be worth putting a comment in the impl that we will attempt to copy over the functions from registerFunctions() but since they are already bound at the target (registerFunctions is invoked as part of Connection::initialize) their registration call will fail but we don't care which is why we cast the return value of that call to void.

Or not, but it could be surprising to the hypothetical someone who unregisters our default functions to replace them with their own and then discovers that the clone has the original set of functions again and then goes to read the code and is none the wiser.

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> Might be worth putting a comment in the impl that we will attempt to copy over
> the functions from registerFunctions() but since they are already bound at the
> target (registerFunctions is invoked as part of Connection::initialize) their
> registration call will fail but we don't care which is why we cast the return
> value of that call to void.
> 
> Or not, but it could be surprising to the hypothetical someone who unregisters
> our default functions to replace them with their own and then discovers that
> the clone has the original set of functions again and then goes to read the
> code and is none the wiser.
That's an interesting case I hadn't thought about.  Will add the comment.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> Might be worth putting a comment in the impl that we will attempt to copy over
> the functions from registerFunctions() but since they are already bound at the
> target (registerFunctions is invoked as part of Connection::initialize) their
> registration call will fail but we don't care which is why we cast the return
> value of that call to void.
> 
> Or not, but it could be surprising to the hypothetical someone who unregisters
> our default functions to replace them with their own and then discovers that
> the clone has the original set of functions again and then goes to read the
> code and is none the wiser.
Alternatively, I could just call removeFunction if Create fails, and then try again (silently failing still).  I think this is actually probably better, agree?

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #10)
> Alternatively, I could just call removeFunction if Create fails, and then try
> again (silently failing still).  I think this is actually probably better,
> agree?

Yes.

Comment 12

[Shawn Wilsher :sdwilsh]

Bug 582703 is now blocking, so this does too.

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: v2.1

per comment 10, with a test that overrides lower.  asuth, feel free to take a look at this, but I don't think there is much value in you doing so, so I'm not going to request another review.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> Comment on attachment 464906 [details] [diff] [review]
> v2.0
> 
> Might be worth putting a comment in the impl that we will attempt to copy over
> the functions from registerFunctions() but since they are already bound at the
> target (registerFunctions is invoked as part of Connection::initialize) their
> registration call will fail but we don't care which is why we cast the return
> value of that call to void.
> 
> Or not, but it could be surprising to the hypothetical someone who unregisters
> our default functions to replace them with their own and then discovers that
> the clone has the original set of functions again and then goes to read the
> code and is none the wiser.
Actually, it won't fail.  sqlite lets you redfine this way, and my test confirms this.  Taking that code I added back out, and leaving the test in.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: v2.2

per comment 14

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 468502 [details] [diff] [review]
v2.2

sr=shaver

Comment 17

[Shawn Wilsher :sdwilsh]

Things I did not expect to happen:
Running the next test: test_clone_readonly
Assertion failed: (isCreate==0 || isReadWrite), function unixOpen, file /builds/slave/tryserver-macosx-debug/build/db/sqlite3/src/sqlite3.c, line 27130.

this is on top of SQLite 3.7.1 on the try server.

Comment 18

[Shawn Wilsher :sdwilsh]

Good news is that the assertion is just checking that "if CREATE is set, then READWRITE must also be set".

This is entirely fixable.

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: v2.3

This should fix the try server failure (pushing to verify now).  Changes are small, but probably a good idea to have it looked at one more time.

Comment 20

[Shawn Wilsher :sdwilsh]

Due to feature freeze, this blocks b5 (adding new API)

Comment 21

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 468718 [details] [diff] [review]
v2.3

You might want to update the doxygen block on the Connection constructor since it explicitly calls out the CREATE bit and you've moved where the forcing logic happens.

Comment 22

[Shawn Wilsher :sdwilsh]

Still hitting that assertion, but that's because clone copies flags.  I'll post an interdiff, but I don't think I'll need review.

Comment 23

[Shawn Wilsher :sdwilsh]

Attachment: v2.3 to v2.4 interdiff

Comment 24

[Shawn Wilsher :sdwilsh]

OK, not quite there yet...

Running the next test: test_clone_readonly
Assertion failed: (isCreate==0 || isReadWrite), function unixOpen, file /builds/slave/tryserver-macosx-debug/build/db/sqlite3/src/sqlite3.c, line 26096.

Comment 25

[Shawn Wilsher :sdwilsh]

Comment on attachment 468812 [details] [diff] [review]
v2.3 to v2.4 interdiff

(In reply to comment #24)
> Running the next test: test_clone_readonly
> Assertion failed: (isCreate==0 || isReadWrite), function unixOpen, file
> /builds/slave/tryserver-macosx-debug/build/db/sqlite3/src/sqlite3.c, line
> 26096.

Largely because I'm dumb:
>+    // Turn off SQLITE_OPEN_CREATE.
>+    flags = (~SQLITE_OPEN_READWRITE & flags);

I added the same assertion locally in the Connection constructor, and it's now passing tests.  Removing the assertion for pushing, but his is now ready.

Comment 26

[Shawn Wilsher :sdwilsh]

Attachment: v2.5

Sorry for all the bugspam.  This one should be good.

Comment 27

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/bc95b9869d3f

Comment 28

[Eric Shepherd [:sheppy]]

The comments in the IDL seem to be misleading; they refer to cloning the database, but this is just cloning the connection to the database, not the database itself, right?

Comment 29

[Eric Shepherd [:sheppy]]

Now documented:

https://developer.mozilla.org/en/mozIStorageConnection#clone%28%29

And mentioned on Fx4 for developers.

Comment 30

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> The comments in the IDL seem to be misleading; they refer to cloning the
> database, but this is just cloning the connection to the database, not the
> database itself, right?
Yeah, "database" refers to a connection pretty much (if not) uniformly in storage docs.  Will try to be more clear in the future.