Thread: Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable

On 10/11/21, 11:03 AM, "Vik Fearing" <vik@postgresfriends.org> wrote:
> On 10/11/21 5:25 PM, PG Bug reporting form wrote:
>>
>> User 'musttu' on IRC reported the following bug: After running "ALTER INDEX
>> some_idx ALTER COLUMN expr SET (n_distinct=100)", the index and table become
>> unusable. All further statements involving the table result in: "ERROR:
>> operator class text_ops has no options".
>>
>> They reported this on the RDS version of 13.3, but I've been able to
>> reproduce this on Debian with 13.4 and 14.0. It does not reproduce on 12.8,
>> all statements succeed on that version.
>
> This was broken by 911e702077 (Implement operator class parameters).

Moving to pgsql-hackers@.

At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
uses the wrong validation function.  I've attached a patch where I've
attempted to fix that and added some tests.

Nathan

On 10/13/21 2:06 AM, Bossart, Nathan wrote:
> On 10/11/21, 11:03 AM, "Vik Fearing" <vik@postgresfriends.org> wrote:
>> On 10/11/21 5:25 PM, PG Bug reporting form wrote:
>>>
>>> User 'musttu' on IRC reported the following bug: After running "ALTER INDEX
>>> some_idx ALTER COLUMN expr SET (n_distinct=100)", the index and table become
>>> unusable. All further statements involving the table result in: "ERROR:
>>> operator class text_ops has no options".
>>>
>>> They reported this on the RDS version of 13.3, but I've been able to
>>> reproduce this on Debian with 13.4 and 14.0. It does not reproduce on 12.8,
>>> all statements succeed on that version.
>>
>> This was broken by 911e702077 (Implement operator class parameters).
> 
> Moving to pgsql-hackers@.
> 
> At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
> uses the wrong validation function.  I've attached a patch where I've
> attempted to fix that and added some tests.

Ah, thank you.  I was in the (slow) process of writing basically this
exact patch.  So I'll stop now and endorse yours.
-- 
Vik Fearing

On 10/12/21, 5:31 PM, "Vik Fearing" <vik@postgresfriends.org> wrote:
> On 10/13/21 2:06 AM, Bossart, Nathan wrote:
>> Moving to pgsql-hackers@.
>>
>> At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
>> uses the wrong validation function.  I've attached a patch where I've
>> attempted to fix that and added some tests.
>
> Ah, thank you.  I was in the (slow) process of writing basically this
> exact patch.  So I'll stop now and endorse yours.

Oops, sorry about that.  Thanks for the endorsement.

Nathan

On Wed, Oct 13, 2021 at 12:06:32AM +0000, Bossart, Nathan wrote:
> At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
> uses the wrong validation function.  I've attached a patch where I've
> attempted to fix that and added some tests.

The gap is larger than than, because ALTER INDEX .. ALTER COLUMN
.. SET is supported by the parser but we don't document it.  The only
thing we document now is SET STATISTICS that applies to a column
*number*.

Anyway, specifying a column name for an ALTER INDEX is not right, no?
Just take for example the case of an expression which has a hardcoded
column name in pg_attribute.  So these are not specific to indexes,
which is why we apply column numbers for the statistics case.  I think
that we'd better just reject those cases until there is a proper
design done here.  As far as I can see, I guess that we should do
things  similarly to what we do for SET STATISTICS with column
numbers when it comes to indexes.
--
Michael

On Wed, Oct 13, 2021 at 9:12 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Oct 13, 2021 at 12:06:32AM +0000, Bossart, Nathan wrote:
> > At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
> > uses the wrong validation function.  I've attached a patch where I've
> > attempted to fix that and added some tests.
>
> The gap is larger than than, because ALTER INDEX .. ALTER COLUMN
> .. SET is supported by the parser but we don't document it.  The only
> thing we document now is SET STATISTICS that applies to a column
> *number*.
>
> Anyway, specifying a column name for an ALTER INDEX is not right, no?
> Just take for example the case of an expression which has a hardcoded
> column name in pg_attribute.  So these are not specific to indexes,
> which is why we apply column numbers for the statistics case.  I think
> that we'd better just reject those cases until there is a proper
> design done here.  As far as I can see, I guess that we should do
> things  similarly to what we do for SET STATISTICS with column
> numbers when it comes to indexes.

+1 it should behave similarly to SET STATISTICS for the index and if
someone tries to set with the column name then it should throw an
error.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On 10/13/21, 1:31 AM, "Dilip Kumar" <dilipbalaut@gmail.com> wrote:
> On Wed, Oct 13, 2021 at 9:12 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Anyway, specifying a column name for an ALTER INDEX is not right, no?
>> Just take for example the case of an expression which has a hardcoded
>> column name in pg_attribute.  So these are not specific to indexes,
>> which is why we apply column numbers for the statistics case.  I think
>> that we'd better just reject those cases until there is a proper
>> design done here.  As far as I can see, I guess that we should do
>> things  similarly to what we do for SET STATISTICS with column
>> numbers when it comes to indexes.
>
> +1 it should behave similarly to SET STATISTICS for the index and if
> someone tries to set with the column name then it should throw an
> error.

Good point.  I agree that rejecting this case is probably the best
option for now.  In addition to the bug mentioned in this thread, this
functionality doesn't even work for supported options currently.

        postgres=> create table test (a tsvector);
        CREATE TABLE
        postgres=> create index on test using gist (a);
        CREATE INDEX
        postgres=> alter index test_a_idx alter column a set (siglen = 100);
        ERROR:  unrecognized parameter "siglen"

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've attached new patch that just adds an ERROR.

Nathan

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

On Thu, Oct 14, 2021 at 11:07:21AM +0900, Michael Paquier wrote:
> 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.

Hearing nothing, and after pondering on this point, I think that
removing the get_attribute_options() is the right way to go for now
if there is a point in the future to get n_distinct done for all index
AMs.

I have reviewed the last patch posted upthread, and while testing
partitioned indexes I have noticed that we don't need to do a custom
check as part of ATExecSetOptions(), because we have already that in
ATSimplePermissions() with details on the relkind failing.  This makes
the patch simpler, with a better error message generated.  I have
added a case for partitioned indexes while on it.

Worth noting that I have spotted an extra weird call of
get_attribute_options() in extended statistics.  This is unrelated to
this thread and I am not done analyzing it yet, but a quick check
shows that we call it with an InvalidOid for expression stats, which
is surprising..  I'll start a thread once/if I find anything
interesting on this one.

Attached is the patch I am finishing with, that should go down to
v13 (this is going to conflict on REL_13_STABLE, for sure).

Thoughts?
--
Michael

On 10/18/21, 12:47 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> I have reviewed the last patch posted upthread, and while testing
> partitioned indexes I have noticed that we don't need to do a custom
> check as part of ATExecSetOptions(), because we have already that in
> ATSimplePermissions() with details on the relkind failing.  This makes
> the patch simpler, with a better error message generated.  I have
> added a case for partitioned indexes while on it.

Ah, yes, that is much better.

> Attached is the patch I am finishing with, that should go down to
> v13 (this is going to conflict on REL_13_STABLE, for sure).

+DROP INDEX btree_tall_tbl_idx2;
+ERROR:  index "btree_tall_tbl_idx2" does not exist

I think this is supposed to be "btree_tall_idx2".  Otherwise, the
patch looks reasonable to me.

Nathan

On Mon, Oct 18, 2021 at 09:43:58PM +0000, Bossart, Nathan wrote:
> +DROP INDEX btree_tall_tbl_idx2;
> +ERROR:  index "btree_tall_tbl_idx2" does not exist
>
> I think this is supposed to be "btree_tall_idx2".  Otherwise, the
> patch looks reasonable to me.

Thanks for double-checking.  Applied and back-patched, with a small
conflict regarding the error message in ~14.
--
Michael

On 10/18/21, 7:09 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Thanks for double-checking.  Applied and back-patched, with a small
> conflict regarding the error message in ~14.

Thanks!

Nathan

On Mon, Oct 18, 2021, at 09:46, Michael Paquier wrote:
> On Thu, Oct 14, 2021 at 11:07:21AM +0900, Michael Paquier wrote:
> Attached is the patch I am finishing with, that should go down to
> v13 (this is going to conflict on REL_13_STABLE, for sure).
>
> Thoughts?

The test case doesn't seem entirely correct to me? The index being dropped (btree_tall_tbl_idx2) doesn't exist.

Also, I don't believe this tests the case of dropping the index when it previously has been altered in this way.

On 10/18/21, 11:49 PM, "Matthijs van der Vleuten" <postgresql@zr40.nl> wrote:
> The test case doesn't seem entirely correct to me? The index being
> dropped (btree_tall_tbl_idx2) doesn't exist.

This was fixed before it was committed [0].

> Also, I don't believe this tests the case of dropping the index when
> it previously has been altered in this way.

That can still fail with the "has no options" ERROR, and fixing it
will still require a manual catalog update.  The ERROR is actually
coming from the call to index_open(), so bypassing it might require
some rather intrusive changes.  Given that it took over a year for
this bug to be reported, I suspect it might be more trouble than it's
worth.

Nathan

[0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fdd8857

On Tue, Oct 19, 2021 at 02:40:04PM +0000, Bossart, Nathan wrote:
> On 10/18/21, 11:49 PM, "Matthijs van der Vleuten" <postgresql@zr40.nl> wrote:
>> The test case doesn't seem entirely correct to me? The index being
>> dropped (btree_tall_tbl_idx2) doesn't exist.
>
> This was fixed before it was committed [0].

Yes, my apologies about this brain fade.  The committed code is
hopefully fine :)

>> Also, I don't believe this tests the case of dropping the index when
>> it previously has been altered in this way.
>
> That can still fail with the "has no options" ERROR, and fixing it
> will still require a manual catalog update.  The ERROR is actually
> coming from the call to index_open(), so bypassing it might require
> some rather intrusive changes.  Given that it took over a year for
> this bug to be reported, I suspect it might be more trouble than it's
> worth.

This may need a mention in the release notes, but the problem is I
guess not that spread enough to worry or people would have complained
more since 13 was out.  Logical dumps discard that automatically and
even for the ANALYZE case, the pre-committed code would have just
ignored the reloptions retrieved by get_attribute_options().
--
Michael