Date: Thu, 14 Oct 2021 11:07:21 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Bossart, Nathan" <bossartn@amazon.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>,
	Vik Fearing <vik@postgresfriends.org>,
	"postgresql@zr40.nl" <postgresql@zr40.nl>,
	Alexander Korotkov <aekorotkov@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	Robert Haas <robertmhaas@gmail.com>
Subject: Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless
 opclass makes index and table unusable
Message-ID: <YWeQ2UXvXUH/Gt4T@paquier.xyz>
References: <17220-15d684c6c2171a83@postgresql.org>
 <9b91d241-4d05-e5a4-67da-5b8acdb9a2f1@postgresfriends.org>
 <E29EF33B-CA1A-4146-BFCF-C8EC08FB91A8@amazon.com>
 <YWZViGJFJkrgBKwk@paquier.xyz>
 <CAFiTN-t9JSHmCtOtB_28BmAXEantUC2=1OKV_irAPAoxnzPadA@mail.gmail.com>
 <678CF71A-934B-4FD0-BF63-E41D795E41C9@amazon.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="6jTfDcuGz5JwzWtO"
Content-Disposition: inline
In-Reply-To: <678CF71A-934B-4FD0-BF63-E41D795E41C9@amazon.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YWeQ2UXvXUH/Gt4T%40paquier.xyz>
Precedence: bulk


--6jTfDcuGz5JwzWtO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Oct 13, 2021 at 05:20:56PM +0000, Bossart, Nathan wrote:
> AFAICT the fact that these commands can succeed at all seems to be
> unintentional, and I wonder if modifying these options requires extra
> steps such as rebuilding the index.

I was looking at all this business with more attention, and this code
block is standing out in analyze.c:
/*
 * Now we can compute the statistics for the expression columns.
 */
if (numindexrows > 0)
{
    MemoryContextSwitchTo(col_context);
    for (i = 0; i < attr_cnt; i++)
    {
        VacAttrStats *stats = thisdata->vacattrstats[i];
        AttributeOpts *aopt =
        get_attribute_options(stats->attr->attrelid,
                              stats->attr->attnum);

        stats->exprvals = exprvals + i;
        stats->exprnulls = exprnulls + i;
        stats->rowstride = attr_cnt;
        stats->compute_stats(stats,
                             ind_fetch_func,
                             numindexrows,
                             totalindexrows);

        /*
         * If the n_distinct option is specified, it overrides the
         * above computation.  For indices, we always use just
         * n_distinct, not n_distinct_inherited.
         */
        if (aopt != NULL && aopt->n_distinct != 0.0)
            stats->stadistinct = aopt->n_distinct;

        MemoryContextResetAndDeleteChildren(col_context);
    }
}

When computing statistics on an index expression, this code means that
we would grab the value of n_distinct from the *index* if set and
force the stats to use it, and not use what the parent table has.  For
example, say:
create table aa (a int);
insert into aa values (generate_series(1,1000));
create index aai on aa((a+a)) where a > 500;
alter index aai alter column expr set (n_distinct = 2);
analyze aa; -- n_distinct forced to 2.0 for the index stats

This code comes from 76a47c0 back in 2010.  In PG <= 12, this would
work, but that does not as of 13~.  Enforcing n_distinct for index
attributes was discussed back when this code was introduced:
https://www.postgresql.org/message-id/603c8f071001101127w3253899vb3f3e15073638774@mail.gmail.com

This means that we've lost the ability to enforce n_distinct for
expression indexes for two years.  But, do we really care about this
case?  My answer to that would be "no" as long as we don't have a
documented grammar rather, and we don't dump them either.  But I think
that we'd better do something with the code in analyze.c rather than
letting it just dead, and my take is that we should remove the call to
get_attribute_options() for this code path.

Any opinions?  @Robert: you were involved in 76a47c0, so I am adding
you in CC.
--
Michael

--6jTfDcuGz5JwzWtO
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmFnkNgACgkQnvQgOdby
QH2Olg//QwBOVZMTWDgRuL9YijUu2KmaOwk/Y4HK8gLm37lf3ArWe+dnBCNtOgIj
ynXpD6RjD1a25WfICXtFQCAbYbBOOAJqF/nIlxJfVCle+PtjBWcX0T1118xivwHJ
OinN0NXi5creFrz9KEl57VfWtR7Lh2BYC4pdbSLJH54Z0ETi2MMp48wRe5jEWoVD
4HX/OeJBTqvbI09mG0pM1vN3is667Km1pj1Rz8I7N8Crwsw9p8llTQtpvN/AvJC8
rZCi5Xdu59KBpIMyGcNceZ2jpR/Mr8YSXIQH6NRixs8KyZxwVyGX8KvMoEu/qiZH
OxS4uSU0jMy0R5vb/ipz9IlD9MXK99v6/cAdk0j/CMz6ysyzd+sefJ5gLkWs5srZ
l2pcw++aN1blzDXRAOenMzzrt4orhDXAdPthQg6u+NKch+riFC+j8mc0syOwzlmb
+pZ+OKAC7bQCp2tlLEfvDSQorArZQRPrJaleqV8HUqU7/Rijimj6nCm1sXaTw8GJ
cRf1lC9UTk2SEe3ergQSkreiVr078ASomdzcxFOdG24ASbZHrAZd+juyBhxQK1zS
JOh/Zxc0OBCX5s0XN1TsXwiB/6Z+12sDeWGgaU4dOWNYKAqLkpjCprWnl+0T1DV5
6teCkRddl59Kr7X/nuyA/zY09JT5EdMlMh2pM7af8lLrYuzJjEA=
=jM1g
-----END PGP SIGNATURE-----

--6jTfDcuGz5JwzWtO--