Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transactions might not work properly #6

Closed
fabiospampinato opened this issue May 17, 2022 · 10 comments
Closed

Transactions might not work properly #6

fabiospampinato opened this issue May 17, 2022 · 10 comments

Comments

@fabiospampinato
Copy link

fabiospampinato commented May 17, 2022

I think transactions might not work properly here, unless I'm misreading the code what's happening is that basically something like the following is executing:

BEGIN TRANSACTION;
QUERY1;
QUERY2;
...
QUERYN;
COMMIT;

But say I'm executing the following:

CREATE TABLE example ( id INTEGER PRIMARY KEY, title TEXT, description TEXT );
BEGIN TRANSACTION;
INSERT INTO example VALUES( 1, 'title1', 'description1' );
INSERT INTO example VALUES( 2, 'title2', 'description2' );
INSERT INTO example VALUES( 1, 'title1', 'description1' );
COMMIT;

The result of that is that 2 rows are inserted into the table, while I think what we actually want is something like: "did all the previous query execute successfully? If so we execute a "COMMIT" at the end, otherwise we execute a "ROLLBACK".

Like I wouldn't expect to see any rows in that table at the end of the transaction.

@fabiospampinato fabiospampinato changed the title Transitions might not work properly Transactions might not work properly May 17, 2022
@WebReflection
Copy link
Owner

How are you querying that? No code, no help, sorry … also, transaction is a helper

@fabiospampinato
Copy link
Author

I'm just looking at the code, not really executing it. I mean the world "ROLLBACK" is not present anywhere in the repo, so this doesn't seem to be working properly. Feel free to close the issue if you like.

@WebReflection
Copy link
Owner

I've limited time these days so if you find anything worth fixing please present a use case, some test to keep this module is 100% test covered, and we'll improve it.

in this case it looks like commit is in but rollback is not, or maybe the rollback should happen if there's any error in the transaction, but now you have a pointer to the code responsible for transactions.

@fabiospampinato
Copy link
Author

fabiospampinato commented May 17, 2022

I understand, I'm not really asking for anything here, just sharing some findings. Unfortunately this is all the info I can provide:

  • Failing transactions are not committed by this module because with the -bail flag the process exits as soon as the first query fails. So probably that flag should be removed otherwise transactions are kind of unusable I think.
  • Once the flag is removed errors in transactions should be caught, if an error is caught something like ROLLBACK TRANSACTION should be executed, otherwise COMMIT should be executed.
  • Transactions implemented like this I don't think are nestable, I think there are some tweaks necessary to make them work when nested too.
  • I've just published my thing, all the code is here: https://github.com/fabiospampinato/tiny-sqlite3

Btw looking at the benchmark our approach is like 10~100x slower than whatever better-sqlite3 is doing (at least on my computer, which runs macOS) 😱

@WebReflection
Copy link
Owner

WebReflection commented May 18, 2022

It’s the second time you do this … I start seeing a pattern: you check other projects and you go out with your thing … this is not how projects grow so I think I’ll stop suggesting you my stuff as that’s not the kind of contribution I expect.

@WebReflection
Copy link
Owner

P.S. in the Enterprise world you’d likely break intellectual property and copyright going this way, be careful out there 😉

@WebReflection
Copy link
Owner

P.S.2 my benchmark is different: persistent connection and no bind …. Bind is unfair so you’re not benchmarking apples vs apples

@WebReflection
Copy link
Owner

WebReflection commented May 18, 2022

about intellectual property: this is a rip-off of this module, it's like people copy/pasting from stack overflow pretending it was them writing the code.

That part, among others, doesn't need an "inspired by" link in the README, that requires the copyright you took the code from, with your modifications. That's how MIT/ISC works: as long as you maintain the copyright you can do whatever you want. If you modify the original implementation you should still maintain the copyright.

I am closing an eye this time but I will file a CR infringement complain the very next time I see this happening.

Closing this as you never cared about contributing here, you just took my time to implement your thing violating copyrights.

This attitude doesn't look good Fabio, please know better than this.

@fabiospampinato
Copy link
Author

Hey Andrea, I'm sorry, I didn't mean for this to happen. For the sake of clarification, hopefully:

  • I added your license to the license file: https://github.com/fabiospampinato/tiny-sqlite3/blob/master/license, I'm no lawyer so if you think this isn't appropriate or it isn't enough I'm not intentionally trying to not make things straight and clear, quite the opposite. I'm open for suggestions of course.
  • I've updated the "thanks" and "license" sections of the readme to make it clearer that this is derivative work https://github.com/fabiospampinato/tiny-sqlite3#thanks
  • I personally think you've put more work into tiny-sqlite3 than I did really, though I also genuinely don't consider it a complete copy/paste with a different name, there are some differences, I hope you'd agree on that.
  • I genuinely opened issues here about things that I thought could be of use to you as I found them, I guess there's no way to prove that, but like there wasn't really anything for me to gain from mentioning that transactions don't seem to work right to me in this module or that support for the Blob data type could perhaps be improved.
  • I don't know what the other module that you are referring to is, I certainly read open-source code and I have a tendency to re-write things, partially or in full, that's for sure. If you know of some place where I messed up with the licensing or just the credits I'd be grateful if you could mention it to me.

Sorry again for the problems.

@WebReflection
Copy link
Owner

Thanks for improving the situation, I appreciate that, yet I shouldn't have "warned" you were doing it wrong.

This project comes after dblite which worked well for at least simple stuff for years, as new modern alternative.

Creating yet another project and call it also tiny, as if my solutions are bloated or something, doesn't help you, doesn't help me, doesn't help this project, it's just code branched out here and there.

I was going to share more insights on other possible solutions but now I've lost any interest because you have now your own thing which, like you said, it's basically my thing moved somewhere else because ... I don't really understand why.

I am closing this conversation as this left me bitter taste in my mouth, I just hope you'll do better out there in the future.

Take care 👋

Repository owner locked as resolved and limited conversation to collaborators May 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants