Обсуждение: ALTER tbl rewrite loses CLUSTER ON index

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.

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

Attach patch sketches a fix.

ts=# SET client_min_messages=debug; DROP TABLE t; CREATE TABLE t(i int); CREATE INDEX ON t(i)WITH(fillfactor=11,
vacuum_cleanup_index_scale_factor=12);CLUSTER t USING t_i_key; ALTER TABLE t ALTER i TYPE bigint; \d t
 
SET
DEBUG:  drop auto-cascades to type t
DEBUG:  drop auto-cascades to type t[]
DEBUG:  drop auto-cascades to index t_i_idx
DROP TABLE
CREATE TABLE
DEBUG:  building index "t_i_idx" on table "t" serially
CREATE INDEX
ERROR:  index "t_i_key" for table "t" does not exist
DEBUG:  rewriting table "t"
DEBUG:  building index "t_i_idx" on table "t" serially
DEBUG:  drop auto-cascades to type pg_temp_3091172777
DEBUG:  drop auto-cascades to type pg_temp_3091172777[]
ALTER TABLE
                 Table "public.t"
 Column |  Type  | Collation | Nullable | Default 
--------+--------+-----------+----------+---------
 i      | bigint |           |          | 
Indexes:
    "t_i_idx" btree (i) WITH (fillfactor='11', vacuum_cleanup_index_scale_factor='12')

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], if not for moving relisclustered
to pg_class anymore?

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.

Thanks,
Amit

[1] https://postgr.es/m/CA%2BHiwqEt1HnXYckCdaO8%2BpOoFs7NNS5byoZ6Xg2B7epKbhS85w%40mail.gmail.com
[2] https://postgr.es/m/10984.1581181029%40sss.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;
CLUSTERtt;
 
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
CONCURRENTLYtt_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

On Mon, Mar 16, 2020 at 04:01:42PM +0900, Amit Langote wrote:
> 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.

Yes, I noticed it too.

Should we use your get_index_isclustered more widely ?

Also, should we call it "is_index_clustered", since otherwise it sounds alot
like "+get_index_clustered" (without "is"), which sounds like it takes a table
and returns which index is clustered.  That might be just as useful for some of
these callers.

-- 
Justin

On 2020-Mar-16, Justin Pryzby wrote:

> Also, should we call it "is_index_clustered", since otherwise it sounds alot
> like "+get_index_clustered" (without "is"), which sounds like it takes a table
> and returns which index is clustered.  That might be just as useful for some of
> these callers.

Amit's proposed name seems to match lsyscache.c usual conventions better.

> Should we use your get_index_isclustered more widely ?

Yeah, in cluster(), mark_index_clustered().

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

On Mon, Mar 16, 2020 at 11:25:23AM -0300, Alvaro Herrera wrote:
> On 2020-Mar-16, Justin Pryzby wrote:
>
> > Also, should we call it "is_index_clustered", since otherwise it sounds alot
> > like "+get_index_clustered" (without "is"), which sounds like it takes a table
> > and returns which index is clustered.  That might be just as useful for some of
> > these callers.
>
> Amit's proposed name seems to match lsyscache.c usual conventions better.

There were no get_index_isvalid() (introduced by me) or
get_index_isreplident() (introduced by Peter) until last week, and
those names have been chosen to be consistent with the existing
get_index_column_opclass(), so using get_index_isclustered is in my
opinion the most consistent choice.

> Yeah, in cluster(), mark_index_clustered().

Patch 0002 from Justin does that, I would keep this refactoring as
HEAD-only material though, and I don't spot any other code paths in
need of patching.

The commit message of patch 0001 is not what you wanted I guess.

     if (OidIsValid(indexOid))
     {
-        indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
-        if (!HeapTupleIsValid(indexTuple))
-            elog(ERROR, "cache lookup failed for index %u", indexOid);
-        indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
-
-        if (indexForm->indisclustered)
-        {
-            ReleaseSysCache(indexTuple);
+        if (get_index_isclustered(indexOid))
             return;
-        }
-
-        ReleaseSysCache(indexTuple);
     }
No need for two layers of if(s) here.

+create index alttype_cluster_a on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_a;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;

Would it make sense to add a second index not used for clustering to
check after the case where isclustered is false?  A second thing would
be to check if relfilenode values match before and after each DDL to
make sure that a rewrite happened or not, see check_ddl_rewrite() for
example in alter_table.sql.

Keeping both RememberClusterOnForRebuilding and
RememberReplicaIdentityForRebuilding as separate looks fine to me.
The code could be a bit more organized though.  We have
RememberIndexForRebuilding which may go through
RememberConstraintForRebuilding if the relation OID changed is a
constraint, and both register the replindent or isclustered
information if present.  Not really something for this patch set to
care about, just a thought while reading this code.

While looking at this bug, I have spotted a behavior which is perhaps
not welcome.  Take this test case:
create table aa (a int);
insert into aa values (1), (1);
create unique index concurrently aai on aa(a); -- fails
alter table aa alter column a type bigint;

This generates the following error:
ERROR:  23505: could not create unique index "aai"
DETAIL:  Key (a)=(1) is duplicated.
SCHEMA NAME:  public
TABLE NAME:  aa
CONSTRAINT NAME:  aai
LOCATION:  comparetup_index_btree, tuplesort.c:4049

After a REINDEX CONCURRENTLY, we may leave behind an invalid index
on the relation's toast table or even normal indexes.  CREATE INDEX
CONCURRENTLY may also leave behind invalid indexes.  If triggering an
ALTER TABLE that causes a rewrite of the relation, we have the
following behavior:
- An invalid toast index is correctly discarded, keeping one valid
toast index.  No problem here.
- Invalid non-toast indexes are all rebuilt.  If the index relies on a
constraint then ALTER TABLE would fail, like the above.

I am wondering if there is an argument for not including invalid
indexes in the rebuilt version, even if the existing behavior may be
useful for some users.
--
Michael

On Tue, Mar 17, 2020 at 02:33:32PM +0900, Michael Paquier wrote:
> > Yeah, in cluster(), mark_index_clustered().
> 
> Patch 0002 from Justin does that, I would keep this refactoring as
> HEAD-only material though, and I don't spot any other code paths in
> need of patching.
> 
> The commit message of patch 0001 is not what you wanted I guess.

That's what git-am does, and I didn't find any option to make it less
unreadable.  I guess I should just delete the email body it inserts.

|       The commit message is formed by the title taken from the "Subject: ", a
|       blank line and the body of the message up to where the patch begins. Excess
|       whitespace at the end of each line is automatically stripped.

-- 
Justin

On Tue, Mar 17, 2020 at 11:20:44AM -0500, Justin Pryzby wrote:
> On Tue, Mar 17, 2020 at 02:33:32PM +0900, Michael Paquier wrote:
>> Patch 0002 from Justin does that, I would keep this refactoring as
>> HEAD-only material though, and I don't spot any other code paths in
>> need of patching.
>>
>> The commit message of patch 0001 is not what you wanted I guess.
>
> That's what git-am does, and I didn't find any option to make it less
> unreadable.  I guess I should just delete the email body it inserts.

Strange...

Anyway, Tom, Alvaro, are you planning to look at what is proposed on
this thread?  I don't want to step on your toes if that's the case and
it seems to me that the approach taken by the patch is sound, using as
basic fix the addition of an AT_ClusterOn sub-command to the list of
commands to execute when rebuilding the table, ensuring that any
follow-up CLUSTER command will use the correct index.
--
Michael

On Wed, Mar 18, 2020 at 11:48:37AM +0900, Michael Paquier wrote:
> Anyway, Tom, Alvaro, are you planning to look at what is proposed on
> this thread?  I don't want to step on your toes if that's the case and
> it seems to me that the approach taken by the patch is sound, using as
> basic fix the addition of an AT_ClusterOn sub-command to the list of
> commands to execute when rebuilding the table, ensuring that any
> follow-up CLUSTER command will use the correct index.

Hearing nothing, I have been looking at the patches sent upthread, and
did some modifications as per the attached for 0001.  The logic looked
fine to me and it is unchanged as you are taking care of normal
indexes as well as constraint indexes.  Please note that I have
tweaked some comments, and removed what was on top of
RememberConstraintForRebuilding() as that was just a duplicate.
Regarding the tests, I was annoyed by the fact that the logic was not
manipulating two indexes at the same time on the relation rewritten
with a normal and a constraint index, and cross-checking both at the
same time to see which one is clustered after each rewrite is good for
consistency.

Now, regarding patch 0002, note that you have a problem for this part:
-            tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
-            if (!HeapTupleIsValid(tuple))    /* probably can't happen */
-            {
-                relation_close(OldHeap, AccessExclusiveLock);
-                pgstat_progress_end_command();
-                return;
-            }
-            indexForm = (Form_pg_index) GETSTRUCT(tuple);
-            if (!indexForm->indisclustered)
+            if (!get_index_isclustered(indexOid))
             {
-                ReleaseSysCache(tuple);
                 relation_close(OldHeap, AccessExclusiveLock);
                 pgstat_progress_end_command();
                 return;
             }
-            ReleaseSysCache(tuple);

On an invalid tuple for pg_index, the new code would issue an error,
while the old code would just return.  And it seems to me that this
can lead to problems because the parent relation is processed and
locked at the beginning of cluster_rel(), *after* we know the index
OID to work on.  The refactoring is fine for the other two areas
though, so I think that there is still value to apply
get_index_isclustered() within mark_index_clustered() and cluster(),
and I would suggest to keep 0002 to that.

Justin, what do you think?
--
Michael

On Thu, Apr 02, 2020 at 03:14:21PM +0900, Michael Paquier wrote:
> Now, regarding patch 0002, note that you have a problem for this part:
> -            tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
> -            if (!HeapTupleIsValid(tuple))    /* probably can't happen */
> -            {
> -                relation_close(OldHeap, AccessExclusiveLock);
> -                pgstat_progress_end_command();
> -                return;
> -            }
> -            indexForm = (Form_pg_index) GETSTRUCT(tuple);
> -            if (!indexForm->indisclustered)
> +            if (!get_index_isclustered(indexOid))
>              {
> -                ReleaseSysCache(tuple);
>                  relation_close(OldHeap, AccessExclusiveLock);
>                  pgstat_progress_end_command();
>                  return;
>              }
> -            ReleaseSysCache(tuple);
> 
> On an invalid tuple for pg_index, the new code would issue an error,
> while the old code would just return.  And it seems to me that this
> can lead to problems because the parent relation is processed and
> locked at the beginning of cluster_rel(), *after* we know the index
> OID to work on.

> The refactoring is fine for the other two areas
> though, so I think that there is still value to apply
> get_index_isclustered() within mark_index_clustered() and cluster(),
> and I would suggest to keep 0002 to that.
> 
> Justin, what do you think?

Sounds right.  Or else get_index_isclustered() could be redefined to take a
boolean "do_elog" flag, and if syscache fails and that's false, then return
false instead of ERROR.

-- 
Justin

On Thu, Apr 02, 2020 at 01:52:09AM -0500, Justin Pryzby wrote:
> Sounds right.  Or else get_index_isclustered() could be redefined to take a
> boolean "do_elog" flag, and if syscache fails and that's false, then return
> false instead of ERROR.

Not sure if that's completely right to do either.  For one, it is not
consistent with the surroundings as of lsyscache.c.
--
Michael

On 2020-Apr-02, Michael Paquier wrote:

> Now, regarding patch 0002, note that you have a problem for this part:
> -            tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
> -            if (!HeapTupleIsValid(tuple))    /* probably can't happen */
> -            {
> -                relation_close(OldHeap, AccessExclusiveLock);
> -                pgstat_progress_end_command();
> -                return;
> -            }
> -            indexForm = (Form_pg_index) GETSTRUCT(tuple);
> -            if (!indexForm->indisclustered)
> +            if (!get_index_isclustered(indexOid))
>              {
> -                ReleaseSysCache(tuple);
>                  relation_close(OldHeap, AccessExclusiveLock);
>                  pgstat_progress_end_command();
>                  return;
>              }
> -            ReleaseSysCache(tuple);
> 
> On an invalid tuple for pg_index, the new code would issue an error,
> while the old code would just return.

I don't think we need to worry about that problem, because we already
checked that the pg_class tuple for the index is there two lines above.
The pg_index tuple cannot have gone away on its own; and the index can't
be deleted either, because cluster_rel holds AEL on the table.  There
isn't "probably" about the can't-happen condition, is there?

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

On Thu, Apr 02, 2020 at 04:24:03PM +0900, Michael Paquier wrote:
> On Thu, Apr 02, 2020 at 01:52:09AM -0500, Justin Pryzby wrote:
>> Sounds right.  Or else get_index_isclustered() could be redefined to take a
>> boolean "do_elog" flag, and if syscache fails and that's false, then return
>> false instead of ERROR.
>
> Not sure if that's completely right to do either.  For one, it is not
> consistent with the surroundings as of lsyscache.c.

Actually, we do have some missing_ok flags lying around already in
lsyscache.c, so it would be much more consistent to use that name that
instead of the do_elog you are suggesting.  Could you update the
patch to reflect that?
--
Michael

On Thu, Apr 02, 2020 at 04:38:36AM -0300, Alvaro Herrera wrote:
> I don't think we need to worry about that problem, because we already
> checked that the pg_class tuple for the index is there two lines above.
> The pg_index tuple cannot have gone away on its own; and the index can't
> be deleted either, because cluster_rel holds AEL on the table.  There
> isn't "probably" about the can't-happen condition, is there?

Yes, you are right here.  I was wondering about an interference with
the multi-relation cluster that would not lock the parent relation at
the upper level of cluster() but the check on the existence of the
index makes sure that we'll never see an invalid entry in pg_index, so
let's keep patch 0002 as originally presented.  As the commit tree is
going to be rather crowded until feature freeze on Sunday, I'll wait
until Monday or Tuesday to finalize this patch set.

Now, would it be better to apply the refactoring patch for HEAD before
feature freeze, or are people fine if this is committed a bit after?
Patch 0002 is neither a new feature, nor an actual bug, and just some
code cleanup, but I am a bit worried about applying that cleanup on
HEAD just after the freeze.
--
Michael

On Fri, Apr 03, 2020 at 03:54:38PM +0900, Michael Paquier wrote:
> Now, would it be better to apply the refactoring patch for HEAD before
> feature freeze, or are people fine if this is committed a bit after?
> Patch 0002 is neither a new feature, nor an actual bug, and just some
> code cleanup, but I am a bit worried about applying that cleanup on
> HEAD just after the freeze.

I have worked more on this one this morning and just applied the bug
fix down to 9.5, and the refactoring on HEAD.  Thanks!
--
Michael