Copying table by Drag&Drop in Browser (2) doesn't copy the table structure correctly

[autra]

What is the bug or the crash?

When using drag and drop to copy a table to another schema in a PostGIS database, the structure of the table is incorrectly copied. There are issues with:

• sequences
• PKs (new syntax)
• other constraints

Steps to reproduce the issue

1. Have a postgis connexion
2. create this table

create table gis_data(
 id bigint generated always as identity primary key, 
  geom geometry(Polygon, 3857) check (st_
isvalid(geom)), 
  name text unique, author text not null
);

3. observe the structure from psql with \d gis_data

                               Table "public.gis_data"
 Column │          Type          │ Collation │ Nullable │           Default
════════╪════════════════════════╪═══════════╪══════════╪══════════════════════════════
 id     │ bigint                 │           │ not null │ generated always as identity
 geom   │ geometry(Polygon,3857) │           │          │
 name   │ text                   │           │          │
 author │ text                   │           │ not null │
Indexes:
    "git_data2_pkey" PRIMARY KEY, btree (id)
    "git_data2_name_key" UNIQUE CONSTRAINT, btree (name)
Check constraints:
    "git_data2_geom_check" CHECK (st_isvalid(geom))

4. create a new postgis schema (from qgis or from psql) create schema test;
5. drag & drop the table to the new schema test
6. observe the new table structure \d test.gis_data

                                        Table "test.gis_data"
 Column │          Type          │ Collation │ Nullable │                   Default
════════╪════════════════════════╪═══════════╪══════════╪══════════════════════════════════════════════
 id_0   │ integer                │           │ not null │ nextval('test.gis_data_id_0_seq'::regclass)
 geom   │ geometry(Polygon,3857) │           │          │
 id     │ bigint                 │           │          │
 name   │ character varying      │           │          │
 author │ character varying      │           │          │
Indexes:
    "gis_data_pkey" PRIMARY KEY, btree (id_0)

7. observe the differences:

• qgis didn't pick the new way of defining auto-incrementing id (generated always as identity) and created a new PK. I've also tested with serial, and while QGis did detect the PK in this case, it also fails to generate a new sequence for the new table, meaning the old and the new PKS are sharing a sequence (this will bring trouble)
• qgis completely failed to copy other constraints, including not null, unique and check.

Versions

QGIS version	3.20.3-Odense	QGIS code revision	495fbae
Qt version	5.12.8
Python version	3.8.10
GDAL/OGR version	3.0.4
PROJ version	6.3.1
EPSG Registry database version	v9.8.6 (2020-01-22)
Compiled against GEOS	3.8.0-CAPI-1.13.1	Running against GEOS	3.8.0-CAPI-1.13.1
SQLite version	3.31.1
PDAL version	2.0.1
PostgreSQL client version	12.8 (Ubuntu 12.8-0ubuntu0.20.04.1)
SpatiaLite version	4.3.0a
QWT version	6.1.4
QScintilla2 version	2.11.2
OS version	Ubuntu 20.04.3 LTS
Active Python plugins	SRTM-Downloaderpgversionqfieldsyncpg-history-viewerqgis2webplugin_reloaderqgis_versioninglayertilesmapcanvasprocessingdb_managerMetaSearch

Supported QGIS version

• I'm running a supported QGIS version according to the roadmap.

New profile

• I tried with a new QGIS profile

Additional context

I haven't tested triggers, but it's not clear to me what's the correct behaviour for them. I'd say it's probably better not to copy it, to be on the safe side of things. I'd argue copying any other constraints should be done though.

Thanks !

[elpaso]

I agree that this would be nice to have but I think we need to aim to a partial fix (or feature request, I'm not sure about what category this would fall into), and here is why:

The current implementation of the exporter is provider-agnostic, the method receives an URI that defines the source of the export (which can be anything: a json layer, a memory layer, a DB layer etc.), some information about the original source is lost (because QGIS does not store it anywhere).

What we know at that point are the fields (including some constraints like UNIQUE and NOT NULL but not much more than that) and the original QGIS layer URI (that does not include PK information because it comes from the browser).

In fact, if the drag comes from the browser, we don't have a layer instance like when the drag is initiated from the TOC/legend, this means that we do not have any information regarding the PK(s) in that case what the exporter tries to do is to create a PK named id, if that name is already take, a id_0, id_1 etc. is attempted.

A possible enhancement would be to be smarter about the presence of numeric UNIQUE/NOT NULL field ant take it as a PK candidate, this would work in simple cases and would probably partially fix this issue, it would fail in case of composite PKs.

[elpaso]

The more I think about it the more I think this is more a feature request than a bug and that the main misunderstanding is about copy vs export: the export functionality was not designed to make a precise copy but to export data from a source to another.

That said, I agree that the layer creation for postgis layers might really be smarter and implement some PK guessing logic.

[autra]

That said, I agree that the layer creation for postgis layers might really be smarter and implement some PK guessing logic.

I'd say we need to be cautious here, if you don't want me to open a bug some time later that'd say "QGIS mistakenly takes my unique constraint as a PK" 😆 My take as a user: I prefer the current behaviour than a PK guess that fails even only 5% of the case. The current PK guess on id is ok I think. That being said, I don't see any obvious problem with your commit referenced above (if no PK, first non-null unique field is the pk).

The more I think about it the more I think this is more a feature request than a bug and that the main misunderstanding is about copy vs export

This makes sense, except for the fact QGIS fails to create a new sequence for the new id (I guess because one with the same name already exists). I really think it should not do that (and that it's a bug). What do you think?

Anyway, from a user POV, the distinction between copy and export is not very. I don't have any suggestion apart from trying to make copy and export the same thing wherever possible, but maybe there could be hints in the UI (not sure how to do that though)?

So yes, think it'd be nice if the datasource informations would somehow be kept, to be able to do more clever things if the datasource type is the same between source and destination for instance. I have absolutely no idea of the implication in the code though 😄

Anyway thanks for the explanation. It makes sense and helped me to better understand the logic.