Closed Bug 583882 Opened 9 years ago Closed 9 years ago

Need a way to clone an existing connection

Categories

(Toolkit :: Storage, defect)

defect
Not set

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)

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.
Attached patch v0.1 (obsolete) — Splinter Review
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.
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.
Now I'm curious... why?
(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.
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #462266 - Attachment is obsolete: true
Attachment #462455 - Flags: superreview?(vladimir)
Attachment #462455 - Flags: review?(bugmail)
Whiteboard: [needs review asuth][needs sr vlad]
Attachment #462455 - Flags: review?(bugmail) → review+
Whiteboard: [needs review asuth][needs sr vlad] → [needs sr vlad]
This patch is missing something important - we should be copying over functions added to the connection too (but busy handlers should not be copied).
Attached patch v2.0 (obsolete) — Splinter Review
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)
Whiteboard: [needs sr vlad] → [needs review asuth][needs sr vlad]
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+
(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.
(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]
(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.
Bug 582703 is now blocking, so this does too.
blocking2.0: --- → betaN+
Attached patch v2.1 (obsolete) — Splinter Review
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)
(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.
Attached patch v2.2 (obsolete) — Splinter Review
per comment 14
Attachment #468499 - Attachment is obsolete: true
Attachment #468502 - Flags: superreview?(vladimir)
Attachment #468499 - Flags: superreview?(vladimir)
Attachment #468502 - Flags: superreview?(vladimir) → superreview?(shaver)
Whiteboard: [needs sr vlad] → [needs sr shaver]
Comment on attachment 468502 [details] [diff] [review]
v2.2

sr=shaver
Attachment #468502 - Flags: superreview?(shaver) → superreview+
Whiteboard: [needs sr shaver] → [can land]
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]
Good news is that the assertion is just checking that "if CREATE is set, then READWRITE must also be set".

This is entirely fixable.
Attached patch v2.3 (obsolete) — Splinter Review
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)
Whiteboard: [needs review asuth]
Due to feature freeze, this blocks b5 (adding new API)
blocking2.0: betaN+ → beta5+
Keywords: dev-doc-needed
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+
Whiteboard: [needs review asuth] → [needs updated patch]
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.
Attached patch v2.3 to v2.4 interdiff (obsolete) — Splinter Review
Whiteboard: [needs updated patch] → [waiting on try server results (67350d60c329)]
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]
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.
Attached patch v2.5Splinter Review
Sorry for all the bugspam.  This one should be good.
Attachment #468718 - Attachment is obsolete: true
Attachment #468812 - Attachment is obsolete: true
Whiteboard: [needs new patch] → [can land]
http://hg.mozilla.org/mozilla-central/rev/bc95b9869d3f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → mozilla2.0b5
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?
(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.
Blocks: 599978
Blocks: 599980
You need to log in before you can comment on or make changes to this bug.