Hello, thanks for your feedback.
> MVStore is actually a third storage engine for H2 and who knows, maybe there will be a new one without this setting or MVStore itself will be changed incompatibly. I think you need to find some other way. I thought about a different solution. I wanted to remove the awareness of H2 whether it is in-memory or persistent entirely. The idea I had was to simply rely on `java.nio.file.FileSystem`. Then H2 could write its data, regardless of storage format, and a restore point would simply make a copy of the data on the filesystem - be it in-memory, e.g. using Jimfs, or on disk. However, since I wanted restore points to work with both modes in H2, in-memory and persistent, and in-memory mode is not transparent (yet), I rejected the idea. I still like the idea though. The current filesystem abstraction in H2 is a good start, but it would need to be able to use `java.nio.file.FileSystem#getPath(...)` on a single `java.nio.file.FileSystem` instance consistently to not end up on a different filesystem. A downside would be that creating a restore point for large databases would be costly when it comes to storage. > Your implementation seems to be not compatible with anything, including Oracle. That's correct. As I said, it is vaguely similar to Oracle. Maybe I should rephrase: It is somewhat inspired by Oracle. However, I don't care (too) much for the specific SQL syntax. I'd be happy to change it to whatever you think fits H2 best. Maybe `create database snapshot` and `restore database from snapshot`. Something like that. I'm open to suggestions. > The integration with oldestVersionToKeep is not ideal, it could certainly be better [...] Do you mean that I shouldn't introduce another variable `restorePointVersion` on top of `oldestVersionToKeep`? I don't think I can make it work without `restorePointVersion` on top, because `oldestVersionToKeep` increments as transactions are being committed. And I need some boundary to tell the actual storage system what to keep and what not. > Please, don't use var and don't invent new interfaces for unit tests. No problem. I got rid of `var` already (not pushed yet). Would it be ok if I added an interface to `BaseTest` similar to `VoidCallable`? Maybe `ThrowingConsumer`? I introduced the interface to deal with "aspects" (connections, statements and auto-commit) and to be able to work with lambdas which can throw exceptions. If you have something else in mind, could you point me in the right direction? > I tried to add some reconnections to your tests and they started to fail. Could you be more specific so I can look into it? The reconnections are one of the reasons I introduced the interface in my unit tests. I.e. everything is executed with and without auto-commit and, unless in-memory, everything is executed with and without reconnections, because I thought those are the most critical aspects when it comes to restore points. Regards, Enno On Friday, November 8, 2024 at 11:09:14 AM UTC+1 Noel Grandin wrote: > > > On 11/8/2024 11:32 AM, Evgenij Ryazanov wrote: > > > > 1. I think it is a very bad idea to implement it on top of > oldestVersionToKeep from MVStore. MVStore by itself creates a > > lot of garbage and this setting will effectively prevent garbage > collection. In case of H2, all these versions also can > > be lost because H2 can rewrite the whole storage during shutdown. > MVStore is actually a third storage engine for H2 and > > who knows, maybe there will be a new one without this setting or MVStore > itself will be changed incompatibly. I think > > you need to find some other way. > > I think the strategy there is sufficient - we don't need to support all > features with all backends. > We can just throw an exception in the future if this feature is used with > a backend which does not support it. > > The integration with oldestVersionToKeep is not ideal, it could certainly > be better, but we should "not let the perfect > be the enemy of the good". > > > 2. Your implementation seems to be not compatible with anything, > including Oracle. You shouldn't use mixed syntax > > partially taken from Oracle, partially your own, because it will prevent > introduction of Oracle-specific features in > > Oracle compatibility mode due to syntax conflict. > > Any such feature is going to be heavily dependant on various details of > how the underlying storage engine works, so I > don't think this is a deal-breaker. I cannot see us ever needing such a > thing in the Oracle compatibility mode. And in > the unlikely event that we do, our Parser class is flexible enough to copy > with having two different syntax paths for > different modes. > > > > > You also should use the same code style as the whole project. Please, > don't use var and don't invent new interfaces for > > unit tests. > > > > I tried to add some reconnections to your tests and they started to > fail. You need to test all these cases too. > > > > Agreed, these areas will need improvement. > > Regards, Noel Grandin > -- You received this message because you are subscribed to the Google Groups "H2 Database" group. To unsubscribe from this group and stop receiving emails from it, send an email to h2-database+unsubscr...@googlegroups.com. To view this discussion visit https://groups.google.com/d/msgid/h2-database/73c42f2e-8c07-4257-814c-8c5c86448c31n%40googlegroups.com.
