Speed up selection of objects in TopoGeometry layers #50906
Conversation
|
Needs to use MATERIALIZED conditionally based on PostgreSQL version. It was tested against PostgreSQL 13 and benefits from an index on |
strk
force-pushed
the
faster-topogeom-selection
branch
from
f7e4885
to
f33c133
Compare
|
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
github-actions
bot
added
the
stale
|
@pigreco did you try this patch ? If you did, can you tell us if and how it changes the user experience ? |
github-actions
bot
removed
the
stale
Hi Sandro, |
|
10 times slower with the changes in this PR is a great info to have. You didn't necessarely do anything wrong, it may be the code itself. See if/how things change if you add an index on id(tgeom) on uso_suolo |
|
Oh, of course the expected speedup is ONLY when having only a small portion of the whole dataset visible in the map view. Viewing the WHOLE map will indeed be slower in any case, with this patch, I'm afraid. |
BINGOOOOOOOOO!!! With the index created: no patch: for small portions: no patch: 2022-12-06_21h46_52.mp4 |
|
Please let's not make this work useless, what does it take to move forward? |
|
On Sun, Dec 18, 2022 at 12:57:31AM -0800, Salvatore Fiandaca wrote:
Please let's not make this work useless, what does it take to move forward?
I guess a check to only use the code IF an index on id(topogeom)
exists, beside a review which is required to merge any QGIS PR
|
|
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
github-actions
bot
added
the
stale
|
why don't any other developers want to review? |
github-actions
bot
removed
the
stale
|
QGIS is a large project, too large for anyone to be able to review every single spot in it. This PR is for a well-defined spot: the PostgreSQL provider. Even narrower: the topology support in the PostgreSQL provider. I'm probably the only developer who cares about it, and this makes it hard for anyone else to review the work. This is my hypothesis. That said, the PR is marked as a work in progress (WIP) because a conditional needs to be added to not use the optimized code where it would slow down rather than speed up operations (presence of index). Are you able to help with that work ? I'm not finding the time to work on it at the moment. Of course it would still be nice if the bot would not keep working toward shooting the (partial) work down by closing the ticket, but that's a battle I seem to be loosing over time... |
I'm not a developer and I can't do the review, so this work is doomed to oblivion. Thank you for the time you have dedicated to us |
I'm not sure exactly what COULD change here -- this PR is clearly marked and described as not ready for merge, and the original author has stated that they can't resolve the outstanding issues due to time pressure. Given that we can't merge a half-complete work, and that there's no paid QGIS staff who can just pick up this work and complete it, what would you see as the solution here? |
Thank you very much for this message and for the final question. In this PR, as in all, a review by another developer is needed in order to merge the new feature. I don't have a ready solution to this problem, but I experience it as a problem. A likely solution would be to greatly expand the pool of developers who can do PR reviews |
@pigreco, while it seems also to me that the review process needs more working time / love / funds / help / ... /, this PR is tagged as "WIP" in the title by the author, i.e. it is a work in progress, a non finished work, so it is not ready to be reviewed. Usually (and obviously) the reviewers don't review a PR until it is ready to be reviewed. |
strk
force-pushed
the
faster-topogeom-selection
branch
from
fcbef17
to
e7ad70a
Compare
| elems AS ( | ||
|
|
||
| -- TODO: skip if layer cannot be composed by nodes | ||
| SELECT ARRAY[n.node_id,1] te |
There was a problem hiding this comment.
Nit: you might end up rewriting this anyway, but returning two columns here is probably easier for readers, and for Postgres too.
There was a problem hiding this comment.
Thanks, ARRAY construction removed. For the record, readers might have recognized I was building (uselessly) "TopoElement" object -- https://postgis.net/docs/topoelement.html
|
I've gathered all my forces and pushed what should be the last bit. Would ask @pigreco to please test again with and without the index, at high and low zoom levels, between QGIS with and without this patch. If everything looks fine I guess next step would be for PostGIS to automatically create those indices (which would have more chances to be implemented if you can create a ticket on the PostGIS trac, crosslinking it to this PR :) |
strk
force-pushed
the
faster-topogeom-selection
branch
from
a810373
to
28a092b
Compare
|
I've to say I'm a bit surprised by the 10x slower report when the index is not present so may be worth further inspecting the query sent to the database to understand where the time is spent. I've improved the query a little bit too, and although I've implemented code to ONLY use it when the index is present I disabled that for now, to allow for tests. Here's what I see on my system (PostgreSQL 13) for the table containing all italian municipalities (~8000) defined against a topology with 27741 faces (one municipality - savona - was further split by lines) BBOX: st_makeenvelope(347910.26990525069413707,4890245.42507795430719852,745029.62383155268616974,5112265.28829615749418736,32632) Old query: New query, no index: New query, indexed: All in all I'm thinking this work needs more time to be really tested in multiple conditions. |
Do I have to do the tests you described or not? |
|
@pigreco the "details" link did not give me any detail, I've now restarted the job, let's hope it will succeed this time. |
|
@pigreco the re-run of CI succeeded so the link should now be there. |
|
I did the required tests I used:
with index
without index
2023-01-19_18h53_38.mp4 |
|
Great, thank you for testing @pigreco so we basically don't need the special handling for presence or absence of the index, which should simplify the code. Worth testing also with lines and with points if you can :) |
strk
force-pushed
the
faster-topogeom-selection
branch
from
3cfc9e6
to
6da1271
Compare
strk
changed the title
|
I've simplified the code by removing the check for presence of an index (given the new code is faster even without an index) and rebased against current master. Removed the WIP indication so this is ready for review too now. |
| int layerLevel; | ||
| enum TopoFeatureType | ||
| { | ||
| Puntal = 1, |
There was a problem hiding this comment.
Maybe Point, Line, Area, Mixed, or something like that?
|
On Fri, Jan 20, 2023 at 10:19:27AM -0800, Laurențiu Nicola wrote:
@lnicola commented on this pull request.
> @@ -432,6 +432,14 @@ class QgsPostgresProvider final: public QgsVectorDataProvider
{
QString topologyName;
long layerId;
+ int layerLevel;
+ enum TopoFeatureType
+ {
+ Puntal = 1,
Maybe `Point`, `Line`, `Area`, `Mixed`, or something like that?
The nomenclature derives from JTS:
https://locationtech.github.io/jts/javadoc/org/locationtech/jts/geom/Lineal.html
https://locationtech.github.io/jts/javadoc/org/locationtech/jts/geom/Puntal.html
https://locationtech.github.io/jts/javadoc/org/locationtech/jts/geom/Polygonal.html
The thing is that "Puntal" can be both Point or Multipoint, and
TopoGeometry makes no distinction (always actually converting to Multi*)
You can see how PostGIS Topology uses the JTS labels too in
TopologySummary output here:
https://postgis.net/docs/TopologySummary.html
On a closer look, "Areal" should be changed to "Polygonal", I see.
BTW, the new code is NOT using those names at all, just the numbers,
which are PostGIS Topology constants...
|
|
Ouch. All right, sorry for that. |
|
On Sat, Jan 21, 2023 at 02:05:52AM -0800, Laurențiu Nicola wrote:
Ouch. All right, sorry for that.
Your review was very very welcome. I'm updating from Areal to
Polygonal :)
…--strk;
|
This is aimed at using spatial indexes on primitive tables. Only affects non-hierarchical TopoGeometry layers access. Thanks Laurențiu Nicola (grayshade) for excellent feedback and Salvatore Fiandaca (pigreco) for testing !
strk
force-pushed
the
faster-topogeom-selection
branch
from
6da1271
to
0b51d37
Compare
|
Pushed the naming change, for consistency. |
strk
changed the title
|
Any chance to get this PR approved before feature freeze ? It's a very localized change, only affects users of PostGIS TopoGeometry layers (very few). Two of the few users have tested it and found to be greatly helping :) |
You're very kind Laurențiu but I'm afraid it takes an approval from someone with a specific level of authorization in order for this PR to be mergeable. I'm not familiar with the internals of GitHub burocracy and cannot find right away the list of such group of people. @nyalldawson do you have a pointer ? |
|
Hi @strk, this PR has been ready for review for only 4 days. Although there are PR waiting for merge for weeks or months, I would suggest you to send a message to the developer ml. |
There was a problem hiding this comment.
Approving, with the caveat that I'm not familiar with that part of the code.
Note: "grep -ri topogeom tests/" shows very little tests (apparently just testing detection of PostGIS topography layers but not much more, unless I'm missing something). That could be something to develop to make changes in that part of the code feel less risky
|
Looks useful to me, thanks @strk |
This is aimed at using spatial indexes on primitive tables. Only affects non-hierarchical TopoGeometry layers access.