ALTER tbl rewrite loses CLUSTER ON index

Hi Justin,

On Mon, Feb 3, 2020 at 1:17 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

Other options are preserved by ALTER (and CLUSTER ON is and most obviously
should be preserved by CLUSTER's rewrite), so I think (SET) CLUSTER should be
preserved by ALTER, too.

Yes.

create table foo (a int primary key);
cluster foo;
ERROR: there is no previously clustered index for table "foo"
cluster foo using foo_pkey;
alter table foo alter a type bigint;
cluster foo;
ERROR: there is no previously clustered index for table "foo"

With your patch, this last error doesn't occur.

Like you, I too suspect that losing indisclustered like this is
unintentional, so should be fixed.

As far as I can see, this should be the responsibility of something in the
vicinity of ATPostAlterTypeParse/RememberIndexForRebuilding.

Attach patch sketches a fix.

While your sketch hits pretty close, it could be done a bit
differently. For one, I don't like the way it's misusing
changedIndexOids and changedIndexDefs.

Instead, we can do something similar to what
RebuildConstraintComments() does for constraint comments. For
example, we can have a PreserveClusterOn() that adds a AT_ClusterOn
command into table's AT_PASS_OLD_INDEX pass commands. Attached patch
shows what I'm thinking. I also added representative tests.

Thanks,
Amit

On Wed, Feb 05, 2020 at 03:53:45PM +0900, Amit Langote wrote:

Hi Justin,

On Mon, Feb 3, 2020 at 1:17 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

Other options are preserved by ALTER (and CLUSTER ON is and most obviously
should be preserved by CLUSTER's rewrite), so I think (SET) CLUSTER should be
preserved by ALTER, too.

Yes.

create table foo (a int primary key);
cluster foo;
ERROR: there is no previously clustered index for table "foo"
cluster foo using foo_pkey;
alter table foo alter a type bigint;
cluster foo;
ERROR: there is no previously clustered index for table "foo"

With your patch, this last error doesn't occur.

Like you, I too suspect that losing indisclustered like this is
unintentional, so should be fixed.

Thanks for checking.

It doesn't need to be said, but your patch is obviously superior.

I ran into this while looking into a suggestion from Alvaro that ALTER should
rewrite in order of a clustered index (if any) rather than in pre-existing heap
order (more on that another day). So while this looks like a bug, and I can't
think how a backpatch would break something, my suggestion is that backpatching
a fix is of low value, so it's only worth +0.

Thanks
Justin

On Thu, Feb 6, 2020 at 10:31 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Feb 6, 2020 at 8:44 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Wed, Feb 05, 2020 at 03:53:45PM +0900, Amit Langote wrote:

diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
+-- alter type shouldn't lose clustered index

My only suggestion is to update the comment
+-- alter type rewrite/rebuild should preserve cluster marking on index

Sure, done.

Deja vu. Last two messages weren't sent to the list; updated patch attached.

Thanks,
Amit

I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
Oid of a clustered index, rather than a boolean in pg_index.

That likely would've avoided (or at least exposed) this issue.
And avoids the possibility of having two indices marked as "clustered".
These would be more trivial:
mark_index_clustered
/* We need to find the index that has indisclustered set. */

On 2020-Feb-06, Justin Pryzby wrote:

I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
Oid of a clustered index, rather than a boolean in pg_index.

Maybe. Do you want to try a patch?

That likely would've avoided (or at least exposed) this issue.
And avoids the possibility of having two indices marked as "clustered".
These would be more trivial:
mark_index_clustered
/* We need to find the index that has indisclustered set. */

You need to be careful when dropping the index ...

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Feb 7, 2020 at 2:24 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2020-Feb-06, Justin Pryzby wrote:

I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
Oid of a clustered index, rather than a boolean in pg_index.

Maybe. Do you want to try a patch?

+1

Thanksm
Amit

On Thu, Feb 06, 2020 at 02:24:47PM -0300, Alvaro Herrera wrote:

On 2020-Feb-06, Justin Pryzby wrote:

I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
Oid of a clustered index, rather than a boolean in pg_index.

Maybe. Do you want to try a patch?

I think the attached is 80% complete (I didn't touch pg_dump).

One objection to this change would be that all relations (including indices)
end up with relclustered fields, and pg_index already has a number of bools, so
it's not like this one bool is wasting a byte.

I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
So I would be -0.5 on moving it to pg_class..

But I think 0001 and 0002 are worthy. Maybe the test in 0002 should live
somewhere else.

Hi Justin,

On Fri, Feb 7, 2020 at 11:39 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Feb 06, 2020 at 02:24:47PM -0300, Alvaro Herrera wrote:

On 2020-Feb-06, Justin Pryzby wrote:

I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
Oid of a clustered index, rather than a boolean in pg_index.

Maybe. Do you want to try a patch?

I think the attached is 80% complete (I didn't touch pg_dump).

One objection to this change would be that all relations (including indices)
end up with relclustered fields, and pg_index already has a number of bools, so
it's not like this one bool is wasting a byte.

I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
So I would be -0.5 on moving it to pg_class..

Are you still for fixing ALTER TABLE losing relisclustered with the
patch we were working on earlier [1]/messages/by-id/CA+HiwqEt1HnXYckCdaO8+pOoFs7NNS5byoZ6Xg2B7epKbhS85w@mail.gmail.com, if not for moving relisclustered
to pg_class anymore?

I have read elsewhere [2]/messages/by-id/10984.1581181029@sss.pgh.pa.us that forcing ALTER TABLE to rewrite in
clustered order might not be a good option, but maybe that one is a
more radical proposal than this.

Thanks,
Amit

[1]: /messages/by-id/CA+HiwqEt1HnXYckCdaO8+pOoFs7NNS5byoZ6Xg2B7epKbhS85w@mail.gmail.com
[2]: /messages/by-id/10984.1581181029@sss.pgh.pa.us

On Mon, Feb 17, 2020 at 02:31:42PM +0900, Amit Langote wrote:

Hi Justin,

On Fri, Feb 7, 2020 at 11:39 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Feb 06, 2020 at 02:24:47PM -0300, Alvaro Herrera wrote:

On 2020-Feb-06, Justin Pryzby wrote:

I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
Oid of a clustered index, rather than a boolean in pg_index.

Maybe. Do you want to try a patch?

I think the attached is 80% complete (I didn't touch pg_dump).

One objection to this change would be that all relations (including indices)
end up with relclustered fields, and pg_index already has a number of bools, so
it's not like this one bool is wasting a byte.

I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
So I would be -0.5 on moving it to pg_class..

In case there's any confusion: "a's" was probably me halfway changing
"someone's" to "a".

Are you still for fixing ALTER TABLE losing relisclustered with the
patch we were working on earlier [1], if not for moving relisclustered
to pg_class anymore?

Thanks for remembering this one.

I think your patch is the correct fix.

I forgot to say it, but moving relisclustered to pg_class doesn't help to avoid
losting indisclustered: it still needs a fix just like this. Anyway, I
withdrew my suggestion for moving to pg_class, since it has more overhead, even
for pg_class rows for relations which can't have indexes.

I have read elsewhere [2] that forcing ALTER TABLE to rewrite in
clustered order might not be a good option, but maybe that one is a
more radical proposal than this.

Right; your fix seems uncontroversial. I ran into this (indisclustered) bug
while starting to write that patch for "ALTER rewrite in clustered order".

--
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:

I think the attached is 80% complete (I didn't touch pg_dump).
One objection to this change would be that all relations (including indices)
end up with relclustered fields, and pg_index already has a number of bools, so
it's not like this one bool is wasting a byte.
I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
So I would be -0.5 on moving it to pg_class..
But I think 0001 and 0002 are worthy. Maybe the test in 0002 should live
somewhere else.

0001 has been superseded by events (faade5d4c), so the cfbot is choking
on that one's failure to apply, and not testing any further. Please
repost without 0001 so that we can get this testing again.

regards, tom lane

On Fri, Feb 28, 2020 at 06:26:04PM -0500, Tom Lane wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

I think the attached is 80% complete (I didn't touch pg_dump).
One objection to this change would be that all relations (including indices)
end up with relclustered fields, and pg_index already has a number of bools, so
it's not like this one bool is wasting a byte.
I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
So I would be -0.5 on moving it to pg_class..
But I think 0001 and 0002 are worthy. Maybe the test in 0002 should live
somewhere else.

0001 has been superseded by events (faade5d4c), so the cfbot is choking
on that one's failure to apply, and not testing any further. Please
repost without 0001 so that we can get this testing again.

I've just noticed while working on (1) that this separately affects REINDEX
CONCURRENTLY, which would be a new bug in v12. Without CONCURRENTLY there's no
issue. I guess we need a separate patch for that case.

(1) https://commitfest.postgresql.org/27/2269/

The ALTER bug goes back further and its fix should be a kept separate.

postgres=# DROP TABLE tt; CREATE TABLE tt(i int unique); CLUSTER tt USING tt_i_key; CLUSTER tt; REINDEX INDEX tt_i_key; CLUSTER tt;
DROP TABLE
CREATE TABLE
CLUSTER
CLUSTER
REINDEX
CLUSTER

postgres=# DROP TABLE tt; CREATE TABLE tt(i int unique); CLUSTER tt USING tt_i_key; CLUSTER tt; REINDEX INDEX CONCURRENTLY tt_i_key; CLUSTER tt;
DROP TABLE
CREATE TABLE
CLUSTER
CLUSTER
REINDEX
ERROR: there is no previously clustered index for table "tt"

--
Justin

On Fri, Feb 28, 2020 at 08:42:02PM -0600, Justin Pryzby wrote:

On Fri, Feb 28, 2020 at 06:26:04PM -0500, Tom Lane wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

I think the attached is 80% complete (I didn't touch pg_dump).
One objection to this change would be that all relations (including indices)
end up with relclustered fields, and pg_index already has a number of bools, so
it's not like this one bool is wasting a byte.
I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
So I would be -0.5 on moving it to pg_class..
But I think 0001 and 0002 are worthy. Maybe the test in 0002 should live
somewhere else.

0001 has been superseded by events (faade5d4c), so the cfbot is choking
on that one's failure to apply, and not testing any further. Please
repost without 0001 so that we can get this testing again.

I've just noticed while working on (1) that this separately affects REINDEX
CONCURRENTLY, which would be a new bug in v12. Without CONCURRENTLY there's no
issue. I guess we need a separate patch for that case.

Rebased Amit's patch and included my own 0002 to fix the REINDEX CONCURRENTLY
issue.

--
Justin

On Sat, Feb 29, 2020 at 10:52:58AM -0600, Justin Pryzby wrote:

Rebased Amit's patch and included my own 0002 to fix the REINDEX CONCURRENTLY
issue.

I have looked at 0002 as that concerns me.

+SELECT indexrelid::regclass FROM pg_index WHERE indrelid='concur_clustered'::regclass;
+       indexrelid       
+------------------------
+ concur_clustered_i_idx
+(1 row)

This test should check after indisclustered. Except that, the patch
is fine so I'll apply it if there are no objections.
--
Michael

On Mon, Mar 02, 2020 at 12:28:18PM +0900, Michael Paquier wrote:

This test should check after indisclustered. Except that, the patch
is fine so I'll apply it if there are no objections.

I got a second look at this one, and applied it down to 12 after some
small modifications in the new test and in the comments.
--
Michael

On Mon, Mar 02, 2020 at 12:28:18PM +0900, Michael Paquier wrote:

+SELECT indexrelid::regclass FROM pg_index WHERE indrelid='concur_clustered'::regclass;

This test should check after indisclustered. Except that, the patch
is fine so I'll apply it if there are no objections.

Oops - I realized that, but didn't send a new patch before you noticed - thanks
for handling it.

--
Justin

@cfbot: resending with only Amit's 0001, since Michael pushed a variation on
0002.

--
Justin

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested

I tested the patch on the master branch (a77315fdf2a197a925e670be2d8b376c4ac02efc) and it works fine.

The new status of this patch is: Ready for Committer

Justin Pryzby <pryzby@telsasoft.com> writes:

@cfbot: resending with only Amit's 0001, since Michael pushed a variation on
0002.

Boy, I really dislike this patch. ATPostAlterTypeParse is documented as
using the supplied definition string, and nothing else, to reconstruct
the index. This breaks that without even the courtesy of documenting
the breakage. Moreover, the reason why it's designed like that is to
avoid requiring the old index objects to still be accessible. So I'm
surprised that this hack works at all. I don't think it would have
worked at the time the code was first written, and I think it's imposing
a constraint we'll have problems with (again?) in future.

The right way to fix this is to note the presence of the indisclustered
flag when we're examining the index earlier, and include a suitable
command in the definition string. So probably pg_get_indexdef_string()
is what needs to change. It doesn't look like that's used anywhere
else, so we can just redefine its behavior as needed.

regards, tom lane

On Fri, Mar 13, 2020 at 2:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Justin Pryzby <pryzby@telsasoft.com> writes:

@cfbot: resending with only Amit's 0001, since Michael pushed a variation on
0002.

Thank you for taking a look at it.

Boy, I really dislike this patch. ATPostAlterTypeParse is documented as
using the supplied definition string, and nothing else, to reconstruct
the index. This breaks that without even the courtesy of documenting
the breakage. Moreover, the reason why it's designed like that is to
avoid requiring the old index objects to still be accessible. So I'm
surprised that this hack works at all. I don't think it would have
worked at the time the code was first written, and I think it's imposing
a constraint we'll have problems with (again?) in future.

Okay, so maybe in the middle of ATPostAlterTypeParse() is not a place
to do it, but don't these arguments apply to
RebuildConstraintComment(), which I based the patch on?

The right way to fix this is to note the presence of the indisclustered
flag when we're examining the index earlier, and include a suitable
command in the definition string. So probably pg_get_indexdef_string()
is what needs to change. It doesn't look like that's used anywhere
else, so we can just redefine its behavior as needed.

I came across a commit that recently went in:

commit 1cc9c2412cc9a2fbe6a381170097d315fd40ccca
Author: Peter Eisentraut <peter@eisentraut.org>
Date: Fri Mar 13 11:28:11 2020 +0100

Preserve replica identity index across ALTER TABLE rewrite

which fixes something very similar to what we are trying to with this
patch. The way it's done looks to me very close to what you are
telling. I have updated the patch to be similar to the above fix.

--
Thank you,
Amit