Fix featureCount on postgres view when flag estimatedmetadata is set

[troopa81]

We can't estimate rows on postgres view.

Checklist

• Commit messages are descriptive and explain the rationale for changes
• Commits which fix bugs include Fixes #11111 at the bottom of the commit message
• I have read the QGIS Coding Standards and this PR complies with them
• New unit tests have been added for core changes
• I have run the scripts/prepare-commit.sh script before each commit
• I have evaluated whether it is appropriate for this PR to be backported, backport requests are left as label or comment

[troopa81]

Not sure about the backport 3.4 though. It's quite an easy fix, so I would say yes

[nyalldawson]

What's the consequence of the bug? Only fixes for crashes or data corruption should be backported now.

[troopa81]

What's the consequence of the bug?

If you choose the option "Use estimated table metadata" in PostGIS connexion window and you want to add a view, the feature count will be always 0. It doesn't crash nor corrupt data. So, I remove the backport label.

[jef-n]

Doesn't this make loading expensive views painfully slow?

[strk]

It surely does. I suggest to use EXPLAIN and parse its output, for estimating number of rows contained in a query.

[strk]

EXPLAIN could actually be also used for queries, dropping another performance bottleneck when user asked for "estimated" metadata...

[strk]

I think EXPLAIN should be used for the estimation

[troopa81]

It surely does. I suggest to use EXPLAIN and parse its output, for estimating number of rows contained in a query.

Isn't it dangerous to rely on EXPLAIN output to estimate the number of rows? How can we be sure that the output is not gonna change in the future, that someday rows will be changed in number_of_rows ?

[strk]

Isn't it dangerous to rely on EXPLAIN output to estimate the number of rows? How can we be sure that the output is not gonna change in the future, that someday rows will be changed in number_of_rows ?

Explain has XML output support since 9.0 (https://www.postgresql.org/docs/9.0/sql-explain.html).
Shall format change in future we'll deal with it. I don't see it that dangerous.

[troopa81]

Explain has XML output support since 9.0 (https://www.postgresql.org/docs/9.0/sql-explain.html).

Thanks for the tip, I was not aware about explain formats. I choose JSON over XML, because it seems to me easy to write 7352e3f .

[jef-n]

should be guarded by connectionRO()->pgVersion() >= 90200 as other occurrences of json. And the featureCount is unreliable with "estimated metadata" anyway and widely used to avoid expensive queries. I think we should just return -1 instead of 0, if there's no shortcut like EXPLAIN.

[troopa81]

should be guarded by connectionRO()->pgVersion() >= 90200

OK, I'll add it (with 90000, this feature was already available in 9.0 )

I think we should just return -1 instead of 0, if there's no shortcut like EXPLAIN

And displaying -1 in the UI? or "Unknown"?

[troopa81]

I let the feature count display as it is, so if the estimate doesn't work, it will display -1

[troopa81]

@strk is it OK for requested changes?

[strk]

Thanks, looks fine here

[haubourg]

Anyone to merge this ?

[haubourg]

thanks @elpaso !

[Gustry]

@troopa81 I understand why you made this change for views, it was indeed correct.
But why you didn't keep the previous behavior for tables ?

The Plan Rows makes quite different results and is not following the VACUUM ANALYSE that people might run on their database.

Look this example:

EXPLAIN (FORMAT JSON) SELECT 1 FROM pgmetadata.contact
-- QUERY PLAN [{'Plan': {'Node Type': 'Seq Scan', 'Parallel Aware': False, 'Relation Name': 'contact', 'Alias': 'contact', 'Startup Cost': 0.0, 'Total Cost': 15.1, 'Plan Rows': 510, 'Plan Width': 4}}]
-- > 'Plan Rows': 510

VACUUM ANALYSE pgmetadata.contact;
SELECT reltuples as approximate_row_count  FROM pg_class WHERE oid = 'pgmetadata.contact'::regclass;
-- 0.0

SELECT COUNT(*) FROM pgmetadata.contact;
-- 0

EXPLAIN (FORMAT JSON) SELECT 1 FROM pgmetadata.contact;
-- Still 510

[troopa81]

@Gustry But why you didn't keep the previous behavior for tables ?

Because it would cost an extra request to check if mQuery is a view or a table (calling relkind)

But, I reproduce your issue when you remove all your data from your table. I'll try to find some time to fix this.

[Gustry]

Because it would cost an extra request to check if mQuery is a view or a table

Hum, I though indeed we would have this info already in the context.

But, I reproduce your issue when you remove all your data from your table.

My tables are indeed neatly all empty for now.

But I tried with a simple line, it's still wrong.

SELECT COUNT(*) FROM pgmetadata.contact;
-- 1

EXPLAIN (FORMAT JSON) SELECT 1 FROM pgmetadata.contact;
-- Still 510

Should I create a ticket ?

[troopa81]

Hum, I though indeed we would have this info already in the context.

It could be, I don't see any reason to retrieve this information several times

But I tried with a simple line, it's still wrong.

My guess here is that the Plan rows approach is more approximate than the reltuples one. I tried with a table containing 3+06 rows and removed all rows. The reltuples did say 0 while the Plan Rows says 300. Both have changed and both are kind of true for an estimation. But the reltuples is more true :)

Should I create a ticket ?

Yes, please