CREATE INDEX CONCURRENTLY on partitioned index

On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote:

Forking this thread, since the existing CFs have been closed.
/messages/by-id/20200914143102.GX18552@telsasoft.com

The strategy is to create catalog entries for all tables with indisvalid=false,
and then process them like REINDEX CONCURRENTLY. If it's interrupted, it
leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, same as
CIC on a plain table.

On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote:

On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote:

On Mon, Jun 15, 2020 at 08:15:05PM +0800, 李杰(慎追) wrote:

As shown above, an error occurred while creating an index in the second partition.
It can be clearly seen that the index of the partitioned table is invalid
and the index of the first partition is normal, the second partition is invalid,
and the Third Partition index does not exist at all.

That's a problem. I really think that we should make the steps of the
concurrent operation consistent across all relations, meaning that all
the indexes should be created as invalid for all the parts of the
partition tree, including partitioned tables as well as their
partitions, in the same transaction. Then a second new transaction
gets used for the index build, followed by a third one for the
validation that switches the indexes to become valid.

Note that the mentioned problem wasn't serious: there was missing index on
child table, therefor the parent index was invalid, as intended. However I
agree that it's not nice that the command can fail so easily and leave behind
some indexes created successfully and some failed some not created at all.

But I took your advice initially creating invalid inds.

...

That gave me the idea to layer CIC on top of Reindex, since I think it does
exactly what's needed.

On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote:

On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote:

It would be good also to check if
we have a partition index tree that maps partially with a partition
table tree (aka no all table partitions have a partition index), where
these don't get clustered because there is no index to work on.

This should not happen, since a incomplete partitioned index is "invalid".

@cfbot: rebased over recent changes to indexcmds.c

--
Justin

On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote:

Forking this thread, since the existing CFs have been closed.
/messages/by-id/20200914143102.GX18552@telsasoft.com

The strategy is to create catalog entries for all tables with indisvalid=false,
and then process them like REINDEX CONCURRENTLY. If it's interrupted, it
leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, same as
CIC on a plain table.

On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote:

On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote:

On Mon, Jun 15, 2020 at 08:15:05PM +0800, 李杰(慎追) wrote:

As shown above, an error occurred while creating an index in the second partition.
It can be clearly seen that the index of the partitioned table is invalid
and the index of the first partition is normal, the second partition is invalid,
and the Third Partition index does not exist at all.

That's a problem. I really think that we should make the steps of the
concurrent operation consistent across all relations, meaning that all
the indexes should be created as invalid for all the parts of the
partition tree, including partitioned tables as well as their
partitions, in the same transaction. Then a second new transaction
gets used for the index build, followed by a third one for the
validation that switches the indexes to become valid.

Note that the mentioned problem wasn't serious: there was missing index on
child table, therefor the parent index was invalid, as intended. However I
agree that it's not nice that the command can fail so easily and leave behind
some indexes created successfully and some failed some not created at all.

But I took your advice initially creating invalid inds.

...

That gave me the idea to layer CIC on top of Reindex, since I think it does
exactly what's needed.

On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote:

On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote:

It would be good also to check if
we have a partition index tree that maps partially with a partition
table tree (aka no all table partitions have a partition index), where
these don't get clustered because there is no index to work on.

This should not happen, since a incomplete partitioned index is "invalid".

@cfbot: rebased over recent changes to indexcmds.c

Status update for a commitfest entry.

This patch has not been updated and "Waiting on Author" status since
Nov 30. Are you still planning to work on this, Justin? If no, I'm
going to set this entry to "Returned with Feedback" barring
objections.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, Jan 28, 2021 at 09:51:51PM +0900, Masahiko Sawada wrote:

On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote:

Forking this thread, since the existing CFs have been closed.
/messages/by-id/20200914143102.GX18552@telsasoft.com

The strategy is to create catalog entries for all tables with indisvalid=false,
and then process them like REINDEX CONCURRENTLY. If it's interrupted, it
leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, same as
CIC on a plain table.

On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote:

On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote:
Note that the mentioned problem wasn't serious: there was missing index on
child table, therefor the parent index was invalid, as intended. However I
agree that it's not nice that the command can fail so easily and leave behind
some indexes created successfully and some failed some not created at all.

But I took your advice initially creating invalid inds.

...

That gave me the idea to layer CIC on top of Reindex, since I think it does
exactly what's needed.

On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote:

On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote:

It would be good also to check if
we have a partition index tree that maps partially with a partition
table tree (aka no all table partitions have a partition index), where
these don't get clustered because there is no index to work on.

This should not happen, since a incomplete partitioned index is "invalid".

@cfbot: rebased over recent changes to indexcmds.c

Status update for a commitfest entry.

This patch has not been updated and "Waiting on Author" status since
Nov 30. Are you still planning to work on this, Justin? If no, I'm
going to set this entry to "Returned with Feedback" barring
objections.

I had been waiting to rebase since there hasn't been any review comments and I
expected additional, future conflicts.

--
Justin

On 28.01.2021 17:30, Justin Pryzby wrote:

On Thu, Jan 28, 2021 at 09:51:51PM +0900, Masahiko Sawada wrote:

On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote:

Forking this thread, since the existing CFs have been closed.
/messages/by-id/20200914143102.GX18552@telsasoft.com

The strategy is to create catalog entries for all tables with indisvalid=false,
and then process them like REINDEX CONCURRENTLY. If it's interrupted, it
leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, same as
CIC on a plain table.

On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote:

On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote:
Note that the mentioned problem wasn't serious: there was missing index on
child table, therefor the parent index was invalid, as intended. However I
agree that it's not nice that the command can fail so easily and leave behind
some indexes created successfully and some failed some not created at all.

But I took your advice initially creating invalid inds.

...

That gave me the idea to layer CIC on top of Reindex, since I think it does
exactly what's needed.

On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote:

On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote:

It would be good also to check if
we have a partition index tree that maps partially with a partition
table tree (aka no all table partitions have a partition index), where
these don't get clustered because there is no index to work on.

This should not happen, since a incomplete partitioned index is "invalid".

I had been waiting to rebase since there hasn't been any review comments and I
expected additional, future conflicts.

I attempted to review this feature, but the last patch conflicts with
the recent refactoring, so I wasn't able to test it properly.
Could you please send a new version?

Meanwhile, here are my questions about the patch:

1) I don't see a reason to change the logic here. We don't skip counting
existing indexes when create parent index. Why should we skip them in
CONCURRENTLY mode?

��� ��� ��� // If concurrent, maybe this should be done after excluding
indexes which already exist ?
pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
��� ��� ��� ��� ��� ��� ��� ��� ��� ��� �nparts);

2) Here we access relation field after closing the relation. Is it safe?

��� /* save lockrelid and locktag for below */
��� heaprelid = rel->rd_lockInfo.lockRelId;

3) leaf_partitions() function only handles indexes, so I suggest to name
it more specifically and add a comment about meaning of 'options' parameter.

4) I don't quite understand the idea of the regression test. Why do we
expect to see invalid indexes there?
+��� "idxpart_a_idx1" UNIQUE, btree (a) INVALID

5) Speaking of documentation, I think we need to add a paragraph about
CIC on partitioned indexes which will explain that invalid indexes may
appear and what user should do to fix them.

6) ReindexIndexesConcurrently() needs some code cleanup.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 28.01.2021 17:30, Justin Pryzby wrote:

On Thu, Jan 28, 2021 at 09:51:51PM +0900, Masahiko Sawada wrote:

On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote:

Forking this thread, since the existing CFs have been closed.
/messages/by-id/20200914143102.GX18552@telsasoft.com

The strategy is to create catalog entries for all tables with indisvalid=false,
and then process them like REINDEX CONCURRENTLY. If it's interrupted, it
leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, same as
CIC on a plain table.

On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote:

On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote:
Note that the mentioned problem wasn't serious: there was missing index on
child table, therefor the parent index was invalid, as intended. However I
agree that it's not nice that the command can fail so easily and leave behind
some indexes created successfully and some failed some not created at all.

But I took your advice initially creating invalid inds.

...

That gave me the idea to layer CIC on top of Reindex, since I think it does
exactly what's needed.

On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote:

On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote:

It would be good also to check if
we have a partition index tree that maps partially with a partition
table tree (aka no all table partitions have a partition index), where
these don't get clustered because there is no index to work on.

This should not happen, since a incomplete partitioned index is "invalid".

I had been waiting to rebase since there hasn't been any review comments and I
expected additional, future conflicts.

I attempted to review this feature, but the last patch conflicts with
the recent refactoring, so I wasn't able to test it properly.
Could you please send a new version?

Meanwhile, here are my questions about the patch:

1) I don't see a reason to change the logic here. We don't skip counting
existing indexes when create parent index. Why should we skip them in
CONCURRENTLY mode?

��� ��� ��� // If concurrent, maybe this should be done after excluding
indexes which already exist ?
pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
��� ��� ��� ��� ��� ��� ��� ��� ��� ��� �nparts);

2) Here we access relation field after closing the relation. Is it safe?

��� /* save lockrelid and locktag for below */
��� heaprelid = rel->rd_lockInfo.lockRelId;

3) leaf_partitions() function only handles indexes, so I suggest to name
it more specifically and add a comment about meaning of 'options' parameter.

4) I don't quite understand the idea of the regression test. Why do we
expect to see invalid indexes there?
+��� "idxpart_a_idx1" UNIQUE, btree (a) INVALID

5) Speaking of documentation, I think we need to add a paragraph about
CIC on partitioned indexes which will explain that invalid indexes may
appear and what user should do to fix them.

6) ReindexIndexesConcurrently() needs some code cleanup.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Feb 15, 2021 at 10:06:47PM +0300, Anastasia Lubennikova wrote:

On 28.01.2021 17:30, Justin Pryzby wrote:

On Thu, Jan 28, 2021 at 09:51:51PM +0900, Masahiko Sawada wrote:

On Mon, Nov 30, 2020 at 5:22 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote:

Forking this thread, since the existing CFs have been closed.
/messages/by-id/20200914143102.GX18552@telsasoft.com

The strategy is to create catalog entries for all tables with indisvalid=false,
and then process them like REINDEX CONCURRENTLY. If it's interrupted, it
leaves INVALID indexes, which can be cleaned up with DROP or REINDEX, same as
CIC on a plain table.

On Sat, Aug 08, 2020 at 01:37:44AM -0500, Justin Pryzby wrote:

On Mon, Jun 15, 2020 at 09:37:42PM +0900, Michael Paquier wrote:
Note that the mentioned problem wasn't serious: there was missing index on
child table, therefor the parent index was invalid, as intended. However I
agree that it's not nice that the command can fail so easily and leave behind
some indexes created successfully and some failed some not created at all.

But I took your advice initially creating invalid inds.

...

That gave me the idea to layer CIC on top of Reindex, since I think it does
exactly what's needed.

On Sat, Sep 26, 2020 at 02:56:55PM -0500, Justin Pryzby wrote:

On Thu, Sep 24, 2020 at 05:11:03PM +0900, Michael Paquier wrote:

It would be good also to check if
we have a partition index tree that maps partially with a partition
table tree (aka no all table partitions have a partition index), where
these don't get clustered because there is no index to work on.

This should not happen, since a incomplete partitioned index is "invalid".

I had been waiting to rebase since there hasn't been any review comments and I
expected additional, future conflicts.

I attempted to review this feature, but the last patch conflicts with the
recent refactoring, so I wasn't able to test it properly.
Could you please send a new version?

I rebased this yesterday, so here's my latest.

2) Here we access relation field after closing the relation. Is it safe?
��� /* save lockrelid and locktag for below */
��� heaprelid = rel->rd_lockInfo.lockRelId;

Thanks, fixed this just now.

3) leaf_partitions() function only handles indexes, so I suggest to name it
more specifically and add a comment about meaning of 'options' parameter.

4) I don't quite understand the idea of the regression test. Why do we
expect to see invalid indexes there?
+��� "idxpart_a_idx1" UNIQUE, btree (a) INVALID

Because of the unique failure:
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+ERROR:  could not create unique index "idxpart2_a_idx2_ccnew"
+DETAIL:  Key (a)=(10) is duplicated.
+\d idxpart

This shows that CIC first creates catalog-only INVALID indexes, and then
reindexes them to "validate".

--
Justin

Hi,
For v13-0006-More-refactoring.patch :

+   /* It's not a shared catalog, so refuse to move it to shared tablespace
*/
+   if (params->tablespaceOid == GLOBALTABLESPACE_OID && false)
+       ereport(ERROR,

Do you intend to remove the ineffective check ?

+       else
+           heapRelation = table_open(heapId,
+                                     ShareUpdateExclusiveLock);
+       table_close(heapRelation, NoLock);

The table_open() seems to be unnecessary since there is no check after the
open.

+ // heapRelationIds = list_make1_oid(heapId);
If the code is not needed, you can remove the above.

For v13-0005-Refactor-to-allow-reindexing-all-index-partition.patch :

+       /* Skip invalid indexes, if requested */
+       if ((options & REINDEXOPT_SKIPVALID) != 0 &&
+               get_index_isvalid(partoid))

The comment seems to diverge from the name of the flag (which says skip
valid index).

Cheers

On Mon, Feb 15, 2021 at 11:34 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Mon, Feb 15, 2021 at 10:07:05PM +0300, Anastasia Lubennikova wrote:

5) Speaking of documentation, I think we need to add a paragraph about CIC
on partitioned indexes which will explain that invalid indexes may appear
and what user should do to fix them.

I'm not sure about that - it's already documented in general, for
nonpartitioned indexes.

--
Justin

Justin Pryzby писал 2021-02-26 21:20:

On Mon, Feb 15, 2021 at 10:07:05PM +0300, Anastasia Lubennikova wrote:

5) Speaking of documentation, I think we need to add a paragraph about
CIC
on partitioned indexes which will explain that invalid indexes may
appear
and what user should do to fix them.

I'm not sure about that - it's already documented in general, for
nonpartitioned indexes.

Hi.

I've rebased patches and tried to fix issues I've seen. I've fixed
reference after table_close() in the first patch (can be seen while
building with CPPFLAGS='-DRELCACHE_FORCE_RELEASE'). It seems childidxs
shouldn't live in ind_context, so I moved it out of it. Updated
documentation to state that CIC can leave invalid or valid indexes on
partitions if it's not succeeded. Also merged old
0002-f-progress-reporting.patch and
0003-WIP-Add-SKIPVALID-flag-for-more-integration.patch. It seems the
first one didn't really fixed issue with progress report (as
ReindexRelationConcurrently() uses pgstat_progress_start_command(),
which seems to mess up the effect of this command in DefineIndex()).
Note, that third patch completely removes attempts to report create
index progress correctly (reindex reports about individual commands, not
the whole CREATE INDEX).

So I've added 0003-Try-to-fix-create-index-progress-report.patch, which
tries to fix the mess with create index progress report. It introduces
new flag REINDEXOPT_REPORT_CREATE_PART to ReindexParams->options. Given
this flag, ReindexRelationConcurrently() will not report about
individual operations start/stop, but ReindexMultipleInternal() will
report about reindexed partitions. To make the issue worse, some
partitions can be handled in ReindexPartitions() and
ReindexMultipleInternal() should know how many to correctly update
PROGRESS_CREATEIDX_PARTITIONS_DONE counter. Also it needs IndexOid to
correctly generate pg_stat_progress_create_index record, so we pass
these parameters to it.
--
Best regards,
Alexander Pyhalov,
Postgres Professional

Hi.

I've added 0005-Mark-intermediate-partitioned-indexes-as-valid.patch
which fixed the following issues - when partitioned index is created,
indexes on intermediate partitioned tables were preserved in invalid
state. Also added some more tests.
--
Best regards,
Alexander Pyhalov,
Postgres Professional

This patch is marked "waiting on author" in the CF. However the most
recent emails have patches and it's not clear to me what's left from
previous reviews that might not be addressed yet. Should this patch be
marked "Needs Review"?

Anastasia and Alexander are marked as reviewers. Are you still able to
review it or are there still pending issues that need to be resolved
from previous reviews?

On Fri, Mar 25, 2022 at 01:05:49AM -0400, Greg Stark wrote:

This patch is marked "waiting on author" in the CF. However the most
recent emails have patches and it's not clear to me what's left from
previous reviews that might not be addressed yet. Should this patch be
marked "Needs Review"?

Anastasia and Alexander are marked as reviewers. Are you still able to
review it or are there still pending issues that need to be resolved
from previous reviews?

I still haven't responded to Alexander's feedback, so I need to do that.
(Sorry).

However, since the patch attracted no attention for 50 some weeks last year, so
now is a weird time to shift attention to it. As such, I will move it to the
next CF.

/messages/by-id/20210226182019.GU20769@telsasoft.com

--
Justin

Hi,

On Thu, Feb 10, 2022 at 06:07:08PM +0300, Alexander Pyhalov wrote:

I've rebased patches and tried to fix issues I've seen. I've fixed reference
after table_close() in the first patch (can be seen while building with
CPPFLAGS='-DRELCACHE_FORCE_RELEASE').

Thanks for finding that.

The patches other than 0001 are more experimental, and need someone to check if
it's even a good approach to use, so I kept them separate from the essential
patch.

Your latest 0005 patch (mark intermediate partitioned indexes as valid) is
probably fixing a bug in my SKIPVALID patch, right ? I'm not sure whether the
SKIPVALID patch should be merged into 0001, and I've been awaiting feedback on
the main patch before handling progress reporting.

Sorry for not responding sooner. The patch saw no activity for ~11 months so I
wasn't prepared to pick it up in March, at least not without guidance from a
committer.

Would you want to take over this patch ? I wrote it following someone's
question, but don't expect that I'd use the feature myself. I can help review
it or try to clarify the organization of my existing patches (but still haven't
managed to work my way through your amendments to my patches).

Thanks for caring about partitioned DDL ;)

--
Justin

Justin Pryzby писал 2022-06-28 21:33:

Hi,

On Thu, Feb 10, 2022 at 06:07:08PM +0300, Alexander Pyhalov wrote:

I've rebased patches and tried to fix issues I've seen. I've fixed
reference
after table_close() in the first patch (can be seen while building
with
CPPFLAGS='-DRELCACHE_FORCE_RELEASE').

Thanks for finding that.

The patches other than 0001 are more experimental, and need someone to
check if
it's even a good approach to use, so I kept them separate from the
essential
patch.

Your latest 0005 patch (mark intermediate partitioned indexes as valid)
is
probably fixing a bug in my SKIPVALID patch, right ? I'm not sure
whether the
SKIPVALID patch should be merged into 0001, and I've been awaiting
feedback on
the main patch before handling progress reporting.

Hi. I think it's more about fixing ReindexPartitions-to-set-indisvalid
patch, as
we also should mark intermediate indexes as valid when reindex succeeds.

Sorry for not responding sooner. The patch saw no activity for ~11
months so I
wasn't prepared to pick it up in March, at least not without guidance
from a
committer.

Would you want to take over this patch ? I wrote it following
someone's
question, but don't expect that I'd use the feature myself. I can help
review
it or try to clarify the organization of my existing patches (but still
haven't
managed to work my way through your amendments to my patches).

Yes, I'm glad to work on the patches, as this for us this is a very
important feature.
--
Best regards,
Alexander Pyhalov,
Postgres Professional

Justin Pryzby писал 2022-06-28 21:33:

Hi,

On Thu, Feb 10, 2022 at 06:07:08PM +0300, Alexander Pyhalov wrote:

I've rebased patches and tried to fix issues I've seen. I've fixed
reference
after table_close() in the first patch (can be seen while building
with
CPPFLAGS='-DRELCACHE_FORCE_RELEASE').

Rebased patches on the current master.
They still require proper review.

--
Best regards,
Alexander Pyhalov,
Postgres Professional

I finally found time to digest and integrate your changes into my local
branch. This fixes the three issues you reported: FORCE_RELEASE, issue
with INVALID partitions issue (for which I adapted your patch into an
earlier patch in my series), and progress reporting. And rebased.

--
Justin

Justin Pryzby писал 2022-11-21 06:00:

I finally found time to digest and integrate your changes into my local
branch. This fixes the three issues you reported: FORCE_RELEASE, issue
with INVALID partitions issue (for which I adapted your patch into an
earlier patch in my series), and progress reporting. And rebased.

Hi.

Thank you for the effort.
I've looked through and tested new patch a bit. Overall it looks good to
me.
The question I have is whether we should update
pg_stat_progress_create_index in reindex_invalid_child_indexes(), when
we skip valid indexes?
--
Best regards,
Alexander Pyhalov,
Postgres Professional

Hi,

Thank you Justin and Alexander for working on this, I have reviewed and
tested the latest patch, it works well, the problems mentioned
previously are all fixed. I like the idea of sharing code of reindex
and index, but I have noticed some peculiarities as a user. 

The reporting is somewhat confusing as it switches to reporting for
reindex concurrently while building child indexes, this should be fixed
with the simple patch I have attached. Another thing that I have
noticed is that REINDEX, which is used under the hood, creates new
indexes with suffix _ccnew, and if the index building fails, the
indexes that could not be build will have the name with _ccnew suffix.
This can actually be seen in your test:

ERROR: could not create unique index "idxpart2_a_idx2_ccnew"

I find it quite confusing and I don't think that this the expected
behavior (if it is, I think it should be documented, like it is for
REINDEX). As an example of problems that it might entail, DROP INDEX
will not drop all the invalid indexes in the inheritance tree, because
it will leave _ccnew indexes in place, which is ok for reindex
concurrently, but that's not how C-I-C works now. I think that fixing
this problem requires some heavy code rewrite and I'm not quite sure
how to go about it, if you have any ideas, I will be happy to try them
out.

Thanks,
Ilya

On Sat, Dec 03, 2022 at 07:13:30PM +0400, Ilya Gladyshev wrote:

Hi,

Thank you Justin and Alexander for working on this, I have reviewed and
tested the latest patch, it works well, the problems mentioned
previously are all fixed. I like the idea of sharing code of reindex
and index, but I have noticed some peculiarities as a user.�

The reporting is somewhat confusing as it switches to reporting for
reindex concurrently while building child indexes, this should be fixed
with the simple patch I have attached. Another thing that I have
noticed is that REINDEX, which is used under the hood, creates new
indexes with suffix _ccnew, and if the index building fails, the
indexes that could not be build will have the name with _ccnew suffix.
This can actually be seen in your test:

ERROR: could not create unique index "idxpart2_a_idx2_ccnew"

I find it quite confusing and I don't think that this the expected
behavior (if it is, I think it should be documented, like it is for
REINDEX). As an example of problems that it might entail, DROP INDEX
will not drop all the invalid indexes in the inheritance tree, because
it will leave _ccnew indexes in place, which is ok for reindex
concurrently, but that's not how C-I-C works now. I think that fixing
this problem requires some heavy code rewrite and I'm not quite sure

This beavior is fixed. I re-factored and re-implented to use
DefineIndex() for building indexes concurrently rather than reindexing.
That makes the patch smaller, actually, and has the added benefit of
splitting off the "Concurrently" part of DefineIndex() into a separate
function.

This currently handles partitions with a loop around the whole CIC
implementation, which means that things like WaitForLockers() happen
once for each index, the same as REINDEX CONCURRENTLY on a partitioned
table. Contrast that with ReindexRelationConcurrently(), which handles
all the indexes on a table in one pass by looping around indexes within
each phase.

BTW, it causes the patch to fail to apply in cfbot when you send an
additional (002) supplementary patch without including the original
(001) patch. You can name it *.txt to avoid the issue.
https://wiki.postgresql.org/wiki/Cfbot#Which_attachments_are_considered_to_be_patches.3F

Thanks for looking.

--
Justin