PostgreSQL provider don't cast bigint PKs to text #35162

espinafre · 2020-03-18T04:42:00Z

This PR is instead of botched #34780 (because of my mistakes with git merging/rebasing); please see the whole discussion there. In addition, a test case for updating tables with positive, zero, negative PKs; I couldn't find a way to test if the queries are being sent to the PostgreSQL backend as designed from within the testing framework. @elpaso , @m-kuhn , is this mergeable? I'm hoping to backport this in time for the next 3.10 and 3.12 point releases.

Fixes #34077
Partially fixes #34264

m-kuhn

It looks generally good to me, thanks.
I can imagine that adding tests for what's sent to the db is hard to do, so I'm good with sticking to functional tests and trusting your performance tests (and hoping no regressions are introduced for the latter).

src/providers/postgres/qgspostgresconn.cpp

tests/src/python/test_provider_postgres.py

elpaso · 2020-03-18T09:00:34Z

LGTM, the test coverage on PG is pretty good, so I guess that if the tests are passing we should not expect regressions.

espinafre · 2020-03-18T14:18:24Z

Travis is still choking on formatting, it is as if it hasn't seen ad8f89b . Locally running ctest -V -R qgis_indentation results in success.

m-kuhn · 2020-03-18T15:48:28Z

See https://github.com/qgis/QGIS/blob/master/.github/CONTRIBUTING.md#contributing-to-qgis for some hints how to get your code formatted automatically

espinafre · 2020-03-18T16:19:39Z

I've ran prepare-commit.sh before pushing the C++ code, but I think I missed it when adding the test code, thanks. What can I do about the failed qgis_sip_uptodate test? It passes when I run locally with ctest -V -R qgis_sip_uptodate

espinafre · 2020-03-18T16:26:50Z

This is the full output of my local ctest -v -R qgis_sip_uptodate (which fails on Travis):

UpdateCTestConfiguration  from :/home/jose/sw/qgis/QGIS/build-master/DartConfiguration.tcl
Parse Config file:/home/jose/sw/qgis/QGIS/build-master/DartConfiguration.tcl
 Add coverage exclude regular expressions.
 Add coverage exclude: /CMakeFiles/CMakeTmp/
SetCTestConfiguration:CMakeCommand:/usr/bin/cmake
UpdateCTestConfiguration  from :/home/jose/sw/qgis/QGIS/build-master/DartConfiguration.tcl
Parse Config file:/home/jose/sw/qgis/QGIS/build-master/DartConfiguration.tcl
Test project /home/jose/sw/qgis/QGIS/build-master
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 577
    Start 577: qgis_sip_uptodate

577: Test command: /home/jose/sw/qgis/QGIS/tests/code_layout/test_sipfiles_uptodate.sh
577: Test timeout computed to be: 1500
1/1 Test #577: qgis_sip_uptodate ................   Passed   85.44 sec

The following tests passed:
        qgis_sip_uptodate

100% tests passed, 0 tests failed out of 1

Total Test time (real) =  85.46 sec

m-kuhn · 2020-03-18T16:28:46Z

I've ran prepare-commit.sh before pushing the C++ code,

You need to run it before committing (it will only work on uncommitted files to save some time).
Does ./scripts/sipify_all.sh modify files?

espinafre · 2020-03-18T17:01:22Z

I've ran prepare-commit.sh before pushing the C++ code,

You need to run it before committing (it will only work on uncommitted files to save some time).
Does ./scripts/sipify_all.sh modify files?

Only testdata/untracked files. After running it, git status gives:

On branch pg_bigint_pk_no_cast
Your branch is up to date with 'origin/pg_bigint_pk_no_cast'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   python/plugins/processing/tests/testdata/custom/circular_strings.gpkg
        modified:   python/plugins/processing/tests/testdata/custom/multi_polys_with_empty_geoms.gpkg
        modified:   python/plugins/processing/tests/testdata/custom/pol.gpkg
        modified:   python/plugins/processing/tests/testdata/expected/merged_pol.gpkg
        modified:   python/plugins/processing/tests/testdata/expected/multi_polys_non_null_one_empty.gpkg
        modified:   python/plugins/processing/tests/testdata/expected/results_remove_null_geometries.gpkg
        modified:   tests/testdata/analysis/dem.tif.aux.xml
        modified:   tests/testdata/analysis/slope.tif.aux.xml
        modified:   tests/testdata/control_images/expected_raster_linedensity/linedensity_testcase1.tif.aux.xml
        modified:   tests/testdata/control_images/expected_raster_linedensity/linedensity_testcase2.tif.aux.xml
        modified:   tests/testdata/control_images/fillNoData/fillnodata_testcase1.tif.aux.xml
        modified:   tests/testdata/control_images/fillNoData/fillnodata_testcase2.tif.aux.xml
        modified:   tests/testdata/control_images/fillNoData/fillnodata_testcase3.tif.aux.xml
        modified:   tests/testdata/curved_polys.gpkg
        modified:   tests/testdata/points_gpkg.gpkg
        modified:   tests/testdata/zonalstatistics/raster.tif.aux.xml
Untracked files:
  (use "git add <file>..." to include in what will be committed)
        patch
        python/analysis/auto_additions/qgsgeometrycheck.py.temp
        python/core/auto_additions/qgis.py.temp
        python/core/auto_additions/qgsabstractdatabaseproviderconnection.py.temp
        python/core/auto_additions/qgsabstractgeometry.py.temp
        python/core/auto_additions/qgsauthmanager.py.temp
        python/core/auto_additions/qgsdataitem.py.temp
        python/core/auto_additions/qgsdatasourceuri.py.temp
        python/core/auto_additions/qgsdefaultvalue.py.temp
        python/core/auto_additions/qgsdxfexport.py.temp
        python/core/auto_additions/qgseditformconfig.py.temp
        python/core/auto_additions/qgsfieldproxymodel.py.temp
  <... snip ...>

m-kuhn · 2020-03-18T17:08:53Z

Ah wait, that remaining thing seems to be something that changed on master meanwhile. The test has a weird behavior with respect to comparing "master merged into pr" with plain "pr".
Can you do a rebase on latest master locally again and force push?

git fetch upstream
git rebase upstream/master
git push -f

espinafre · 2020-03-18T17:41:41Z

Ah wait, that remaining thing seems to be something that changed on master meanwhile. The test has a weird behavior with respect to comparing "master merged into pr" with plain "pr".
Can you do a rebase on latest master locally again and force push?
git fetch upstream
git rebase upstream/master
git push -f

Done. Let's see how Travis likes this, thank you for the tip.

espinafre · 2020-03-18T17:58:03Z

Travis failed the same test...

espinafre · 2020-03-18T18:03:50Z

I've done a make clean and re-ran ./scripts/sipify_all.sh, which made the change in commit 6767395; this is what I just pushed.

espinafre · 2020-03-18T20:46:41Z

Now another test failed on Travis, but passed locally: PyQgsPostgresRasterProvider. I've got no idea why.

espinafre · 2020-03-18T22:19:58Z

Did nothing this time, just fetched upstream/master, rebased against it, and pushed -f, according to @m-kuhn instructions. The previous failing test in Travis still passes locally (PyQgsPostgresRasterProvider).

nyalldawson · 2020-03-18T22:26:12Z

If you really want to test this fully (and I would strongly suggest you do), then you need to implement the ProviderTestCase in a class which uses a bigint primary key.

Checkout TestPyQgsPostgresProviderCompoundKey in test_provider_postgres.py for a template on how to do this

espinafre · 2020-03-18T22:47:32Z

If you really want to test this fully (and I would strongly suggest you do), then you need to implement the ProviderTestCase in a class which uses a bigint primary key.

Checkout TestPyQgsPostgresProviderCompoundKey in test_provider_postgres.py for a template on how to do this

Hi, don't you think that the methods testPktUpdateBigintPk and testPktUpdateBigintPkNonFirst that I wrote in 0853029 are enough? Those exercise loading and saving features with bigint PKs, for a table with an ordinary bigint pk and another table whose PK is not the first one defined.

nyalldawson · 2020-03-18T22:59:40Z

Hi, don't you think that the methods testPktUpdateBigintPk and testPktUpdateBigintPkNonFirst that I wrote in 0853029 are enough? Those exercise loading and saving features with bigint PKs, for a table with an ordinary bigint pk and another table whose PK is not the first one defined.

Depends how confident you want to be. If you implement the ProviderTestCase then you can 100% sleep at night knowing you're covered in all existing conceivable situations, and will also be automatically covered whenever new functionality is added to the providers...

nyalldawson · 2020-03-18T23:00:34Z

(just to clarify: the provider test class is a mega stress test. It throws thousands of edge cases at the provider to make sure they handle them correctly).

espinafre · 2020-03-18T23:54:37Z

(just to clarify: the provider test class is a mega stress test. It throws thousands of edge cases at the provider to make sure they handle them correctly).

Thanks for the pointer, and for enlightening me as always, I'm working on it. Just subclassing the ProviderTestCase and adjusting the data source? Is there any requirements for the data source (geometry type for example)? Also, is there a need for one subclass for each kind of table (that is, with a single ordinary PK, with a PK which is not defined as the first field, with a composite PK in which at least one member is a bigint...)? Sorry for the torrent of questions, but you provoked it :D

espinafre · 2020-03-18T23:57:24Z

I still cannot figure out why the test PyQgsPostgresRasterProvider fails on Travis but not on my own system (PostgreSQL 12 + postgisraster 3.0), neither on Azure.

nyalldawson · 2020-03-19T00:01:52Z

I still cannot figure out why the test PyQgsPostgresRasterProvider fails on Travis but not on my own system (PostgreSQL 12 + postgisraster 3.0), neither on Azure.

It's unrelated -- it fails randomly regardless of the PR

nyalldawson · 2020-03-19T00:06:52Z

Just subclassing the ProviderTestCase and adjusting the data source? Is there any requirements for the data source (geometry type for example)?

Yes - look at how "someDataCompound" is loaded in tests/testdata/provider/testdata_pg.sql. You need to load in the same reference data rows (but with the different primary key type). Everything from the "pk" field onward needs to stay the same.

If you want to also run the editing reference tests then you need to implement getEditableLayer in your test subclass. See TestPyQgsPostgresProvider.getEditableLayer for how this is done (basically it needs to drop the existing editable version of the table, then recreate it anew, since each test in the editable part of the test suite wants a clean copy of this table to work from, and it'll mangle it up during its tests.)

Also, is there a need for one subclass for each kind of table (that is, with a single ordinary PK, with a PK which is not defined as the first field, with a composite PK in which at least one member is a bigint...)

If they aren't already covered, then yes -- this would be very valuable! It's probably the biggest (only?) remaining gap in the test coverage of postgres provider.

elpaso · 2020-03-20T22:02:25Z

I still cannot figure out why the test PyQgsPostgresRasterProvider fails on Travis but not on my own system (PostgreSQL 12 + postgisraster 3.0), neither on Azure.

It's unrelated -- it fails randomly regardless of the PR

I hoped it was fixed. Is it still flacky?

roya0045 · 2020-03-20T22:18:56Z

@elpaso a rebase should fix this, but now the azure build is bjorked.

…irstSinglePk

Create a ProviderTestCase subclass, TestPyQgsPostgresProviderBigintSinglePk, with the editing reference tests. The methods which differ from the upper FeatureSourceTestCase class are rewritten and marked with TODO annotations to highlight the provider deficiencies.

espinafre · 2020-04-16T04:03:54Z

I've added a FeatureSourceTestCase subclass to the test cases, as instructed by @nyalldawson , with the getSource/getEditableLayer implemented. That's why it took so long, and it was good too, because I caught an omission that I corrected in my last "real" commit ( 742454b ).

I've noted on the method overrides (with TODO annotations) where the code diverges from the original classes, and why, in order to correct those issues in the future.

All those tests pass locally, on my machine, with PostgreSQL version 12.1 and LANG=en_US.UTF-8.

Is this mergeable?

espinafre · 2020-04-16T17:12:37Z

I think @nyalldawson should take a look at the Python test ( d4672b0 ). Also, this should be backportable to 3.12 and 3.10 without issues. The github interface doesn't allow me to add @nyalldawson to the list of reviewers. @m-kuhn , can you do that? Thank you very much.

elpaso · 2020-04-17T06:32:47Z

@espinafre we don't usually assign anything to anyone, unless we know for sure that the developer agreed.

m-kuhn

It looks good to me as far as I can tell. It's not trivial to understand each change (but that's in the nature of this code, not related to your work), so I'll trust the tests and general good look.

Some minor code style requests and we are good to ship it in my opinion.

I would like to wait for one or two months with a backport to LTR to have some time for any eventual regression to be sorted out first.

src/providers/postgres/qgspostgresprovider.cpp

src/providers/postgres/qgspostgresfeatureiterator.cpp

src/providers/postgres/qgspostgresprovider.cpp

espinafre · 2020-04-17T15:38:23Z

Commit 9607570 addresses and incorporates all suggestions by @m-kuhn . Local tests still pass. Is backporting to 3.12 automatic?

m-kuhn · 2020-04-17T15:46:02Z

Commit 9607570 addresses and incorporates all suggestions by @m-kuhn . Local tests still pass. Is backporting to 3.12 automatic?

👍

Now it is.

espinafre · 2020-04-17T16:23:57Z

@espinafre we don't usually assign anything to anyone, unless we know for sure that the developer agreed.

@elpaso I've said "assigned to me" in jest, along the lines of "the teacher assigned homework to the students" :)

espinafre · 2020-04-17T18:14:57Z

@m-kuhn Does the MXE build failure prevent the merge?

elpaso · 2020-04-17T18:21:20Z

NoOn Apr 17, 2020 20:15, "José de Paula Rodrigues N. Assis" <notifications@github.com> wrote: @m-kuhn Does the MXE build failure prevent the merge? —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.

m-kuhn · 2020-04-17T18:23:56Z

Thanks @espinafre nice work!

roya0045 · 2020-04-17T18:28:52Z

@espinafre don't worry about MXE, the whole service seems to be having issues today.

github-actions bot added this to the 3.14.0 milestone Mar 18, 2020

espinafre mentioned this pull request Mar 18, 2020

Fix pg bigint pk not text #34780

Closed

m-kuhn approved these changes Mar 18, 2020

View reviewed changes

src/providers/postgres/qgspostgresconn.cpp Show resolved Hide resolved

tests/src/python/test_provider_postgres.py Outdated Show resolved Hide resolved

tests/src/python/test_provider_postgres.py Outdated Show resolved Hide resolved

espinafre force-pushed the pg_bigint_pk_no_cast branch from 4a0db47 to b24798d Compare March 18, 2020 17:40

espinafre force-pushed the pg_bigint_pk_no_cast branch from 6767395 to 36b7be5 Compare March 18, 2020 22:16

m-kuhn closed this Mar 21, 2020

m-kuhn reopened this Mar 21, 2020

espinafre added 8 commits April 16, 2020 00:50

Improved tests, more in line with QGIS codebase.

47ea6bd

New attempt to pacify Travis (source code formatting).

2fae4c4

More formatting.

5f5e39f

More feature tests for bigint PostgreSQL pks.

690e073

Formatting; mistake on pk field of TestPyQgsPostgresProviderBiginNonf…

a6696b5

…irstSinglePk

Handling of multiple bigint PKs in whereClause.

742454b

Src formatting. "I shall run prepare-commit.sh before git add."

1a4a1f0

espinafre force-pushed the pg_bigint_pk_no_cast branch from 7ba1a03 to d4672b0 Compare April 16, 2020 03:51

espinafre requested a review from m-kuhn April 16, 2020 17:08

m-kuhn approved these changes Apr 17, 2020

View reviewed changes

Adjustments following code review.

9607570

m-kuhn added the backport release-3_12 label Apr 17, 2020

m-kuhn merged commit f48e1c8 into qgis:master Apr 17, 2020

qgis-bot mentioned this pull request Apr 17, 2020

[Backport release-3_12] PostgreSQL provider don't cast bigint PKs to text #35832

Merged

espinafre mentioned this pull request Jun 10, 2020

Tests to check correctness of WHERE clauses for PostgreSQL #37115

Merged

PostgreSQL provider don't cast bigint PKs to text #35162

PostgreSQL provider don't cast bigint PKs to text #35162

Conversation

espinafre commented Mar 18, 2020 • edited

m-kuhn left a comment

Choose a reason for hiding this comment

elpaso commented Mar 18, 2020

espinafre commented Mar 18, 2020

m-kuhn commented Mar 18, 2020

espinafre commented Mar 18, 2020

espinafre commented Mar 18, 2020

m-kuhn commented Mar 18, 2020

espinafre commented Mar 18, 2020 • edited

m-kuhn commented Mar 18, 2020

espinafre commented Mar 18, 2020

espinafre commented Mar 18, 2020

espinafre commented Mar 18, 2020

espinafre commented Mar 18, 2020

espinafre commented Mar 18, 2020

nyalldawson commented Mar 18, 2020

espinafre commented Mar 18, 2020

nyalldawson commented Mar 18, 2020

nyalldawson commented Mar 18, 2020

espinafre commented Mar 18, 2020

espinafre commented Mar 18, 2020

nyalldawson commented Mar 19, 2020

nyalldawson commented Mar 19, 2020

elpaso commented Mar 20, 2020

roya0045 commented Mar 20, 2020

espinafre commented Apr 16, 2020

espinafre commented Apr 16, 2020 • edited

elpaso commented Apr 17, 2020

m-kuhn left a comment

Choose a reason for hiding this comment

espinafre commented Apr 17, 2020

m-kuhn commented Apr 17, 2020

espinafre commented Apr 17, 2020 • edited

espinafre commented Apr 17, 2020

elpaso commented Apr 17, 2020 via email

m-kuhn commented Apr 17, 2020

roya0045 commented Apr 17, 2020

espinafre commented Mar 18, 2020 •

edited

espinafre commented Mar 18, 2020 •

edited

espinafre commented Apr 16, 2020 •

edited

espinafre commented Apr 17, 2020 •

edited