Thread: [HACKERS] ALTER INDEX .. SET STATISTICS ... behaviour

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> I've discovered that PostgreSQL is able to run following kind of queries in
> order to change statistic-gathering target for an indexed expression.

> ALTER INDEX index_name ALTER COLUMN expr SET STATISTICS stat_target;

> It's been previously discussed in [1].

> I think this should be fixed not just in docs.  This is why I've started
> thread in pgsql-hackers.  For me usage of internal column names "expr",
> "expr1", "expr2" etc. looks weird.  And I think we should replace it with a
> better syntax.  What do you think about these options?

> ALTER INDEX index_name ALTER EXPRESSION 0 SET STATISTICS stat_target; --
> Refer expression by its number
> ALTER INDEX index_name ALTER EXPRESSION (x + y) SET STATISTICS stat_target;
> -- Refer expression by its definition

Don't like either of those particularly, but what about just targeting
a column by column number, independently of whether it's an expression?
ALTER INDEX myindex ALTER COLUMN 4 SET STATISTICS 1000;
        regards, tom lane

On Thu, Jun 1, 2017 at 6:40 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Wed, May 31, 2017 at 7:18 PM, Alexander Korotkov
> <a.korotkov@postgrespro.ru> wrote:
>>
>>>
>>> Don't like either of those particularly, but what about just targeting
>>> a column by column number, independently of whether it's an expression?
>>>
>>>         ALTER INDEX myindex ALTER COLUMN 4 SET STATISTICS 1000;
>>
>>
>> I completely agree with that.
>> If no objections, I will write a patch for that.
>
>
> Please, find it in attached patch.
>
> I decided to forbid referencing columns by numbers for tables, because those
> numbers could contain gaps.  Also, I forbid altering statistics target for
> non-expression index columns, because it has no effect.
>

In order to avoid losing track of this patch, I think it is better to
add it in open items list for 10.



-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit.kapila16@gmail.com> writes:
> In order to avoid losing track of this patch, I think it is better to
> add it in open items list for 10.

This is an entirely new feature, not a bug fix, and thus certainly not an
open item for v10.  Please stick it in the next commitfest, instead.
        regards, tom lane

On Sun, Jun 4, 2017 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> In order to avoid losing track of this patch, I think it is better to
>> add it in open items list for 10.
>
> This is an entirely new feature, not a bug fix, and thus certainly not an
> open item for v10.  Please stick it in the next commitfest, instead.
>

Okay, that makes sense.  Sorry, I missed the point in the first read
of the mail.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 08/30/2017 02:26 PM, Alexander Korotkov wrote:
> Patch rebased to current master is attached.

Hello,

I reviewed this patch. It works as expected but I have few remarks :

  * There is a warning during compilation :

gram.y: In function ‘base_yyparse’:
gram.y:2090:6: warning: ISO C90 forbids mixed declarations and code
[-Wdeclaration-after-statement]
      AlterTableCmd *n = makeNode(AlterTableCmd);
      ^~~~~~~~~~~~~

If I understand we should declare AlterTableCmd *n [...] before "if"?

  * I propose to add "Stats target" information in psql verbose output command
\d+ (patch attached with test)

\d+ tmp_idx
                     Index "public.tmp_idx"
 Column |       Type       | Definition | Storage | Stats target
--------+------------------+------------+---------+--------------
 a      | integer          | a          | plain   |
 expr   | double precision | (d + e)    | plain   | 1000
 b      | cstring          | b          | plain   |
btree, for table "public.tmp"


  * psql completion is missing (added to previous patch)


Regards,

--
Adrien NAYRAT

http://dalibo.com - http://dalibo.org

On 09/04/2017 06:16 PM, Alexander Korotkov wrote:
> Looks good for me.  I've integrated those changes in the patch.
> New revision is attached.

Thanks, I changed status to "ready for commiter".

--
Adrien NAYRAT

http://dalibo.com - http://dalibo.org

On 4 September 2017 at 10:30, Adrien Nayrat <adrien.nayrat@dalibo.com> wrote:
> On 09/04/2017 06:16 PM, Alexander Korotkov wrote:
>> Looks good for me.  I've integrated those changes in the patch.
>> New revision is attached.
>
> Thanks, I changed status to "ready for commiter".

This looks useful and good to me.

I've changed this line of code to return NULL rather than return tuple
if (!HeapTupleIsValid(tuple))   return tuple;

Other than that, I'm good to commit.

Any last minute objections?

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Simon Riggs <simon@2ndquadrant.com> writes:
> Other than that, I'm good to commit.
> Any last minute objections?

The docs and comments could stand a bit closer copy-editing by a
native English speaker.  Other than that, it seemed sane in a
quick read-through.
        regards, tom lane

On 6 September 2017 at 10:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
>> Other than that, I'm good to commit.
>> Any last minute objections?
>
> The docs and comments could stand a bit closer copy-editing by a
> native English speaker.  Other than that, it seemed sane in a
> quick read-through.

Will do

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services