Thread: [HACKERS] Allowing extended stats on foreign and partitioned tables
While reviewing extended stats I noticed that it was possible to create extended stats on many object types, including sequences. I mentioned that this should be disallowed. Statistics were then changed to be only allowed on plain tables and materialized views. This should be relaxed again to allow anything ANALYZE is allowed on. The attached does this. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
This isn't exactly about this particular thread. But I noticed, that after we introduced RELKIND_PARTITIONED_TABLE, we required to change a number of conditions to include this relkind. We missed some places in initial commits and fixed those later. I am wondering whether we should creates macros clubbing relevant relkinds together based on the purpose of the tests e.g. IS_RELKIND_HAS_STORAGE(). When a new relkind is added, one can examine these macros to check whether the new relkind fits in the given macro. If all those macros are placed together, there is a high chance that we will not miss any place in the initial commit itself. For example, if we had a macro IS_RELKIND_WITH_STATS defined as #define IS_RELKIND_WITH_STATS(relkind) \ ((relkind) == RELKIND_RELATION || \ (relkind) == RELKIND_MATVIEW) and CreateStatistics() and getExtendedStatistics() had following conditions if (!IS_RELKIND_WITH_STATS(rel->rd_rel->relkind)) and if (!IS_RELKIND_WITH_STATS(tabinfo->relkind)) resp. The patch would be just adding (relkind) == RELKIND_FOREIGN_TABLE || \ (relkind) == RELKIND_PARTITIONED_TABLE) to that macro without requiring to find out where all we need to add those two relkinds for statistics purposes. On Mon, Apr 10, 2017 at 3:09 PM, David Rowley <david.rowley@2ndquadrant.com> wrote: > While reviewing extended stats I noticed that it was possible to > create extended stats on many object types, including sequences. I > mentioned that this should be disallowed. Statistics were then changed > to be only allowed on plain tables and materialized views. > > This should be relaxed again to allow anything ANALYZE is allowed on. > > The attached does this. > > -- > David Rowley http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On Mon, Apr 10, 2017 at 09:39:22PM +1200, David Rowley wrote: > While reviewing extended stats I noticed that it was possible to > create extended stats on many object types, including sequences. I > mentioned that this should be disallowed. Statistics were then changed > to be only allowed on plain tables and materialized views. > > This should be relaxed again to allow anything ANALYZE is allowed on. > > The attached does this. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Alvaro, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
Noah Misch wrote: > On Mon, Apr 10, 2017 at 09:39:22PM +1200, David Rowley wrote: > > While reviewing extended stats I noticed that it was possible to > > create extended stats on many object types, including sequences. I > > mentioned that this should be disallowed. Statistics were then changed > > to be only allowed on plain tables and materialized views. > > > > This should be relaxed again to allow anything ANALYZE is allowed on. > > > > The attached does this. > > [Action required within three days. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 10 open item. Alvaro, > since you committed the patch believed to have created it, you own this open > item. I'm going to review David's patch and post a detailed review on Wednesday 19th. I think the patch is reasonable, but it's lacking new tests, and I'm worried that the error message changes do not affect the existing tests in the area. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 4/10/17 06:18, Ashutosh Bapat wrote: > This isn't exactly about this particular thread. But I noticed, that > after we introduced RELKIND_PARTITIONED_TABLE, we required to change a > number of conditions to include this relkind. We missed some places in > initial commits and fixed those later. I am wondering whether we > should creates macros clubbing relevant relkinds together based on the > purpose of the tests e.g. IS_RELKIND_HAS_STORAGE(). When a new relkind > is added, one can examine these macros to check whether the new > relkind fits in the given macro. If all those macros are placed > together, there is a high chance that we will not miss any place in > the initial commit itself. I think this is worth a try. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Apr 15, 2017 at 5:27 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 4/10/17 06:18, Ashutosh Bapat wrote: >> This isn't exactly about this particular thread. But I noticed, that >> after we introduced RELKIND_PARTITIONED_TABLE, we required to change a >> number of conditions to include this relkind. We missed some places in >> initial commits and fixed those later. I am wondering whether we >> should creates macros clubbing relevant relkinds together based on the >> purpose of the tests e.g. IS_RELKIND_HAS_STORAGE(). When a new relkind >> is added, one can examine these macros to check whether the new >> relkind fits in the given macro. If all those macros are placed >> together, there is a high chance that we will not miss any place in >> the initial commit itself. > > I think this is worth a try. Thanks, will try to come up with something ASAP. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
David Rowley wrote: > While reviewing extended stats I noticed that it was possible to > create extended stats on many object types, including sequences. I > mentioned that this should be disallowed. Statistics were then changed > to be only allowed on plain tables and materialized views. > > This should be relaxed again to allow anything ANALYZE is allowed on. > > The attached does this. The error message was inconsistent in the case of indexes, because of using heap_open instead of relation_open. That seemed gratuitous, so I flipped it, added test for the whole thing and pushed. Thanks for reporting. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 18 April 2017 at 09:01, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > David Rowley wrote: >> While reviewing extended stats I noticed that it was possible to >> create extended stats on many object types, including sequences. I >> mentioned that this should be disallowed. Statistics were then changed >> to be only allowed on plain tables and materialized views. >> >> This should be relaxed again to allow anything ANALYZE is allowed on. >> >> The attached does this. > > The error message was inconsistent in the case of indexes, because of > using heap_open instead of relation_open. That seemed gratuitous, so I > flipped it, added test for the whole thing and pushed. Thanks for changing that and pushing this. -- David Rowley http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services