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

Garbage-collection is not performed after deletion of vector layer from geopackage #27719

Closed
qgib opened this issue Sep 19, 2018 · 8 comments
Closed

Comments

@qgib
Copy link
Contributor

qgib commented Sep 19, 2018

Author Name: Andrea Giudiceandrea (@agiudiceandrea)
Original Redmine Issue: 19895

Redmine category:browser
Assignee: Alessandro Pasotti


With QGIS 3.3.0-master (d99d506), deleting a vector layer from a geopackage, through the "Browser" panel or the "DB Manager", is not followed by a proper garbage-collection (shrink/compact) of the geopackage.

I think a VACUUM sql command should be automatically executed after deleting a layer from a geopackage or a contextual menu option should be added in the "Browser" panel and in the "DB Manager" to let the user perform such operation.

@qgib
Copy link
Contributor Author

qgib commented Sep 20, 2018

Author Name: Alessandro Pasotti (@elpaso)


I might be wrong but IIRC this was intentional, because a VACUUM can take a very long time on large layers, I think that a possible solution would be to add a "Run vacuum" option to the browser data item of a geopackage layer.

What do you think?


  • status_id was changed from Open to Feedback
  • assigned_to_id was configured as Alessandro Pasotti

@qgib
Copy link
Contributor Author

qgib commented Sep 20, 2018

Author Name: Andrea Giudiceandrea (@agiudiceandrea)


It could be OK for me to add "Run vacuum" or "Compact geopackage" or something similar in the Browser panel for geopackages (along with "Add Connection"/"Remove connection" and "Create a New Layer or Table...") and eventually also in DB Manager (along with "Re-connect" and "Remove").

However, a VACUUM sql command is still executed automatically after a raster layer has been deleted from a geopackage, as you can see in https://github.com/qgis/QGIS/blob/master/src/providers/ogr/qgsgeopackagedataitems.cpp#L441-L450

@qgib
Copy link
Contributor Author

qgib commented Sep 20, 2018

Author Name: Alessandro Pasotti (@elpaso)


mmm looks like I wrote that code :) So, if the VACUUM is already executed I don't really know what to do here.

@qgib
Copy link
Contributor Author

qgib commented Sep 20, 2018

Author Name: Andrea Giudiceandrea (@agiudiceandrea)


It seems that the VACUUM is only executed (in deleteGeoPackageRasterLayer) after a RASTER layers is deleted. I will confirm this when a new build of qgis-dev with your commit e62c4eb will be available on osgeo4w

For VECTOR layer I cannot trace down in the source code if and where VACUUM is executed. The fact is that the geopackage is not "vacuumed" after a vector layer is deleted.

@qgib
Copy link
Contributor Author

qgib commented Sep 21, 2018

Author Name: Alessandro Pasotti (@elpaso)


Yes, you are right of course, as I mentioned in my first reply IIRC it was not implemented on purpose and the idea was to add a menu item to do that.

Btw, that's technically a new feature, but it could also be considered a bug, so let's see if I can do it!


  • tracker_id was changed from 1 to 2

@qgib
Copy link
Contributor Author

qgib commented Sep 21, 2018

Author Name: Andrea Giudiceandrea (@agiudiceandrea)


Thank you Alessandro! I'm not so skilled to add such an options by myself (without the risk of breaking something).

I was writing this before reading your last comment, so now it may not be so useful anymore:

I confirm that VACUUM is executed after a RASTER layer is deleted, and that is not executed after a VECTOR layer is deleted.

As I can see in the source code (qgsgeopackagedataitems.ccp):

  • for QgsGeoPackageRasterLayerItem, executeDeleteLayer() calls deleteGeoPackageRasterLayer() which executes all the sql commands in order to delete a RASTER layer and its references and executes the VACUUM sql command at the end;

  • for QgsGeoPackageVectorLayerItem, executeDeleteLayer() calls deleteLayer() so relying on GDAL (GDALGeoPackageDataset::DeleteLayer) to execute all the sql commands required to delete a VECTOR layer and its references: DeleteLayer() does not executes a VACUUM sql command at the end.

So I think we could:

  • add a "Run vacuum" or "Compact geopackage" options as previously said (which is anyway needed to allow users to easily shrink geopackages)

and one of the following

  1. +add the VACUUM sql command execution after ::deleteLayer()+ in QgsGeoPackageVectorLayerItem::executeDeleteLayer() or in a little deleteGeoPackageVectorLayer() (for symmetry)

or

  1. +remove the VACUUM sql command from deleteGeoPackageRasterLayer+ (provided we add the "Run vacuum" option).

By the way - you probably know it already - I realised now that since GDAL 2.3.0 there are two new functions in GDALGeoPackageDataset: DeleteRasterLayer() and DeleteVectorOrRasterLayer() along with DeleteLayer().

@qgib
Copy link
Contributor Author

qgib commented Sep 21, 2018

Author Name: Alessandro Pasotti (@elpaso)


PR #7976


  • status_id was changed from Feedback to In Progress
  • pull_request_patch_supplied was changed from 0 to 1

@qgib
Copy link
Contributor Author

qgib commented Sep 21, 2018

Author Name: Anónimo (Anónimo)


Applied in changeset b51cb21.


  • done_ratio was changed from 0 to 100
  • status_id was changed from In Progress to Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant