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 [email protected]. To view this discussion visit https://groups.google.com/d/msgid/h2-database/73c42f2e-8c07-4257-814c-8c5c86448c31n%40googlegroups.com.
