Thread
Commits
-
Fix definition of pg_statio_all_tables view
- ef11051bbe96 13.0 landed
-
Fix for pg_statio_all_tables
Alexander Korotkov <a.korotkov@postgrespro.ru> — 2020-04-20T23:44:45Z
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 -
Re: Fix for pg_statio_all_tables
Michael Paquier <michael@paquier.xyz> — 2020-04-21T01:38:32Z
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
-
Re: Fix for pg_statio_all_tables
Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-21T04:58:20Z
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
-
Re: Fix for pg_statio_all_tables
Alexander Korotkov <a.korotkov@postgrespro.ru> — 2020-04-21T10:17:40Z
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
-
Re: Fix for pg_statio_all_tables
Alexander Korotkov <a.korotkov@postgrespro.ru> — 2020-04-21T10:19:56Z
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
-
Re: Fix for pg_statio_all_tables
Tom Lane <tgl@sss.pgh.pa.us> — 2020-04-21T13:59:07Z
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
-
Re: Fix for pg_statio_all_tables
Alexander Korotkov <a.korotkov@postgrespro.ru> — 2020-04-22T14:23:57Z
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