diff --git a/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp b/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp index c9d62d602e07..2aa330ad5101 100644 --- a/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp +++ b/ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp @@ -9721,7 +9721,8 @@ OGRErr GDALGeoPackageDataset::RollbackTransaction() } } - OGRErr eErr = OGRSQLiteBaseDataSource::RollbackTransaction(); + const OGRErr eErr = OGRSQLiteBaseDataSource::RollbackTransaction(); + #ifdef ENABLE_GPKG_OGR_CONTENTS if (!abAddTriggers.empty()) { diff --git a/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp b/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp index b4485c61a137..1407058c3b79 100644 --- a/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp +++ b/ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp @@ -1849,10 +1849,12 @@ OGRErr OGRGeoPackageTableLayer::CreateField(const OGRFieldDefn *poField, { m_apoFieldDefnChanges.emplace_back( std::make_unique(oFieldDefn), - m_poFeatureDefn->GetFieldCount() - 1, FieldChangeType::ADD_FIELD); + m_poFeatureDefn->GetFieldCount() - 1, FieldChangeType::ADD_FIELD, + /* bGenerated */ false); } m_abGeneratedColumns.resize(m_poFeatureDefn->GetFieldCount()); + m_abGeneratedColumns.back() = false; // explicit is better than implicit if (m_pszFidColumn != nullptr && EQUAL(oFieldDefn.GetNameRef(), m_pszFidColumn)) @@ -2029,7 +2031,8 @@ OGRGeoPackageTableLayer::CreateGeomField(const OGRGeomFieldDefn *poGeomFieldIn, { m_apoGeomFieldDefnChanges.emplace_back( std::make_unique(oGeomField), - m_poFeatureDefn->GetGeomFieldCount(), FieldChangeType::ADD_FIELD); + m_poFeatureDefn->GetGeomFieldCount(), FieldChangeType::ADD_FIELD, + /* bGenerated */ false); } whileUnsealing(m_poFeatureDefn)->AddGeomFieldDefn(&oGeomField); @@ -3811,6 +3814,43 @@ bool OGRGeoPackageTableLayer::DoJobAtTransactionRollback() SyncToDisk(); m_bDeferredSpatialIndexCreation = bDeferredSpatialIndexCreationBackup; } + + // If we are restoring any deleted field or removing any added one we have to + // rebuild the array of generated fields + for (int i = static_cast(m_apoFieldDefnChanges.size()) - 1; i >= 0; + i--) + { + auto &oFieldChange = m_apoFieldDefnChanges[i]; + switch (oFieldChange.eChangeType) + { + case FieldChangeType::ADD_FIELD: + { + CPLAssert(oFieldChange.iField < + static_cast(m_abGeneratedColumns.size())); + m_abGeneratedColumns.erase(m_abGeneratedColumns.begin() + + oFieldChange.iField); + break; + }; + case FieldChangeType::DELETE_FIELD: + { + CPLAssert(oFieldChange.iField <= + static_cast(m_abGeneratedColumns.size())); + m_abGeneratedColumns.insert(m_abGeneratedColumns.begin() + + oFieldChange.iField, + oFieldChange.bGenerated); + break; + }; + case FieldChangeType::ALTER_FIELD: + { + CPLAssert(oFieldChange.iField < + static_cast(m_abGeneratedColumns.size())); + m_abGeneratedColumns[oFieldChange.iField] = + oFieldChange.bGenerated; + break; + }; + } + } + ResetReading(); return true; } @@ -6590,7 +6630,8 @@ OGRErr OGRGeoPackageTableLayer::DeleteField(int iFieldToDelete) { m_apoFieldDefnChanges.emplace_back( std::move(poFieldDefn), iFieldToDelete, - FieldChangeType::DELETE_FIELD); + FieldChangeType::DELETE_FIELD, + m_abGeneratedColumns[iFieldToDelete]); } else { @@ -6606,6 +6647,8 @@ OGRErr OGRGeoPackageTableLayer::DeleteField(int iFieldToDelete) if (eErr == OGRERR_NONE) { #if SQLITE_VERSION_NUMBER >= 3035005L + CPLAssert(iFieldToDelete < + static_cast(m_abGeneratedColumns.size())); m_abGeneratedColumns.erase(m_abGeneratedColumns.begin() + iFieldToDelete); #else @@ -7098,7 +7141,8 @@ OGRErr OGRGeoPackageTableLayer::AlterFieldDefn(int iFieldToAlter, { m_apoFieldDefnChanges.emplace_back( std::make_unique(poFieldDefnToAlter), - iFieldToAlter, FieldChangeType::ALTER_FIELD); + iFieldToAlter, FieldChangeType::ALTER_FIELD, + m_abGeneratedColumns[iFieldToAlter]); } auto oTemporaryUnsealer(poFieldDefnToAlter->GetTemporaryUnsealer()); diff --git a/ogr/ogrsf_frmts/ogrsf_frmts.h b/ogr/ogrsf_frmts/ogrsf_frmts.h index c9cefb6955f3..f0f75b150bb4 100644 --- a/ogr/ogrsf_frmts/ogrsf_frmts.h +++ b/ogr/ogrsf_frmts/ogrsf_frmts.h @@ -285,8 +285,8 @@ class CPL_DLL OGRLayer : public GDALMajorObject //! @cond Doxygen_Suppress // Keep field definitions in sync with transactions - void PrepareStartTransaction(); - void FinishRollbackTransaction(); + virtual void PrepareStartTransaction(); + virtual void FinishRollbackTransaction(); //! @endcond virtual const char *GetFIDColumn(); @@ -414,15 +414,17 @@ class CPL_DLL OGRLayer : public GDALMajorObject { FieldDefnChange(std::unique_ptr &&poFieldDefnIn, int iFieldIn, - FieldChangeType eChangeTypeIn) + FieldChangeType eChangeTypeIn, bool bGeneratedIn) : poFieldDefn(std::move(poFieldDefnIn)), iField(iFieldIn), - eChangeType(eChangeTypeIn) + eChangeType(eChangeTypeIn), bGenerated(bGeneratedIn) { } std::unique_ptr poFieldDefn; int iField; FieldChangeType eChangeType; + // Used by drivers (GPKG) to track generated fields + bool bGenerated; }; std::vector> m_apoFieldDefnChanges{}; diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp index b133594200d2..f3cf706cecc8 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitedatasource.cpp @@ -4023,6 +4023,12 @@ OGRErr OGRSQLiteBaseDataSource::StartTransaction(CPL_UNUSED int bForce) return OGRERR_FAILURE; } + for (int i = 0; i < GetLayerCount(); i++) + { + OGRLayer *poLayer = GetLayer(i); + poLayer->PrepareStartTransaction(); + } + OGRErr eErr = SoftStartTransaction(); if (eErr != OGRERR_NONE) return eErr; @@ -4041,8 +4047,6 @@ OGRErr OGRSQLiteDataSource::StartTransaction(int bForce) cpl::down_cast(poLayer.get()); poTableLayer->RunDeferredCreationIfNecessary(); } - - poLayer->PrepareStartTransaction(); } return OGRSQLiteBaseDataSource::StartTransaction(bForce); diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp index 80a2fe120349..0579138e371d 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitetablelayer.cpp @@ -1617,7 +1617,8 @@ OGRErr OGRSQLiteTableLayer::CreateField(const OGRFieldDefn *poFieldIn, { m_apoFieldDefnChanges.emplace_back( std::make_unique(oField), - m_poFeatureDefn->GetFieldCount() - 1, FieldChangeType::ADD_FIELD); + m_poFeatureDefn->GetFieldCount() - 1, FieldChangeType::ADD_FIELD, + /* bGenerated */ false); } if (m_pszFIDColumn != nullptr && EQUAL(oField.GetNameRef(), m_pszFIDColumn)) @@ -1723,7 +1724,8 @@ OGRSQLiteTableLayer::CreateGeomField(const OGRGeomFieldDefn *poGeomFieldIn, { m_apoGeomFieldDefnChanges.emplace_back( std::make_unique(*poGeomField), - m_poFeatureDefn->GetGeomFieldCount(), FieldChangeType::ADD_FIELD); + m_poFeatureDefn->GetGeomFieldCount(), FieldChangeType::ADD_FIELD, + /* bGenerated */ false); } m_poFeatureDefn->AddGeomFieldDefn(std::move(poGeomField)); @@ -2177,7 +2179,7 @@ OGRErr OGRSQLiteTableLayer::DeleteField(int iFieldToDelete) { m_apoFieldDefnChanges.emplace_back( std::move(poFieldDefn), iFieldToDelete, - FieldChangeType::DELETE_FIELD); + FieldChangeType::DELETE_FIELD, false); } else { @@ -2414,7 +2416,7 @@ OGRErr OGRSQLiteTableLayer::AlterFieldDefn(int iFieldToAlter, { m_apoFieldDefnChanges.emplace_back( std::make_unique(poFieldDefn), iFieldToAlter, - FieldChangeType::ALTER_FIELD); + FieldChangeType::ALTER_FIELD, /* bGenerated */ false); } if (nActualFlags & ALTER_TYPE_FLAG)