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.

Sign up for GitHub

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

GPKG: fixes to make most operations compatible with PRAGMA foreign_keys=1 #9141

Merged
merged 1 commit into from
Jan 29, 2024
Merged

GPKG: fixes to make most operations compatible with PRAGMA foreign_keys=1 #9141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 38 additions & 21 deletions autotest/ogr/ogr_gpkg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1884,11 +1884,11 @@ def test_ogr_gpkg_20a(tmp_vsimem):
ds = None

# Unable to parse srs_id '4326' well-known text 'invalid'
with gdal.quiet_errors():
with gdal.config_option("OGR_SQLITE_PRAGMA", "FOREIGN_KEYS=0"), gdal.quiet_errors():
ds = ogr.Open(fname, update=1)
ds.ExecuteSQL("DELETE FROM gpkg_spatial_ref_sys WHERE srs_id = 4326")
ds = None

ds.ExecuteSQL("DELETE FROM gpkg_spatial_ref_sys WHERE srs_id = 4326")
ds = None
with gdal.config_option(
"OGR_GPKG_FOREIGN_KEY_CHECK", "NO"
), gdaltest.error_handler():
Expand All @@ -1901,24 +1901,25 @@ def test_ogr_gpkg_20b(tmp_vsimem):

fname = tmp_vsimem / "ogr_gpkg_20.gpkg"

ds = gdaltest.gpkg_dr.CreateDataSource(fname)
srs = osr.SpatialReference()
srs.ImportFromEPSG(4326)
lyr = ds.CreateLayer("foo4326", srs=srs)
assert lyr is not None
with gdal.config_option("OGR_SQLITE_PRAGMA", "FOREIGN_KEYS=0"):
ds = gdaltest.gpkg_dr.CreateDataSource(fname)
srs = osr.SpatialReference()
srs.ImportFromEPSG(4326)
lyr = ds.CreateLayer("foo4326", srs=srs)
assert lyr is not None

ds.ExecuteSQL("DROP TABLE gpkg_spatial_ref_sys")
ds.ExecuteSQL(
"CREATE TABLE gpkg_spatial_ref_sys (srs_name TEXT, "
"srs_id INTEGER, organization TEXT, "
"organization_coordsys_id INTEGER, definition TEXT)"
)
ds.ExecuteSQL(
"INSERT INTO gpkg_spatial_ref_sys "
"(srs_name,srs_id,organization,organization_coordsys_id,"
"definition) VALUES (NULL,4326,NULL,NULL,NULL)"
)
ds = None
ds.ExecuteSQL("DROP TABLE gpkg_spatial_ref_sys")
ds.ExecuteSQL(
"CREATE TABLE gpkg_spatial_ref_sys (srs_name TEXT, "
"srs_id INTEGER, organization TEXT, "
"organization_coordsys_id INTEGER, definition TEXT)"
)
ds.ExecuteSQL(
"INSERT INTO gpkg_spatial_ref_sys "
"(srs_name,srs_id,organization,organization_coordsys_id,"
"definition) VALUES (NULL,4326,NULL,NULL,NULL)"
)
ds = None

# Warning 1: null definition for srs_id '4326' in gpkg_spatial_ref_sys
with gdal.config_option(
Expand Down Expand Up @@ -3430,7 +3431,14 @@ def test_ogr_gpkg_35(tmp_vsimem, tmp_path):
):
f.DumpReadable()
pytest.fail()
lyr = None
lyr_nonspatial = None
ds = None

with gdaltest.config_option("OGR_SQLITE_PRAGMA", "FOREIGN_KEYS=0"):
ds = ogr.Open(dbname, update=1)
lyr = ds.GetLayerByName("test")
lyr_nonspatial = ds.GetLayerByName("test_nonspatial")
lyr.StartTransaction()
ret = lyr_nonspatial.DeleteField(1)
lyr.CommitTransaction()
Expand Down Expand Up @@ -5069,7 +5077,8 @@ def test_ogr_gpkg_57(tmp_vsimem):
out_filename = tmp_vsimem / "test_ogr_gpkg_57.gpkg"
ogr.GetDriverByName("GPKG").CreateDataSource(out_filename)

ds = ogr.Open(out_filename, update=1)
with gdal.config_option("OGR_SQLITE_PRAGMA", "FOREIGN_KEYS=0"):
ds = ogr.Open(out_filename, update=1)
ds.ExecuteSQL("DROP TABLE gpkg_contents")
ds.ExecuteSQL(
"CREATE TABLE gpkg_contents (table_name,data_type,identifier,description,last_change,min_x, min_y,max_x, max_y,srs_id)"
Expand Down Expand Up @@ -10004,3 +10013,11 @@ def test_ogr_gpkg_extent3d_on_2d_dataset_with_filters(tmp_vsimem):
assert ext3d == (3, 3, 4, 4, float("inf"), float("-inf"))

ds = None


@gdaltest.enable_exceptions()
def test_ogr_gpkg_creation_with_foreign_key_constraint_enabled(tmp_vsimem):

with gdaltest.config_option("OGR_SQLITE_PRAGMA", "FOREIGN_KEYS=1"):
out_filename = str(tmp_vsimem / "out.gpkg")
gdal.VectorTranslate(out_filename, "data/poly.shp")
41 changes: 41 additions & 0 deletions ogr/ogrsf_frmts/gpkg/ogr_geopackage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ class GDALGeoPackageDataset final : public OGRSQLiteBaseDataSource,
mutable int m_nHasMetadataTables = -1; // -1 = unknown, 0 = false, 1 = true
int m_nCreateMetadataTables = -1; // -1 = on demand, 0 = false, 1 = true

// Set by CreateTileGriddedTable() and used by FinalizeRasterRegistration()
std::string m_osSQLInsertIntoGpkg2dGriddedCoverageAncillary{};

CPLString m_osIdentifier{};
bool m_bIdentifierAsCO = false;
CPLString m_osDescription{};
Expand Down Expand Up @@ -411,6 +414,44 @@ class GDALGeoPackageDataset final : public OGRSQLiteBaseDataSource,

GDALDataset *GetRasterLayerDataset(const char *pszLayerName);

//! Instance of that class temporarily disable foreign key checks
class TemporaryForeignKeyCheckDisabler
{
public:
explicit TemporaryForeignKeyCheckDisabler(GDALGeoPackageDataset *poDS)
: m_poDS(poDS),
m_nPragmaForeignKeysOldValue(SQLGetInteger(
m_poDS->GetDB(), "PRAGMA foreign_keys", nullptr))
{
if (m_nPragmaForeignKeysOldValue)
{
CPL_IGNORE_RET_VAL(
SQLCommand(m_poDS->GetDB(), "PRAGMA foreign_keys = 0"));
}
}

~TemporaryForeignKeyCheckDisabler()
{
if (m_nPragmaForeignKeysOldValue)
{
CPL_IGNORE_RET_VAL(
SQLCommand(m_poDS->GetDB(), "PRAGMA foreign_keys = 1"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is no need for setting that value back as that is only a connection wide setting. This connection won't be reused outside the geopackage writer, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it is not expensive to turn it on, and wouldn't it be better to play safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

This connection won't be reused outside the geopackage writer, right?

Users of the API could potentially run the GDALDataset::ExecuteSQL("PRAGMA foreign_keys = 1") , so better restore to the setting they used

}
}

private:
CPL_DISALLOW_COPY_ASSIGN(TemporaryForeignKeyCheckDisabler)

GDALGeoPackageDataset *m_poDS = nullptr;
int m_nPragmaForeignKeysOldValue = 0;
};

//! Return an object that, while alive, temporarily disables foreign key checks
TemporaryForeignKeyCheckDisabler GetTemporaryForeignKeyCheckDisabler()
{
return TemporaryForeignKeyCheckDisabler(this);
}

protected:
virtual CPLErr IRasterIO(GDALRWFlag, int, int, int, int, void *, int, int,
GDALDataType, int, int *, GSpacing, GSpacing,
Expand Down
41 changes: 39 additions & 2 deletions ogr/ogrsf_frmts/gpkg/ogrgeopackagedatasource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,10 @@ bool GDALGeoPackageDataset::ConvertGpkgSpatialRefSysToExtensionWkt2(
if (!oResultTable)
return false;

// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
GetTemporaryForeignKeyCheckDisabler());

bool bRet = SoftStartTransaction() == OGRERR_NONE;

if (bRet)
Expand Down Expand Up @@ -3247,7 +3251,8 @@ CPLErr GDALGeoPackageDataset::FinalizeRasterRegistration()
m_adfGeoTransform[0] + nRasterXSize * m_adfGeoTransform[1];
double dfGDALMaxY = m_adfGeoTransform[3];

SoftStartTransaction();
if (SoftStartTransaction() != OGRERR_NONE)
return CE_Failure;

const char *pszCurrentDate =
CPLGetConfigOption("OGR_CURRENT_DATE", nullptr);
Expand All @@ -3270,7 +3275,10 @@ CPLErr GDALGeoPackageDataset::FinalizeRasterRegistration()
eErr = SQLCommand(hDB, pszSQL);
sqlite3_free(pszSQL);
if (eErr != OGRERR_NONE)
{
SoftRollbackTransaction();
return CE_Failure;
}

double dfTMSMaxX = m_dfTMSMinX + nTileXCountZoomLevel0 * nTileWidth *
dfPixelXSizeZoomLevel0;
Expand All @@ -3286,7 +3294,10 @@ CPLErr GDALGeoPackageDataset::FinalizeRasterRegistration()
eErr = SQLCommand(hDB, pszSQL);
sqlite3_free(pszSQL);
if (eErr != OGRERR_NONE)
{
SoftRollbackTransaction();
return CE_Failure;
}

m_papoOverviewDS = static_cast<GDALGeoPackageDataset **>(
CPLCalloc(sizeof(GDALGeoPackageDataset *), m_nZoomLevel));
Expand Down Expand Up @@ -3323,7 +3334,10 @@ CPLErr GDALGeoPackageDataset::FinalizeRasterRegistration()
eErr = SQLCommand(hDB, pszSQL);
sqlite3_free(pszSQL);
if (eErr != OGRERR_NONE)
{
SoftRollbackTransaction();
return CE_Failure;
}

if (i < m_nZoomLevel)
{
Expand All @@ -3339,6 +3353,18 @@ CPLErr GDALGeoPackageDataset::FinalizeRasterRegistration()
}
}

if (!m_osSQLInsertIntoGpkg2dGriddedCoverageAncillary.empty())
{
eErr = SQLCommand(
hDB, m_osSQLInsertIntoGpkg2dGriddedCoverageAncillary.c_str());
m_osSQLInsertIntoGpkg2dGriddedCoverageAncillary.clear();
if (eErr != OGRERR_NONE)
{
SoftRollbackTransaction();
return CE_Failure;
}
}

SoftCommitTransaction();

m_nOverviewCount = m_nZoomLevel;
Expand Down Expand Up @@ -5842,7 +5868,7 @@ bool GDALGeoPackageDataset::CreateTileGriddedTable(char **papszOptions)
m_dfOffset, m_dfPrecision, osGridCellEncoding.c_str(),
osUom.empty() ? nullptr : osUom.c_str(), osFieldName.c_str(),
osQuantityDefinition.c_str());
osSQL += pszSQL;
m_osSQLInsertIntoGpkg2dGriddedCoverageAncillary = pszSQL;
sqlite3_free(pszSQL);

// Requirement 3 /gpkg-spatial-ref-sys-row
Expand Down Expand Up @@ -6729,6 +6755,10 @@ int GDALGeoPackageDataset::FindLayerIndex(const char *pszLayerName)

OGRErr GDALGeoPackageDataset::DeleteLayerCommon(const char *pszLayerName)
{
// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
GetTemporaryForeignKeyCheckDisabler());

char *pszSQL = sqlite3_mprintf(
"DELETE FROM gpkg_contents WHERE lower(table_name) = lower('%q')",
pszLayerName);
Expand Down Expand Up @@ -6858,6 +6888,10 @@ OGRErr GDALGeoPackageDataset::DeleteLayer(int iLayer)

CPLDebug("GPKG", "DeleteLayer(%s)", osLayerName.c_str());

// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
GetTemporaryForeignKeyCheckDisabler());

OGRErr eErr = SoftStartTransaction();

if (eErr == OGRERR_NONE)
Expand Down Expand Up @@ -6924,6 +6958,9 @@ OGRErr GDALGeoPackageDataset::DeleteLayer(int iLayer)

OGRErr GDALGeoPackageDataset::DeleteRasterLayer(const char *pszLayerName)
{
// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
GetTemporaryForeignKeyCheckDisabler());

OGRErr eErr = SoftStartTransaction();

Expand Down
36 changes: 29 additions & 7 deletions ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5142,8 +5142,14 @@ OGRErr OGRGeoPackageTableLayer::Rename(const char *pszDstTableName)
return OGRERR_FAILURE;
}

// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
m_poDS->GetTemporaryForeignKeyCheckDisabler());

if (m_poDS->SoftStartTransaction() != OGRERR_NONE)
{
return OGRERR_FAILURE;
}

#ifdef ENABLE_GPKG_OGR_CONTENTS
DisableFeatureCountTriggers(false);
Expand Down Expand Up @@ -5736,11 +5742,6 @@ OGRErr OGRGeoPackageTableLayer::RunDeferredCreationIfNecessary()
/* Update gpkg_contents with the table info */
const OGRwkbGeometryType eGType = GetGeomType();
const bool bIsSpatial = (eGType != wkbNone);
if (bIsSpatial)
err = RegisterGeometryColumn();

if (err != OGRERR_NONE)
return OGRERR_FAILURE;

if (bIsSpatial || m_eASpatialVariant == GPKG_ATTRIBUTES)
{
Expand All @@ -5766,6 +5767,15 @@ OGRErr OGRGeoPackageTableLayer::RunDeferredCreationIfNecessary()
return OGRERR_FAILURE;
}

if (bIsSpatial)
{
// Insert into gpkg_geometry_columns after gpkg_contents because of
// foreign key constraints
err = RegisterGeometryColumn();
if (err != OGRERR_NONE)
return OGRERR_FAILURE;
}

#ifdef ENABLE_GPKG_OGR_CONTENTS
if (m_poDS->m_bHasGPKGOGRContents)
{
Expand Down Expand Up @@ -6172,11 +6182,17 @@ OGRErr OGRGeoPackageTableLayer::DeleteField(int iFieldToDelete)
/* -------------------------------------------------------------------- */
m_poDS->ResetReadingAllLayers();

// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
m_poDS->GetTemporaryForeignKeyCheckDisabler());

if (m_poDS->SoftStartTransaction() != OGRERR_NONE)
{
return OGRERR_FAILURE;
}

// ALTER TABLE ... DROP COLUMN ... was first implemented in 3.35.0 but
// there was bug fixes related to it until 3.35.5
// ALTER TABLE ... DROP COLUMN ... was first implemented in 3.35.0 but
// there was bug fixes related to it until 3.35.5
#if SQLITE_VERSION_NUMBER >= 3035005L
OGRErr eErr = SQLCommand(
m_poDS->GetDB(), CPLString()
Expand Down Expand Up @@ -7098,6 +7114,10 @@ OGRErr OGRGeoPackageTableLayer::AlterGeomFieldDefn(
(poOldSRS != nullptr && poNewSRS != nullptr &&
!poOldSRS->IsSame(poNewSRS.get(), apszOptions)))
{
// Temporary remove foreign key checks
const auto oTemporaryForeignKeyCheckDisabler(
m_poDS->GetTemporaryForeignKeyCheckDisabler());

if (m_poDS->SoftStartTransaction() != OGRERR_NONE)
return OGRERR_FAILURE;

Expand Down Expand Up @@ -7169,7 +7189,9 @@ OGRErr OGRGeoPackageTableLayer::AlterGeomFieldDefn(
}

if (m_poDS->SoftCommitTransaction() != OGRERR_NONE)
{
return OGRERR_FAILURE;
}

m_iSrs = nNewSRID;
OGRSpatialReference *poSRS = poNewSRS.release();
Expand Down
Loading