kvdb: add sqlite #7251

ellemouton · 2022-12-14T18:06:21Z

In this PR, the kvdb package is first adapted so that all the non-postgres specific code in the kvdb/postgres package is moved into a new kvdb/sqlbase package. Then a new kvdb/sqlite package is added which also makes use of the sqlbase package.

Since the kvdb package is a module, the code that will make use of the new sqlite code will be done in a follow up PR.

Roasbeef

Nice work with the quick turn around here! Just a few things I need to dig a bit deeper into, but I get the extra step to extract out some of the SQL logic, though I think a bit more consolidation is possible.

kvdb/common_sql/db.go

kvdb/common_sql/schema.go

kvdb/common_sql/db.go

kvdb/common_sql/readwrite_bucket.go

kvdb/go.mod

kvdb/kvdb_sqlite.go

Roasbeef · 2022-12-14T20:20:00Z

kvdb/sqlite/db.go

+// postgresReplacements define a set of postgres keywords that should be swapped
+// out with certain other sqlite keywords in any queries.
+var postgresReplacements = common_sql.PostgresCmdReplacements{
+	"BYTEA":                 "BLOB",


See the other comment about potentially reversing this operation (also name inconsistency here).

Roasbeef · 2022-12-15T01:38:04Z

Noting that we'll need to tag a new kvdb version before the dependent PR can land.

kvdb/sqlite/db.go

ellemouton

Thanks for the quick review on this @Roasbeef 💪

kvdb/go.mod

kvdb/sqlite/db.go

kvdb/common_sql/schema.go

kvdb/postgres/db_conn_set.go

kvdb/common_sql/db.go

kvdb/common_sql/schema.go

lightninglabs-deploy · 2022-12-28T17:09:35Z

@Roasbeef: review reminder

guggero

Very nice and straightforward! LGTM, only a few nits or suggestions from my end 🎉

guggero · 2023-01-04T12:40:03Z

kvdb/kvdb_no_sqlite.go

@@ -0,0 +1,21 @@
+//go:build !kvdb_sqlite || (windows && (arm || 386)) || (linux && (ppc64 || mips || mipsle || mips64))


So SQLite won't be available on Windows i386/ARM and any PPC/MIPS platform? Not sure if we need to adjust the error message below to be a bit more verbose about why SQLite isn't available (e.g. "sqlite backend not available for this operating system and/or architecture").

kvdb/sqlite/driver.go

kvdb/go.mod

kvdb/postgres/db.go

kvdb/common_sql/schema.go

kvdb/sqlbase/schema.go

kvdb/backend.go

kvdb/go.mod

Roasbeef · 2023-01-05T02:40:31Z

Found a cool blog post w/ some more options we might want to mess around with: https://phiresky.github.io/blog/2020/sqlite-performance-tuning/

Stand outs there include the auto vacuum and the change to the synchronous arg.

Roasbeef

This PR is really shaping up well! Just a few comments, moving on to some hands on testing now, also have some ideas w.r.t other params we can tune.

kvdb/go.mod

kvdb/postgres/db.go

Roasbeef · 2023-01-05T02:56:00Z

kvdb/sqlbase/readwrite_tx.go

-	if readOnly {
-		locker = db.lock.RLocker()
+	locker := newNoopLocker()
+	if db.cfg.WithTxLevelLock {


Was this a response to that comment above postgres' concurrency control? So then this would be on by default for postgres?

yeah it was in response to the comment about noticing that this tx level locking was not necessary as sqlite handles it. But did not want to make changes to the postgres logic (also ran into itest failures when removing this for postgres).
It is not on by default - in the postgres config, we now set WithTxLevelLock: true

kvdb/go.mod

kvdb/kvdb_no_sqlite.go

kvdb/kvdb_sqlite.go

kvdb/sqlite/config.go

Roasbeef · 2023-01-06T01:17:12Z

Looking pretty good now, looks like one of the test fails due to the new build tags:

Error: cannot read source of file "/home/runner/work/lnd/lnd/kvdb/postgres/db_conn_set.go": open /home/runner/work/lnd/lnd/kvdb/postgres/db_conn_set.go: no such file or directory
Error: Error: The process '/home/runner/work/_actions/shogo82148/actions-goveralls/v1/bin/goveralls_linux_amd64' failed with exit code 1

Roasbeef

LGTM 🍭

guggero · 2023-01-06T07:25:56Z

Looking pretty good now, looks like one of the test fails due to the new build tags:

Error: cannot read source of file "/home/runner/work/lnd/lnd/kvdb/postgres/db_conn_set.go": open /home/runner/work/lnd/lnd/kvdb/postgres/db_conn_set.go: no such file or directory
Error: Error: The process '/home/runner/work/_actions/shogo82148/actions-goveralls/v1/bin/goveralls_linux_amd64' failed with exit code 1

That's weird... The coverage reporter is attempting to read a file that was renamed/moved. Maybe something weird with Go submodules and coverage? Going to re-run to see if that fixes it. Otherwise it will be fixed as soon as we tag the submodule and update it in the follow-up PR.

ellemouton · 2023-01-06T07:38:40Z

yeah I dont think it is cause of the build tags but rather just that it seems to not know how to deal with deleted/moved files. Wasn't worried about this since the test passes in the follow up PR that points to this one

guggero · 2023-01-06T07:44:55Z

yeah I dont think it is cause of the build tags but rather just that it seems to not know how to deal with deleted/moved files. Wasn't worried about this since the test passes in the follow up PR that points to this one

Okay, but let's hold off on merging this PR until the follow-up PR is close to be merged as well. Otherwise the coverage step will be broken on master for a while.

ellemouton · 2023-01-06T10:15:13Z

latest update just adds auto_vacuum=increment to the default pragma options.

Also removed the DBDir option from the sqlite config since we now place it in LNDs chain dir

Roasbeef

LGTM 🎄

kvdb/sqlite/db.go

joostjager

Great to see sqlite being added to lnd!

I did have some trouble understanding the split of the work between this PR and the next #7252. I'd expect this PR to only do prep without any mention of sqlite really yet.

kvdb/postgres/db.go

kvdb/sqlbase/db.go

joostjager · 2023-01-16T13:36:58Z

kvdb/sqlbase/schema.go

+`)
+
+	for from, to := range replacements {
+		finalCmd = strings.Replace(finalCmd, from, to, -1)


An alternative to string replacements after the create definition could perhaps be to have helper functions that output "BLOB" and "BIGSERIAL" based on the requested dialect and use those in the create definition above?

The helper functions could be provided through an interface so that each backend can expose it's own keywords.

This will allow us to write migration *.sql files with the DDL statements in one dialect and then translate them at migration time into other dialects. That avoids needing to stitch together the DDL statements in the code, which I think is nice.

Are you sure you want *.sql files that need to be packaged into the lnd binary?

For other projects I switched to a different migration lib that can also use variables that contain sql code (#6176 (comment)). In that case, you could still use those helper functions.

Yes, that works very well with the virtual file system added in a recent Go version. I saw your comment about the migration library. We didn't run into the described problem yet, but a thing to keep an eye on in the upcoming invoice database migration PR.

I assume you mean you didn't run into it yet with golang-migrate/migrate. It is very easy to trigger it. Just add a panic somewhere in the migration. Then you need the cli tool to clear the dirty flag again.

golang-migrate/migrate#518

kvdb/kvdb_etcd.go

kvdb/sqlite/config.go

kvdb/sqlite/db.go

ellemouton · 2023-01-16T15:58:32Z

Hi @joostjager , thanks for your review!

I did have some trouble understanding the split of the work between this PR and the next #7252.

the reason for the split is because this PR makes changes to the kvdb package which is its own module. So all the changes (including adding sqlite functionality) is added to the kvdb package in this PR. Once it is merged, we can create a new tag for the kvdb package and then we can point to that tag from the follow up PR which adds sqlite as a lnd db backend option.

I will take a look at the rest of your comments some time tomorrow 👍

joostjager · 2023-01-17T09:03:09Z

kvdb/sqlite/db.go

+// sqliteDB contains two sqlite connections, one is optimised for reads as it
+// uses the default BEGIN DEFERRED directive for starting a db transaction and
+// the other is optimised for writes as it uses the BEGIN IMMEDIATE directive
+// for starting a db transaction.


I was reading up on sqlite transactions on https://www.sqlite.org/lang_transaction.html and saw this:

While a read transaction is active, any changes to the database that are implemented by separate database connections will not be seen by the database connection that started the read transaction. If database connection X is holding a read transaction, it is possible that some other database connection Y might change the content of the database while X's transaction is still open, however X will not be able to see those changes until after the transaction ends. While its read transaction is active, X will continue to see an historic snapshot of the database prior to the changes implemented by Y.

To what extent does this apply to your setup with two connections? It seems different from bbolt where you're never reading an historic snapshot afaik?

Good question re how connections work under the hood. I wish both sqlite and bbolt used more formal terminology here such as "snapshot isolation", or "linearizability", etc. AFACIT, this snapshot behavior seems to be identical to how bolt functions: read transactions can happen concurrently, they operate based on a snapshot of the database (the B+Tree is cloned from leaf up to root), write transactions can happen while read transactions are active.

Bolt allows only one read-write transaction at a time but allows as many read-only transactions as you want at a time. Each transaction has a consistent view of the data as it existed when the transaction started.

https://github.com/boltdb/bolt#transactions

I see, yes that sounds mostly identical. What I don't think happens in bbolt is that read and write transactions actually run concurrently? So in bbolt the write only starts after all the reads have finished. For sqlite, it does sound a bit as if they can run in parallel, with the read txes having their snapshot. In lnd, there are sometimes non-database things happening inside a db tx block. Not sure if this may lead to differences in behavior with bbolt vs sqlite, given the potentially parallel transaction execution?

That I don't think happens in bbolt is that read and write transactions actually run concurrently?

My understanding is that there can only be a single writer at a time (there's a write mutex in the main db struct). Read transactions grab the read side of a RWMutex (for the mmap), while write transactions grab a write mutex (distinct from the mmap mutex, which needs to be obtained to update the memory map to commit). The read transaction then operates on a snapshot of the DB at that time. Readers also don't block other readers. If a write transaction is created, then readers can still be created (they copy the B+Tre root with the existing pointers). All other write transactions are blocked until that write finishes.

An example trace through bolt DB transactions:

For read transactions, a RWMutex is held for the mmap

Root bucket gets copied, which effectively copies the B+Tree root along w/ the other pointers (for both)

For write transactions, the write lock is obtained

So with the above flow, a writer can start (grab the write block) and then get a snapshot copy, while several other readers also start and obtain a snapshot copy.

. For sqlite, it does sound a bit as if they can run in parallel, with the read txes having their snapshot.

So this is basically the goal of the WAL: readers can look at the WAL, then start a transaction and ensure they apply the changes in the WAL. Writers just append to the end of the WAL. There's an index that has an exclusive lock on it, which needs to be consulted so both sides know what the latest state of the WAL is.

My interpretation of the "database connection" terminology in the sqlite docs is the object return from the sqlite.Open call in C. sqlite has a few diff flags that can be passed to open. Our library defaults to SQLITE_OPEN_FULLMUTEX.

I think this is the clearest set of docs from sqlite about their isolation model: https://www.sqlite.org/isolation.html. The WAL docs explain the read/write interaction (section 2.2): https://www.sqlite.org/wal.html.

If the same database is being read and written using two different database connections (two different sqlite3 objects returned by separate calls to sqlite3_open()) and the two database connections do not have a shared cache, then the reader is only able to see complete committed transactions from the writer. Partial changes by the writer that have not been committed are invisible to the reader. This is true regardless of whether the two database connections are in the same thread, in different threads of the same process, or in different processes. This is the usual and expected behavior for SQL database systems.

Thanks for all these pointers. It indeed seems that the original concern that I had is ungrounded.

joostjager · 2023-01-17T09:29:28Z

kvdb/sqlite/db.go

+
+// sqliteDB contains two sqlite connections, one is optimised for reads as it
+// uses the default BEGIN DEFERRED directive for starting a db transaction and
+// the other is optimised for writes as it uses the BEGIN IMMEDIATE directive


Perhaps explain why BEGIN IMMEDIATE optimizes for writes?

I saw #7252 (comment) and that DEFERRED creates busy errors. It seems that IMMEDIATE makes this less likely to happen, but is it true that it still can happen when a write tx runs for longer than the busy timeout while another write is waiting for that duration?

So immediate will immediately grab the exclusive lock with means no other transactions can start. Without this, it's possible that with deferred, a transaction starts as a read, then needs to upgrade to a write and gets locked out, which triggers the busy timeout. With this we're trying to mark all write transactions as such early, so it doesn't need to upgrade at all.

An EXCLUSIVE lock is needed in order to write to the database file. Only one EXCLUSIVE lock is allowed on the file and no other locks of any kind are allowed to coexist with an EXCLUSIVE lock. In order to maximize concurrency, SQLite works to minimize the amount of time that EXCLUSIVE locks are held.

It's still possible for this to return sqlite_busy though if another write is started on another db connection:

IMMEDIATE cause the database connection to start a new write immediately, without waiting for a write statement. The BEGIN IMMEDIATE might fail with SQLITE_BUSY if another write transaction is already active on another database connection.

As is, we have a distinct DB connection for writes, so this doesn't appear to apply.

If it's possible instead have the library let us set the txn type on the single connection level, then we'd just switch over to that, and we wouldn't need to worry about having multiple db connections.

(also note that this is part of the other PR, we need to merge that and tag in order to move forward)

it's possible that with deferred, a transaction starts as a read, then needs to upgrade to a write and gets locked out, which triggers the busy timeout.

Thinking about this more - if there is one dedicated connection for reads and one for writes ("As is, we have a distinct DB connection for writes, so this doesn't appear to apply."), how can a write get locked out really? That one write connection can only handle one write tx, so there can never be another one? I am probably still missing a part of the model, perhaps related to the threading model that sqlite runs in?

(also note that this is part of the other PR, we need to merge that and tag in order to move forward)

I thought the code is introduced in this PR, so I comment on it here?

Btw, by my original comment starting with 'perhaps explain', I meant capturing this rationale in the code comment.

Thinking about this more - if there is one dedicated connection for reads and one for writes ("As is, we have a distinct DB connection for writes, so this doesn't appear to apply."), how can a write get locked out really?

The reason we added the two connections, was so we can use BEGIN IMMEDIATE via a round about way. We're trying to allow the following situation to be possible:

If X starts a transaction that will initially only read but X knows it will eventually want to write and does not want to be troubled with possible SQLITE_BUSY_SNAPSHOT errors that arise because another connection jumped ahead of it in line, then X can issue BEGIN IMMEDIATE to start its transaction instead of just an ordinary BEGIN. The BEGIN IMMEDIATE command goes ahead and starts a write transaction, and thus blocks all other writers. If the BEGIN IMMEDIATE operation succeeds, then no subsequent operations in that transaction will ever fail with an SQLITE_BUSY error.

A write can get locked out simply because another writer is active. A reader can get kicked out (SQLITE_BUSY) if it starts a read, then attempts an update while another write transaction exists concurrently that's also updating.

That one write connection can only handle one write tx, so there can never be another one?

Yeah there can only be a single write transaction active for a connection, where "connection" is defined as the object returned from sqlite.Open. While that is active, the others will block iiuc.

Also I got confused myself w.r.t which PR I was commenting on 😅

Confusion level isn't going down with these threads, but I'll try to not miss anything. Maybe a single PR just for review and then splitting up right before merge to get the tagging right wouldn't have been a bad option either.

The reason we added the two connections, was so we can use BEGIN IMMEDIATE via a round about way.

I found this, but not sure if it is a good idea. Maybe it can realize tx-level IMMEDIATE: mattn/go-sqlite3#400 (comment)

It's still possible for this to return sqlite_busy though if another write is started on another db connection
As is, we have a distinct DB connection for writes, so this doesn't appear to apply.

That single write connection is used by all kinds of goroutines in lnd. So I do think this still applies. I ran a quick test with multiple goroutines writing to the same sqlite connection, and if one of those is slow, the others will happily return sqlite_busy.

Also as you mentioned in #7252 (comment), it isn't really a single connection, because database/sql is still making a pool internally.

review and then splitting up right before merge to get the tagging right wouldn't have been a bad option either.

the other PR does contain all the commits. So could just comment on there?

Not sure if I am way off here, but I have opened an issue on the modernc.org/sqlite repo to see if they cant set/not set the begin directives based on the passed in driver.TxOptions: https://gitlab.com/cznic/sqlite/-/issues/127

If im on the right track there then that would remove the need for the two conns on our end

the other PR does contain all the commits. So could just comment on there?

Yes, probably should have done that from the start. But then I didn't understand how they were related. And now, this is where we are 😄

I think that issue that you opened is a really smart solution to the problem.

I think that issue that you opened is a really smart solution to the problem.

awesome! I have opened a PR on their repo. Hopefully we can have fast turn around on that side. Does look like they create a new tag after almost every PR they merge.

The MR on the gitlab sqlite repo was merged and a new tag was released 🎉 so I have removed this two-connection work around now

Roasbeef · 2023-01-18T02:00:03Z

So re the thread in the other PR, I think we should just go back to synchronous=full as it's the safest default option. With this, the docs advertise that even in the face of sudden power failure, then DB contents remain consistent, and transactions won't be rolled back. We can either just not set the value (which seems to be the default), or make it explicit.

ellemouton · 2023-01-18T12:04:06Z

I think we should just go back to synchronous=full as it's the safest default option.

Even with WAL mode? WAL mode is safe from corruption with synchronous=NORMAL.

joostjager · 2023-01-18T12:46:59Z

Yes, safe from corruption, but it may still lose data in case of power failure. So not suitable for channel state data.

Roasbeef · 2023-01-18T19:49:19Z

Yeah my understanding matches @joostjager's above. Those that want to make a diff tradeoff can set the sync normal field on start up.

Bump the go version to go1.18 so that we can get rid of the '// +build' comments in the upcoming commits where some of the '//go:build' comments will become too complex to write as '// +build' comments.

In this commit, a mutex is added to guard access to the global dbConns variable in the kvdb/postgres package.

Due to the update of the kvdb package to go1.18, the `// +build` directives are no longer required. They are removed in this commit in order to simplify upcoming commits. Along the way, a few typos are fixed and an unused struct is removed.

In this commit, changes are made to the `kvdb/postgres` package so that all all the non-postgres-specific code is generalised to be applicable for all sql code. A follow up commit will move all the general sql code into its own package.

In this commit, all the sql, non-postgres-specific, code is moved out of the postgres package and into a new sqlbase package. This will make it more easily reusable for future sql integrations.

ellemouton · 2023-01-23T07:23:24Z

Since the two-connection work around has now been removed here - I think this PR is ok to merge since we can continue playing with the various pragma options in the other PR. This one only hardcodes foreign_keys=on, journal_mode=wal and busy_timeout=<passed-in-timeout which are all options that I think we all agree that we want to keep.

Roasbeef · 2023-01-24T01:25:56Z

Ok, based on the above merging this (then will tag), since all the param tuning is now in the other PR and we use a single connection here.

ellemouton mentioned this pull request Dec 14, 2022

multi: add sqlite backend option #7252

Merged

Roasbeef mentioned this pull request Dec 14, 2022

sqlite db backend #6570

Closed

11 tasks

Roasbeef linked an issue Dec 14, 2022 that may be closed by this pull request

add SQLite3 backend option #6176

Closed

Roasbeef requested changes Dec 14, 2022

View reviewed changes

Roasbeef reviewed Dec 15, 2022

View reviewed changes

kvdb/sqlite/db.go Show resolved Hide resolved

ellemouton force-pushed the sqlite-pt1 branch from c503d8b to c01520c Compare December 16, 2022 10:56

ellemouton commented Dec 16, 2022

View reviewed changes

ellemouton requested a review from Roasbeef December 16, 2022 12:13

ellemouton force-pushed the sqlite-pt1 branch 3 times, most recently from f6d3e6b to ce63f61 Compare December 22, 2022 07:40

saubyk requested a review from guggero January 3, 2023 18:12

guggero approved these changes Jan 4, 2023

View reviewed changes

Roasbeef requested changes Jan 5, 2023

View reviewed changes

ellemouton force-pushed the sqlite-pt1 branch 2 times, most recently from fc8ce96 to 2884ded Compare January 5, 2023 11:15

ellemouton requested a review from Roasbeef January 5, 2023 15:55

Roasbeef approved these changes Jan 6, 2023

View reviewed changes

ellemouton force-pushed the sqlite-pt1 branch from 2884ded to 3e91909 Compare January 6, 2023 10:13

Roasbeef approved these changes Jan 12, 2023

View reviewed changes

kvdb/sqlite/db.go Outdated Show resolved Hide resolved

ellemouton force-pushed the sqlite-pt1 branch 2 times, most recently from 4a4cb6c to 9c4a3ba Compare January 13, 2023 09:08

ellemouton force-pushed the sqlite-pt1 branch from 9c4a3ba to 24c8a68 Compare January 16, 2023 08:47

joostjager reviewed Jan 16, 2023

View reviewed changes

joostjager reviewed Jan 17, 2023

View reviewed changes

ellemouton force-pushed the sqlite-pt1 branch 3 times, most recently from e6f9eab to fb49b63 Compare January 17, 2023 12:55

positiveblue mentioned this pull request Jan 19, 2023

sqldb: new package supporting sql backends #7343

Merged

ellemouton force-pushed the sqlite-pt1 branch from fb49b63 to 5d6c888 Compare January 20, 2023 06:50

ellemouton and others added 8 commits January 23, 2023 09:05

kvdb: bump go version to 1.18

72dbc3d

Bump the go version to go1.18 so that we can get rid of the '// +build' comments in the upcoming commits where some of the '//go:build' comments will become too complex to write as '// +build' comments.

kvdb/postgres: make global dbConns thread safe

c6abf58

In this commit, a mutex is added to guard access to the global dbConns variable in the kvdb/postgres package.

kvdb+refactor: move all general sqlite code to seprate dir

170160f

In this commit, all the sql, non-postgres-specific, code is moved out of the postgres package and into a new sqlbase package. This will make it more easily reusable for future sql integrations.

kvdb: unify how the db backend for tests is set

3d91bb6

kvdb: add sqlite

74b9c9c

docs: update release notes

43bc273

ellemouton force-pushed the sqlite-pt1 branch from 5d6c888 to 43bc273 Compare January 23, 2023 07:20

Roasbeef merged commit 266cd97 into lightningnetwork:master Jan 24, 2023

ellemouton deleted the sqlite-pt1 branch January 24, 2023 04:50

guggero mentioned this pull request Jan 26, 2023

[bug]: run unit tests (btcd unit-cover) failure due to build tag mismatch #7362

Closed

		@@ -0,0 +1,21 @@
		//go:build !kvdb_sqlite \|\| (windows && (arm \|\| 386)) \|\| (linux && (ppc64 \|\| mips \|\| mipsle \|\| mips64))

kvdb: add sqlite #7251

kvdb: add sqlite #7251

Conversation

ellemouton commented Dec 14, 2022 • edited Loading

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Dec 15, 2022

ellemouton left a comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Dec 28, 2022

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Jan 5, 2023

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Jan 6, 2023

Roasbeef left a comment

Choose a reason for hiding this comment

guggero commented Jan 6, 2023

ellemouton commented Jan 6, 2023

guggero commented Jan 6, 2023

ellemouton commented Jan 6, 2023 • edited Loading

Roasbeef left a comment

Choose a reason for hiding this comment

joostjager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

joostjager Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef commented Jan 18, 2023

ellemouton commented Jan 18, 2023

joostjager commented Jan 18, 2023

Roasbeef commented Jan 18, 2023

ellemouton commented Jan 23, 2023

Roasbeef commented Jan 24, 2023

ellemouton commented Dec 14, 2022 •

edited

Loading

ellemouton commented Jan 6, 2023 •

edited

Loading

Roasbeef Jan 18, 2023 •

edited

Loading

Roasbeef Jan 17, 2023 •

edited

Loading

joostjager Jan 17, 2023 •

edited

Loading

joostjager Jan 18, 2023 •

edited

Loading