init: Use VACUUM for large, modified SQLite files only

[wenzeslaus]

For SQLite database, VACUUM is used when the session is ending. There is a cost in running the task even when the database is small and no vacuuming is needed. This change adds a check for the size of the database and ignores databases under 100MB.

The message is now only verbose and printed when the task is running.

Unlike the original implementation, the vacuum task is now synchronous so that there is no running GRASS process after the session has ended.

The original implementation comes from Trac ticket 3697 (https://trac.osgeo.org/grass/ticket/3697)
and is best visible in a bulk backport commit 73736 (f790d68).

[wenzeslaus]

The original code runs relatively quickly, because it just starts the vacuum subprocess which then runs on its own. This dangling subprocess may cause unexpected behavior. The new code waits for the process to finish, but vacuums only for bigger database files.

Bug fix

The issues:

1. The vacuum subprocess in the original code prevents new process from using the mapset (the SQLite database is locked assuming that the locking is even enabled; worse things may happen if not) while the main process finished and the mapset is already unlocked.
2. In scenarios with shared or acquired resources (remote servers, HPC, cloud), running the vacuum subprocess after the main process already finished may cause unexpected behavior such as termination of the vacuum subprocess or violation of resource usage policy.

The fix:

The new code waits for the vacuum process to finish.

Performance

When the database is small, the code now runs much faster. When the database is not small, the code is slower than before and the time depends on the size of the database. The old code has a constant speed regardless of the size of the database.

Tiny database

code	time	note
original	0.2003 sec +- 0.67%	perf with --post "sleep 1" to give SQLite time
original	0.6240 sec +- 4.43%	with additional 230+ sec of busy SQLite messages
new	0.0823 sec +- 0.37%	no check, database small
new	0.5290 sec +- 1.46%	code modified to check always

Repeats: 100

SQLite database size: 25MiB (27MB)

Tested command:

grass ~/grassdata/nc_spm_08_grass7/PERMANENT/ --exec g.mapset -p

perf command:

perf stat -r 100 ...

Smallest not small database

code	time	note
original	0.21 sec +- 3.64%	perf with --post "sleep 4" to give SQLite time
new	4.10 sec +- 6.92%	using the default threshold of 300 MiB

Repeats: 10

SQLite database size: 378MiB (397MB)

[wenzeslaus]

Or a simpler alternative: #2449 removes the automatic vacuum completely.

[metzm]

Suggested change

	process = gs.start_command("db.execute", sql="VACUUM")
	gs.verbose(_("Cleaning up default SQLite database..."))
	process.wait()
	gs.verbose(_("Cleaning up default SQLite database..."))
	process = gs.run_command("db.execute", sql="VACUUM")

should do the same?

[wenzeslaus]

Almost the same. The message job runs in parallel. Because the message is a subprocess, it takes some tiny but measurable time to execute, so I tried to shave off some time by showing it in parallel while the vacuum is running. I tried to clarify that now in the comment.

With the greater threshold for vacuum I introduced later, the practical difference is likely negligible, but doesn't do any harm. It breaks the promise of no multiple processes, but not in a data processing context, so likely permissible. Anyway, I can remove it if it seems too strange.

[metzm]

Looks good to me. I prefer this approach over #2449 because the initial motivation for this automatic vacuum was that users may not be aware that they can save potentially GBs of disk space with db.execute sql=VACUUM. Back then we found it to be quite convenient for users that automatic cleaning is performed at the end of a session, even if it might take some time.

[metzm]

I understand you like this new syntax (*, keyword), but is it really needed here? Golden rule: Keep code sufficiently simple.

[wenzeslaus]

I don't think it is necessary in this particular function or PR, but it contributes to both documentation and enforcement of correct use and it seems as something which is becoming standard Python now (also judging from introduction of the / to mark positional-only).

Python is using it (at least os.path in 3.10: os.path.realpath(path, *, strict=False)) and scipy does too: scipy.stats.sampling.DiscreteAliasUrn(dist, *, domain=None, urn_factor=1, random_state=None).

As documentation, it is not needed for running the code, but it seems to be increasingly used for documenting Python.

As an enforcement of using start_time as only a keyword argument, * is the minimal code needed to specify that rule.

* says "I may add more parameters in the future and I don't know how I will order them." This is quite fitting here and many other places in the Python API.

[wenzeslaus]

Thanks for the explanation of the original motivation.

...we found it to be quite convenient for users that automatic cleaning is performed at the end of a session, even if it might take some time.

The issue is that it takes significant time (0.5s for 25MB, 4s for 380MB) to complete. With the original code, there was no visible slowdown because the session ended without waiting for the vacuum process, so with interactive session, one did not feel the cost of the vacuum back then.

With the PR, in extreme example, you pay 4s or more for deleting one row from the database. Disk space is now usually not that costly. For example, Git makes the assumption that you have plenty of disk space. We may work with larger data, but perhaps for us, too, time is more valuable than disk space (this would be different from raster compression where we save I/O time and disk space, right?).

...the initial motivation for this automatic vacuum was that users may not be aware that they can save potentially GBs of disk space with db.execute sql=VACUUM.

Maybe, the same can be addressed by a new module which does db.execute sql=VACUUM and/or turns on SQLite auto-vacuum mentioned by @tmszi in #2449. It somewhat promotes the issue and its solution, but it is opt-in (as with the default SQLite behavior) and it is managed separately (as with PostgreSQL).

[metzm]

I approve the change that running time for cleaning up the db is now apparent to the user (no longer running in the background).

It is true that disk space is not that costly, but how many users are able and allowed to change the drive in their laptop for a larger one? IMHO, disk space is still an issue, as is time, and this PR is a good compromise.

[wenzeslaus]

OSGeo4W jobs are failing now in general, but before OSGeo4W check passed.

[wenzeslaus]

I'm ready to merge this. It improves the situation significantly when you don't edit the database. (Any edit, e.g., adding a vector map with a table, triggers the vacuum when the file is larger than MEMORYMB.)