Skip to content
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

[SQLite] Transaction field handling: intial implementation #11609

Merged
merged 12 commits into from
Jan 10, 2025

Conversation

elpaso
Copy link
Collaborator

@elpaso elpaso commented Jan 8, 2025

Fix out of sync (not restored) fields after a ROLLBACK.

See: qgis/QGIS#59797 (comment)

This is an SQLite only fix, if the approach sounds reasonable we should consider supporting more drivers.

Funded by: QGIS.org

@elpaso elpaso added the bug label Jan 8, 2025
@elpaso elpaso force-pushed the gpkg-transaction-fields-rollback branch 5 times, most recently from 23c1cb9 to fc50c12 Compare January 8, 2025 14:00
Fix out of sync (not restored) fields after a ROLLBACK.
@elpaso elpaso force-pushed the gpkg-transaction-fields-rollback branch from fc50c12 to add4a0d Compare January 8, 2025 14:16
ogr/ogr_feature.h Outdated Show resolved Hide resolved
ogr/ogrgeomfielddefn.cpp Show resolved Hide resolved
ogr/ogrsf_frmts/sqlite/ogrsqlitelayer.cpp Outdated Show resolved Hide resolved
@@ -1710,6 +1717,13 @@ OGRSQLiteTableLayer::CreateGeomField(const OGRGeomFieldDefn *poGeomFieldIn,
}
}

if (m_poDS->IsInTransaction())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bit of dissymetry with the call to AddFieldDefn() in CreateField() having been done before the "if IsInTransation())" logic and here where AddGeomFieldDefn() is done after the IsInTransaction() test (note that if you symmetrize things you need to add/remove a -1 index to GetFieldCount/GetGeomFieldCount)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason for the dissimetry is because poGeomField is (already was) a unique ptr whic would be null after the move. I will add a comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment added

@elpaso elpaso force-pushed the gpkg-transaction-fields-rollback branch from eb87b6e to 90f441a Compare January 9, 2025 09:38
@rouault
Copy link
Member

rouault commented Jan 9, 2025

In the documentation of RollbackTransaction() (in ogr/ogrsf_frmts/ogrsf_frmts.dox), it should probably be mentioned that OGRFeature* instances acquired or created between the StartTransaction() and RollbackTransaction() should be destroyed before RollbackTransaction() if the field structure has been modified during the transaction.
In particular, the following is invalid:

lyr->StartTransaction();
lyr->DeleteField(...);
f = new OGRFeature(lyr->GetLayerDefn());
lyr->RollbackTransaction();
// f is in a inconsistent state at that point, given its array of fields doesn't match the updated layer definition, and thus it cannot even be safely deleted !

this must be corrected as:

lyr->StartTransaction();
lyr->DeleteField(...);
f = new OGRFeature(lyr->GetLayerDefn());
delete f;
lyr->RollbackTransaction();

ogr/ogrfielddefn.cpp Outdated Show resolved Hide resolved
ogr/ogrfielddefn.cpp Outdated Show resolved Hide resolved
{
ADD,
ALTER,
DELETE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears there must be a #define DELETE somewhere in a Windows system header... So maybe switch to Add/Alter/Delete or add a _FIELD suffix ?

@elpaso elpaso marked this pull request as ready for review January 9, 2025 16:08
@coveralls
Copy link
Collaborator

coveralls commented Jan 9, 2025

Coverage Status

coverage: 70.084% (+0.009%) from 70.075%
when pulling 5ab8543 on elpaso:gpkg-transaction-fields-rollback
into eb84213 on OSGeo:master.

@rouault
Copy link
Member

rouault commented Jan 9, 2025

macos_build CI failure unrelated and will be addressed per #11621

@rouault rouault merged commit 4cc3cd9 into OSGeo:master Jan 10, 2025
37 of 38 checks passed
@rouault rouault added this to the 3.11.0 milestone Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants