Horton

Thread

Commits

  1. Fix ORDER BY clause in new regression test of REINDEX CONCURRENTLY

  2. Preserve pg_attribute.attstattarget across REINDEX CONCURRENTLY

  1. Preserve attstattarget on REINDEX CONCURRENTLY

    Ronan Dunklau <ronan@dunklau.fr> — 2021-02-04T10:04:38Z

    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

  2. Re: Preserve attstattarget on REINDEX CONCURRENTLY

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-02-04T14:46:50Z

    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

  3. Re: Preserve attstattarget on REINDEX CONCURRENTLY

    Ronan Dunklau <ronan@dunklau.fr> — 2021-02-04T14:52:44Z

    > 
> 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

  4. Re: Preserve attstattarget on REINDEX CONCURRENTLY

    Michael Paquier <michael@paquier.xyz> — 2021-02-05T02:17:48Z

    On Thu, Feb 04, 2021 at 03:52:44PM +0100, Ronan Dunklau wrote:
>> Hmmm, that sure seems like a bug, or at least unexpected behavior (that
>> I don't see mentioned in the docs).

Yeah, per the rule of consistency, this classifies as a bug to me.

> Please find attached a correct patch.

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.

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.
--
Michael

  5. Re: Preserve attstattarget on REINDEX CONCURRENTLY

    Ronan Dunklau <ronan@dunklau.fr> — 2021-02-05T07:22:17Z

    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

  6. Re: Preserve attstattarget on REINDEX CONCURRENTLY

    Michael Paquier <michael@paquier.xyz> — 2021-02-05T07:43:44Z

    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?
--
Michael

  7. Re: Preserve attstattarget on REINDEX CONCURRENTLY

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2021-02-06T21:39:53Z

    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

  8. Re: Preserve attstattarget on REINDEX CONCURRENTLY

    Michael Paquier <michael@paquier.xyz> — 2021-02-07T00:39:36Z

    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

  9. Re: Preserve attstattarget on REINDEX CONCURRENTLY

    Michael Paquier <michael@paquier.xyz> — 2021-02-10T04:35:16Z

    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

  10. Re: Preserve attstattarget on REINDEX CONCURRENTLY

    Justin Pryzby <pryzby@telsasoft.com> — 2021-02-10T06:58:05Z

    On Fri, Feb 05, 2021 at 11:17:48AM +0900, Michael Paquier wrote:
> 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

> +++ b/src/test/regress/sql/create_index.sql
> @@ -1103,6 +1104,13 @@ SELECT starelid::regclass, count(*) FROM pg_statistic WHERE starelid IN (
>    'concur_exprs_index_pred'::regclass,
>    'concur_exprs_index_pred_2'::regclass)
>    GROUP BY starelid ORDER BY starelid::regclass::text;
> +-- attstattarget should remain intact
> +SELECT attrelid::regclass, attnum, attstattarget
> +  FROM pg_attribute WHERE attrelid IN (
> +    'concur_exprs_index_expr'::regclass,
> +    'concur_exprs_index_pred'::regclass,
> +    'concur_exprs_index_pred_2'::regclass)
> +  ORDER BY 'concur_exprs_index_expr'::regclass::text, attnum;

If I'm not wrong, you meant to ORDER BY attrelid::regclass::text, attnum;

-- 
Justin

  11. Re: Preserve attstattarget on REINDEX CONCURRENTLY

    Michael Paquier <michael@paquier.xyz> — 2021-02-10T08:04:04Z

    On Wed, Feb 10, 2021 at 12:58:05AM -0600, Justin Pryzby wrote:
> If I'm not wrong, you meant to ORDER BY attrelid::regclass::text, attnum;

Indeed, I meant that.  Thanks, Justin!
--
Michael