Thread: CREATE INDEX CONCURRENTLY on partitioned index

Forking this thread, since the existing CFs have been closed.
https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd

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

-- 
Justin

On Sat, Oct 31, 2020 at 01:31:17AM -0500, Justin Pryzby wrote:
> Forking this thread, since the existing CFs have been closed.
> https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd
> 
> 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.
> > https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd
> >
> > 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.
> > >
https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd
> > >
> > > 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.
>>>>
https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd
>>>>
>>>> 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.
>>>>
https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd
>>>>
>>>> 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.
> > > > >
https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd
> > > > > 
> > > > > 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

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.

https://www.postgresql.org/message-id/flat/20210226182019.GU20769@telsasoft.com#da169a0a518bf8121604437d9ab053b3

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

On Sun, 2022-12-04 at 13:09 -0600, Justin Pryzby wrote:
> 
> 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.

Nice, I think it turned out pretty concise. I played around with the
patch quite a bit, didn't find any major problems, the only minor thing
that I can note is that we should skip the top parent index itself in
the loop not to increment the pg_stat counter, something like this:

diff --git a/src/backend/commands/indexcmds.c
b/src/backend/commands/indexcmds.c
index cfab45b999..9049540b5b 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1515,6 +1515,9 @@ DefineIndex(Oid relationId,
                        Oid                     indrelid =
lfirst_oid(lc);
                        Oid                     tabrelid =
IndexGetRelation(indrelid, false);
 
+                       if (indrelid == indexRelationId)
+                               continue;
+
                        if
(RELKIND_HAS_STORAGE(get_rel_relkind(indrelid)) &&
                                !get_index_isvalid(indrelid))
                        {
> 
> 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.
> 
My bad, didn't know about this, thanks for the link.

On a side note, I noticed that reindex behaviour is strange on
partitioned tables, it doesn't mark partitioned tables as valid after
reindexing children, as I could understand from the code and mailing
lists, this is the intended behaviour, but I can't quite understand the
rationale for it, do you know why it is done this way?

On Sun, Dec 04, 2022 at 01:09:35PM -0600, Justin Pryzby wrote:
> 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.

Rebased over the progress reporting fix (27f5c712b).

I added a list of (intermediate) partitioned tables, rather than looping
over the list of inheritors again, to save calling rel_get_relkind().

I think this patch is done.

-- 
Justin

Justin Pryzby писал 2023-03-26 17:51:
> On Sun, Dec 04, 2022 at 01:09:35PM -0600, Justin Pryzby wrote:
>> 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.
> 
> Rebased over the progress reporting fix (27f5c712b).
> 
> I added a list of (intermediate) partitioned tables, rather than 
> looping
> over the list of inheritors again, to save calling rel_get_relkind().
> 
> I think this patch is done.

Hi.

Overall looks good to me. However, I think that using 'partitioned' as 
list of partitioned index oids in DefineIndex() is a bit misleading - 
we've just used it as boolean, specifying if we are dealing with a 
partitioned relation.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

On Mon, Mar 27, 2023 at 01:28:24PM +0300, Alexander Pyhalov wrote:
> Justin Pryzby писал 2023-03-26 17:51:
> > On Sun, Dec 04, 2022 at 01:09:35PM -0600, Justin Pryzby wrote:
> > > 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.
> > 
> > Rebased over the progress reporting fix (27f5c712b).
> > 
> > I added a list of (intermediate) partitioned tables, rather than looping
> > over the list of inheritors again, to save calling rel_get_relkind().
> > 
> > I think this patch is done.
> 
> Overall looks good to me. However, I think that using 'partitioned' as list
> of partitioned index oids in DefineIndex() is a bit misleading - we've just
> used it as boolean, specifying if we are dealing with a partitioned
> relation.

Right.  This is also rebased on 8c852ba9a4 (Allow some exclusion
constraints on partitions).

-- 
Justin

Justin Pryzby писал 2023-07-13 05:27:
> On Mon, Mar 27, 2023 at 01:28:24PM +0300, Alexander Pyhalov wrote:
>> Justin Pryzby писал 2023-03-26 17:51:
>> > On Sun, Dec 04, 2022 at 01:09:35PM -0600, Justin Pryzby wrote:
>> > > 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.
>> >
>> > Rebased over the progress reporting fix (27f5c712b).
>> >
>> > I added a list of (intermediate) partitioned tables, rather than looping
>> > over the list of inheritors again, to save calling rel_get_relkind().
>> >
>> > I think this patch is done.
>> 
>> Overall looks good to me. However, I think that using 'partitioned' as 
>> list
>> of partitioned index oids in DefineIndex() is a bit misleading - we've 
>> just
>> used it as boolean, specifying if we are dealing with a partitioned
>> relation.
> 
> Right.  This is also rebased on 8c852ba9a4 (Allow some exclusion
> constraints on partitions).

Hi.
I have some more question.
In the following code (indexcmds.c:1640 and later)

1640                         rel = table_open(relationId, 
ShareUpdateExclusiveLock);
1641                         heaprelid = rel->rd_lockInfo.lockRelId;
1642                         table_close(rel, ShareUpdateExclusiveLock);
1643                         SET_LOCKTAG_RELATION(heaplocktag, 
heaprelid.dbId, heaprelid.relId);

should we release ShareUpdateExclusiveLock before getting session lock 
in DefineIndexConcurrentInternal()?
Also we unlock parent table there between reindexing childs in the end 
of DefineIndexConcurrentInternal():

1875         /*
1876          * Last thing to do is release the session-level lock on 
the parent table.
1877          */
1878         UnlockRelationIdForSession(&heaprelid, 
ShareUpdateExclusiveLock);
1879 }


Is it safe? Shouldn't we hold session lock on the parent table while 
rebuilding child indexes?



-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

Hi.

I've rebased patch on master and it'seems to me there's one more issue -

when we call DefineIndexConcurrentInternal() in partitioned case, it 
waits for transactions, locking tableId, not tabrelid - heaprelid 
LockRelId is constructed for parent index relation, not for child index 
relation.

Attaching fixed version.

Also I'm not sure what to do with locking of child relations. If we 
don't do anything, you can drop one of the partitioned table childs 
while CIC is in progress, and get error

ERROR:  cache lookup failed for index 16399

If you try to lock all child tables in CIC session, you'll get 
deadlocks.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

Hi,

I think it's well worth the effort to revive the patch, so I rebased it 
on master, updated it and will return it back to the commitfest. 
Alexander, Justin feel free to add yourselves as authors

On 29.01.2024 12:43, Alexander Pyhalov wrote:
> Hi.
>
> I've rebased patch on master and it'seems to me there's one more issue -
>
> when we call DefineIndexConcurrentInternal() in partitioned case, it 
> waits for transactions, locking tableId, not tabrelid - heaprelid 
> LockRelId is constructed for parent index relation, not for child 
> index relation.
>
> Attaching fixed version.
>
> Also I'm not sure what to do with locking of child relations. If we 
> don't do anything, you can drop one of the partitioned table childs 
> while CIC is in progress, and get error
>
> ERROR:  cache lookup failed for index 16399
I agree that we need to do something about it, in particular, I think we 
should lock all the partitions inside the transaction that builds the 
catalog entries. Fixed this in the new version.
> If you try to lock all child tables in CIC session, you'll get deadlocks.

Do you mean the deadlock between the transaction that drops a partition 
and the transaction doing CIC? I think this is unavoidable and can be 
reproduced even without partitioning.

Also not sure why a list of children relation was obtained with 
ShareLock that CIC is supposed to avoid not to block writes, changed 
that to ShareUpdateExclusive.

Regards,

Ilya

Ilya Gladyshev писал(а) 2024-05-24 00:14:
> Hi,

Hi.

> 
> I think it's well worth the effort to revive the patch, so I rebased it 
> on master, updated it and will return it back to the commitfest. 
> Alexander, Justin feel free to add yourselves as authors
> 
> On 29.01.2024 12:43, Alexander Pyhalov wrote:
>> Hi.
>> 
>> I've rebased patch on master and it'seems to me there's one more issue 
>> -
>> 
>> when we call DefineIndexConcurrentInternal() in partitioned case, it 
>> waits for transactions, locking tableId, not tabrelid - heaprelid 
>> LockRelId is constructed for parent index relation, not for child 
>> index relation.
>> 
>> Attaching fixed version.
>> 
>> Also I'm not sure what to do with locking of child relations. If we 
>> don't do anything, you can drop one of the partitioned table childs 
>> while CIC is in progress, and get error
>> 
>> ERROR:  cache lookup failed for index 16399
> I agree that we need to do something about it, in particular, I think 
> we should lock all the partitions inside the transaction that builds 
> the catalog entries. Fixed this in the new version.
>> If you try to lock all child tables in CIC session, you'll get 
>> deadlocks.
> 
> Do you mean the deadlock between the transaction that drops a partition 
> and the transaction doing CIC? I think this is unavoidable and can be 
> reproduced even without partitioning.

Yes, it seems we trade this error for possible deadlock between 
transaction, dropping a partition, and CIC.

> 
> Also not sure why a list of children relation was obtained with 
> ShareLock that CIC is supposed to avoid not to block writes, changed 
> that to ShareUpdateExclusive.
> 

I expect that it wasn't an issue due to the fact that it's held for a 
brief period until DefineIndexConcurrentInternal() commits for the first 
time. But it seems, it's more correct to use ShareUpdateExclusive lock 
here.


Also I'd like to note that in new patch version there's a strange 
wording in documentation:

"This can be very convenient as not only will all existing partitions be
  indexed, but any future partitions will be as well.
  <command>CREATE INDEX ... CONCURRENTLY</command> can incur long lock 
times
  on huge partitioned tables, to avoid that you can
  use <command>CREATE INDEX ON ONLY</command> the partitioned table, 
which
  creates the new index marked as invalid, preventing automatic 
application
  to existing partitions."

All the point of CIC is to avoid long lock times. So it seems this 
paragraph should be rewritten in the following way:

"To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or 
CREATE INDEX ON ONLY</command> the partitioned table..."


-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

On 24.05.2024 10:04, Alexander Pyhalov wrote:
> Ilya Gladyshev писал(а) 2024-05-24 00:14:
>> Hi,
>
> Hi.
>
>>
>> I think it's well worth the effort to revive the patch, so I rebased 
>> it on master, updated it and will return it back to the commitfest. 
>> Alexander, Justin feel free to add yourselves as authors
>>
>> On 29.01.2024 12:43, Alexander Pyhalov wrote:
>>> Hi.
>>>
>>> I've rebased patch on master and it'seems to me there's one more 
>>> issue -
>>>
>>> when we call DefineIndexConcurrentInternal() in partitioned case, it 
>>> waits for transactions, locking tableId, not tabrelid - heaprelid 
>>> LockRelId is constructed for parent index relation, not for child 
>>> index relation.
>>>
>>> Attaching fixed version.
>>>
>>> Also I'm not sure what to do with locking of child relations. If we 
>>> don't do anything, you can drop one of the partitioned table childs 
>>> while CIC is in progress, and get error
>>>
>>> ERROR:  cache lookup failed for index 16399
>> I agree that we need to do something about it, in particular, I think 
>> we should lock all the partitions inside the transaction that builds 
>> the catalog entries. Fixed this in the new version.
>>> If you try to lock all child tables in CIC session, you'll get 
>>> deadlocks.
>>
>> Do you mean the deadlock between the transaction that drops a 
>> partition and the transaction doing CIC? I think this is unavoidable 
>> and can be reproduced even without partitioning.
>
> Yes, it seems we trade this error for possible deadlock between 
> transaction, dropping a partition, and CIC.
>
>>
>> Also not sure why a list of children relation was obtained with 
>> ShareLock that CIC is supposed to avoid not to block writes, changed 
>> that to ShareUpdateExclusive.
>>
>
> I expect that it wasn't an issue due to the fact that it's held for a 
> brief period until DefineIndexConcurrentInternal() commits for the 
> first time. But it seems, it's more correct to use 
> ShareUpdateExclusive lock here.
>
>
> Also I'd like to note that in new patch version there's a strange 
> wording in documentation:
>
> "This can be very convenient as not only will all existing partitions be
>  indexed, but any future partitions will be as well.
>  <command>CREATE INDEX ... CONCURRENTLY</command> can incur long lock 
> times
>  on huge partitioned tables, to avoid that you can
>  use <command>CREATE INDEX ON ONLY</command> the partitioned table, which
>  creates the new index marked as invalid, preventing automatic 
> application
>  to existing partitions."
>
> All the point of CIC is to avoid long lock times. So it seems this 
> paragraph should be rewritten in the following way:
>
> "To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or 
> CREATE INDEX ON ONLY</command> the partitioned table..."


True, the current wording doesn't look right. Right now CREATE INDEX ON 
ONLY is described as a workaround for the missing CIC. I think it rather 
makes sense to say that it gives more fine-grained control of partition 
locking than both CIC and ordinary CREATE INDEX. See the updated patch.

Ilya Gladyshev писал(а) 2024-05-28 02:52:

>> Also I'd like to note that in new patch version there's a strange 
>> wording in documentation:
>> 
>> "This can be very convenient as not only will all existing partitions 
>> be
>>  indexed, but any future partitions will be as well.
>>  <command>CREATE INDEX ... CONCURRENTLY</command> can incur long lock 
>> times
>>  on huge partitioned tables, to avoid that you can
>>  use <command>CREATE INDEX ON ONLY</command> the partitioned table, 
>> which
>>  creates the new index marked as invalid, preventing automatic 
>> application
>>  to existing partitions."
>> 
>> All the point of CIC is to avoid long lock times. So it seems this 
>> paragraph should be rewritten in the following way:
>> 
>> "To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or 
>> CREATE INDEX ON ONLY</command> the partitioned table..."
> 
> 
> True, the current wording doesn't look right. Right now CREATE INDEX ON 
> ONLY is described as a workaround for the missing CIC. I think it 
> rather makes sense to say that it gives more fine-grained control of 
> partition locking than both CIC and ordinary CREATE INDEX. See the 
> updated patch.

Hi.

Not sure if it's worth removing mentioning of CIC in

       creates the new index marked as invalid, preventing automatic 
application
       to existing partitions.  Instead, indexes can then be created 
individually
-     on each partition using <literal>CONCURRENTLY</literal> and
+     on each partition and
       <firstterm>attached</firstterm> to the partitioned index on the 
parent
       using <command>ALTER INDEX ... ATTACH PARTITION</command>.  Once 
indexes for
       all the partitions are attached to the parent index, the parent 
index will

but at least now it looks better.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

On 28.05.2024 07:05, Alexander Pyhalov wrote:
> Ilya Gladyshev писал(а) 2024-05-28 02:52:
>
>>> Also I'd like to note that in new patch version there's a strange 
>>> wording in documentation:
>>>
>>> "This can be very convenient as not only will all existing 
>>> partitions be
>>>  indexed, but any future partitions will be as well.
>>>  <command>CREATE INDEX ... CONCURRENTLY</command> can incur long 
>>> lock times
>>>  on huge partitioned tables, to avoid that you can
>>>  use <command>CREATE INDEX ON ONLY</command> the partitioned table, 
>>> which
>>>  creates the new index marked as invalid, preventing automatic 
>>> application
>>>  to existing partitions."
>>>
>>> All the point of CIC is to avoid long lock times. So it seems this 
>>> paragraph should be rewritten in the following way:
>>>
>>> "To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or 
>>> CREATE INDEX ON ONLY</command> the partitioned table..."
>>
>>
>> True, the current wording doesn't look right. Right now CREATE INDEX 
>> ON ONLY is described as a workaround for the missing CIC. I think it 
>> rather makes sense to say that it gives more fine-grained control of 
>> partition locking than both CIC and ordinary CREATE INDEX. See the 
>> updated patch.
>
> Hi.
>
> Not sure if it's worth removing mentioning of CIC in
>
>       creates the new index marked as invalid, preventing automatic 
> application
>       to existing partitions.  Instead, indexes can then be created 
> individually
> -     on each partition using <literal>CONCURRENTLY</literal> and
> +     on each partition and
>       <firstterm>attached</firstterm> to the partitioned index on the 
> parent
>       using <command>ALTER INDEX ... ATTACH PARTITION</command>.  Once 
> indexes for
>       all the partitions are attached to the parent index, the parent 
> index will
>
> but at least now it looks better.

The current patch version locks all the partitions in the first 
transaction up until each of them is built, which makes for long lock 
times for partitions that are built last. Having looked at the 
implementation of REINDEX CONCURRENTLY for partitioned tables, I think 
we can improve this by using the same approach of just skipping the 
relations that we find out are dropped when trying to lock them. 
Incidentally, this implementation in the new patch version is also simpler.

In addition, I noticed that progress tracking is once again broken for 
partitioned tables, while looking at REINDEX implementation, attaching 
the second patch to fix it.

On Thu, May 23, 2024 at 10:14:57PM +0100, Ilya Gladyshev wrote:
> Hi,
> 
> I think it's well worth the effort to revive the patch, so I rebased it on
> master, updated it and will return it back to the commitfest. Alexander,
> Justin feel free to add yourselves as authors

Thanks -- I was intending to write about this.

I realized that the patch will need some isolation tests to exercise its
concurrent behavior.

-- 
Justin

On Thu, May 23, 2024 at 10:14:57PM +0100, Ilya Gladyshev wrote:

Hi,

I think it's well worth the effort to revive the patch, so I rebased it on
master, updated it and will return it back to the commitfest. Alexander,
Justin feel free to add yourselves as authors

Thanks -- I was intending to write about this.

I realized that the patch will need some isolation tests to exercise its
concurrent behavior.

Thanks for the suggestion, added an isolation test that verifies behaviour of partitioned CIC with simultaneous partition drop/detach going on. Also fixed some issues in the new patch that I found while writing the test.

On Sat, Jun 15, 2024 at 07:56:38PM +0100, Ilya Gladyshev wrote:
> In addition, I noticed that progress tracking is once again broken for
> partitioned tables, while looking at REINDEX implementation, attaching the
> second patch to fix it.

Thanks for the fixes, I started reviewing them but need some more time
to digest.

Do you mean that progress reporting is broken in master, for REINDEX, or
just with this patch ?

-- 
Justin

It is broken in master, I just didn’t want to create a separate thread, but it can be fixed independently. As I
remember,the problem is that progress is tracked for each table in the hierarchy as if the table is processed
separately,without ever setting partitions_total and partitions_done counters. 

> 11 июля 2024 г., в 13:31, Justin Pryzby <pryzby@telsasoft.com> написал(а):
>
> On Sat, Jun 15, 2024 at 07:56:38PM +0100, Ilya Gladyshev wrote:
>> In addition, I noticed that progress tracking is once again broken for
>> partitioned tables, while looking at REINDEX implementation, attaching the
>> second patch to fix it.
>
> Thanks for the fixes, I started reviewing them but need some more time
> to digest.
>
> Do you mean that progress reporting is broken in master, for REINDEX, or
> just with this patch ?
>
> --
> Justin

On Thu, Jul 11, 2024 at 09:35:24PM +0100, Ilya Gladyshev wrote:
> It is broken in master, I just didn’t want to create a separate
> thread, but it can be fixed independently. As I remember, the
> problem is that progress is tracked for each table in the hierarchy
> as if the table is processed separately, without ever setting
> partitions_total and partitions_done counters.

Please let's move this point to its own thread and deal with it with
an independent patch.  Hiding that in a thread that's already quite
long is not a good idea.  This needs proper review, and a separate
thread with a good subject to describe the problem will attract a
better audience to deal with the problem you are seeing.

I was not paying much attention, until you've mentioned that this was
an issue with HEAD.
--
Michael

Please let's move this point to its own thread and deal with it with
an independent patch.  Hiding that in a thread that's already quite
long is not a good idea.  This needs proper review, and a separate
thread with a good subject to describe the problem will attract a
better audience to deal with the problem you are seeing.

I was not paying much attention, until you've mentioned that this was
an issue with HEAD.
--
Michael

Sure, created a separate thread [1]. Please disregard the second patch in this thread. Duplicating the last version of the relevant patch here to avoid any confusion.

[1] https://www.postgresql.org/message-id/b72f2d89-820a-4fa2-9058-b155cf646f4f%40gmail.com

On Fri, Jul 12, 2024 at 11:17:25PM +0100, Ilya Gladyshev wrote:
> Sure, created a separate thread [1]. Please disregard the second patch in
> this thread. Duplicating the last version of the relevant patch here to
> avoid any confusion.
>
> [1] https://www.postgresql.org/message-id/b72f2d89-820a-4fa2-9058-b155cf646f4f%40gmail.com

Thanks, will check that.
--
Michael