-
Notifications
You must be signed in to change notification settings - Fork 71
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
Be consistent in DDL for integer primary keys #517
Comments
The description of the semantics of |
After discussing this with the SWG today, it does not appear that we need the |
I confirmed that integer primary key values are generated automatically by SQLite. Our table definition SQL is technically normative (except for user-defined tables, of course) so I added a note stating that minor modifications to the SQL are permitted. https://github.com/opengeospatial/geopackage/pull/521/files#diff-e76f7b8609c8bf2c4d6c52402c6a30a5R5 |
There is still some outside discussion on this topic. The main point of concern is in RTE relationships. Consider this scenario:
Since ROWIDs will be reused in this circumstance, you will now have RTE rows pointing to the wrong thing. What do you think? |
I agree that is an issue with not only RTE, but any potential extension someone or some software is not aware of. The user table ids are basically the only guaranteed row identifying information, used throughout the spec and extensions. Related tables do not force foreign key constraints. Even if they did, sqlite defaults do not enforce them unless manually enabled. Intelligent database design prevents unique generated ids from reuse so that a record id is guaranteed to only represent that record. Any new record should be uniquely identifiable without reusing an id. Other than performance, most arguments against using autoincrement in a database encourage the use of serial ids instead. But by omitting autoincrement, GeoPackage is instead encouraging non serial auto generated ids. Both autoincremented and generated serial ids maintain database integrity, but auto generated max+1 ids do not. All implementations should be able to function on the base GeoPackage w/o the risking data integrity. There are so many ways to corrupt data as it is, we should do the best to prevent it when possible. We need to think more broadly about GeoPackages shared between different types of users, software, and usages.
I see this as a potential issue for data integrity, especially for shared GeoPackages. There is also the case of external id tracking for export scenarios. A record is only externally uniquely identifiable and traceable within a versioned GeoPackage by its' table and id. The estimated 10-12% performance gain from omitting autoincrement may not offset the potential downsides with database integrity and traceability. Point number 4 should be enough to justify the inclusion: https://www.sqlite.org/autoinc.html
I would at least specify autoincrements in the spec and strongly encourage them on generated ids. By default, implementations will still function on tables that do omit them. |
One possibility is reversing the guidance - putting |
Are you are doing batch inserts? Committing in chunks within a transaction instead of each row? |
I can't speak for others but I have been. Batches of 1000 seem to work best. |
@jyutzler I would support including the AUTOINCREMENT in the reference normative SQL, but not making it a hard requirement, so that for those for whom performance and storage size is paramount, and have a clear understanding of the implications, it can be omitted. |
Although I agree 100% with #517 (comment), I think that we should support this kind of proposals with an experiment with synthetic or real data. For example, we can create at least two geopackages with the same feature but only one with the IMHO, when a GeoPackage decision design is conditioned by a specific behaviour of SQLite and there are concerns related to size/speed/consistency, we should have reference experiments that support the decision. I am worried about the use of magic numbers for taking decisions without experimental support (e.g. the classic 80% of data is geographic). |
While I agree that it would be nice to have performance testing for this case, I am not aware of anyone who is available to do this work. |
While these numbers are not GeoPackage specific, here is a link to an experiment that someone ran for the question of AUTOINCREMENT vs without. His conclusion was that it was 8-12% slower, but let us look at the numbers. On an HTC One M8 phone from 2014, it took ~3.5 seconds to insert 10000 items one at a time with AUTOINCREMENT on, and ~3.1 seconds without. Inserting as one batch took ~680 milliseconds with AUTOINCREMENT and ~600 milliseconds without. |
8-12% slower on a process that takes a long time is quite significant... So I would stronly support @jyutzler 's suggestion:
|
I would also be curious to see figures for size. Are you able to run your experiment, inserting billions of records, with both AUTOINCREMENT and without and share the results? |
@danielbarela This is not our current focus right now, but I would be happy to when we get around to it. |
An HTC One M8 phone from 2014 isn't what you'll be inserting billions of records into GeoPackages on though? Performance comparisons don't scale across hardware very well — the completely different cpu architectures/cpu caches/memory/storage/OS/etc makes it virtually a random number. |
This S/O post here shines more light onto this... https://stackoverflow.com/questions/52700893/sqlite-best-practices-about-using-autoincrement (It also points to that post with the 8-12% figure -- https://stackoverflow.com/questions/45384919/what-are-the-overheads-of-using-autoincrement-for-sqlite-on-android) According to this, the only time that the 'autoincrement' behavior looping back to re-use IDs occur is when the ROWIDs have gone over 9,223,372,036,854,775,807. At that point, the difference is that adding a record will FAIL (getting a SQLITE_FULL). A VACUUM will change all ROWIDs regardless of AUTOINCREMENT being present or not, and should adjust all reference to the primary keys... Unless I am missunderstanding this explanation, I do not believe adding AUTOINCREMENT helps with any of the scenarios that were mentioned here as a justification for using it. So if I understand this right, AUTOINCREMENT adds zero benefit at all, and whichever performance, memory or disk usage, wheteher it's 20, 10, 5 or 1%, is a complete waste. As https://www.sqlite.org/autoinc.html says very clearly:
I believe this is a bit of an understatement, and in practice it really is never needed. EDIT: I might have misunderstood that... The posts talks about max(rowid), so if you delete the biggest rowID row it might be re-used on the next add. So if a client deletes the largest rowid row and some referencing table does not get updated, it might go from pointing to a missing row to pointing to a wrong row when a record is added later (without autoincrement). |
This is only in the case of where a INTEGER PRIMARY KEY is not declared which does not apply here. See #516 for a little more insight on this. And yes, I would say there is a clear benefit as specified in the documentation you referenced.
Are we really expecting people to insert over nine quintillion features into one GeoPackage table? The problems with memory, speed, and handling that kind of database should be interesting. |
18 quintillion max rows per table in theory, but 140 TB file limit so about 10 trillion max rows.
|
Well, we're going to discuss (again!) during tomorrow's SWG meeting. I'm not sure this is the best use of SWG resources, but the squeaky wheel gets the oil. |
I edited and ran a quick personal laptop performance test on our Java library.
Omitting auto increment is faster as expected, but that rate varied (1.5% - 17.15% in just a few runs). So not large enough of a test case or representative of most use cases, but gives an idea that the performance hit can be negligible up to about what we thought. Edit: |
I created a simple script that creates this table:
And populates with this sentence:
In M transactions of N INSERTS each using the
By removing the layers that add ORMLite, Xerial JDBC and Java JNI, this result supports the finding of @bosborn and shows that there is a performance problem when bulk loading and Recommending the use of |
#515 currently proposes modifying the DDL for the Metadata Extension to be more consistent. I guess we should also sweep all of our DDL (Annex C) for consistency in defining primary keys. It looks like we need make this fix in multiple places. Are we in agreement that the correct way to define an integer autoincrement primary key is like this:
And that everything else is superfluous?
The text was updated successfully, but these errors were encountered: