Need a way to clone an existing connection

RESOLVED FIXED in mozilla2.0b5

Status

()

Toolkit
Storage
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla2.0b5
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

7 years ago
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

7 years ago
Created attachment 462266 [details] [diff] [review]
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.
(Assignee)

Comment 2

7 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

7 years ago
Now I'm curious... why?
(Assignee)

Comment 4

7 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

7 years ago
Created attachment 462455 [details] [diff] [review]
v1.0
Attachment #462266 - Attachment is obsolete: true
Attachment #462455 - Flags: superreview?(vladimir)
Attachment #462455 - Flags: review?(bugmail)
(Assignee)

Updated

7 years ago
Whiteboard: [needs review asuth][needs sr vlad]
Attachment #462455 - Flags: review?(bugmail) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [needs review asuth][needs sr vlad] → [needs sr vlad]
(Assignee)

Comment 6

7 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

7 years ago
Created attachment 464906 [details] [diff] [review]
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.
Attachment #462455 - Attachment is obsolete: true
Attachment #464906 - Flags: superreview?(vladimir)
Attachment #464906 - Flags: review?(bugmail)
Attachment #462455 - Flags: superreview?(vladimir)
(Assignee)

Updated

7 years ago
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+
(Assignee)

Comment 9

7 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

7 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]
(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

7 years ago
Bug 582703 is now blocking, so this does too.
blocking2.0: --- → betaN+
(Assignee)

Comment 13

7 years ago
Created attachment 468499 [details] [diff] [review]
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.
Attachment #464906 - Attachment is obsolete: true
Attachment #468499 - Flags: superreview?(vladimir)
Attachment #464906 - Flags: superreview?(vladimir)
(Assignee)

Comment 14

7 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

7 years ago
Created attachment 468502 [details] [diff] [review]
v2.2

per comment 14
Attachment #468499 - Attachment is obsolete: true
Attachment #468502 - Flags: superreview?(vladimir)
Attachment #468499 - Flags: superreview?(vladimir)
(Assignee)

Updated

7 years ago
Attachment #468502 - Flags: superreview?(vladimir) → superreview?(shaver)
(Assignee)

Updated

7 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

7 years ago
Whiteboard: [needs sr shaver] → [can land]
(Assignee)

Comment 17

7 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

7 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

7 years ago
Created attachment 468718 [details] [diff] [review]
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.
Attachment #468502 - Attachment is obsolete: true
Attachment #468718 - Flags: review?(bugmail)
(Assignee)

Updated

7 years ago
Whiteboard: [needs review asuth]
(Assignee)

Comment 20

7 years ago
Due to feature freeze, this blocks b5 (adding new API)
blocking2.0: betaN+ → beta5+
(Assignee)

Updated

7 years ago
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+
(Assignee)

Updated

7 years ago
Whiteboard: [needs review asuth] → [needs updated patch]
(Assignee)

Comment 22

7 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

7 years ago
Created attachment 468812 [details] [diff] [review]
v2.3 to v2.4 interdiff
(Assignee)

Updated

7 years ago
Whiteboard: [needs updated patch] → [waiting on try server results (67350d60c329)]
(Assignee)

Comment 24

7 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

7 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

7 years ago
Created attachment 469197 [details] [diff] [review]
v2.5

Sorry for all the bugspam.  This one should be good.
Attachment #468718 - Attachment is obsolete: true
Attachment #468812 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Whiteboard: [needs new patch] → [can land]
(Assignee)

Comment 27

7 years ago
http://hg.mozilla.org/mozilla-central/rev/bc95b9869d3f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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?
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

7 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.
(Assignee)

Updated

7 years ago
Blocks: 599978
(Assignee)

Updated

7 years ago
Blocks: 599980
You need to log in before you can comment on or make changes to this bug.