Обсуждение: Preserve attstattarget on REINDEX CONCURRENTLY

Hello !

We encountered the following bug recently in production: when running REINDEX 
CONCURRENTLY on an index, the attstattarget is reset to 0.

Consider the following example: 

junk=# \d+ t1_date_trunc_idx 
                                    Index "public.t1_date_trunc_idx"
   Column   |            Type             | Key? |         Definition          
| Storage | Stats target 
------------+-----------------------------+------
+-----------------------------+---------+--------------
 date_trunc | timestamp without time zone | yes  | date_trunc('day'::text, ts) 
| plain   | 1000
btree, for table "public.t1"

junk=# REINDEX INDEX t1_date_trunc_idx;
REINDEX
junk=# \d+ t1_date_trunc_idx 
                                    Index "public.t1_date_trunc_idx"
   Column   |            Type             | Key? |         Definition          
| Storage | Stats target 
------------+-----------------------------+------
+-----------------------------+---------+--------------
 date_trunc | timestamp without time zone | yes  | date_trunc('day'::text, ts) 
| plain   | 1000
btree, for table "public.t1"

junk=# REINDEX INDEX CONCURRENTLY t1_date_trunc_idx;
REINDEX
junk=# \d+ t1_date_trunc_idx 
                                    Index "public.t1_date_trunc_idx"
   Column   |            Type             | Key? |         Definition          
| Storage | Stats target 
------------+-----------------------------+------
+-----------------------------+---------+--------------
 date_trunc | timestamp without time zone | yes  | date_trunc('day'::text, ts) 
| plain   | 
btree, for table "public.t1"


I'm attaching a patch possibly solving the problem, but maybe the proposed 
changes will be too intrusive ?

Regards,

-- 
Ronan Dunklau

On 2/4/21 11:04 AM, Ronan Dunklau wrote:
> Hello !
> 
> ...
> 
> junk=# REINDEX INDEX CONCURRENTLY t1_date_trunc_idx;
> REINDEX
> junk=# \d+ t1_date_trunc_idx 
>                                     Index "public.t1_date_trunc_idx"
>    Column   |            Type             | Key? |         Definition          
> | Storage | Stats target 
> ------------+-----------------------------+------
> +-----------------------------+---------+--------------
>  date_trunc | timestamp without time zone | yes  | date_trunc('day'::text, ts) 
> | plain   | 
> btree, for table "public.t1"
> 
> 
> I'm attaching a patch possibly solving the problem, but maybe the proposed 
> changes will be too intrusive ?
> 

Hmmm, that sure seems like a bug, or at least unexpected behavior (that
I don't see mentioned in the docs).

But the patch seems borked in some way:

$ patch -p1 < ~/keep_attstattargets_on_reindex_concurrently.patch
patch: **** Only garbage was found in the patch input.

There seem to be strange escape characters and so on, how did you create
the patch? Maybe some syntax coloring, or something?

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> 
> Hmmm, that sure seems like a bug, or at least unexpected behavior (that
> I don't see mentioned in the docs).
> 
> But the patch seems borked in some way:
> 
> $ patch -p1 < ~/keep_attstattargets_on_reindex_concurrently.patch
> patch: **** Only garbage was found in the patch input.
> 
> There seem to be strange escape characters and so on, how did you 
> create
> the patch? Maybe some syntax coloring, or something?

You're right, I had syntax coloring in the output, sorry.

Please find attached a correct patch.

Regards,

--
Ronan Dunklau

Le vendredi 5 février 2021, 03:17:48 CET Michael Paquier a écrit :
> ConstructTupleDescriptor() does not matter much, but this patch is not
> acceptable to me as it touches the area of the index creation while
> statistics on an index expression can only be changed with a special
> flavor of ALTER INDEX with column numbers.  This would imply an ABI
> breakage, so it cannot be backpatched as-is.

I'm not surprised by this answer, the good news is it's being back-patched.

>
> Let's copy this data in index_concurrently_swap() instead.  The
> attached patch does that, and adds a test cheaper than what was
> proposed.  There is a minor release planned for next week, so I may be
> better to wait after that so as we have enough time to agree on a
> solution.

Looks good to me ! Thank you.

--
Ronan Dunklau

On 2/5/21 8:43 AM, Michael Paquier wrote:
> On Fri, Feb 05, 2021 at 08:22:17AM +0100, Ronan Dunklau wrote:
>> I'm not surprised by this answer, the good news is it's being back-patched.
> 
> Yes, I have no problem with that.  Until this gets fixed, the damage
> can be limited with an extra ALTER INDEX, that takes a
> ShareUpdateExclusiveLock so there is no impact on the concurrent
> activity.
> 
>> Looks good to me ! Thank you.
> 
> Thanks for looking at it.  Tomas, do you have any comments?
> --

Not really.

Copying this info in index_concurrently_swap seems a bit strange - we're 
copying other stuff there, but this is modifying something we've already 
copied before. I understand why we do it there to make this 
backpatchable, but maybe it'd be good to mention this in a comment (or 
at least the commit message). We could do this in the backbranches only 
and the "correct" way in master, but that does not seem worth it.

One minor comment - the code says this:

     /* no need for a refresh if both match */
     if (attstattarget == att->attstattarget)
         continue;

Isn't that just a different way to say "attstattarget is not default")?


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Feb 06, 2021 at 10:39:53PM +0100, Tomas Vondra wrote:
> Copying this info in index_concurrently_swap seems a bit strange - we're
> copying other stuff there, but this is modifying something we've already
> copied before. I understand why we do it there to make this backpatchable,
> but maybe it'd be good to mention this in a comment (or at least the commit
> message). We could do this in the backbranches only and the "correct" way in
> master, but that does not seem worth it.

Thanks.

> One minor comment - the code says this:
>
>     /* no need for a refresh if both match */
>     if (attstattarget == att->attstattarget)
>         continue;
>
> Isn't that just a different way to say "attstattarget is not default")?

For REINDEX CONCURRENTLY, yes.  I was thinking here about the case
where this code is used for other purposes in the future, where
attstattarget may not be -1.

I'll see about applying this stuff after the next version is tagged
then.
--
Michael

On Sun, Feb 07, 2021 at 09:39:36AM +0900, Michael Paquier wrote:
> I'll see about applying this stuff after the next version is tagged
> then.

The new versions have been tagged, so done as of bd12080 and
back-patched.  I have added a note in the commit log about the
approach to use index_create() instead for HEAD.
--
Michael