fix to rules returning multiple rows

SPlanzer commented 3 years ago

Fixes: #

165

Change Description:

Advancing testing showed that the returning rules create to provide view integration resulted in the #165 error.

These changes ensure that only one row is returned when insert against the view are called.

...

Notes for Testing:

This has been manually tested to prove it resolves the #165 issues. Automated tests first detected this issue. These test (which are still under development) pass with this change.

Source Code Documentation Tasks:

[ ] README updated (where applicable)
[ ] CHANGELOG updated

User Documentation Tasks:

[ ] Confluence user guide updated (where applicable)

Testing Tasks:

[ ] Added tests that fail without this change
[ ] All tests are passing in development environment
[ ] Reviewers assigned

Pull Request Management:

[ ] Linked to sub-task
[ ] Linked to the epic

billgeo commented 3 years ago

This looks a bit odd. Might need you to take me through this one.

SPlanzer commented 3 years ago

@billgeo I have concerns with this also and placed this PR draft while I address them. I will leave it in draft while I write test for the functionality that interacts with these rules. Note - Initial tests when implementing these changes are passing.

Background

This is following the suggested fix via the qgis-developer mailing list. https://lists.osgeo.org/pipermail/qgis-developer/2017-January/046784.html

and https://www.postgresql.org/docs/8.2/rules-update.html - I not the insert returning example here appear to be returning multiple row. QGIS errors if an insert returns more than one row

INSERT RETURNING:

returning is limited as the insert value as well as the pseudorelations NEW and OLD can not be accessed in the returning clause

There is a strong preference for triggers. I have been putting this off as it requires some re-write and I had concern over time required to do so. It is a much more suited pattern in terms of returning on inserts.

This is highlight by the postgres docs

If you want to support INSERT RETURNING and so on, then be sure to put a suitable RETURNING clause into each of these rules. Alternatively, an updatable view can be implemented using INSTEAD OF triggers (see CREATE TRIGGER).

also https://stackoverflow.com/questions/59881342/postgres-trigger-upsert-with-returning

Moving to "INSTEAD OF triggers" does not look like as much over head as I was initially concerned with and should be considered.

* Also note in the mailing list discussion the talk about dummy returning values

billgeo commented 3 years ago

Why are you doing limit 1 and not group by. Are you sure you're returning the right data?

billgeo commented 3 years ago

Sorry just saw your additional comment.

I have concerns with this also and placed this PR draft while I address them.

Seems fine to stick with rules I think. Triggers obviously recommended, but if it's not to much work to use rules let's make minimum changes for now. My main question is why it's returning more than one row and what's the best way to resolve that.

SPlanzer commented 3 years ago

Why are you doing limit 1 and not group by.

The structure that is returned must match the view (that the QGIS layer is attached to).
Must be of the same data types
Must be only one row that represents the inserted data.

Would a group by not return an aggregate rather than the inserted data?

Are you sure you're returning the right data?

tests in QGIS show this is the case

SPlanzer commented 3 years ago

My concerns were around the many sub queries. I have do some more research and many of the data values can be accessed when returning on the insert. It is just those values that are not supplied in the insert or part of the range-table entries for NEW that are not accessible.

I have pushed a commit to this PR removing the extraneous subqueries.

I am going to leave this in draft until I written all test for geom edits

billgeo commented 3 years ago

Would a group by not return an aggregate rather than the inserted data?

Only if you use an aggregate function and that query returns more than one value.

SELECT shape
FROM gazetteer.feature_geometry
ORDER BY geom_id DESC
LIMIT 1

This just returns the geom for the first ID. Is it:

returning more than one geometry and id, or
multiple rows of the same geometry If 1, then are you sure the first one is the right one? If 2, then you could use a group by such as:
```
SELECT shape
FROM gazetteer.feature_geometry
GROUP BY shape
```

SPlanzer commented 3 years ago

@strk can I get you review on this database changes?

The issue here is QGIS now requires insert to views via rules to be returning. This is was the proposed changes address.

I can confirm via extensive automated tests (#170) this resolves the QGIS error and does not introduces others

strk commented 3 years ago

Here's a single test of mine on PostgreSQL 8.4.22:

test=# create table t (i serial, a text);
CREATE TABLE
test=# create view tv as select * from t;
CREATE VIEW                                
test=# create rule tv_ins as on insert to tv do instead insert into t (a) values (upper(NEW.a)) returning *;
CREATE RULE
test=# insert into tv (a) values ('lower') returning *;
 i |   a   
---+-------
 1 | LOWER
(1 row)

I thought it'd be useful to get rid of some more subselects (you can return * from an INSERT and it will return whatever values will be inserted in the final table.

linz / gazetteer