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

Primary key issue when copying a table by Drag&Drop #23163

Closed
qgib opened this issue Jul 6, 2016 · 19 comments
Closed

Primary key issue when copying a table by Drag&Drop #23163

qgib opened this issue Jul 6, 2016 · 19 comments
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! DB Manager Relating to the DB Manager core plugin
Milestone

Comments

@qgib
Copy link
Contributor

qgib commented Jul 6, 2016

Author Name: Reinhard Reiterer (Reinhard Reiterer)
Original Redmine Issue: 15226
Affected QGIS version: 2.16.3
Redmine category:db_manager
Assignee: Sandro Santilli


When copying a table (PostGIS) in the 'DB Manager' by D&D, the name of the primary key is placed in quotes (e.g. "gid"). As a result, there is a "gid" and a gid column in the new table.


@qgib
Copy link
Contributor Author

qgib commented Jul 7, 2016

Author Name: Reinhard Reiterer (Reinhard Reiterer)


  • 10144 was configured as 15226.png

@qgib
Copy link
Contributor Author

qgib commented Jul 7, 2016

Author Name: Reinhard Reiterer (Reinhard Reiterer)


The background shows the input table (see 15226.png).

More D&D issues:

#15946 (when copying a table by d&d from one schema to another the serial property is lost in the pk)
#22673 (DB-Manager DEFAULT value d&d issue)

@qgib
Copy link
Contributor Author

qgib commented Oct 12, 2016

Author Name: Sandro Santilli (@strk)


Reinhard can you help me reproduce the issue ? Where should I drag the table from ? Where should I drag it to ? Which versions did you test ? Can you provide SQL to initialize such example test table ?

@qgib
Copy link
Contributor Author

qgib commented Oct 12, 2016

Author Name: Sandro Santilli (@strk)


I've tried dragging the qgis_test.base_table_good table (part of testsuite in qgis_test database) into a new schema and it worked fine with 2.16 branch (2.16.3), 2.14 branch (2.14.7) and master_2 branch (2.17).


  • status_id was changed from Open to Feedback
  • assigned_to_id was configured as Sandro Santilli

@qgib
Copy link
Contributor Author

qgib commented Oct 12, 2016

Author Name: Reinhard Reiterer (Reinhard Reiterer)


Hi Sandro, I've uploaded a screencast. Unfortunately, I've no idea how to reproduce this issue.


  • 10462 was configured as 15226_screencast.wmv

@qgib
Copy link
Contributor Author

qgib commented Oct 12, 2016

Author Name: Sandro Santilli (@strk)


Interesting, can you inspect the two tables (the one which works and the one which doesn't) with the "psql" command-line database tool ? Can it be the "gid" field is really encoded differently ? Or, can you reproduce after dumping and reloading that table ? Can you spot any other difference between the two tables ? Does it always happen on that table, no matter the order in which you copy them ?

@qgib
Copy link
Contributor Author

qgib commented Oct 12, 2016

Author Name: Reinhard Reiterer (Reinhard Reiterer)


Here comes a sample database. Please let me know if this is helpful for locating the issue.


  • 10463 was configured as 15226_screencast_2.mp4
  • 10464 was configured as qgis_test.backup

@qgib
Copy link
Contributor Author

qgib commented Oct 12, 2016

Author Name: Sandro Santilli (@strk)


Yes, I can reproduce with the table you provided. And not with other tables. I'm on it.
2.14.7 is not affected, 2.16.3 and 2.17 are.


  • version was changed from master to 2.16.3
  • fixed_version_id was configured as Version 2.18
  • status_id was changed from Feedback to In Progress

@qgib
Copy link
Contributor Author

qgib commented Oct 12, 2016

Author Name: Sandro Santilli (@strk)


Views are affected, while real table aren't (or so it looks).
The bug was introduced by the commit fixing #21737 (daa6510)

\cc Jurgen

@qgib
Copy link
Contributor Author

qgib commented Oct 13, 2016

Author Name: Sandro Santilli (@strk)


Jurgen, once again this issue is about properly defining semantic for some fields.
In this case is the "keyColumn()" return from QgsDatasourceUri.
Is that supposed to contain quoted or unquoted names ?

@qgib
Copy link
Contributor Author

qgib commented Oct 13, 2016

Author Name: Jürgen Fischer (@jef-n)


Sandro Santilli wrote:

Jurgen, once again this issue is about properly defining semantic for some fields.
In this case is the "keyColumn()" return from QgsDatasourceUri.
Is that supposed to contain quoted or unquoted names ?

the postgres provider handles quoted and unquoted content from keyColumn() - dbmanager should do too.

@qgib
Copy link
Contributor Author

qgib commented Oct 13, 2016

Author Name: Sandro Santilli (@strk)


On further research, I found that we get quoted identifiers in the url when the primary key type is a FidMap. This happens for multi-column keys or for non-integer keys. In both these cases, even with QGIS 2.14, we get quoted attributes in the QgsDatasourceUri keyColumn() return.

In all these cases, the DBManager drag & drop operation results in a target table having quotes in the key column names.
Even in QGIS 2.14.

I'll see how DBManager should deal with quotes, but given the above I guess the only sane way to deal with quotes would be knowing if the quotes were added or were part of the actual field name, so somehow the "FidMap" nature should be exposed.
or we should always quote the columns ?

BTW, I'm not sure it's DBManager or the provider's "CreateEmptyLayer" being responsible of parsing the URL.
Will keep looking.

@qgib
Copy link
Contributor Author

qgib commented Oct 13, 2016

Author Name: Sandro Santilli (@strk)


I confirm it should be createEmptyTable, within the Postgres provider, to interpret the URI.
Hopefully the semantic of that "key" parameter of the URI will only be needed from within the provider

@qgib
Copy link
Contributor Author

qgib commented Oct 13, 2016

Author Name: Sandro Santilli (@strk)


So with PR #3599 I've made PostgreSQL layer importer (createEmptyLayer) support quoted identifiers in Datasource URI -- for backward compatibility with projects created #21737 was fixed (that ticket lacks a target version) -- and I've reverted the commit which did add quoted identifiers in Datasource URI as they do seem redundant and inconsistent (I did test that #21737 isn't back).

In its current state, the code in the PR not only fixes this bug but also adds support for Drag&Dropp'ing tables with multi-column keys (including those with mixed-cased-colum names)

@qgib
Copy link
Contributor Author

qgib commented Oct 13, 2016

Author Name: Sandro Santilli (@strk)


  • done_ratio was changed from 0 to 90

@qgib
Copy link
Contributor Author

qgib commented Oct 13, 2016

Author Name: Reinhard Reiterer (Reinhard Reiterer)


Sandro, it's great to see this bug fixed.

Sandro Santilli wrote:

Views are affected, while real table aren't (or so it looks).

As seen in screencast '15226_screencast.wmv' copying tables by D&D may also fail. Is there anything else to do?

@qgib
Copy link
Contributor Author

qgib commented Oct 13, 2016

Author Name: Sandro Santilli (@strk)


Fixed in changeset "5abdfcb8064f3b3a848358afb95723b932dd3b05".


  • status_id was changed from In Progress to Closed

@qgib
Copy link
Contributor Author

qgib commented Oct 13, 2016

Author Name: Sandro Santilli (@strk)


To the best of my knowledge copying tables by D&D also succeeds now. If you can test, the working code is in master_2 branch, since dd85796 - reopen if you still see a problem with Drag&Drop and primary key.

@qgib
Copy link
Contributor Author

qgib commented Oct 13, 2016

Author Name: Sandro Santilli (@strk)


  • resolution was changed from to fixed/implemented
  • done_ratio was changed from 90 to 100

@qgib qgib added Bug Either a bug report, or a bug fix. Let's hope for the latter! DB Manager Relating to the DB Manager core plugin labels May 25, 2019
@qgib qgib added this to the Version 2.18 milestone May 25, 2019
@qgib qgib closed this as completed May 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! DB Manager Relating to the DB Manager core plugin
Projects
None yet
Development

No branches or pull requests

1 participant