Preserve attstattarget on REINDEX CONCURRENTLY

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

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

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

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

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

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