Closed
Bug 583882
Opened 14 years ago
Closed 14 years ago
Need a way to clone an existing connection
Categories
(Toolkit :: Storage, defect)
Toolkit
Storage
Tracking
()
RESOLVED
FIXED
mozilla2.0b5
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta5+ |
People
(Reporter: sdwilsh, Assigned: sdwilsh)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 7 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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.
Assignee | ||
Comment 2•14 years ago
|
||
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•14 years ago
|
||
Now I'm curious... why?
Assignee | ||
Comment 4•14 years ago
|
||
(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.
Assignee | ||
Comment 5•14 years ago
|
||
Attachment #462266 -
Attachment is obsolete: true
Attachment #462455 -
Flags: superreview?(vladimir)
Attachment #462455 -
Flags: review?(bugmail)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review asuth][needs sr vlad]
Updated•14 years ago
|
Attachment #462455 -
Flags: review?(bugmail) → review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review asuth][needs sr vlad] → [needs sr vlad]
Assignee | ||
Comment 6•14 years ago
|
||
This patch is missing something important - we should be copying over functions added to the connection too (but busy handlers should not be copied).
Assignee | ||
Comment 7•14 years ago
|
||
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.
Attachment #462455 -
Attachment is obsolete: true
Attachment #464906 -
Flags: superreview?(vladimir)
Attachment #464906 -
Flags: review?(bugmail)
Attachment #462455 -
Flags: superreview?(vladimir)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs sr vlad] → [needs review asuth][needs sr vlad]
Comment 8•14 years ago
|
||
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.
Attachment #464906 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 9•14 years ago
|
||
(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.
Assignee | ||
Comment 10•14 years ago
|
||
(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?
Whiteboard: [needs review asuth][needs sr vlad] → [needs sr vlad]
Comment 11•14 years ago
|
||
(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.
Assignee | ||
Comment 12•14 years ago
|
||
Bug 582703 is now blocking, so this does too.
blocking2.0: --- → betaN+
Assignee | ||
Comment 13•14 years ago
|
||
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.
Attachment #464906 -
Attachment is obsolete: true
Attachment #468499 -
Flags: superreview?(vladimir)
Attachment #464906 -
Flags: superreview?(vladimir)
Assignee | ||
Comment 14•14 years ago
|
||
(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.
Assignee | ||
Comment 15•14 years ago
|
||
per comment 14
Attachment #468499 -
Attachment is obsolete: true
Attachment #468502 -
Flags: superreview?(vladimir)
Attachment #468499 -
Flags: superreview?(vladimir)
Assignee | ||
Updated•14 years ago
|
Attachment #468502 -
Flags: superreview?(vladimir) → superreview?(shaver)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs sr vlad] → [needs sr shaver]
Comment on attachment 468502 [details] [diff] [review]
v2.2
sr=shaver
Attachment #468502 -
Flags: superreview?(shaver) → superreview+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs sr shaver] → [can land]
Assignee | ||
Comment 17•14 years ago
|
||
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.
Whiteboard: [can land]
Assignee | ||
Comment 18•14 years ago
|
||
Good news is that the assertion is just checking that "if CREATE is set, then READWRITE must also be set".
This is entirely fixable.
Assignee | ||
Comment 19•14 years ago
|
||
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.
Attachment #468502 -
Attachment is obsolete: true
Attachment #468718 -
Flags: review?(bugmail)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review asuth]
Assignee | ||
Comment 20•14 years ago
|
||
Due to feature freeze, this blocks b5 (adding new API)
blocking2.0: betaN+ → beta5+
Assignee | ||
Updated•14 years ago
|
Keywords: dev-doc-needed
Comment 21•14 years ago
|
||
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.
Attachment #468718 -
Flags: review?(bugmail) → review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs review asuth] → [needs updated patch]
Assignee | ||
Comment 22•14 years ago
|
||
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.
Assignee | ||
Comment 23•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs updated patch] → [waiting on try server results (67350d60c329)]
Assignee | ||
Comment 24•14 years ago
|
||
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.
Whiteboard: [waiting on try server results (67350d60c329)] → [needs new patch]
Assignee | ||
Comment 25•14 years ago
|
||
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.
Assignee | ||
Comment 26•14 years ago
|
||
Sorry for all the bugspam. This one should be good.
Attachment #468718 -
Attachment is obsolete: true
Attachment #468812 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: [needs new patch] → [can land]
Assignee | ||
Comment 27•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → mozilla2.0b5
Comment 28•14 years ago
|
||
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•14 years ago
|
||
Now documented:
https://developer.mozilla.org/en/mozIStorageConnection#clone%28%29
And mentioned on Fx4 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 30•14 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•