Merge pull request OSGeo#11680 from elpaso/fix-rollback-crash

[GPKG] Fix crash in GPKG rollback after PR OSGeo#11609
innFactory · Jan 17, 2025 · f41af17 · f41af17
2 parents eda0808 + 8e4a70b
commit f41af17
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 15 deletions.
diff --git 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
@@ -1849,10 +1849,12 @@ OGRErr OGRGeoPackageTableLayer::CreateField(const OGRFieldDefn *poField,
     {
         m_apoFieldDefnChanges.emplace_back(
             std::make_unique<OGRFieldDefn>(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<OGRGeomFieldDefn>(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<int>(m_apoFieldDefnChanges.size()) - 1; i >= 0;
+         i--)
+    {
+        auto &oFieldChange = m_apoFieldDefnChanges[i];
+        switch (oFieldChange.eChangeType)
+        {
+            case FieldChangeType::ADD_FIELD:
+            {
+                CPLAssert(oFieldChange.iField <
+                          static_cast<int>(m_abGeneratedColumns.size()));
+                m_abGeneratedColumns.erase(m_abGeneratedColumns.begin() +
+                                           oFieldChange.iField);
+                break;
+            };
+            case FieldChangeType::DELETE_FIELD:
+            {
+                CPLAssert(oFieldChange.iField <=
+                          static_cast<int>(m_abGeneratedColumns.size()));
+                m_abGeneratedColumns.insert(m_abGeneratedColumns.begin() +
+                                                oFieldChange.iField,
+                                            oFieldChange.bGenerated);
+                break;
+            };
+            case FieldChangeType::ALTER_FIELD:
+            {
+                CPLAssert(oFieldChange.iField <
+                          static_cast<int>(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<int>(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<OGRFieldDefn>(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
@@ -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<T> &&poFieldDefnIn, int iFieldIn,
-                        FieldChangeType eChangeTypeIn)
+                        FieldChangeType eChangeTypeIn, bool bGeneratedIn)
             : poFieldDefn(std::move(poFieldDefnIn)), iField(iFieldIn),
-              eChangeType(eChangeTypeIn)
+              eChangeType(eChangeTypeIn), bGenerated(bGeneratedIn)
         {
         }
 
         std::unique_ptr<T> poFieldDefn;
         int iField;
         FieldChangeType eChangeType;
+        // Used by drivers (GPKG) to track generated fields
+        bool bGenerated;
     };
 
     std::vector<FieldDefnChange<OGRFieldDefn>> m_apoFieldDefnChanges{};

diff --git 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<OGRSQLiteTableLayer *>(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
@@ -1617,7 +1617,8 @@ OGRErr OGRSQLiteTableLayer::CreateField(const OGRFieldDefn *poFieldIn,
     {
         m_apoFieldDefnChanges.emplace_back(
             std::make_unique<OGRFieldDefn>(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<OGRGeomFieldDefn>(*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<OGRFieldDefn>(poFieldDefn), iFieldToAlter,
-            FieldChangeType::ALTER_FIELD);
+            FieldChangeType::ALTER_FIELD, /* bGenerated */ false);
     }
 
     if (nActualFlags & ALTER_TYPE_FLAG)