Horton

Thread

Commits

  1. Tighten up relation kind checks for extended statistics

  1. Allowing extended stats on foreign and partitioned tables

    David Rowley <david.rowley@2ndquadrant.com> — 2017-04-10T09:39:22Z

    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

  2. Re: Allowing extended stats on foreign and partitioned tables

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-10T10:18:42Z

    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

  3. Re: Allowing extended stats on foreign and partitioned tables

    Noah Misch <noah@leadboat.com> — 2017-04-14T05:53:10Z

    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

  4. Re: Allowing extended stats on foreign and partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-04-14T12:40:26Z

    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

  5. Re: Allowing extended stats on foreign and partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-04-14T23:57:30Z

    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

  6. Re: Allowing extended stats on foreign and partitioned tables

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-17T05:04:16Z

    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

  7. Re: Allowing extended stats on foreign and partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-04-17T21:01:29Z

    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

  8. Re: Allowing extended stats on foreign and partitioned tables

    David Rowley <david.rowley@2ndquadrant.com> — 2017-04-17T21:42:51Z

    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