Thread: Fix for pg_statio_all_tables
Hi!
It appears that definition of pg_statio_all_tables has bug.
CREATE VIEW pg_statio_all_tables AS
SELECT
C.oid AS relid,
N.nspname AS schemaname,
C.relname AS relname,
pg_stat_get_blocks_fetched(C.oid) -
pg_stat_get_blocks_hit(C.oid) AS heap_blks_read,
pg_stat_get_blocks_hit(C.oid) AS heap_blks_hit,
sum(pg_stat_get_blocks_fetched(I.indexrelid) -
pg_stat_get_blocks_hit(I.indexrelid))::bigint AS
idx_blks_read,
sum(pg_stat_get_blocks_hit(I.indexrelid))::bigint AS idx_blks_hit,
pg_stat_get_blocks_fetched(T.oid) -
pg_stat_get_blocks_hit(T.oid) AS toast_blks_read,
pg_stat_get_blocks_hit(T.oid) AS toast_blks_hit,
sum(pg_stat_get_blocks_fetched(X.indexrelid) -
pg_stat_get_blocks_hit(X.indexrelid))::bigint AS
tidx_blks_read,
sum(pg_stat_get_blocks_hit(X.indexrelid))::bigint AS tidx_blks_hit
FROM pg_class C LEFT JOIN
pg_index I ON C.oid = I.indrelid LEFT JOIN
pg_class T ON C.reltoastrelid = T.oid LEFT JOIN
pg_index X ON T.oid = X.indrelid
LEFT JOIN pg_namespace N ON (N.oid = C.relnamespace)
WHERE C.relkind IN ('r', 't', 'm')
GROUP BY C.oid, N.nspname, C.relname, T.oid, X.indrelid;
Among all the joined tables, only "pg_index I" is expected to have
multiple rows associated with single relation. But we do sum() for
toast index "pg_index X" as well. As the result, we multiply
statistics for toast index by the number of relation indexes. This is
obviously wrong.
Attached patch fixes the view definition to count toast index statistics once.
As a bugfix, I think this should be backpatched. But this patch
requires catalog change. Were similar cases there before? If so,
how did we resolve them?
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachment
On Tue, Apr 21, 2020 at 02:44:45AM +0300, Alexander Korotkov wrote: > Among all the joined tables, only "pg_index I" is expected to have > multiple rows associated with single relation. But we do sum() for > toast index "pg_index X" as well. As the result, we multiply > statistics for toast index by the number of relation indexes. This is > obviously wrong. Oops. > As a bugfix, I think this should be backpatched. But this patch > requires catalog change. Were similar cases there before? If so, > how did we resolve them? A backpatch can happen in such cases, see for example b6e39ca9. In this case, the resolution was done with a backpatch to system_views.sql and the release notes include an additional note saying that the fix applies itself only on already-initialized clusters. For other clusters, it was necessary to apply a SQL query, given also in the release notes, to fix the issue (just grep for CVE-2017-7547 in release-9.6.sgml on the REL9_6_STABLE branch). -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Apr 21, 2020 at 02:44:45AM +0300, Alexander Korotkov wrote:
>> As a bugfix, I think this should be backpatched. But this patch
>> requires catalog change. Were similar cases there before? If so,
>> how did we resolve them?
> A backpatch can happen in such cases, see for example b6e39ca9. In
> this case, the resolution was done with a backpatch to
> system_views.sql and the release notes include an additional note
> saying that the fix applies itself only on already-initialized
> clusters. For other clusters, it was necessary to apply a SQL query,
> given also in the release notes, to fix the issue (just grep for
> CVE-2017-7547 in release-9.6.sgml on the REL9_6_STABLE branch).
Yeah, but that was for a security hole. I am doubtful that the
severity of this problem is bad enough to justify jumping through
similar hoops. Even if we fixed it and documented it, how many
users would bother to apply the manual correction?
regards, tom lane
On Tue, Apr 21, 2020 at 4:38 AM Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Apr 21, 2020 at 02:44:45AM +0300, Alexander Korotkov wrote: > > Among all the joined tables, only "pg_index I" is expected to have > > multiple rows associated with single relation. But we do sum() for > > toast index "pg_index X" as well. As the result, we multiply > > statistics for toast index by the number of relation indexes. This is > > obviously wrong. > > Oops. > > > As a bugfix, I think this should be backpatched. But this patch > > requires catalog change. Were similar cases there before? If so, > > how did we resolve them? > > A backpatch can happen in such cases, see for example b6e39ca9. In > this case, the resolution was done with a backpatch to > system_views.sql and the release notes include an additional note > saying that the fix applies itself only on already-initialized > clusters. For other clusters, it was necessary to apply a SQL query, > given also in the release notes, to fix the issue (just grep for > CVE-2017-7547 in release-9.6.sgml on the REL9_6_STABLE branch). Thank you for pointing! ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
On Tue, Apr 21, 2020 at 7:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Michael Paquier <michael@paquier.xyz> writes: > > On Tue, Apr 21, 2020 at 02:44:45AM +0300, Alexander Korotkov wrote: > >> As a bugfix, I think this should be backpatched. But this patch > >> requires catalog change. Were similar cases there before? If so, > >> how did we resolve them? > > > A backpatch can happen in such cases, see for example b6e39ca9. In > > this case, the resolution was done with a backpatch to > > system_views.sql and the release notes include an additional note > > saying that the fix applies itself only on already-initialized > > clusters. For other clusters, it was necessary to apply a SQL query, > > given also in the release notes, to fix the issue (just grep for > > CVE-2017-7547 in release-9.6.sgml on the REL9_6_STABLE branch). > > Yeah, but that was for a security hole. I am doubtful that the > severity of this problem is bad enough to justify jumping through > similar hoops. Even if we fixed it and documented it, how many > users would bother to apply the manual correction? Sure, only most conscious users will do the manual correction. But if there are only two option: backpatch it this way or don't backpatch at all, then I would choose the first one. ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> On Tue, Apr 21, 2020 at 7:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah, but that was for a security hole. I am doubtful that the
>> severity of this problem is bad enough to justify jumping through
>> similar hoops. Even if we fixed it and documented it, how many
>> users would bother to apply the manual correction?
> Sure, only most conscious users will do the manual correction. But if
> there are only two option: backpatch it this way or don't backpatch at
> all, then I would choose the first one.
Well, if it were something that you could just do and forget, then
maybe. But actually, you are proposing to invest a lot of *other*
people's time --- notably me, as the likely author of the next
set of release notes --- so it's not entirely up to you.
Given the lack of field complaints, I'm still of the opinion that
this isn't really worth back-patching.
regards, tom lane
On Tue, Apr 21, 2020 at 4:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alexander Korotkov <a.korotkov@postgrespro.ru> writes: > > On Tue, Apr 21, 2020 at 7:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Yeah, but that was for a security hole. I am doubtful that the > >> severity of this problem is bad enough to justify jumping through > >> similar hoops. Even if we fixed it and documented it, how many > >> users would bother to apply the manual correction? > > > Sure, only most conscious users will do the manual correction. But if > > there are only two option: backpatch it this way or don't backpatch at > > all, then I would choose the first one. > > Well, if it were something that you could just do and forget, then > maybe. But actually, you are proposing to invest a lot of *other* > people's time --- notably me, as the likely author of the next > set of release notes --- so it's not entirely up to you. Sure, this is not entirely up to me. > Given the lack of field complaints, I'm still of the opinion that > this isn't really worth back-patching. So, what exact strategy do you propose? I don't like idea to postpone decision of what we do with backbranches. We may decide not to fix it in previous releases. But in order to handle this decision correctly I think we should document this bug there. I'm OK with doing this. And I can put my efforts on fixing it in the head and backpatching the documentation. But does this save significant resources in comparison with fixing bug in backbranches? ------ Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company