Re: Support for REINDEX CONCURRENTLY

Hi all,

One of the outputs on the discussions about the integration of pg_reorg in
core
was that Postgres should provide some ways to do REINDEX, CLUSTER and ALTER
TABLE concurrently with low-level locks in a way similar to CREATE INDEX
CONCURRENTLY.

The discussions done can be found on this thread:
http://archives.postgresql.org/pgsql-hackers/2012-09/msg00746.php

Well, I spent some spare time working on the implementation of REINDEX
CONCURRENTLY.
This basically allows to perform read and write operations on a table whose
index(es) are
reindexed at the same time. Pretty useful for a production environment. The
caveats of this
feature is that it is slower than normal reindex, and impacts other
backends with the extra CPU,
memory and IO it uses to process. The implementation is based on something
on the same ideas
as pg_reorg and on an idea of Andres.
Please find attached a version that I consider as a base for the next
discussions, perhaps
a version that could be submitted to the commit fest next month. Patch is
aligned with postgres
master at commit 09ac603.

With this feature, you can rebuild a table or an index with such commands:
REINDEX INDEX ind CONCURRENTLY;
REINDEX TABLE tab CONCURRENTLY;

The following restrictions are applied.
- REINDEX [ DATABASE | SYSTEM ] cannot be run concurrently.
- REINDEX CONCURRENTLY cannot run inside a transaction block.
- Shared tables cannot be reindexed concurrently
- indexes for exclusion constraints cannot be reindexed concurrently.
- toast relations are reindexed non-concurrently when table reindex is done
and that this table has toast relations

Here is a description of what happens when reorganizing an index
concurrently
(the beginning of the process is similar to CREATE INDEX CONCURRENTLY):
1) creation of a new index based on the same columns and restrictions as the
index that is rebuilt (called here old index). This new index has as name
$OLDINDEX_cct. So only a suffix _cct is added. It is marked as invalid and
not ready.
2) Take session locks on old and new index(es), and the parent table to
prevent
unfortunate drops.
3) Commit and start a new transaction
4) Wait until no running transactions could have the table open with the
old list of indexes.
5) Build the new indexes. All the new indexes are marked as indisready.
6) Commit and start a new transaction
7) Wait until no running transactions could have the table open with the
old list of indexes.
8) Take a reference snapshot and validate the new indexes
9) Wait for the old snapshots based on the reference snapshot
10) mark the new indexes as indisvalid
11) Commit and start a new transaction. At this point the old and new
indexes are both valid
12) Take a new reference snapshot and wait for the old snapshots to insure
that old
indexes are not corrupted,
13) Mark the old indexes as invalid
14) Swap new and old indexes, consisting here in switching their names.
15) Old indexes are marked as invalid.
16) Commit and start a new transaction
17) Wait for transactions that might use the old indexes
18) Old indexes are marked as not ready
19) Commit and start a new transaction
20) Drop the old indexes

The following process might be reducible, but I would like that to be
decided depending on
the community feedback and experience on such concurrent features.
For the time being I took an approach that looks slower, but secured to my
mind with multiple
waits (perhaps sometimes unnecessary?) and subtransactions.

If during the process an error occurs, the table will finish with either
the old or new index
as invalid. In this case the user will be in charge to drop the invalid
index himself.
The concurrent index can be easily identified with its suffix *_cct.

This patch has required some refactorisation effort as I noticed that the
code of index
for concurrent operations was not very generic. In order to do that, I
created some
new functions in index.c called index_concurrent_* which are used by CREATE
INDEX
and REINDEX in my patch. Some refactoring has also been done regarding the
wait processes.
REINDEX TABLE and REINDEX INDEX follow the same code path
(ReindexConcurrentIndexes
in indexcmds.c). The patch structure is relying a maximum on the functions
of index.c
when creating, building and validating concurrent index.

Based on the comments of this thread, I would like to submit the patch at
the next
commit fest. Just let me know if the approach taken by the current
implementation
is OK ot if it needs some modifications. That would be really helpful.

The patch includes some regression tests for error checks and also some
documentation.
Regressions are passing, code has no whitespaces and no compilation
warnings.
I have also done tests checking for read and write operations on index scan
of parent table
at each step of the process (by using gdb to stop the reindex process at
precise places).

Thanks, and looking forward to your feedback,
--
Michael Paquier
http://michael.otacoo.com

On 3 October 2012 02:14, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:

> Well, I spent some spare time working on the implementation of REINDEX
> CONCURRENTLY.

Thanks

> The following restrictions are applied.
> - REINDEX [ DATABASE | SYSTEM ] cannot be run concurrently.

Fair enough

> - indexes for exclusion constraints cannot be reindexed concurrently.
> - toast relations are reindexed non-concurrently when table reindex is done
> and that this table has toast relations

Those restrictions are important ones to resolve since they prevent
the CONCURRENTLY word from being true in a large proportion of cases.

We need to be clear that the remainder of this can be done in user
space already, so the proposal doesn't move us forwards very far,
except in terms of packaging. IMHO this needs to be more than just
moving a useful script into core.

> Here is a description of what happens when reorganizing an index
> concurrently

There are four waits for every index, again similar to what is
possible in user space.

When we refactor that, I would like to break things down into N
discrete steps, if possible. Each time we hit a wait barrier, a
top-level process would be able to switch to another task to avoid
waiting. This would then allow us to proceed more quickly through the
task. I would admit that is a later optimisation, but it would be
useful to have the innards refactored to allow for that more easily
later. I'd accept Not yet, if doing that becomes a problem in short
term.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi,

On Wednesday, October 03, 2012 03:14:17 AM Michael Paquier wrote:
> One of the outputs on the discussions about the integration of pg_reorg in
> core
> was that Postgres should provide some ways to do REINDEX, CLUSTER and ALTER
> TABLE concurrently with low-level locks in a way similar to CREATE INDEX
> CONCURRENTLY.
>
> The discussions done can be found on this thread:
> http://archives.postgresql.org/pgsql-hackers/2012-09/msg00746.php
>
> Well, I spent some spare time working on the implementation of REINDEX
> CONCURRENTLY.
Very cool!

> This basically allows to perform read and write operations on a table whose
> index(es) are reindexed at the same time. Pretty useful for a production
> environment. The caveats of this feature is that it is slower than normal
> reindex, and impacts other backends with the extra CPU, memory and IO it
> uses to process. The implementation is based on something on the same ideas
> as pg_reorg and on an idea of Andres.

> The following restrictions are applied.
> - REINDEX [ DATABASE | SYSTEM ] cannot be run concurrently.
I would like to support something like REINDEX USER TABLES; or similar at some
point, but that very well can be a second phase.

> - REINDEX CONCURRENTLY cannot run inside a transaction block.

> - toast relations are reindexed non-concurrently when table reindex is done
> and that this table has toast relations
Why that restriction?

> Here is a description of what happens when reorganizing an index
> concurrently
> (the beginning of the process is similar to CREATE INDEX CONCURRENTLY):
> 1) creation of a new index based on the same columns and restrictions as
> the index that is rebuilt (called here old index). This new index has as
> name $OLDINDEX_cct. So only a suffix _cct is added. It is marked as
> invalid and not ready.
You probably should take a SHARE UPDATE EXCLUSIVE lock on the table at that
point already, to prevent schema changes.

> 8) Take a reference snapshot and validate the new indexes
Hm. Unless you factor in corrupt indices, why should this be needed?

> 14) Swap new and old indexes, consisting here in switching their names.
I think switching based on their names is not going to work very well because
indexes are referenced by oid at several places. Swapping pg_index.indexrelid
or pg_class.relfilenode seems to be the better choice to me. We expect
relfilenode changes for such commands, but not ::regclass oid changes.

Such a behaviour would at least be complicated for pg_depend and
pg_constraint.

> The following process might be reducible, but I would like that to be
> decided depending on the community feedback and experience on such
> concurrent features.
> For the time being I took an approach that looks slower, but secured to my
> mind with multiple waits (perhaps sometimes unnecessary?) and
> subtransactions.

> If during the process an error occurs, the table will finish with either
> the old or new index as invalid. In this case the user will be in charge to
> drop the invalid index himself.
> The concurrent index can be easily identified with its suffix *_cct.
I am not really happy about relying on some arbitrary naming here. That still
can result in conflicts and such.

> This patch has required some refactorisation effort as I noticed that the
> code of index for concurrent operations was not very generic. In order to do
> that, I created some new functions in index.c called index_concurrent_*
> which are used by CREATE INDEX and REINDEX in my patch. Some refactoring has
> also been done regarding the> wait processes.

> REINDEX TABLE and REINDEX INDEX follow the same code path
> (ReindexConcurrentIndexes in indexcmds.c). The patch structure is relying a
> maximum on the functions of index.c when creating, building and validating
> concurrent index.
I haven't looked at the patch yet, but I was pretty sure that you would need
to do quite some refactoring to implement this and this looks like roughly the
right direction...

> Thanks, and looking forward to your feedback,
I am very happy that youre taking this on!

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Oct 3, 2012 at 5:10 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> > This basically allows to perform read and write operations on a table
> whose
> > index(es) are reindexed at the same time. Pretty useful for a production
> > environment. The caveats of this feature is that it is slower than
> normal
> > reindex, and impacts other backends with the extra CPU, memory and IO it
> > uses to process. The implementation is based on something on the same
> ideas
> > as pg_reorg and on an idea of Andres.
>
>
> > The following restrictions are applied.
> > - REINDEX [ DATABASE | SYSTEM ] cannot be run concurrently.
> I would like to support something like REINDEX USER TABLES; or similar at
> some
> point, but that very well can be a second phase.

This is something out of scope for the time being honestly. Later? why
not...

> > - REINDEX CONCURRENTLY cannot run inside a transaction block.
>
> > - toast relations are reindexed non-concurrently when table reindex is
> done
> > and that this table has toast relations
> Why that restriction?
>
This is the state of the current version of the patch. And not what the
final version should do. I agree that toast relations should also be
reindexed concurrently as the others. Regarding this current restriction,
my point was just to get some feedback before digging deeper. I should have
told that though...

>
> > Here is a description of what happens when reorganizing an index
> > concurrently
> > (the beginning of the process is similar to CREATE INDEX CONCURRENTLY):
> > 1) creation of a new index based on the same columns and restrictions as
> > the index that is rebuilt (called here old index). This new index has as
> > name $OLDINDEX_cct. So only a suffix _cct is added. It is marked as
> > invalid and not ready.
> You probably should take a SHARE UPDATE EXCLUSIVE lock on the table at that
> point already, to prevent schema changes.
>
> > 8) Take a reference snapshot and validate the new indexes
> Hm. Unless you factor in corrupt indices, why should this be needed?
>
> > 14) Swap new and old indexes, consisting here in switching their names.
> I think switching based on their names is not going to work very well
> because
> indexes are referenced by oid at several places. Swapping
> pg_index.indexrelid
> or pg_class.relfilenode seems to be the better choice to me. We expect
> relfilenode changes for such commands, but not ::regclass oid changes.
>
OK, so you mean to create an index, then switch only the relfilenode. Why
not. This is largely doable. I think that what is important here is to
choose a way of doing an keep it until the end.

>
> Such a behaviour would at least be complicated for pg_depend and
> pg_constraint.
>
> > The following process might be reducible, but I would like that to be
> > decided depending on the community feedback and experience on such
> > concurrent features.
> > For the time being I took an approach that looks slower, but secured to
> my
> > mind with multiple waits (perhaps sometimes unnecessary?) and
> > subtransactions.
>
> > If during the process an error occurs, the table will finish with either
> > the old or new index as invalid. In this case the user will be in charge
> to
> > drop the invalid index himself.
> > The concurrent index can be easily identified with its suffix *_cct.
> I am not really happy about relying on some arbitrary naming here. That
> still
> can result in conflicts and such.
>
The concurrent names are generated automatically with a function in
indexcmds.c, the same way a pkey indexes. Let's imagine that the
reindex concurrently command is run twice after a failure. The second
concurrent index will not have *_cct as suffix but _cct1. However I am open
to more ideas here. What I feel about the concurrent index is that it needs
a pg_class entry, even if it is just temporary, and this entry needs a name.

> > This patch has required some refactorisation effort as I noticed that the
> > code of index for concurrent operations was not very generic. In order
> to do
> > that, I created some new functions in index.c called index_concurrent_*
> > which are used by CREATE INDEX and REINDEX in my patch. Some refactoring
> has
> > also been done regarding the> wait processes.
>
> > REINDEX TABLE and REINDEX INDEX follow the same code path
> > (ReindexConcurrentIndexes in indexcmds.c). The patch structure is
> relying a
> > maximum on the functions of index.c when creating, building and
> validating
> > concurrent index.
> I haven't looked at the patch yet, but I was pretty sure that you would
> need
> to do quite some refactoring to implement this and this looks like roughly
> the
> right direction...
>
Thanks for spending time on it.
--
Michael Paquier
http://michael.otacoo.com

On 3 October 2012 09:10, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

>> The following restrictions are applied.
>> - REINDEX [ DATABASE | SYSTEM ] cannot be run concurrently.

> I would like to support something like REINDEX USER TABLES; or similar at some
> point, but that very well can be a second phase.

Yes, that would be a nice feature anyway, even without concurrently.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Just for background. The showstopper for REINDEX concurrently was not
that it was particularly hard to actually do the reindexing. But it's
not obvious how to obtain a lock on both the old and new index without
creating a deadlock risk. I don't remember exactly where the deadlock
risk lies but there are two indexes to lock and whichever order you
obtain the locks it might be possible for someone else to be waiting
to obtain them in the opposite order.

I'm sure it's possible to solve the problem. But the footwork needed
to release locks then reobtain them in the right order and verify that
the index hasn't changed out from under you might be a lot of
headache.

Perhaps a good way to tackle it is to have a generic "verify two
indexes are equivalent and swap the underlying relfilenodes" operation
that can be called from both regular reindex and reindex concurrently.
As long as it's the only function that ever locks two indexes then it
can just determine what locking discipline it wants to use.

--
greg

On Wednesday, October 03, 2012 12:59:25 PM Greg Stark wrote:
> Just for background. The showstopper for REINDEX concurrently was not
> that it was particularly hard to actually do the reindexing. But it's
> not obvious how to obtain a lock on both the old and new index without
> creating a deadlock risk. I don't remember exactly where the deadlock
> risk lies but there are two indexes to lock and whichever order you
> obtain the locks it might be possible for someone else to be waiting
> to obtain them in the opposite order.
>
> I'm sure it's possible to solve the problem. But the footwork needed
> to release locks then reobtain them in the right order and verify that
> the index hasn't changed out from under you might be a lot of
> headache.
Maybe I am missing something here, but reindex concurrently should do
1) BEGIN
2) Lock table in share update exlusive
3) lock old index
3) create new index
4) obtain session locks on table, old index, new index
5) commit
6) process till newindex->insisready (no new locks)
7) process till newindex->indisvalid (no new locks)
8) process till !oldindex->indisvalid (no new locks)
9) process till !oldindex->indisready (no new locks)
10) drop all session locks
11) lock old index exlusively which should be "invisible" now
12) drop old index

I don't see where the deadlock danger is hidden in that?

I didn't find anything relevant in a quick search of the archives...

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Oct 3, 2012 at 8:08 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On Wednesday, October 03, 2012 12:59:25 PM Greg Stark wrote:
> > Just for background. The showstopper for REINDEX concurrently was not
> > that it was particularly hard to actually do the reindexing. But it's
> > not obvious how to obtain a lock on both the old and new index without
> > creating a deadlock risk. I don't remember exactly where the deadlock
> > risk lies but there are two indexes to lock and whichever order you
> > obtain the locks it might be possible for someone else to be waiting
> > to obtain them in the opposite order.
> >
> > I'm sure it's possible to solve the problem. But the footwork needed
> > to release locks then reobtain them in the right order and verify that
> > the index hasn't changed out from under you might be a lot of
> > headache.
> Maybe I am missing something here, but reindex concurrently should do
> 1) BEGIN
> 2) Lock table in share update exlusive
> 3) lock old index
> 3) create new index
> 4) obtain session locks on table, old index, new index
> 5) commit

Build new index.

> 6) process till newindex->insisready (no new locks)
>
validate new index

> 7) process till newindex->indisvalid (no new locks)
>
Forgot the swap old index/new index.

> 8) process till !oldindex->indisvalid (no new locks)
> 9) process till !oldindex->indisready (no new locks)
> 10) drop all session locks
> 11) lock old index exclusively which should be "invisible" now
> 12) drop old index
>
The code I sent already does that more or less btw. Just that it can be
more simplified...

> I don't see where the deadlock danger is hidden in that?
>
> I didn't find anything relevant in a quick search of the archives...
>
About the deadlock issues, do you mean the case where 2 sessions are
running REINDEX and/or REINDEX CONCURRENTLY on the same table or index in
parallel?
--
Michael Paquier
http://michael.otacoo.com

On Wednesday, October 03, 2012 01:15:27 PM Michael Paquier wrote:
> On Wed, Oct 3, 2012 at 8:08 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
> > On Wednesday, October 03, 2012 12:59:25 PM Greg Stark wrote:
> > > Just for background. The showstopper for REINDEX concurrently was not
> > > that it was particularly hard to actually do the reindexing. But it's
> > > not obvious how to obtain a lock on both the old and new index without
> > > creating a deadlock risk. I don't remember exactly where the deadlock
> > > risk lies but there are two indexes to lock and whichever order you
> > > obtain the locks it might be possible for someone else to be waiting
> > > to obtain them in the opposite order.
> > >
> > > I'm sure it's possible to solve the problem. But the footwork needed
> > > to release locks then reobtain them in the right order and verify that
> > > the index hasn't changed out from under you might be a lot of
> > > headache.
> >
> > Maybe I am missing something here, but reindex concurrently should do
> > 1) BEGIN
> > 12) drop old index
>
> The code I sent already does that more or less btw. Just that it can be
> more simplified...
The above just tried to describe the stuff thats relevant for locking, maybe I
wasn't clear enough on that ;)

> > I don't see where the deadlock danger is hidden in that?
> > I didn't find anything relevant in a quick search of the archives...
>
> About the deadlock issues, do you mean the case where 2 sessions are
> running REINDEX and/or REINDEX CONCURRENTLY on the same table or index in
> parallel?
No idea. The bit about deadlocks originally came from Greg, not me ;)

I guess its more the interaction with normal sessions, because the locking
used (SHARE UPDATE EXLUSIVE) prevents another CONCURRENT action running at the
same time. I don't really see the danger there though because we should never
need to acquire locks that we don't already have except the final
AccessExclusiveLock but thats after we dropped other locks and after the index
is made unusable.

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> Maybe I am missing something here, but reindex concurrently should do
> 1) BEGIN
> 2) Lock table in share update exlusive
> 3) lock old index
> 3) create new index
> 4) obtain session locks on table, old index, new index
> 5) commit
> 6) process till newindex->insisready (no new locks)
> 7) process till newindex->indisvalid (no new locks)
> 8) process till !oldindex->indisvalid (no new locks)
> 9) process till !oldindex->indisready (no new locks)
> 10) drop all session locks
> 11) lock old index exlusively which should be "invisible" now
> 12) drop old index

You can't drop the session locks until you're done. Consider somebody
else trying to do a DROP TABLE between steps 10 and 11, for instance.

regards, tom lane

On Wednesday, October 03, 2012 04:28:59 PM Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > Maybe I am missing something here, but reindex concurrently should do
> > 1) BEGIN
> > 2) Lock table in share update exlusive
> > 3) lock old index
> > 3) create new index
> > 4) obtain session locks on table, old index, new index
> > 5) commit
> > 6) process till newindex->insisready (no new locks)
> > 7) process till newindex->indisvalid (no new locks)
> > 8) process till !oldindex->indisvalid (no new locks)
> > 9) process till !oldindex->indisready (no new locks)
> > 10) drop all session locks
> > 11) lock old index exlusively which should be "invisible" now
> > 12) drop old index
>
> You can't drop the session locks until you're done. Consider somebody
> else trying to do a DROP TABLE between steps 10 and 11, for instance.
Yea, the session lock on the table itself probably shouldn't be dropped. If
were holding only that one there shouldn't be any additional deadlock dangers
when dropping the index due to lock upgrades as were doing the normal dance
any DROP INDEX does. They seem pretty unlikely in a !valid !ready table
anyway.

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012/10/03, at 23:52, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> On Wednesday, October 03, 2012 04:28:59 PM Tom Lane wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>>> Maybe I am missing something here, but reindex concurrently should do
>>> 1) BEGIN
>>> 2) Lock table in share update exlusive
>>> 3) lock old index
>>> 3) create new index
>>> 4) obtain session locks on table, old index, new index
>>> 5) commit
>>> 6) process till newindex->insisready (no new locks)
>>> 7) process till newindex->indisvalid (no new locks)
>>> 8) process till !oldindex->indisvalid (no new locks)
>>> 9) process till !oldindex->indisready (no new locks)
>>> 10) drop all session locks
>>> 11) lock old index exlusively which should be "invisible" now
>>> 12) drop old index
>>
>> You can't drop the session locks until you're done. Consider somebody
>> else trying to do a DROP TABLE between steps 10 and 11, for instance.
> Yea, the session lock on the table itself probably shouldn't be dropped. If
> were holding only that one there shouldn't be any additional deadlock dangers
> when dropping the index due to lock upgrades as were doing the normal dance
> any DROP INDEX does. They seem pretty unlikely in a !valid !ready table
>
Just à note...
My patch drops the locks on parent table and indexes at the end of process, after dropping the old indexes ;)

Michael
>
> Greetings,
>
> Andres
> --
> Andres Freund http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services

On Wednesday, October 03, 2012 10:12:58 PM Michael Paquier wrote:
> On 2012/10/03, at 23:52, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On Wednesday, October 03, 2012 04:28:59 PM Tom Lane wrote:
> >> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> >>> Maybe I am missing something here, but reindex concurrently should do
> >>> 1) BEGIN
> >>> 2) Lock table in share update exlusive
> >>> 3) lock old index
> >>> 3) create new index
> >>> 4) obtain session locks on table, old index, new index
> >>> 5) commit
> >>> 6) process till newindex->insisready (no new locks)
> >>> 7) process till newindex->indisvalid (no new locks)
> >>> 8) process till !oldindex->indisvalid (no new locks)
> >>> 9) process till !oldindex->indisready (no new locks)
> >>> 10) drop all session locks
> >>> 11) lock old index exlusively which should be "invisible" now
> >>> 12) drop old index
> >>
> >> You can't drop the session locks until you're done. Consider somebody
> >> else trying to do a DROP TABLE between steps 10 and 11, for instance.
> >
> > Yea, the session lock on the table itself probably shouldn't be dropped.
> > If were holding only that one there shouldn't be any additional deadlock
> > dangers when dropping the index due to lock upgrades as were doing the
> > normal dance any DROP INDEX does. They seem pretty unlikely in a !valid
> > !ready table
>
> Just à note...
> My patch drops the locks on parent table and indexes at the end of process,
> after dropping the old indexes ;)
I think that might result in deadlocks with concurrent sessions in some
circumstances if those other sessions already have a lower level lock on the
index. Thats why I think dropping the lock on the index and then reacquiring
an access exlusive might be necessary.
Its not a too likely scenario, but why not do it right if its just 3 lines...

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012/10/04, at 5:41, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:

> On Wednesday, October 03, 2012 10:12:58 PM Michael Paquier wrote:
>> On 2012/10/03, at 23:52, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> On Wednesday, October 03, 2012 04:28:59 PM Tom Lane wrote:
>>>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>>>>> Maybe I am missing something here, but reindex concurrently should do
>>>>> 1) BEGIN
>>>>> 2) Lock table in share update exlusive
>>>>> 3) lock old index
>>>>> 3) create new index
>>>>> 4) obtain session locks on table, old index, new index
>>>>> 5) commit
>>>>> 6) process till newindex->insisready (no new locks)
>>>>> 7) process till newindex->indisvalid (no new locks)
>>>>> 8) process till !oldindex->indisvalid (no new locks)
>>>>> 9) process till !oldindex->indisready (no new locks)
>>>>> 10) drop all session locks
>>>>> 11) lock old index exlusively which should be "invisible" now
>>>>> 12) drop old index
>>>>
>>>> You can't drop the session locks until you're done. Consider somebody
>>>> else trying to do a DROP TABLE between steps 10 and 11, for instance.
>>>
>>> Yea, the session lock on the table itself probably shouldn't be dropped.
>>> If were holding only that one there shouldn't be any additional deadlock
>>> dangers when dropping the index due to lock upgrades as were doing the
>>> normal dance any DROP INDEX does. They seem pretty unlikely in a !valid
>>> !ready table
>>
>> Just à note...
>> My patch drops the locks on parent table and indexes at the end of process,
>> after dropping the old indexes ;)
> I think that might result in deadlocks with concurrent sessions in some
> circumstances if those other sessions already have a lower level lock on the
> index. Thats why I think dropping the lock on the index and then reacquiring
> an access exlusive might be necessary.
> Its not a too likely scenario, but why not do it right if its just 3 lines...
Tom is right. This scenario does not cover the case where you drop the parent table or you drop the index, which is indeed invisible, but still has a pg_class and a pg_index entry, from a different session after step 10 and before step 11. So you cannot either drop the locks on indexes until you are done at step 12.
>
> Andres
> --
> Andres Freund http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services

On Wednesday, October 03, 2012 11:42:25 PM Michael Paquier wrote:
> On 2012/10/04, at 5:41, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On Wednesday, October 03, 2012 10:12:58 PM Michael Paquier wrote:
> >> On 2012/10/03, at 23:52, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >>> On Wednesday, October 03, 2012 04:28:59 PM Tom Lane wrote:
> >>>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> >>>>> Maybe I am missing something here, but reindex concurrently should do
> >>>>> 1) BEGIN
> >>>>> 2) Lock table in share update exlusive
> >>>>> 3) lock old index
> >>>>> 3) create new index
> >>>>> 4) obtain session locks on table, old index, new index
> >>>>> 5) commit
> >>>>> 6) process till newindex->insisready (no new locks)
> >>>>> 7) process till newindex->indisvalid (no new locks)
> >>>>> 8) process till !oldindex->indisvalid (no new locks)
> >>>>> 9) process till !oldindex->indisready (no new locks)
> >>>>> 10) drop all session locks
> >>>>> 11) lock old index exlusively which should be "invisible" now
> >>>>> 12) drop old index
> >>>>
> >>>> You can't drop the session locks until you're done. Consider somebody
> >>>> else trying to do a DROP TABLE between steps 10 and 11, for instance.
> >>>
> >>> Yea, the session lock on the table itself probably shouldn't be
> >>> dropped. If were holding only that one there shouldn't be any
> >>> additional deadlock dangers when dropping the index due to lock
> >>> upgrades as were doing the normal dance any DROP INDEX does. They seem
> >>> pretty unlikely in a !valid !ready table
> >>
> >> Just à note...
> >> My patch drops the locks on parent table and indexes at the end of
> >> process, after dropping the old indexes ;)
> >
> > I think that might result in deadlocks with concurrent sessions in some
> > circumstances if those other sessions already have a lower level lock on
> > the index. Thats why I think dropping the lock on the index and then
> > reacquiring an access exlusive might be necessary.
> > Its not a too likely scenario, but why not do it right if its just 3
> > lines...
>
> Tom is right. This scenario does not cover the case where you drop the
> parent table or you drop the index, which is indeed invisible, but still
> has a pg_class and a pg_index entry, from a different session after step
> 10 and before step 11. So you cannot either drop the locks on indexes
> until you are done at step 12.
Yep:
> Yea, the session lock on the table itself probably shouldn't be dropped.
But that does *not* mean you cannot avoid lock upgrade issues by dropping the
lower level lock on the index first and only then acquiring the access exlusive
lock. Note that dropping an index always includes *first* getting a lock on the
table so doing it that way is safe and just the same as a normal drop index.

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Oct 3, 2012 at 5:10 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> > 14) Swap new and old indexes, consisting here in switching their names.
> I think switching based on their names is not going to work very well
> because
> indexes are referenced by oid at several places. Swapping
> pg_index.indexrelid
> or pg_class.relfilenode seems to be the better choice to me. We expect
> relfilenode changes for such commands, but not ::regclass oid changes.
>
OK, if there is a choice to be made, switching relfilenode would be a
better choice as it points to the physical storage itself. It looks more
straight-forward than switching oids, and takes the switch at the root.

Btw, there is still something I wanted to clarify. You mention in your
ideas "old" and "new" indexes.
Such as we create a new index at the begininning and drop the old one at
the end. This is not completely true in the case of switching relfilenode.
What happens is that we create a new index with a new physical storage,
then at swap step, we switch the old storage and the new storage. Once swap
is done, the index that needs to be set as invalid and not ready is not the
old index. but the index created at the beginning of process that has now
the old relfilenode. Then the relation that is indeed dropped at the end of
process is also the index with the old relfilenode, so the index created
indeed at the beginning of process. I understand that this is playing with
the words, but I just wanted to confirm that we are on the same line.
--
Michael Paquier
http://michael.otacoo.com

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Wed, Oct 3, 2012 at 5:10 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
> 14) Swap new and old indexes, consisting here in switching their names.
>> I think switching based on their names is not going to work very well
>> because
>> indexes are referenced by oid at several places. Swapping
>> pg_index.indexrelid
>> or pg_class.relfilenode seems to be the better choice to me. We expect
>> relfilenode changes for such commands, but not ::regclass oid changes.

> OK, if there is a choice to be made, switching relfilenode would be a
> better choice as it points to the physical storage itself. It looks more
> straight-forward than switching oids, and takes the switch at the root.

Andres is quite right that "switch by name" is out of the question ---
for the most part, the system pays no attention to index names at all.
It just gets a list of the OIDs of indexes belonging to a table and
works with that.

However, I'm pretty suspicious of the idea of switching relfilenodes as
well. You generally can't change the relfilenode of a relation (either
a table or an index) without taking an exclusive lock on it, because
changing the relfilenode *will* break any concurrent operations on the
index. And there is not anyplace in the proposed sequence where it's
okay to have exclusive lock on both indexes, at least not if the goal
is to not block concurrent updates at any time.

I think what you'd have to do is drop the old index (relying on the
assumption that no one is accessing it anymore after a certain point, so
you can take exclusive lock on it now) and then rename the new index
to have the old index's name. However, renaming an index without
exclusive lock on it still seems a bit risky. Moreover, what if you
crash right after committing the drop of the old index?

I'm really not convinced that we have a bulletproof solution yet,
at least not if you insist on the replacement index having the same name
as the original. How badly do we need that?

regards, tom lane

On 2012/10/04, at 10:00, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> On Wed, Oct 3, 2012 at 5:10 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
>> 14) Swap new and old indexes, consisting here in switching their names.
>>> I think switching based on their names is not going to work very well
>>> because
>>> indexes are referenced by oid at several places. Swapping
>>> pg_index.indexrelid
>>> or pg_class.relfilenode seems to be the better choice to me. We expect
>>> relfilenode changes for such commands, but not ::regclass oid changes.
>
>> OK, if there is a choice to be made, switching relfilenode would be a
>> better choice as it points to the physical storage itself. It looks more
>> straight-forward than switching oids, and takes the switch at the root.
>
> Andres is quite right that "switch by name" is out of the question ---
> for the most part, the system pays no attention to index names at all.
> It just gets a list of the OIDs of indexes belonging to a table and
> works with that.
Sure. The switching being done by changing the index name is just the direction taken by the first version of the patch, and only that. I just wrote this version without really looking for a bulletproof solution but only for something to discuss about.

>
> However, I'm pretty suspicious of the idea of switching relfilenodes as
> well. You generally can't change the relfilenode of a relation (either
> a table or an index) without taking an exclusive lock on it, because
> changing the relfilenode *will* break any concurrent operations on the
> index. And there is not anyplace in the proposed sequence where it's
> okay to have exclusive lock on both indexes, at least not if the goal
> is to not block concurrent updates at any time.
Ok. As the goal is to allow concurrent operations, this is not reliable either. So what is remaining is the method switching the OIDs of old and new indexes in pg_index? Any other candidates?

>
> I think what you'd have to do is drop the old index (relying on the
> assumption that no one is accessing it anymore after a certain point, so
> you can take exclusive lock on it now) and then rename the new index
> to have the old index's name. However, renaming an index without
> exclusive lock on it still seems a bit risky. Moreover, what if you
> crash right after committing the drop of the old index?
>
> I'm really not convinced that we have a bulletproof solution yet,
> at least not if you insist on the replacement index having the same name as the original. How badly do we need that?
And we do not really need such a solution as I am not insisting on the method that switches indexes by changing names. I am open to a reliable and robust method, and I hope this method could be decided in this thread.

Thanks for those arguments, I am feeling it is really leading the discussion to the good direction.

Thanks.

Michael

On Thu, Oct 4, 2012 at 2:19 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>> I think what you'd have to do is drop the old index (relying on the
>> assumption that no one is accessing it anymore after a certain point, so
>> you can take exclusive lock on it now) and then rename the new index
>> to have the old index's name. However, renaming an index without
>> exclusive lock on it still seems a bit risky. Moreover, what if you
>> crash right after committing the drop of the old index?

I think this would require a new state which is the converse of
indisvalid=f. Right now there's no state the index can be in that
means the index should be ignored for both scans and maintenance but
might have old sessions that might be using it or maintaining it.

I'm a bit puzzled why we're so afraid of swapping the relfilenodes
when that's what the current REINDEX does. It seems flaky to have two
different mechanisms depending on which mode is being used. It seems
more conservative to use the same mechanism and just figure out what's
required to ensure it's safe in both modes. At least there won't be
any bugs from unexpected consequences that aren't locking related if
it's using the same mechanics.

--
greg

Greg Stark <stark(at)mit(dot)edu> writes:
> I'm a bit puzzled why we're so afraid of swapping the relfilenodes
> when that's what the current REINDEX does.

Swapping the relfilenodes is fine *as long as you have exclusive lock*.
The trick is to make it safe without that. It will definitely not work
to do that without exclusive lock, because at the instant you would try
it, people will be accessing the new index (by OID).

regards, tom lane

On Thu, Oct 4, 2012 at 11:51 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Greg Stark <stark(at)mit(dot)edu> writes:
> > I'm a bit puzzled why we're so afraid of swapping the relfilenodes
> > when that's what the current REINDEX does.
>
> Swapping the relfilenodes is fine *as long as you have exclusive lock*.
> The trick is to make it safe without that. It will definitely not work
> to do that without exclusive lock, because at the instant you would try
> it, people will be accessing the new index (by OID).
>
OK, so index swapping could be done by:
1) Index name switch. This is not thought as safe as the system does not
pay attention on index names at all.

2) relfilenode switch. An ExclusiveLock is necessary.The lock that would be
taken is not compatible with a concurrent operation, except if we consider
that the lock will not be taken for a long time, only during the swap
moment. Reindex uses this mechanism, so it would be good for consistency.

3) Switch the OIDs of indexes. Looks safe from the system prospective and
it will be necessary to invalidate the cache entries for both relations
after swap. Any opinions on this one?
--
Michael Paquier
http://michael.otacoo.com

On Thursday, October 04, 2012 04:51:29 AM Tom Lane wrote:
> Greg Stark <stark(at)mit(dot)edu> writes:
> > I'm a bit puzzled why we're so afraid of swapping the relfilenodes
> > when that's what the current REINDEX does.
>
> Swapping the relfilenodes is fine *as long as you have exclusive lock*.
> The trick is to make it safe without that. It will definitely not work
> to do that without exclusive lock, because at the instant you would try
> it, people will be accessing the new index (by OID).
I can understand hesitation around that.. I would like to make sure I
understand the problem correctly. When we get to the point where we switch
indexes we should be in the following state:
- both indexes are indisready
- old should be invalid
- new index should be valid
- have the same indcheckxmin
- be locked by us preventing anybody else from making changes

Lets assume we have index a_old(relfilenode 1) as the old index and a rebuilt
index a_new (relfilenode 2) as the one we just built. If we do it properly
nobody will have 'a' open for querying, just for modifications (its indisready)
as we had waited for everyone that could have seen a as valid to finish.

As far as I understand the code a session using a_new will also have built a
relcache entry for a_old.
Two problems:
* relying on the relcache to be built for both indexes seems hinky
* As the relcache is built with SnapshotNow it could read the old definition
for a_new and the new one for a_old (or the reverse) and thus end up with both
pointing to the same relfilenode. Which would be ungood.

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Oct 5, 2012 at 6:58 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On Thursday, October 04, 2012 04:51:29 AM Tom Lane wrote:
> I can understand hesitation around that.. I would like to make sure I
> understand the problem correctly. When we get to the point where we switch
> indexes we should be in the following state:
> - both indexes are indisready
> - old should be invalid
> - new index should be valid
> - have the same indcheckxmin
> - be locked by us preventing anybody else from making changes
>
Looks like a good presentation of the problem. I am not sure if marking the
new index as valid is necessary though. As long as it is done inside the
same transaction as the swap there are no problems, no?

> Lets assume we have index a_old(relfilenode 1) as the old index and a
> rebuilt
> index a_new (relfilenode 2) as the one we just built. If we do it properly
> nobody will have 'a' open for querying, just for modifications (its
> indisready)
> as we had waited for everyone that could have seen a as valid to finish.
>
> As far as I understand the code a session using a_new will also have built
> a
> relcache entry for a_old.
> Two problems:
> * relying on the relcache to be built for both indexes seems hinky
> * As the relcache is built with SnapshotNow it could read the old
> definition
> for a_new and the new one for a_old (or the reverse) and thus end up with
> both
> pointing to the same relfilenode. Which would be ungood.
>
OK, so the problem here is that the relcache, as the syscache, are relying
on SnapshotNow which cannot be used safely as the false index definition
could be read by other backends. So this looks to bring back the discussion
to the point where a higher lock level is necessary to perform a safe
switch of the indexes.

I assume that the switch phase is not the longest phase of the concurrent
operation, as you also need to build and validate the new index at prior
steps. I am just wondering if it is acceptable to you guys to take a
stronger lock only during this switch phase. This won't make the reindex
being concurrently all the time but it would avoid any visibility issues
and have an index switch processing which is more consistent with the
existing implementation as it could rely on the same mechanism as normal
reindex that switches relfilenode.
--
Michael Paquier
http://michael.otacoo.com

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> OK, so the problem here is that the relcache, as the syscache, are relying
> on SnapshotNow which cannot be used safely as the false index definition
> could be read by other backends.

That's one problem. It's definitely not the only one, if we're trying
to change an index's definition while an index-accessing operation is in
progress.

> I assume that the switch phase is not the longest phase of the concurrent
> operation, as you also need to build and validate the new index at prior
> steps. I am just wondering if it is acceptable to you guys to take a
> stronger lock only during this switch phase.

We might be forced to fall back on such a solution, but it's pretty
undesirable. Even though the exclusive lock would only need to be held
for a short time, it can create a big hiccup in processing. The key
reason is that once the ex-lock request is queued, it blocks ordinary
operations coming in behind it. So effectively it's stopping operations
not just for the length of time the lock is *held*, but for the length
of time it's *awaited*, which could be quite long.

Note that allowing subsequent requests to jump the queue would not be a
good fix for this; if you do that, it's likely the ex-lock will never be
granted, at least not till the next system idle time. Which if you've
got one, you don't need a feature like this at all; you might as well
just reindex normally during your idle time.

regards, tom lane

Tom Lane escribió:

> Note that allowing subsequent requests to jump the queue would not be a
> good fix for this; if you do that, it's likely the ex-lock will never be
> granted, at least not till the next system idle time. Which if you've
> got one, you don't need a feature like this at all; you might as well
> just reindex normally during your idle time.

Not really. The time to run a complete reindex might be several hours.
If the idle time is just a few minutes or seconds long, it may be more
than enough to complete the switch operation, but not to run the
complete reindex.

Maybe another idea is that the reindexing is staged: the user would
first run a command to create the replacement index, and leave both
present until the user runs a second command (which acquires a strong
lock) that executes the switch. Somehow similar to a constraint created
as NOT VALID (which runs without a strong lock) which can be later
validated separately.

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Maybe another idea is that the reindexing is staged: the user would
> first run a command to create the replacement index, and leave both
> present until the user runs a second command (which acquires a strong
> lock) that executes the switch. Somehow similar to a constraint created
> as NOT VALID (which runs without a strong lock) which can be later
> validated separately.

Yeah. We could consider

CREATE INDEX CONCURRENTLY (already exists)
SWAP INDEXES (requires ex-lock, swaps names and constraint dependencies;
or maybe just implement as swap of relfilenodes?)
DROP INDEX CONCURRENTLY

The last might have some usefulness in its own right, anyway.

regards, tom lane

On Sat, Oct 6, 2012 at 6:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > Maybe another idea is that the reindexing is staged: the user would
> > first run a command to create the replacement index, and leave both
> > present until the user runs a second command (which acquires a strong
> > lock) that executes the switch. Somehow similar to a constraint created
> > as NOT VALID (which runs without a strong lock) which can be later
> > validated separately.
>
> Yeah. We could consider
>
> CREATE INDEX CONCURRENTLY (already exists)
> SWAP INDEXES (requires ex-lock, swaps names and constraint dependencies;
> or maybe just implement as swap of relfilenodes?)
> DROP INDEX CONCURRENTLY
>
OK. That is a different approach and would limit strictly the amount of
code necessary for the feature, but I feel that it breaks the nature of
CONCURRENTLY which should run without any exclusive locks. The possibility
to do that in a single command would be also better perhaps seen from the
user.

Until now all the approaches investigated (switch of relfilenode, switch of
index OID) need to have an exclusive lock because we try to maintain index
OID as consistent. In the patch I submitted, the new index created has a
different OID than the old index, and simply switches names. So after the
REINDEX CONCURRENTLY the OID of index on the table is different, but seen
from user the name is the same. Is it acceptable to consider that a reindex
concurrently could change the OID of the index rebuild? Is it a postgres
requirement to maintain the object OIDs consistent between DDL operations?
If the OID of old and new index are different, the relcache entries of each
index will be completely separated, and this would take care of any
visibility problems regarding visibility. pg_reorg for example changes the
relation OID of the table reorganized after operation is completed.

Thoughts about that?
--
Michael Paquier
http://michael.otacoo.com

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Sat, Oct 6, 2012 at 6:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> CREATE INDEX CONCURRENTLY (already exists)
>> SWAP INDEXES (requires ex-lock, swaps names and constraint dependencies;
>> or maybe just implement as swap of relfilenodes?)
>> DROP INDEX CONCURRENTLY

> OK. That is a different approach and would limit strictly the amount of
> code necessary for the feature, but I feel that it breaks the nature of
> CONCURRENTLY which should run without any exclusive locks.

Hm? The whole point is that the CONCURRENTLY commands don't require
exclusive locks. Only the SWAP command would.

> Until now all the approaches investigated (switch of relfilenode, switch of
> index OID) need to have an exclusive lock because we try to maintain index
> OID as consistent. In the patch I submitted, the new index created has a
> different OID than the old index, and simply switches names. So after the
> REINDEX CONCURRENTLY the OID of index on the table is different, but seen
> from user the name is the same. Is it acceptable to consider that a reindex
> concurrently could change the OID of the index rebuild?

That is not going to work without ex-lock somewhere. If you change the
index's OID then you will have to change pg_constraint and pg_depend
entries referencing it, and that creates race condition hazards for
other processes looking at those catalogs. I'm not convinced that you
can even do a rename safely without ex-lock. Basically, any DDL update
on an active index is going to be dangerous and probably impossible
without lock, IMO.

To answer your question, I don't think anyone would object to the
index's OID changing if the operation were safe otherwise. But I don't
think that allowing that gets us to a safe solution.

regards, tom lane

On Sat, Oct 6, 2012 at 8:40 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> > On Sat, Oct 6, 2012 at 6:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > OK. That is a different approach and would limit strictly the amount of
> > code necessary for the feature, but I feel that it breaks the nature of
> > CONCURRENTLY which should run without any exclusive locks.
>
> Hm? The whole point is that the CONCURRENTLY commands don't require
> exclusive locks. Only the SWAP command would.
>
Yes, but my point is that it is more user-friendly to have such a
functionality with a single command.
By having something without locks, you could use the concurrent APIs to
perform a REINDEX automatically in autovacuum for example.
Also, the possibility to perform concurrent operations entirely without
exclusive locks is not a problem limited to REINDEX, there would be for
sure similar problems if CLUSTER CONCURRENTLY or ALTER TABLE CONCURRENTLY
are wanted.

>
> > Until now all the approaches investigated (switch of relfilenode, switch
> of
> > index OID) need to have an exclusive lock because we try to maintain
> index
> > OID as consistent. In the patch I submitted, the new index created has a
> > different OID than the old index, and simply switches names. So after the
> > REINDEX CONCURRENTLY the OID of index on the table is different, but seen
> > from user the name is the same. Is it acceptable to consider that a
> reindex
> > concurrently could change the OID of the index rebuild?
>
> That is not going to work without ex-lock somewhere. If you change the
> index's OID then you will have to change pg_constraint and pg_depend
> entries referencing it, and that creates race condition hazards for
> other processes looking at those catalogs. I'm not convinced that you
> can even do a rename safely without ex-lock. Basically, any DDL update
> on an active index is going to be dangerous and probably impossible
> without lock, IMO.
>
In the current version of the patch, at the beginning of process a new
index is created. It is a twin of the index it has to replace, meaning that
it copies the dependencies of old index and creates twin entries of the old
index even in pg_depend and pg_constraint also if necessary. So the old
index and the new index have exactly the same data in catalog, they are
completely decoupled, and you do not need to worry about the OID
replacements and the visibility consequences.
Knowing that both indexes are completely separate entities, isn't this
enough to change the new index as the old one with a low-level lock? In the
case of my patch only the names are simply exchanged and make the user
unaware of what is happening in background. This behaves similarly to
pg_reorg, explaining why the OIDs of tables reorganized are changed after
being pg_reorg'ed.

To answer your question, I don't think anyone would object to the
> index's OID changing if the operation were safe otherwise. But I don't
> think that allowing that gets us to a safe solution.
>
OK thanks.
--
Michael Paquier
http://michael.otacoo.com

On 10/05/2012 09:03 PM, Tom Lane wrote:
> Note that allowing subsequent requests to jump the queue would not be a
> good fix for this; if you do that, it's likely the ex-lock will never be
> granted, at least not till the next system idle time.

Offering that option to the admin sounds like a good thing, since
(as Alvaro points out) the build of the replacement index could take
considerable time but be done without the lock. Then the swap
done in the first quiet period (but without further admin action),
and the drop started.

One size doesn't fit all. It doesn't need to be the only method.
--
Cheers,
Jeremy

On 10/5/12 9:57 PM, Michael Paquier wrote:
> In the current version of the patch, at the beginning of process a new index is created. It is a twin of the index it has to replace, meaning that it copies the dependencies of old index and creates twin entries of the old index even in pg_depend and pg_constraint also if necessary. So the old index and the new index have exactly the same data in catalog, they are completely decoupled, and you do not need to worry about the OID replacements and the visibility consequences.

Yeah, what's the risk to renaming an index during concurrent access? The only thing I can think of is an "old" backend referring to the wrong index name in an elog. That's certainly not great, but could possibly be dealt with.

Are there any other things that are directly tied to the name of an index (or of any object for that matter)?
--
Jim C. Nasby, Database Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

On Monday, October 08, 2012 11:57:46 PM Jim Nasby wrote:
> On 10/5/12 9:57 PM, Michael Paquier wrote:
> > In the current version of the patch, at the beginning of process a new
> > index is created. It is a twin of the index it has to replace, meaning
> > that it copies the dependencies of old index and creates twin entries of
> > the old index even in pg_depend and pg_constraint also if necessary. So
> > the old index and the new index have exactly the same data in catalog,
> > they are completely decoupled, and you do not need to worry about the
> > OID replacements and the visibility consequences.
>
> Yeah, what's the risk to renaming an index during concurrent access? The
> only thing I can think of is an "old" backend referring to the wrong index
> name in an elog. That's certainly not great, but could possibly be dealt
> with.
We cannot have two indexes with the same oid in the catalog, so the two
different names will have to have different oids. Unfortunately the indexes oid
is referred to by other tables (e.g. pg_constraint), so renaming the indexes
while differering in the oid isn't really helpful :(...

Right now I don't see anything that would make switching oids easier than
relfilenodes.

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Jim Nasby <jim(at)nasby(dot)net> writes:
> Yeah, what's the risk to renaming an index during concurrent access?

SnapshotNow searches for the pg_class row could get broken by *any*
transactional update of that row, whether it's for a change of relname
or some other field.

A lot of these problems would go away if we rejiggered the definition of
SnapshotNow to be more like MVCC. We have discussed that in the past,
but IIRC it's not exactly a simple or risk-free change in itself.
Still, maybe we should start thinking about doing that instead of trying
to make REINDEX CONCURRENTLY safe given the existing infrastructure.

regards, tom lane

On 10/8/12 5:08 PM, Andres Freund wrote:
> On Monday, October 08, 2012 11:57:46 PM Jim Nasby wrote:
>> >On 10/5/12 9:57 PM, Michael Paquier wrote:
>>> > >In the current version of the patch, at the beginning of process a new
>>> > >index is created. It is a twin of the index it has to replace, meaning
>>> > >that it copies the dependencies of old index and creates twin entries of
>>> > >the old index even in pg_depend and pg_constraint also if necessary. So
>>> > >the old index and the new index have exactly the same data in catalog,
>>> > >they are completely decoupled, and you do not need to worry about the
>>> > >OID replacements and the visibility consequences.
>> >
>> >Yeah, what's the risk to renaming an index during concurrent access? The
>> >only thing I can think of is an "old" backend referring to the wrong index
>> >name in an elog. That's certainly not great, but could possibly be dealt
>> >with.
> We cannot have two indexes with the same oid in the catalog, so the two
> different names will have to have different oids. Unfortunately the indexes oid
> is referred to by other tables (e.g. pg_constraint), so renaming the indexes
> while differering in the oid isn't really helpful :(...

Hrm... the claim was made that everything relating to the index, including pg_depend and pg_contstraint, got duplicated. But I don't know how you could duplicate a constraint without also playing name games. Perhaps name games are being played there as well...

> Right now I don't see anything that would make switching oids easier than
> relfilenodes.

Yeah... in order to make either of those schemes work I think there would need to non-trivial internal changes so that we weren't just passing around raw OIDs/filenodes.

BTW, it occurs to me that this problem might be easier to deal with if we had support for accessing the catalog with the same snapshot as the main query was using... IIRC that's been discussed in the past for other issues.
--
Jim C. Nasby, Database Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

On 10/8/12 6:12 PM, Tom Lane wrote:
> Jim Nasby <jim(at)nasby(dot)net> writes:
>> Yeah, what's the risk to renaming an index during concurrent access?
>
> SnapshotNow searches for the pg_class row could get broken by *any*
> transactional update of that row, whether it's for a change of relname
> or some other field.
>
> A lot of these problems would go away if we rejiggered the definition of
> SnapshotNow to be more like MVCC. We have discussed that in the past,
> but IIRC it's not exactly a simple or risk-free change in itself.
> Still, maybe we should start thinking about doing that instead of trying
> to make REINDEX CONCURRENTLY safe given the existing infrastructure.

Yeah, I was just trying to remember what other situations this has come up in. My recollection is that there's been a couple other cases where that would be useful.

My recollection is also that such a change would be rather large... but it might be smaller than all the other work-arounds that are needed because we don't have that...
--
Jim C. Nasby, Database Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

On Tue, Oct 9, 2012 at 8:12 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Jim Nasby <jim(at)nasby(dot)net> writes:
> > Yeah, what's the risk to renaming an index during concurrent access?
>
> SnapshotNow searches for the pg_class row could get broken by *any*
> transactional update of that row, whether it's for a change of relname
> or some other field.
>
Does it include updates on the relation names of pg_class, or ready and
valid flags in pg_index? Tables refer to the indexes with OIDs only so if
the index and its concurrent are completely separated entries in pg_index,
pg_constraint and pg_class, what is the problem?
Is it that the Relation fetched from system cache might become inconsistent
because of SnapshotNow?

> A lot of these problems would go away if we rejiggered the definition of
> SnapshotNow to be more like MVCC. We have discussed that in the past,
> but IIRC it's not exactly a simple or risk-free change in itself.
> Still, maybe we should start thinking about doing that instead of trying
> to make REINDEX CONCURRENTLY safe given the existing infrastructure.
>
+1. This is something to dig if operations like OID switch are envisaged
for concurrent operations. This does not concern only REINDEX. Things like
CLUSTER, or ALTER TABLE would need something similar.
--
Michael Paquier
http://michael.otacoo.com

On Tue, Oct 9, 2012 at 8:14 AM, Jim Nasby <jim(at)nasby(dot)net> wrote:

> Hrm... the claim was made that everything relating to the index, including
> pg_depend and pg_contstraint, got duplicated. But I don't know how you
> could duplicate a constraint without also playing name games. Perhaps name
> games are being played there as well...

Yes, it is what was originally intended. Please note the pg_constraint
entry was not duplicated correctly in the first version of the patch
because of a bug I already fixed.
I will provide another version soon if necessary.

>
>
>
>> Right now I don't see anything that would make switching oids easier than
>> relfilenodes.
>>
>
> Yeah... in order to make either of those schemes work I think there would
> need to non-trivial internal changes so that we weren't just passing around
> raw OIDs/filenodes.
>
> BTW, it occurs to me that this problem might be easier to deal with if we
> had support for accessing the catalog with the same snapshot as the main
> query was using... IIRC that's been discussed in the past for other issues.

Yes, it would be better and helpful to have such a mechanism even for other
operations.
--
Michael Paquier
http://michael.otacoo.com

* Jim Nasby (jim(at)nasby(dot)net) wrote:
> Yeah, I was just trying to remember what other situations this has come up in. My recollection is that there's been a couple other cases where that would be useful.

Yes, I've run into similar issues in the past also. It'd be really neat
to somehow make the SnapshotNow (and I'm guessing the whole SysCache
system) behave more like MVCC.

> My recollection is also that such a change would be rather large... but it might be smaller than all the other work-arounds that are needed because we don't have that...

Perhaps.. Seems like it'd be a lot of work tho, to do it 'right', and I
suspect there's a lot of skeletons out there that we'd run into..

Thanks,

Stephen

Hi all,

Please find attached the version 2 of the patch for this feature, it
corrects the following things:
- toast relations are now rebuilt concurrently as well as other indexes
- concurrent constraint indexes (PRIMARY KEY, UNIQUE, EXCLUSION) are
dropped correctly at the end of process
- exclusion constraints are supported, at least it looks to work correctly.
- Fixed a couple of bugs when constraint indexes were involved in process.

I am adding this version to the commit fest of next month for review.
Regards,
--
Michael Paquier
http://michael.otacoo.com

Hi all,

Long time this thread has not been updated...
Please find attached the version 3 of the patch for support of REINDEX
CONCURRENTLY.
The code has been realigned with master up to commit da07a1e (6th December).

Here are the things modified:
- Improve code to use index_set_state_flag introduced by Tom in commit
3c84046
- One transaction is used for each index swap (N transactions if N indexes
reindexed at the same time)
- Fixed a bug to drop the old indexes concurrently at the end of process

The index swap is managed by switching the names of the new and old indexes
using RenameRelationInternal several times. This API takes an exclusive
lock on the relation that is renamed until the end of the transaction
managing the swap. This has been discussed in this thread and other
threads, but it is important to mention it for people who have not read the
patch.

There are still two things that are missing in this patch, but I would like
to have more feedback before moving forward:
- REINDEX CONCURRENTLY needs tests in src/test/isolation
- There is still a problem with toast indexes. If the concurrent reindex of
a toast index fails for a reason or another, pg_relation will finish with
invalid toast index entries. I am still wondering about how to clean up
that. Any ideas?

Comments?
--
Michael Paquier
http://michael.otacoo.com

On 7 December 2012 12:37, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:

> There are still two things that are missing in this patch, but I would like
> to have more feedback before moving forward:
> - REINDEX CONCURRENTLY needs tests in src/test/isolation

Yes, it needs those

> - There is still a problem with toast indexes. If the concurrent reindex of
> a toast index fails for a reason or another, pg_relation will finish with
> invalid toast index entries. I am still wondering about how to clean up
> that. Any ideas?

Build another toast index, rather than reindexing the existing one,
then just use the new oid.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-12-07 21:37:06 +0900, Michael Paquier wrote:
> Hi all,
>
> Long time this thread has not been updated...
> Please find attached the version 3 of the patch for support of REINDEX
> CONCURRENTLY.
> The code has been realigned with master up to commit da07a1e (6th December).
>
> Here are the things modified:
> - Improve code to use index_set_state_flag introduced by Tom in commit
> 3c84046
> - One transaction is used for each index swap (N transactions if N indexes
> reindexed at the same time)
> - Fixed a bug to drop the old indexes concurrently at the end of process
>
> The index swap is managed by switching the names of the new and old indexes
> using RenameRelationInternal several times. This API takes an exclusive
> lock on the relation that is renamed until the end of the transaction
> managing the swap. This has been discussed in this thread and other
> threads, but it is important to mention it for people who have not read the
> patch.

Won't working like this cause problems when dependencies towards that
index exist? E.g. an index-based constraint?

As you have an access exlusive lock you should be able to just switch
the relfilenodes of both and concurrently drop the *_cci index with the
old relfilenode afterwards, that would preserve the index states.

Right now I think clearing checkxmin is all you would need to other than
that. We know we don't need it in the concurrent context.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On 7 December 2012 12:37, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>> - There is still a problem with toast indexes. If the concurrent reindex of
>> a toast index fails for a reason or another, pg_relation will finish with
>> invalid toast index entries. I am still wondering about how to clean up
>> that. Any ideas?

> Build another toast index, rather than reindexing the existing one,
> then just use the new oid.

Um, I don't think you can swap in a new toast index OID without taking
exclusive lock on the parent table at some point.

One sticking point is the need to update pg_class.reltoastidxid. I
wonder how badly we need that field though --- could we get rid of it
and treat toast-table indexes just the same as normal ones? (Whatever
code is looking at the field could perhaps instead rely on
RelationGetIndexList.)

regards, tom lane

On 2012-12-07 12:01:52 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On 7 December 2012 12:37, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> >> - There is still a problem with toast indexes. If the concurrent reindex of
> >> a toast index fails for a reason or another, pg_relation will finish with
> >> invalid toast index entries. I am still wondering about how to clean up
> >> that. Any ideas?
>
> > Build another toast index, rather than reindexing the existing one,
> > then just use the new oid.

Thats easier said than done in the first place. toast_save_datum()
explicitly opens/modifies the one index it needs and updates it.

> Um, I don't think you can swap in a new toast index OID without taking
> exclusive lock on the parent table at some point.

The whole swapping issue isn't solved satisfyingly as whole yet :(.

If we just swap the index relfilenodes in the pg_index entries itself,
we wouldn't need to modify the main table's pg_class at all.

> One sticking point is the need to update pg_class.reltoastidxid. I
> wonder how badly we need that field though --- could we get rid of it
> and treat toast-table indexes just the same as normal ones? (Whatever
> code is looking at the field could perhaps instead rely on
> RelationGetIndexList.)

We could probably just set Relation->rd_toastidx when building the
relcache entry for the toast table so it doesn't have to search the
whole indexlist all the time. Not that that would be too big, but...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 7 December 2012 17:19, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2012-12-07 12:01:52 -0500, Tom Lane wrote:
>> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> > On 7 December 2012 12:37, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>> >> - There is still a problem with toast indexes. If the concurrent reindex of
>> >> a toast index fails for a reason or another, pg_relation will finish with
>> >> invalid toast index entries. I am still wondering about how to clean up
>> >> that. Any ideas?
>>
>> > Build another toast index, rather than reindexing the existing one,
>> > then just use the new oid.
>
> Thats easier said than done in the first place. toast_save_datum()
> explicitly opens/modifies the one index it needs and updates it.

Well, yeh, I know what I'm saying: it would need to maintain 2 indexes
for a while.

The point is to use the same trick we do manually now, which works
fine for normal indexes and can be made to work for toast indexes
also.

>> Um, I don't think you can swap in a new toast index OID without taking
>> exclusive lock on the parent table at some point.
>
> The whole swapping issue isn't solved satisfyingly as whole yet :(.
>
> If we just swap the index relfilenodes in the pg_index entries itself,
> we wouldn't need to modify the main table's pg_class at all.

yes

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Dec 7, 2012 at 10:33 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 7 December 2012 12:37, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
> > - There is still a problem with toast indexes. If the concurrent reindex
> of
> > a toast index fails for a reason or another, pg_relation will finish with
> > invalid toast index entries. I am still wondering about how to clean up
> > that. Any ideas?
>
> Build another toast index, rather than reindexing the existing one,
> then just use the new oid.
>
Hum? The patch already does that. It creates concurrently a new index which
is a duplicate of the existing one, then the old and new indexes are
swapped. Finally the old index is dropped concurrently.

The problem I still see is the following one:
If a toast index, or a relation having a toast index, is being reindexed
concurrently, and that the server crashes during the process, there will be
invalid toast indexes in the server. If the crash happens before the swap,
the new toast index is invalid. If the crash happens after the swap, the
old toast index is invalid.
I am not sure the user is able to clean up such invalid toast indexes
manually as they are not visible to him.
--
Michael Paquier
http://michael.otacoo.com

On Sat, Dec 8, 2012 at 2:19 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2012-12-07 12:01:52 -0500, Tom Lane wrote:
> > Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > > On 7 December 2012 12:37, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
> > >> - There is still a problem with toast indexes. If the concurrent
> reindex of
> > >> a toast index fails for a reason or another, pg_relation will finish
> with
> > >> invalid toast index entries. I am still wondering about how to clean
> up
> > >> that. Any ideas?
> >
> > > Build another toast index, rather than reindexing the existing one,
> > > then just use the new oid.
>
> Thats easier said than done in the first place. toast_save_datum()
> explicitly opens/modifies the one index it needs and updates it.
>
> > Um, I don't think you can swap in a new toast index OID without taking
> > exclusive lock on the parent table at some point.
>
> The whole swapping issue isn't solved satisfyingly as whole yet :(.
>
> If we just swap the index relfilenodes in the pg_index entries itself,
> we wouldn't need to modify the main table's pg_class at all.
>
I think you are mistaking here, relfilenode is a column of pg_class and not
pg_index.
So whatever the method used for swapping: relfilenode switch or relname
switch, you need to modify the pg_class entry of the old and new indexes.
--
Michael Paquier
http://michael.otacoo.com

On Sat, Dec 8, 2012 at 2:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Um, I don't think you can swap in a new toast index OID without taking
> exclusive lock on the parent table at some point.
>
> One sticking point is the need to update pg_class.reltoastidxid. I
> wonder how badly we need that field though --- could we get rid of it
> and treat toast-table indexes just the same as normal ones? (Whatever
> code is looking at the field could perhaps instead rely on
> RelationGetIndexList.)
>
Yes. reltoastidxid refers to the index of the toast table so it is
necessary to take a lock on the parent relation in this case. I haven't
thought of that. I also do not really know how far this is used by the
toast process, but just by thinking safety taking a lock on the parent
relation would be better.
For a normal index, locking the parent table is not necessary as we do not
need to modify anything in the parent relation entry in pg_class.
--
Michael Paquier
http://michael.otacoo.com

On 2012-12-08 21:24:47 +0900, Michael Paquier wrote:
> On Sat, Dec 8, 2012 at 2:19 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
>
> > On 2012-12-07 12:01:52 -0500, Tom Lane wrote:
> > > Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > > > On 7 December 2012 12:37, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> > wrote:
> > > >> - There is still a problem with toast indexes. If the concurrent
> > reindex of
> > > >> a toast index fails for a reason or another, pg_relation will finish
> > with
> > > >> invalid toast index entries. I am still wondering about how to clean
> > up
> > > >> that. Any ideas?
> > >
> > > > Build another toast index, rather than reindexing the existing one,
> > > > then just use the new oid.
> >
> > Thats easier said than done in the first place. toast_save_datum()
> > explicitly opens/modifies the one index it needs and updates it.
> >
> > > Um, I don't think you can swap in a new toast index OID without taking
> > > exclusive lock on the parent table at some point.
> >
> > The whole swapping issue isn't solved satisfyingly as whole yet :(.
> >
> > If we just swap the index relfilenodes in the pg_index entries itself,
> > we wouldn't need to modify the main table's pg_class at all.
> >
> I think you are mistaking here, relfilenode is a column of pg_class and not
> pg_index.
> So whatever the method used for swapping: relfilenode switch or relname
> switch, you need to modify the pg_class entry of the old and new indexes.

The point is that with a relname switch the pg_class.oid of the index
changes. Which is a bad idea because it will possibly be referred to by
pg_depend entries. Relfilenodes - which certainly live in pg_class too,
thats not the point - aren't referred to externally though. So if
everything else in pg_class/pg_index stays the same a relfilenode switch
imo saves you a lot of trouble.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2012-12-08 21:24:47 +0900, Michael Paquier wrote:
>> So whatever the method used for swapping: relfilenode switch or relname
>> switch, you need to modify the pg_class entry of the old and new indexes.

> The point is that with a relname switch the pg_class.oid of the index
> changes. Which is a bad idea because it will possibly be referred to by
> pg_depend entries. Relfilenodes - which certainly live in pg_class too,
> thats not the point - aren't referred to externally though. So if
> everything else in pg_class/pg_index stays the same a relfilenode switch
> imo saves you a lot of trouble.

I do not believe that it is safe to modify an index's relfilenode *nor*
its OID without exclusive lock; both of those are going to be in use to
identify and access the index in concurrent sessions. The only things
we could possibly safely swap in a REINDEX CONCURRENTLY are the index
relnames, which are not used for identification by the system itself.
(I think. It's possible that even this breaks something.)

Even then, any such update of the pg_class rows is dependent on
switching to MVCC-style catalog access, which frankly is pie in the sky
at the moment; the last time pgsql-hackers talked seriously about that,
there seemed to be multiple hard problems besides mere performance.
If you want to wait for that, it's a safe bet that we won't see this
feature for a few years.

I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
preserve the index name exactly. Something like adding or removing
trailing underscores would probably serve to generate a nonconflicting
name that's not too unsightly. Or just generate a new name using the
same rules that CREATE INDEX would when no name is specified. Yeah,
it's a hack, but what about the CONCURRENTLY commands isn't a hack?

regards, tom lane

On 2012-12-08 09:40:43 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2012-12-08 21:24:47 +0900, Michael Paquier wrote:
> >> So whatever the method used for swapping: relfilenode switch or relname
> >> switch, you need to modify the pg_class entry of the old and new indexes.
>
> > The point is that with a relname switch the pg_class.oid of the index
> > changes. Which is a bad idea because it will possibly be referred to by
> > pg_depend entries. Relfilenodes - which certainly live in pg_class too,
> > thats not the point - aren't referred to externally though. So if
> > everything else in pg_class/pg_index stays the same a relfilenode switch
> > imo saves you a lot of trouble.
>
> I do not believe that it is safe to modify an index's relfilenode *nor*
> its OID without exclusive lock; both of those are going to be in use to
> identify and access the index in concurrent sessions. The only things
> we could possibly safely swap in a REINDEX CONCURRENTLY are the index
> relnames, which are not used for identification by the system itself.
> (I think. It's possible that even this breaks something.)

Well, the patch currently *does* take an exlusive lock in an extra
transaction just for the swapping. In that case it should actually be
safe.
Although that obviously removes part of the usefulness of the feature.

> Even then, any such update of the pg_class rows is dependent on
> switching to MVCC-style catalog access, which frankly is pie in the sky
> at the moment; the last time pgsql-hackers talked seriously about that,
> there seemed to be multiple hard problems besides mere performance.
> If you want to wait for that, it's a safe bet that we won't see this
> feature for a few years.

Yea :(

> I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
> preserve the index name exactly. Something like adding or removing
> trailing underscores would probably serve to generate a nonconflicting
> name that's not too unsightly. Or just generate a new name using the
> same rules that CREATE INDEX would when no name is specified. Yeah,
> it's a hack, but what about the CONCURRENTLY commands isn't a hack?

I have no problem with ending up with a new name or something like
that. If that is what it takes: fine, no problem.

The issue I raised above is just about keeping the pg_depend entries
pointing to something valid... And not changing the indexes pg_class.oid
seems to be the easiest solution for that.

I have some vague schemes in my had that we can solve the swapping issue
with 3 entries for the index in pg_class, but they all only seem to come
to my head while I don't have anything to write them down, so they are
probably bogus.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> The issue I raised above is just about keeping the pg_depend entries
> pointing to something valid... And not changing the indexes pg_class.oid
> seems to be the easiest solution for that.

Yeah, we would have to update pg_depend, pg_constraint, maybe some other
places if we go with that. I think that would be safe because we'd be
holding ShareRowExclusive lock on the parent table throughout, so nobody
else should be doing anything that's critically dependent on seeing such
rows. But it'd be a lot of ugly code, for sure.

Maybe the best way is to admit that we need a short-term exclusive lock
for the swapping step. Or we could wait for MVCC catalog access ...

regards, tom lane

On 8 December 2012 15:14, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Maybe the best way is to admit that we need a short-term exclusive lock
> for the swapping step.

Which wouldn't be so bad if this is just for the toast index, since in
many cases the index itself is completely empty anyway, which must
offer opportunities for optimization.

> Or we could wait for MVCC catalog access ...

If there was a published design for that, it would help believe in it more.

Do you think one exists?

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On 8 December 2012 15:14, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Or we could wait for MVCC catalog access ...

> If there was a published design for that, it would help believe in it more.
> Do you think one exists?

Well, there have been discussion threads about it in the past. I don't
recall whether any insoluble issues were raised. I think the concerns
were mostly about performance, if we start taking many more snapshots
than we have in the past.

The basic idea isn't hard: anytime a catalog scan is requested with
SnapshotNow, replace that with a freshly taken MVCC snapshot. I think
we'd agreed that this could safely be optimized to "only take a new
snapshot if any new heavyweight lock has been acquired since the last
one". But that'll still be a lot of snapshots, and we know the
snapshot-getting code is a bottleneck already. I think the discussions
mostly veered off at this point into how to make snapshots cheaper.

regards, tom lane

I have updated the patch (v4) to take care of updating reltoastidxid for
toast parent relations at the swap step by using index_update_stats. In
prior versions of the patch this was done when concurrent index was built,
leading to toast relations using invalid indexes if there was a failure
before the swap phase. The update of reltoastidxids of toast relation is
done with RowExclusiveLock.
I also added a couple of tests in src/test/isolation. Btw, as for the time
being the swap step uses AccessExclusiveLock to switch old and new
relnames, it does not have any meaning to run them...

On Sat, Dec 8, 2012 at 11:55 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2012-12-08 09:40:43 -0500, Tom Lane wrote:
> > Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
> > preserve the index name exactly. Something like adding or removing
> > trailing underscores would probably serve to generate a nonconflicting
> > name that's not too unsightly. Or just generate a new name using the
> > same rules that CREATE INDEX would when no name is specified. Yeah,
> > it's a hack, but what about the CONCURRENTLY commands isn't a hack?
>
> I have no problem with ending up with a new name or something like
> that. If that is what it takes: fine, no problem.
>
For the indexes that are created internally by the system like toast or
internal primary keys this is acceptable. However in the case of indexes
that have been created externally I do not think it is acceptable as this
impacts the user that created those indexes with a specific name.

pg_reorg itself also uses the relname switch method when rebuilding indexes
and people using it did not complain about the heavy lock taken at swap
phase, but praised it as it really helps in reducing the lock taken for
reindex at index rebuild and validation, which are the phases that take the
largest amount of time in the REINDEX process btw.
--
Michael Paquier
http://michael.otacoo.com

On 10 December 2012 06:03, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On 2012-12-08 09:40:43 -0500, Tom Lane wrote:
>> > Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>> > I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
>> > preserve the index name exactly. Something like adding or removing
>> > trailing underscores would probably serve to generate a nonconflicting
>> > name that's not too unsightly. Or just generate a new name using the
>> > same rules that CREATE INDEX would when no name is specified. Yeah,
>> > it's a hack, but what about the CONCURRENTLY commands isn't a hack?
>>
>> I have no problem with ending up with a new name or something like
>> that. If that is what it takes: fine, no problem.
>
> For the indexes that are created internally by the system like toast or
> internal primary keys this is acceptable. However in the case of indexes
> that have been created externally I do not think it is acceptable as this
> impacts the user that created those indexes with a specific name.

If I have to choose between (1) keeping the same name OR (2) avoiding
an AccessExclusiveLock then I would choose (2). Most other people
would also, especially when all we would do is add/remove an
underscore. Even if that is user visible. And if it is we can support
a LOCK option that does (1) instead.

If we make it an additional constraint on naming, it won't be a
problem... namely that you can't create an index with/without an
underscore at the end, if a similar index already exists that has an
identical name apart from the suffix.

There are few, if any, commands that need the index name to remain the
same. For those, I think we can bend them to accept the index name and
then add/remove the underscore to get that to work.

That's all a little bit crappy, but this is too small a problem with
an important feature to allow us to skip.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Michael Paquier
http://michael.otacoo.com

On 2012/12/10, at 18:28, Simon Riggs <simon(at)2ndQuadrant(dot)com> wrote:

> On 10 December 2012 06:03, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> On 2012-12-08 09:40:43 -0500, Tom Lane wrote:
>>>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>>>> I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
>>>> preserve the index name exactly. Something like adding or removing
>>>> trailing underscores would probably serve to generate a nonconflicting
>>>> name that's not too unsightly. Or just generate a new name using the
>>>> same rules that CREATE INDEX would when no name is specified. Yeah,
>>>> it's a hack, but what about the CONCURRENTLY commands isn't a hack?
>>>
>>> I have no problem with ending up with a new name or something like
>>> that. If that is what it takes: fine, no problem.
>>
>> For the indexes that are created internally by the system like toast or
>> internal primary keys this is acceptable. However in the case of indexes
>> that have been created externally I do not think it is acceptable as this
>> impacts the user that created those indexes with a specific name.
>
> If I have to choose between (1) keeping the same name OR (2) avoiding
> an AccessExclusiveLock then I would choose (2). Most other people
> would also, especially when all we would do is add/remove an
> underscore. Even if that is user visible. And if it is we can support
> a LOCK option that does (1) instead.
>
> If we make it an additional constraint on naming, it won't be a
> problem... namely that you can't create an index with/without an
> underscore at the end, if a similar index already exists that has an
> identical name apart from the suffix.
>
> There are few, if any, commands that need the index name to remain the
> same. For those, I think we can bend them to accept the index name and
> then add/remove the underscore to get that to work.
>
> That's all a little bit crappy, but this is too small a problem with
> an important feature to allow us to skip.
Ok. Removing the switch name part is only deleting 10 lines of code in index_concurrent_swap.
Then, do you guys have a preferred format for the concurrent index name? For the time being an inelegant _cct suffix is used. The underscore at the end?

Michael

On 2012-12-10 15:03:59 +0900, Michael Paquier wrote:
> I have updated the patch (v4) to take care of updating reltoastidxid for
> toast parent relations at the swap step by using index_update_stats. In
> prior versions of the patch this was done when concurrent index was built,
> leading to toast relations using invalid indexes if there was a failure
> before the swap phase. The update of reltoastidxids of toast relation is
> done with RowExclusiveLock.
> I also added a couple of tests in src/test/isolation. Btw, as for the time
> being the swap step uses AccessExclusiveLock to switch old and new
> relnames, it does not have any meaning to run them...

Btw, as an example of the problems caused by renaming:

postgres=# CREATE TABLE a (id serial primary key); CREATE TABLE b(id
serial primary key, a_id int REFERENCES a);
CREATE TABLE
Time: 137.840 ms
CREATE TABLE
Time: 143.500 ms
postgres=# \d b
Table "public.b"
Column | Type | Modifiers
--------+---------+------------------------------------------------
id | integer | not null default nextval('b_id_seq'::regclass)
a_id | integer |
Indexes:
"b_pkey" PRIMARY KEY, btree (id)
Foreign-key constraints:
"b_a_id_fkey" FOREIGN KEY (a_id) REFERENCES a(id)

postgres=# REINDEX TABLE a CONCURRENTLY;
NOTICE: drop cascades to constraint b_a_id_fkey on table b
REINDEX
Time: 248.992 ms
postgres=# \d b
Table "public.b"
Column | Type | Modifiers
--------+---------+------------------------------------------------
id | integer | not null default nextval('b_id_seq'::regclass)
a_id | integer |
Indexes:
"b_pkey" PRIMARY KEY, btree (id)

Looking at the patch for a bit now.

Regards,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-12-10 15:51:40 +0100, Andres Freund wrote:
> On 2012-12-10 15:03:59 +0900, Michael Paquier wrote:
> > I have updated the patch (v4) to take care of updating reltoastidxid for
> > toast parent relations at the swap step by using index_update_stats. In
> > prior versions of the patch this was done when concurrent index was built,
> > leading to toast relations using invalid indexes if there was a failure
> > before the swap phase. The update of reltoastidxids of toast relation is
> > done with RowExclusiveLock.
> > I also added a couple of tests in src/test/isolation. Btw, as for the time
> > being the swap step uses AccessExclusiveLock to switch old and new
> > relnames, it does not have any meaning to run them...
>
> Btw, as an example of the problems caused by renaming:
> Looking at the patch for a bit now.

Some review comments:

* Some of the added !is_reindex in index_create don't seem safe to
me. Why do we now support reindexing exlusion constraints?

* REINDEX DATABASE .. CONCURRENTLY doesn't work, a variant that does the
concurrent reindexing for user-tables and non-concurrent for system
tables would be very useful. E.g. for the upgrade from 9.1.5->9.1.6...

* ISTM index_concurrent_swap should get exlusive locks on the relation
*before* printing their names. This shouldn't be required because we
have a lock prohibiting schema changes on the parent table, but it
feels safer.

* temporary index names during swapping should also be named via
ChooseIndexName

* why does create_toast_table pass an unconditional 'is_reindex' to
index_create?

* would be nice (but thats probably a step #2 thing) to do the
individual steps of concurrent reindex over multiple relations to
avoid too much overall waiting for other transactions.

* ReindexConcurrentIndexes:

* says " Such indexes are simply bypassed if caller has not specified
anything." but ERROR's. Imo ERROR is fine, but the comment should be
adjusted...

* should perhaps be names ReindexIndexesConcurrently?

* Imo the PHASE 1 comment should be after gathering/validitating the
chosen indexes

* It seems better to me to do use individual transactions + snapshots
for each index, no need to keep very long transactions open (PHASE
2/3)

* s/same whing/same thing/

* Shouldn't a CacheInvalidateRelcacheByRelid be done after PHASE 2 and
5 as well?

* PHASE 6 should acquire exlusive locks on the indexes

* can some of index_concurrent_* infrastructure be reused for
DROP INDEX CONCURRENTLY?

* in CREATE/DROP INDEX CONCURRENTLY 'CONCURRENTLY comes before the
object name, should we keep that conventioN?

Thats all I have for now.

Very nice work! Imo the code looks cleaner after your patch...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On 2012/12/10, at 18:28, Simon Riggs <simon(at)2ndQuadrant(dot)com> wrote:
>> If I have to choose between (1) keeping the same name OR (2) avoiding
>> an AccessExclusiveLock then I would choose (2). Most other people
>> would also, especially when all we would do is add/remove an
>> underscore. Even if that is user visible. And if it is we can support
>> a LOCK option that does (1) instead.

> Ok. Removing the switch name part is only deleting 10 lines of code in index_concurrent_swap.
> Then, do you guys have a preferred format for the concurrent index name? For the time being an inelegant _cct suffix is used. The underscore at the end?

You still need to avoid conflicting name assignments, so my
recommendation would really be to use the select-a-new-name code already
in use for CREATE INDEX without an index name. The underscore idea is
cute, but I doubt it's worth the effort to implement, document, or
explain it in a way that copes with repeated REINDEXes and conflicts.

regards, tom lane

On 12/8/12 9:40 AM, Tom Lane wrote:
> I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
> preserve the index name exactly. Something like adding or removing
> trailing underscores would probably serve to generate a nonconflicting
> name that's not too unsightly.

If you think you can rename an index without an exclusive lock, then why
not rename it back to the original name when you're done?

On 10 December 2012 22:18, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 12/8/12 9:40 AM, Tom Lane wrote:
>> I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
>> preserve the index name exactly. Something like adding or removing
>> trailing underscores would probably serve to generate a nonconflicting
>> name that's not too unsightly.
>
> If you think you can rename an index without an exclusive lock, then why
> not rename it back to the original name when you're done?

Because the index isn't being renamed. An alternate equivalent index
is being created instead.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 12/10/12 5:21 PM, Simon Riggs wrote:
> On 10 December 2012 22:18, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> On 12/8/12 9:40 AM, Tom Lane wrote:
>>> I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
>>> preserve the index name exactly. Something like adding or removing
>>> trailing underscores would probably serve to generate a nonconflicting
>>> name that's not too unsightly.
>>
>> If you think you can rename an index without an exclusive lock, then why
>> not rename it back to the original name when you're done?
>
> Because the index isn't being renamed. An alternate equivalent index
> is being created instead.

Right, basically, you can do this right now using

CREATE INDEX CONCURRENTLY ${name}_tmp ...
DROP INDEX CONCURRENTLY ${name};
ALTER INDEX ${name}_tmp RENAME TO ${name};

The only tricks here are if ${name}_tmp is already taken, in which case
you might as well just error out (or try a few different names), and if
${name} is already in use by the time you get to the last line, in which
case you can log a warning or an error.

What am I missing?

On 10 December 2012 22:27, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 12/10/12 5:21 PM, Simon Riggs wrote:
>> On 10 December 2012 22:18, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>>> On 12/8/12 9:40 AM, Tom Lane wrote:
>>>> I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
>>>> preserve the index name exactly. Something like adding or removing
>>>> trailing underscores would probably serve to generate a nonconflicting
>>>> name that's not too unsightly.
>>>
>>> If you think you can rename an index without an exclusive lock, then why
>>> not rename it back to the original name when you're done?
>>
>> Because the index isn't being renamed. An alternate equivalent index
>> is being created instead.
>
> Right, basically, you can do this right now using
>
> CREATE INDEX CONCURRENTLY ${name}_tmp ...
> DROP INDEX CONCURRENTLY ${name};
> ALTER INDEX ${name}_tmp RENAME TO ${name};
>
> The only tricks here are if ${name}_tmp is already taken, in which case
> you might as well just error out (or try a few different names), and if
> ${name} is already in use by the time you get to the last line, in which
> case you can log a warning or an error.
>
> What am I missing?

That this is already recorded in my book> ;-)

And also that REINDEX CONCURRENTLY doesn't work like that, yet.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-12-10 17:27:45 -0500, Peter Eisentraut wrote:
> On 12/10/12 5:21 PM, Simon Riggs wrote:
> > On 10 December 2012 22:18, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> >> On 12/8/12 9:40 AM, Tom Lane wrote:
> >>> I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
> >>> preserve the index name exactly. Something like adding or removing
> >>> trailing underscores would probably serve to generate a nonconflicting
> >>> name that's not too unsightly.
> >>
> >> If you think you can rename an index without an exclusive lock, then why
> >> not rename it back to the original name when you're done?
> >
> > Because the index isn't being renamed. An alternate equivalent index
> > is being created instead.
>
> Right, basically, you can do this right now using
>
> CREATE INDEX CONCURRENTLY ${name}_tmp ...
> DROP INDEX CONCURRENTLY ${name};
> ALTER INDEX ${name}_tmp RENAME TO ${name};
>
> The only tricks here are if ${name}_tmp is already taken, in which case
> you might as well just error out (or try a few different names), and if
> ${name} is already in use by the time you get to the last line, in which
> case you can log a warning or an error.
>
> What am I missing?

I don't think this is the problematic side of the patch.

The question is rather how to transfer the dependencies without too much
ugliness or how to swap oids without a race. Either by accepting an
exlusive lock or by playing some games, the latter possibly being easier
with renaming...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2012-12-10 22:33:50 +0000, Simon Riggs wrote:
> On 10 December 2012 22:27, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > On 12/10/12 5:21 PM, Simon Riggs wrote:
> >> On 10 December 2012 22:18, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> >>> On 12/8/12 9:40 AM, Tom Lane wrote:
> >>>> I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
> >>>> preserve the index name exactly. Something like adding or removing
> >>>> trailing underscores would probably serve to generate a nonconflicting
> >>>> name that's not too unsightly.
> >>>
> >>> If you think you can rename an index without an exclusive lock, then why
> >>> not rename it back to the original name when you're done?
> >>
> >> Because the index isn't being renamed. An alternate equivalent index
> >> is being created instead.
> >
> > Right, basically, you can do this right now using
> >
> > CREATE INDEX CONCURRENTLY ${name}_tmp ...
> > DROP INDEX CONCURRENTLY ${name};
> > ALTER INDEX ${name}_tmp RENAME TO ${name};
> >
> > The only tricks here are if ${name}_tmp is already taken, in which case
> > you might as well just error out (or try a few different names), and if
> > ${name} is already in use by the time you get to the last line, in which
> > case you can log a warning or an error.
> >
> > What am I missing?
>
> That this is already recorded in my book> ;-)
>
> And also that REINDEX CONCURRENTLY doesn't work like that, yet.

The last submitted patch works pretty similar:

CREATE INDEX CONCURRENTLY $name_cct;
ALTER INDEX $name RENAME TO cct_$name;
ALTER INDEX $name_tmp RENAME TO $tmp;
ALTER INDEX $name_tmp RENAME TO $name_cct;
DROP INDEX CONURRENCTLY $name_cct;

It does that under an exlusive locks, but doesn't handle dependencies
yet...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Dec 10, 2012 at 11:51 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> Btw, as an example of the problems caused by renaming:
>
> postgres=# CREATE TABLE a (id serial primary key); CREATE TABLE b(id
> serial primary key, a_id int REFERENCES a);
> CREATE TABLE
> Time: 137.840 ms
> CREATE TABLE
> Time: 143.500 ms
> postgres=# \d b
> Table "public.b"
> Column | Type | Modifiers
> --------+---------+------------------------------------------------
> id | integer | not null default nextval('b_id_seq'::regclass)
> a_id | integer |
> Indexes:
> "b_pkey" PRIMARY KEY, btree (id)
> Foreign-key constraints:
> "b_a_id_fkey" FOREIGN KEY (a_id) REFERENCES a(id)
>
> postgres=# REINDEX TABLE a CONCURRENTLY;
> NOTICE: drop cascades to constraint b_a_id_fkey on table b
> REINDEX
> Time: 248.992 ms
> postgres=# \d b
> Table "public.b"
> Column | Type | Modifiers
> --------+---------+------------------------------------------------
> id | integer | not null default nextval('b_id_seq'::regclass)
> a_id | integer |
> Indexes:
> "b_pkey" PRIMARY KEY, btree (id)
>
Oops. I will fix that in the next version of the patch. There should be an
elegant way to change the dependencies at the swap phase.
--
Michael Paquier
http://michael.otacoo.com

On Mon, Dec 10, 2012 at 5:18 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 12/8/12 9:40 AM, Tom Lane wrote:
>> I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
>> preserve the index name exactly. Something like adding or removing
>> trailing underscores would probably serve to generate a nonconflicting
>> name that's not too unsightly.
>
> If you think you can rename an index without an exclusive lock, then why
> not rename it back to the original name when you're done?

Yeah... and also, why do you think that? I thought the idea that we
could do any such thing had been convincingly refuted.

Frankly, I think that if REINDEX CONCURRENTLY is just shorthand for
"CREATE INDEX CONCURRENTLY with a different name and then DROP INDEX
CONCURRENTLY on the old name", it's barely worth doing. People can do
that already, and do, and then we don't have to explain the wart that
the name changes under you.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2012-12-11 15:23:52 -0500, Robert Haas wrote:
> On Mon, Dec 10, 2012 at 5:18 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > On 12/8/12 9:40 AM, Tom Lane wrote:
> >> I'm tempted to propose that REINDEX CONCURRENTLY simply not try to
> >> preserve the index name exactly. Something like adding or removing
> >> trailing underscores would probably serve to generate a nonconflicting
> >> name that's not too unsightly.
> >
> > If you think you can rename an index without an exclusive lock, then why
> > not rename it back to the original name when you're done?
>
> Yeah... and also, why do you think that? I thought the idea that we
> could do any such thing had been convincingly refuted.
>
> Frankly, I think that if REINDEX CONCURRENTLY is just shorthand for
> "CREATE INDEX CONCURRENTLY with a different name and then DROP INDEX
> CONCURRENTLY on the old name", it's barely worth doing. People can do
> that already, and do, and then we don't have to explain the wart that
> the name changes under you.

Its fundamentally different in that you can do it with constraints
referencing the index present. And that it works with toast tables.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Thanks for all your comments.
The new version (v5) of this patch fixes the error you found when
reindexing indexes being referenced in foreign keys.
The fix is done with switchIndexConstraintOnForeignKey:pg_constraint.c, in
charge of scanning pg_constraint for foreign keys that refer the parent
relation (confrelid) of the index being swapped and then switch conindid to
the new index if the old index was referenced.
This API also takes care of switching the dependency between the foreign
key and the old index by calling changeDependencyFor.
I also added a regression test for this purpose.

On Tue, Dec 11, 2012 at 12:28 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> Some review comments:
>
> * Some of the added !is_reindex in index_create don't seem safe to
> me.
>
This is added to control concurrent index relation for toast indexes. If we
do not add an additional flag for that it will not be possible to reindex
concurrently a toast index.

> * Why do we now support reindexing exclusion constraints?
>
CREATE INDEX CONCURRENTLY is not supported for exclusive constraints but I
played around with exclusion constraints with my patch and did not
particularly see any problems in supporting them as for example index_build
performs a second scan of the heap when running so it looks enough solid
for that. Is it because the structure of REINDEX CONCURRENTLY patch is
different? Honestly I think no so is there something I am not aware of?

* REINDEX DATABASE .. CONCURRENTLY doesn't work, a variant that does the
> concurrent reindexing for user-tables and non-concurrent for system
> tables would be very useful. E.g. for the upgrade from 9.1.5->9.1.6...
>
OK. I thought that this was out of scope for the time being. I haven't done
anything about that yet. Supporting that will not be complicated as
ReindexRelationsConcurrently (new API) is more flexible now, the only thing
needed is to gather the list of relations that need to be reindexed.

* ISTM index_concurrent_swap should get exlusive locks on the relation
> *before* printing their names. This shouldn't be required because we
> have a lock prohibiting schema changes on the parent table, but it
> feels safer.
>
Done. AccessExclusiveLock is taken before calling RenameRelationInternal
now.

* temporary index names during swapping should also be named via
> ChooseIndexName
>
Done. I used instead ChooseRelationName which is externalized through
defrem.h.

> * why does create_toast_table pass an unconditional 'is_reindex' to
> index_create?
>
Done. The flag is changed to false.

> * would be nice (but thats probably a step #2 thing) to do the
> individual steps of concurrent reindex over multiple relations to
> avoid too much overall waiting for other transactions.
>
I think I did that by now using one transaction per index for each
operation except the drop phase...

> * ReindexConcurrentIndexes:
>

I renamed ReindexConcurrentIndexes to ReindexRelationsConcurrently and
changed the arguments it used to something more generic:
ReindexRelationsConcurrently(List *relationIds)
relationIds is a list of relation Oids that can be include tables and/or
indexes Oid.
Based on this list of relation Oid, we build the list of indexes that are
rebuilt, including the toast indexes if necessary.

* says " Such indexes are simply bypassed if caller has not specified
> anything." but ERROR's. Imo ERROR is fine, but the comment should be
> adjusted...
>
Done.

>
> * should perhaps be names ReindexIndexesConcurrently?
>
Kind of done.

> * Imo the PHASE 1 comment should be after gathering/validitating the
> chosen indexes
>
Comment is moved. Thanks.

> * It seems better to me to do use individual transactions + snapshots
> for each index, no need to keep very long transactions open (PHASE
> 2/3)
>
Good point. I did that. Now individual transactions are used for each index.

> * s/same whing/same thing/
>
Done.

> * Shouldn't a CacheInvalidateRelcacheByRelid be done after PHASE 2 and
> 5 as well?
>
Done. Nice catch.

> * PHASE 6 should acquire exlusive locks on the indexes
>
The necessary lock is taken when calling index_drop through
performMultipleDeletion. Do you think it is not enough and that i should
add an Exclusive lock inside index_concurrent_drop?

* can some of index_concurrent_* infrastructure be reused for
> DROP INDEX CONCURRENTLY?
>
Indeed. After looking at the code I found that that 2 steps are done in a
concurrent context: invalidating the index and set it as dead.
As REINDEX CONCURRENTLY does the following 2 steps in batch for a list of
indexes, I added index_concurrent_set_dead to set up the dropped indexes as
dead, and index_concurrent_clear_valid. Those 2 functions are used by both
REINDEX CONCURRENTLY and DROP INDEX CONCURRENTLY.

* in CREATE/DROP INDEX CONCURRENTLY 'CONCURRENTLY comes before the
> object name, should we keep that conventioN?
>
Good point. I changed the grammar to REINDEX obj [ CONCURRENTLY ] objname.

Thanks,
--
Michael Paquier
http://michael.otacoo.com

On 2012-12-17 11:44:00 +0900, Michael Paquier wrote:
> Thanks for all your comments.
> The new version (v5) of this patch fixes the error you found when
> reindexing indexes being referenced in foreign keys.
> The fix is done with switchIndexConstraintOnForeignKey:pg_constraint.c, in
> charge of scanning pg_constraint for foreign keys that refer the parent
> relation (confrelid) of the index being swapped and then switch conindid to
> the new index if the old index was referenced.
> This API also takes care of switching the dependency between the foreign
> key and the old index by calling changeDependencyFor.
> I also added a regression test for this purpose.

Ok. Are there no other depencencies towards indexes? I don't know of any
right now, but I have the feeling there were some other cases.

> On Tue, Dec 11, 2012 at 12:28 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
>
> > Some review comments:
> >
> > * Some of the added !is_reindex in index_create don't seem safe to
> > me.
> >
> This is added to control concurrent index relation for toast indexes. If we
> do not add an additional flag for that it will not be possible to reindex
> concurrently a toast index.

I think some of them were added for cases that didn't seem to be related
to that. I'll recheck in the current version.

> > * Why do we now support reindexing exclusion constraints?
> >
> CREATE INDEX CONCURRENTLY is not supported for exclusive constraints but I
> played around with exclusion constraints with my patch and did not
> particularly see any problems in supporting them as for example index_build
> performs a second scan of the heap when running so it looks enough solid
> for that. Is it because the structure of REINDEX CONCURRENTLY patch is
> different? Honestly I think no so is there something I am not aware of?

I think I asked because you had added an && !is_reindex to one of the
checks.

If I recall the reason why concurrent index builds couldn't support
exclusion constraints correctly - namely that we cannot use them to
check for new row versions when the index is in the ready && !valid
state - that shouldn't be a problem when we have a valid version of an
old index arround because that enforces everything. It would maybe need
an appropriate if (!isvalid) in the exclusion constraint code, but that
should be it.

> * REINDEX DATABASE .. CONCURRENTLY doesn't work, a variant that does the
> > concurrent reindexing for user-tables and non-concurrent for system
> > tables would be very useful. E.g. for the upgrade from 9.1.5->9.1.6...
> >
> OK. I thought that this was out of scope for the time being. I haven't done
> anything about that yet. Supporting that will not be complicated as
> ReindexRelationsConcurrently (new API) is more flexible now, the only thing
> needed is to gather the list of relations that need to be reindexed.

Imo that so greatly reduces the usability of this patch that you should
treat it as in scope ;). Especially as you say, it really shouldn't be
that much work with all the groundwork built.

> > * would be nice (but thats probably a step #2 thing) to do the
> > individual steps of concurrent reindex over multiple relations to
> > avoid too much overall waiting for other transactions.
> >
> I think I did that by now using one transaction per index for each
> operation except the drop phase...

Without yet having read the new version, I think thats not what I
meant. There currently is a wait for concurrent transactions to end
after most of the phases for every relation, right? If you have a busy
database with somewhat longrunning transactions thats going to slow
everything down with waiting quite bit. I wondered whether it would make
sense to do PHASE1 for all indexes in all relations, then wait once,
then PHASE2...
That obviously has some space and index maintainece overhead issues, but
its probably sensible anyway in many cases.

> > * PHASE 6 should acquire exlusive locks on the indexes
> >
> The necessary lock is taken when calling index_drop through
> performMultipleDeletion. Do you think it is not enough and that i should
> add an Exclusive lock inside index_concurrent_drop?

It seems to be safer to acquire it earlier, otherwise the likelihood for
deadlocks seems to be slightly higher as youre increasing the lock
severity. And it shouldn't cause any disadvantages,s o ...

Starts to look really nice now!

Isn't the following block content thats mostly available somewhere else
already?

> + <refsect2 id="SQL-REINDEX-CONCURRENTLY">
> + <title id="SQL-REINDEX-CONCURRENTLY-title">Rebuilding Indexes Concurrently</title>
> +
> + <indexterm zone="SQL-REINDEX-CONCURRENTLY">
> + <primary>index</primary>
> + <secondary>rebuilding concurrently</secondary>
> + </indexterm>
> +
> + <para>
> + Rebuilding an index can interfere with regular operation of a database.
> + Normally <productname>PostgreSQL</> locks the table whose index is rebuilt
> + against writes and performs the entire index build with a single scan of the
> + table. Other transactions can still read the table, but if they try to
> + insert, update, or delete rows in the table they will block until the
> + index rebuild is finished. This could have a severe effect if the system is
> + a live production database. Very large tables can take many hours to be
> + indexed, and even for smaller tables, an index rebuild can lock out writers
> + for periods that are unacceptably long for a production system.
> + </para>
...
> + <para>
> + Regular index builds permit other regular index builds on the
> + same table to occur in parallel, but only one concurrent index build
> + can occur on a table at a time. In both cases, no other types of schema
> + modification on the table are allowed meanwhile. Another difference
> + is that a regular <command>REINDEX TABLE</> or <command>REINDEX INDEX</>
> + command can be performed within a transaction block, but
> + <command>REINDEX CONCURRENTLY</> cannot. <command>REINDEX DATABASE</> is
> + by default not allowed to run inside a transaction block, so in this case
> + <command>CONCURRENTLY</> is not supported.
> + </para>
> +

> - if (concurrent && is_exclusion)
> + if (concurrent && is_exclusion && !is_reindex)
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg_internal("concurrent index creation for exclusion constraints is not supported")));

This is what I referred to above wrt reindex and CONCURRENTLY. We
shouldn't pass concurrently if we don't deem it to be safe for exlusion
constraints.

> +/*
> + * index_concurrent_drop
> + *
> + * Drop a list of indexes as the last step of a concurrent process. Deletion is
> + * done through performDeletion or dependencies of the index are not dropped.
> + * At this point all the indexes are already considered as invalid and dead so
> + * they can be dropped without using any concurrent options.
> + */
> +void
> +index_concurrent_drop(List *indexIds)
> +{
> + ListCell *lc;
> + ObjectAddresses *objects = new_object_addresses();
> +
> + Assert(indexIds != NIL);
> +
> + /* Scan the list of indexes and build object list for normal indexes */
> + foreach(lc, indexIds)
> + {
> + Oid indexOid = lfirst_oid(lc);
> + Oid constraintOid = get_index_constraint(indexOid);
> + ObjectAddress object;
> +
> + /* Register constraint or index for drop */
> + if (OidIsValid(constraintOid))
> + {
> + object.classId = ConstraintRelationId;
> + object.objectId = constraintOid;
> + }
> + else
> + {
> + object.classId = RelationRelationId;
> + object.objectId = indexOid;
> + }
> +
> + object.objectSubId = 0;
> +
> + /* Add object to list */
> + add_exact_object_address(&object, objects);
> + }
> +
> + /* Perform deletion for normal and toast indexes */
> + performMultipleDeletions(objects,
> + DROP_RESTRICT,
> + 0);
> +}

Just for warm and fuzzy feeling I think it would be a good idea to
recheck that indexes are !indislive here.

> diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
> index 5e8c6da..55c092d 100644
> +
> +/*
> + * switchIndexConstraintOnForeignKey
> + *
> + * Switch foreign keys references for a given index to a new index created
> + * concurrently. This process is used when swapping indexes for a concurrent
> + * process. All the constraints that are not referenced externally like primary
> + * keys or unique indexes should be switched using the structure of index.c for
> + * concurrent index creation and drop.
> + * This function takes care of also switching the dependencies of the foreign
> + * key from the old index to the new index in pg_depend.
> + *
> + * In order to complete this process, the following process is done:
> + * 1) Scan pg_constraint and extract the list of foreign keys that refer to the
> + * parent relation of the index being swapped as conrelid.
> + * 2) Check in this list the foreign keys that use the old index as reference
> + * here with conindid
> + * 3) Update field conindid to the new index Oid on all the foreign keys
> + * 4) Switch dependencies of the foreign key to the new index
> + */
> +void
> +switchIndexConstraintOnForeignKey(Oid parentOid,
> + Oid oldIndexOid,
> + Oid newIndexOid)
> +{
> + ScanKeyData skey[1];
> + SysScanDesc conscan;
> + Relation conRel;
> + HeapTuple htup;
> +
> + /*
> + * Search pg_constraint for the foreign key constraints associated
> + * with the index by scanning using conrelid.
> + */
> + ScanKeyInit(&skey[0],
> + Anum_pg_constraint_confrelid,
> + BTEqualStrategyNumber, F_OIDEQ,
> + ObjectIdGetDatum(parentOid));
> +
> + conRel = heap_open(ConstraintRelationId, AccessShareLock);
> + conscan = systable_beginscan(conRel, ConstraintForeignRelidIndexId,
> + true, SnapshotNow, 1, skey);
> +
> + while (HeapTupleIsValid(htup = systable_getnext(conscan)))
> + {
> + Form_pg_constraint contuple = (Form_pg_constraint) GETSTRUCT(htup);
> +
> + /* Check if a foreign constraint uses the index being swapped */
> + if (contuple->contype == CONSTRAINT_FOREIGN &&
> + contuple->confrelid == parentOid &&
> + contuple->conindid == oldIndexOid)
> + {
> + /* Found an index, so update its pg_constraint entry */
> + contuple->conindid = newIndexOid;
> + /* And write it back in place */
> + heap_inplace_update(conRel, htup);

I am pretty doubtful that using heap_inplace_update is the correct thing
to do here. What if we fail later? Even if there's some justification
for it being safe it deserves a big comment.

The other cases where heap_inplace_update is used in the context of
CONCURRENTLY are pretty careful about where to do it and have special
state flags of indicating that this has been done...

>
> +bool
> +ReindexRelationsConcurrently(List *relationIds)
> +{
> + foreach(lc, relationIds)
> + {
> + Oid relationOid = lfirst_oid(lc);
> +
> + switch (get_rel_relkind(relationOid))
> + {
> + case RELKIND_RELATION:
> + {
> + /*
> + * In the case of a relation, find all its indexes
> + * including toast indexes.
> + */
> + Relation heapRelation = heap_open(relationOid,
> + ShareUpdateExclusiveLock);
> +
> + /* Relation on which is based index cannot be shared */
> + if (heapRelation->rd_rel->relisshared)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("concurrent reindex is not supported for shared relations")));
> +
> + /* Add all the valid indexes of relation to list */
> + foreach(lc2, RelationGetIndexList(heapRelation))
> + {
> + Oid cellOid = lfirst_oid(lc2);
> + Relation indexRelation = index_open(cellOid,
> + ShareUpdateExclusiveLock);
> +
> + if (!indexRelation->rd_index->indisvalid)
> + ereport(WARNING,
> + (errcode(ERRCODE_INDEX_CORRUPTED),
> + errmsg("cannot reindex concurrently invalid index \"%s.%s\", bypassing",
> + get_namespace_name(get_rel_namespace(cellOid)),
> + get_rel_name(cellOid))));
> + else
> + indexIds = list_append_unique_oid(indexIds,
> + cellOid);
> +
> + index_close(indexRelation, ShareUpdateExclusiveLock);
> + }

Why are we releasing the locks here if we are going to reindex the
relations? They might change inbetween. I think we should take an
appropriate lock here, including the locks on the parent relations. Yes,
its slightly more duplicative code, and not acquiring locks multiple
times is somewhat complicated, but I think its required.

I think you should also explicitly do the above in a transaction...

> + /*
> + * Phase 2 of REINDEX CONCURRENTLY
> + *
> + * Build concurrent indexes in a separate transaction for each index to
> + * avoid having open transactions for an unnecessary long time. We also
> + * need to wait until no running transactions could have the parent table
> + * of index open. A concurrent build is done for each concurrent
> + * index that will replace the old indexes.
> + */
> +
> + /* Get the first element of concurrent index list */
> + lc2 = list_head(concurrentIndexIds);
> +
> + foreach(lc, indexIds)
> + {
> + Relation indexRel;
> + Oid indOid = lfirst_oid(lc);
> + Oid concurrentOid = lfirst_oid(lc2);
> + Oid relOid;
> + bool primary;
> + LOCKTAG *heapLockTag = NULL;
> + ListCell *cell;
> +
> + /* Move to next concurrent item */
> + lc2 = lnext(lc2);
> +
> + /* Start new transaction for this index concurrent build */
> + StartTransactionCommand();
> +
> + /* Get the parent relation Oid */
> + relOid = IndexGetRelation(indOid, false);
> +
> + /*
> + * Find the locktag of parent table for this index, we need to wait for
> + * locks on it.
> + */
> + foreach(cell, lockTags)
> + {
> + LOCKTAG *localTag = (LOCKTAG *) lfirst(cell);
> + if (relOid == localTag->locktag_field2)
> + heapLockTag = localTag;
> + }
> +
> + Assert(heapLockTag && heapLockTag->locktag_field2 != InvalidOid);
> + WaitForVirtualLocks(*heapLockTag, ShareLock);

Why do we have to do the WaitForVirtualLocks here? Shouldn't we do this
once for all relations after each phase? Otherwise the waiting time will
really start to hit when you do this on a somewhat busy server.

> + /*
> + * Invalidate the relcache for the table, so that after this commit all
> + * sessions will refresh any cached plans taht might reference the index.
> + */
> + CacheInvalidateRelcacheByRelid(relOid);

I am not sure whether I suggested adding a
CacheInvalidateRelcacheByRelid here, but afaics its not required yet,
the plan isn't valid yet, so no need for replanning.

> + indexRel = index_open(indOid, ShareUpdateExclusiveLock);

I wonder we should directly open it exlusive here given its going to
opened exclusively in a bit anyway. Not that that will really reduce the
deadlock likelihood since we already hold the ShareUpdateExclusiveLock
in session mode ...

> + /*
> + * Phase 5 of REINDEX CONCURRENTLY
> + *
> + * The old indexes need to be marked as not ready. We need also to wait for
> + * transactions that might use them. Each operation is performed with a
> + * separate transaction.
> + */
> +
> + /* Mark the old indexes as not ready */
> + foreach(lc, indexIds)
> + {
> + LOCKTAG *heapLockTag;
> + Oid indOid = lfirst_oid(lc);
> + Oid relOid;
> +
> + StartTransactionCommand();
> + relOid = IndexGetRelation(indOid, false);
> +
> + /*
> + * Find the locktag of parent table for this index, we need to wait for
> + * locks on it.
> + */
> + foreach(lc2, lockTags)
> + {
> + LOCKTAG *localTag = (LOCKTAG *) lfirst(lc2);
> + if (relOid == localTag->locktag_field2)
> + heapLockTag = localTag;
> + }
> +
> + Assert(heapLockTag && heapLockTag->locktag_field2 != InvalidOid);
> +
> + /* Finish the index invalidation and set it as dead */
> + index_concurrent_set_dead(indOid, relOid, *heapLockTag);
> +
> + /* Commit this transaction to make the update visible. */
> + CommitTransactionCommand();
> + }

No waiting here?

> + StartTransactionCommand();
> +
> + /* Get fresh snapshot for next step */
> + PushActiveSnapshot(GetTransactionSnapshot());
> +
> + /*
> + * Phase 6 of REINDEX CONCURRENTLY
> + *
> + * Drop the old indexes. This needs to be done through performDeletion
> + * or related dependencies will not be dropped for the old indexes. The
> + * internal mechanism of DROP INDEX CONCURRENTLY is not used as here the
> + * indexes are already considered as dead and invalid, so they will not
> + * be used by other backends.
> + */
> + index_concurrent_drop(indexIds);
> +
> + /*
> + * Last thing to do is release the session-level lock on the parent table
> + * and the indexes of table.
> + */
> + foreach(lc, relationLocks)
> + {
> + LockRelId lockRel = * (LockRelId *) lfirst(lc);
> + UnlockRelationIdForSession(&lockRel, ShareUpdateExclusiveLock);
> + }
> +
> + /* We can do away with our snapshot */
> + PopActiveSnapshot();

I think I would do the drop in individual transactions as well.

More at another time, shouldn't have started doing this now...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

OK. I am back to this patch after a too long time.

Please find an updated version of the patch attached (v6). I address all
the previous comments, except regarding the support for REINDEX DATABASE
CONCURRENTLY. I am working on that precisely but I am not sure it is that
straight-forward...

On Wed, Dec 19, 2012 at 11:24 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2012-12-17 11:44:00 +0900, Michael Paquier wrote:
> > Thanks for all your comments.
> > The new version (v5) of this patch fixes the error you found when
> > reindexing indexes being referenced in foreign keys.
> > The fix is done with switchIndexConstraintOnForeignKey:pg_constraint.c,
> in
> > charge of scanning pg_constraint for foreign keys that refer the parent
> > relation (confrelid) of the index being swapped and then switch conindid
> to
> > the new index if the old index was referenced.
> > This API also takes care of switching the dependency between the foreign
> > key and the old index by calling changeDependencyFor.
> > I also added a regression test for this purpose.
>
> Ok. Are there no other depencencies towards indexes? I don't know of any
> right now, but I have the feeling there were some other cases.
>
The patch cover the cases of PRIMARY, UNIQUE, normal indexes, exclusion
constraints and foreign keys. Just based on the docs, I don't think there
is something missing.
http://www.postgresql.org/docs/9.2/static/ddl-constraints.html

> > * REINDEX DATABASE .. CONCURRENTLY doesn't work, a variant that does the
> > > concurrent reindexing for user-tables and non-concurrent for system
> > > tables would be very useful. E.g. for the upgrade from
> 9.1.5->9.1.6...
> > >
> > OK. I thought that this was out of scope for the time being. I haven't
> done
> > anything about that yet. Supporting that will not be complicated as
> > ReindexRelationsConcurrently (new API) is more flexible now, the only
> thing
> > needed is to gather the list of relations that need to be reindexed.
>
> Imo that so greatly reduces the usability of this patch that you should
> treat it as in scope ;). Especially as you say, it really shouldn't be
> that much work with all the groundwork built.
>
OK. So... What should we do when a REINDEX DATABASE CONCURRENTLY is done?
- only reindex user tables and bypass system tables?
- reindex user tables concurrently and system tables non-concurrently?
- forbid this operation when this operation is done on a database having
system tables?
Some input?
Btw, the attached version of the patch does not include this feature yet
but I am working on it.

>
> > > * would be nice (but thats probably a step #2 thing) to do the
> > > individual steps of concurrent reindex over multiple relations to
> > > avoid too much overall waiting for other transactions.
> > >
> > I think I did that by now using one transaction per index for each
> > operation except the drop phase...
>
> Without yet having read the new version, I think thats not what I
> meant. There currently is a wait for concurrent transactions to end
> after most of the phases for every relation, right? If you have a busy
> database with somewhat longrunning transactions thats going to slow
> everything down with waiting quite bit. I wondered whether it would make
> sense to do PHASE1 for all indexes in all relations, then wait once,
> then PHASE2...

That obviously has some space and index maintainece overhead issues, but
> its probably sensible anyway in many cases.
>
OK, phase 1 is done with only one transaction for all the indexes. Do you
mean that we should do that with a single transaction for each index?

> Isn't the following block content thats mostly available somewhere else
> already?
> [... doc extract ...]
>
Yes, this portion of the docs is pretty similar to what is findable in
CREATE INDEX CONCURRENTLY. Why not creating a new common documentation
section that CREATE INDEX CONCURRENTLY and REINDEX CONCURRENTLY could refer
to? I think we should first work on the code and then do the docs properly
though.

> > - if (concurrent && is_exclusion)
> > + if (concurrent && is_exclusion && !is_reindex)
> > ereport(ERROR,
> > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > errmsg_internal("concurrent index
> creation for exclusion constraints is not supported")));
>
> This is what I referred to above wrt reindex and CONCURRENTLY. We
> shouldn't pass concurrently if we don't deem it to be safe for exlusion
> constraints.
>
So does that mean that it is not possible to create an exclusive constraint
in a concurrent context? Code path used by REINDEX concurrently permits to
create an index in parallel of an existing one and not a completely new
index. Shouldn't this work for indexes used by exclusion indexes also?

> > +/*
> > + * index_concurrent_drop
> > + *
> > + * Drop a list of indexes as the last step of a concurrent process.
> Deletion is
> > + * done through performDeletion or dependencies of the index are not
> dropped.
> > + * At this point all the indexes are already considered as invalid and
> dead so
> > + * they can be dropped without using any concurrent options.
> > + */
> > +void
> > +index_concurrent_drop(List *indexIds)
> > +{
> > + ListCell *lc;
> > + ObjectAddresses *objects = new_object_addresses();
> > +
> > + Assert(indexIds != NIL);
> > +
> > + /* Scan the list of indexes and build object list for normal
> indexes */
> > + foreach(lc, indexIds)
> > + {
> > + Oid indexOid = lfirst_oid(lc);
> > + Oid constraintOid =
> get_index_constraint(indexOid);
> > + ObjectAddress object;
> > +
> > + /* Register constraint or index for drop */
> > + if (OidIsValid(constraintOid))
> > + {
> > + object.classId = ConstraintRelationId;
> > + object.objectId = constraintOid;
> > + }
> > + else
> > + {
> > + object.classId = RelationRelationId;
> > + object.objectId = indexOid;
> > + }
> > +
> > + object.objectSubId = 0;
> > +
> > + /* Add object to list */
> > + add_exact_object_address(&object, objects);
> > + }
> > +
> > + /* Perform deletion for normal and toast indexes */
> > + performMultipleDeletions(objects,
> > + DROP_RESTRICT,
> > + 0);
> > +}
>
> Just for warm and fuzzy feeling I think it would be a good idea to
> recheck that indexes are !indislive here.
>
OK done. The indexes with indislive set at true are not bypassed now.

> diff --git a/src/backend/catalog/pg_constraint.c
> b/src/backend/catalog/pg_constraint.c
> > index 5e8c6da..55c092d 100644
> > +
> > +/*
> > + * switchIndexConstraintOnForeignKey
> > + *
> > + * Switch foreign keys references for a given index to a new index
> created
> > + * concurrently. This process is used when swapping indexes for a
> concurrent
> > + * process. All the constraints that are not referenced externally like
> primary
> > + * keys or unique indexes should be switched using the structure of
> index.c for
> > + * concurrent index creation and drop.
> > + * This function takes care of also switching the dependencies of the
> foreign
> > + * key from the old index to the new index in pg_depend.
> > + *
> > + * In order to complete this process, the following process is done:
> > + * 1) Scan pg_constraint and extract the list of foreign keys that
> refer to the
> > + * parent relation of the index being swapped as conrelid.
> > + * 2) Check in this list the foreign keys that use the old index as
> reference
> > + * here with conindid
> > + * 3) Update field conindid to the new index Oid on all the foreign keys
> > + * 4) Switch dependencies of the foreign key to the new index
> > + */
> > +void
> > +switchIndexConstraintOnForeignKey(Oid parentOid,
> > + Oid
> oldIndexOid,
> > + Oid
> newIndexOid)
> > +{
> > + ScanKeyData skey[1];
> > + SysScanDesc conscan;
> > + Relation conRel;
> > + HeapTuple htup;
> > +
> > + /*
> > + * Search pg_constraint for the foreign key constraints associated
> > + * with the index by scanning using conrelid.
> > + */
> > + ScanKeyInit(&skey[0],
> > + Anum_pg_constraint_confrelid,
> > + BTEqualStrategyNumber, F_OIDEQ,
> > + ObjectIdGetDatum(parentOid));
> > +
> > + conRel = heap_open(ConstraintRelationId, AccessShareLock);
> > + conscan = systable_beginscan(conRel, ConstraintForeignRelidIndexId,
> > + true,
> SnapshotNow, 1, skey);
> > +
> > + while (HeapTupleIsValid(htup = systable_getnext(conscan)))
> > + {
> > + Form_pg_constraint contuple = (Form_pg_constraint)
> GETSTRUCT(htup);
> > +
> > + /* Check if a foreign constraint uses the index being
> swapped */
> > + if (contuple->contype == CONSTRAINT_FOREIGN &&
> > + contuple->confrelid == parentOid &&
> > + contuple->conindid == oldIndexOid)
> > + {
> > + /* Found an index, so update its pg_constraint
> entry */
> > + contuple->conindid = newIndexOid;
> > + /* And write it back in place */
> > + heap_inplace_update(conRel, htup);
>
> I am pretty doubtful that using heap_inplace_update is the correct thing
> to do here. What if we fail later? Even if there's some justification
> for it being safe it deserves a big comment.
>
> The other cases where heap_inplace_update is used in the context of
> CONCURRENTLY are pretty careful about where to do it and have special
> state flags of indicating that this has been done...
>
Oops, fixed. I changed it to simple_heap_update.

> >
> > +bool
> > +ReindexRelationsConcurrently(List *relationIds)
> > +{
> > + foreach(lc, relationIds)
> > + {
> > + Oid relationOid = lfirst_oid(lc);
> > +
> > + switch (get_rel_relkind(relationOid))
> > + {
> > + case RELKIND_RELATION:
> > + {
> > + /*
> > + * In the case of a relation, find
> all its indexes
> > + * including toast indexes.
> > + */
> > + Relation heapRelation =
> heap_open(relationOid,
> > +
> ShareUpdateExclusiveLock);
> > +
> > + /* Relation on which is based
> index cannot be shared */
> > + if
> (heapRelation->rd_rel->relisshared)
> > + ereport(ERROR,
> > +
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > +
> errmsg("concurrent reindex is not supported for shared relations")));
> > +
> > + /* Add all the valid indexes of
> relation to list */
> > + foreach(lc2,
> RelationGetIndexList(heapRelation))
> > + {
> > + Oid
> cellOid = lfirst_oid(lc2);
> > + Relation
> indexRelation = index_open(cellOid,
> > +
> ShareUpdateExclusiveLock);
> > +
> > + if
> (!indexRelation->rd_index->indisvalid)
> > + ereport(WARNING,
> > +
> (errcode(ERRCODE_INDEX_CORRUPTED),
> > +
> errmsg("cannot reindex concurrently invalid index \"%s.%s\", bypassing",
> > +
> get_namespace_name(get_rel_namespace(cellOid)),
> > +
> get_rel_name(cellOid))));
> > + else
> > + indexIds =
> list_append_unique_oid(indexIds,
> > +
> cellOid);
> > +
> > + index_close(indexRelation,
> ShareUpdateExclusiveLock);
> > + }
>
> Why are we releasing the locks here if we are going to reindex the
> relations? They might change inbetween. I think we should take an
> appropriate lock here, including the locks on the parent relations. Yes,
> its slightly more duplicative code, and not acquiring locks multiple
> times is somewhat complicated, but I think its required.
>
OK, the locks are now maintained until the end of the transaction and when
the session locks are taken on those relations, so it will not be possible
to have schema changes between the moment where the list of indexes is
built and the moment the session locks are taken.

> I think you should also explicitly do the above in a transaction...
>
I am not sure I get your point here. This phase is in place to gather the
list of all the indexes to reindex based on the list of relations given by
caller.

>
> > + /*
> > + * Phase 2 of REINDEX CONCURRENTLY
> > + *
> > + * Build concurrent indexes in a separate transaction for each
> index to
> > + * avoid having open transactions for an unnecessary long time.
> We also
> > + * need to wait until no running transactions could have the
> parent table
> > + * of index open. A concurrent build is done for each concurrent
> > + * index that will replace the old indexes.
> > + */
> > +
> > + /* Get the first element of concurrent index list */
> > + lc2 = list_head(concurrentIndexIds);
> > +
> > + foreach(lc, indexIds)
> > + {
> > + Relation indexRel;
> > + Oid indOid = lfirst_oid(lc);
> > + Oid concurrentOid = lfirst_oid(lc2);
> > + Oid relOid;
> > + bool primary;
> > + LOCKTAG *heapLockTag = NULL;
> > + ListCell *cell;
> > +
> > + /* Move to next concurrent item */
> > + lc2 = lnext(lc2);
> > +
> > + /* Start new transaction for this index concurrent build */
> > + StartTransactionCommand();
> > +
> > + /* Get the parent relation Oid */
> > + relOid = IndexGetRelation(indOid, false);
> > +
> > + /*
> > + * Find the locktag of parent table for this index, we
> need to wait for
> > + * locks on it.
> > + */
> > + foreach(cell, lockTags)
> > + {
> > + LOCKTAG *localTag = (LOCKTAG *) lfirst(cell);
> > + if (relOid == localTag->locktag_field2)
> > + heapLockTag = localTag;
> > + }
> > +
> > + Assert(heapLockTag && heapLockTag->locktag_field2 !=
> InvalidOid);
> > + WaitForVirtualLocks(*heapLockTag, ShareLock);
>
> Why do we have to do the WaitForVirtualLocks here? Shouldn't we do this
> once for all relations after each phase? Otherwise the waiting time will
> really start to hit when you do this on a somewhat busy server.
>
Each new index is built and set as ready in a separate single transaction,
so doesn't it make sense to wait for the parent relation each time. It is
possible to wait for a parent relation only once during this phase but in
this case all the indexes of the same relation need to be set as ready in
the same transaction. So here the choice is either to wait for the same
relation multiple times for a single index or wait once for a parent
relation but we build all the concurrent indexes within the same
transaction. Choice 1 makes the code clearer and more robust to my mind as
the phase 2 is done clearly for each index separately. Thoughts?

>
> > + /*
> > + * Invalidate the relcache for the table, so that after
> this commit all
> > + * sessions will refresh any cached plans taht might
> reference the index.
> > + */
> > + CacheInvalidateRelcacheByRelid(relOid);
>
> I am not sure whether I suggested adding a
> CacheInvalidateRelcacheByRelid here, but afaics its not required yet,
> the plan isn't valid yet, so no need for replanning.
>
Sure I removed it.

> > + indexRel = index_open(indOid, ShareUpdateExclusiveLock);
>
> I wonder we should directly open it exlusive here given its going to
> opened exclusively in a bit anyway. Not that that will really reduce the
> deadlock likelihood since we already hold the ShareUpdateExclusiveLock
> in session mode ...
>
I tried to use an AccessExclusiveLock here but it happens that this is not
compatible with index_set_state_flags. Does taking an exclusive lock
increments the transaction ID of running transaction? Because what I am
seeing is that taking AccessExclusiveLock on this index does a transaction
update.
For those reasons current code sticks with ShareUpdateExclusiveLock. Not a
big deal btw...

> > + /*
> > + * Phase 5 of REINDEX CONCURRENTLY
> > + *
> > + * The old indexes need to be marked as not ready. We need also to
> wait for
> > + * transactions that might use them. Each operation is performed
> with a
> > + * separate transaction.
> > + */
> > +
> > + /* Mark the old indexes as not ready */
> > + foreach(lc, indexIds)
> > + {
> > + LOCKTAG *heapLockTag;
> > + Oid indOid = lfirst_oid(lc);
> > + Oid relOid;
> > +
> > + StartTransactionCommand();
> > + relOid = IndexGetRelation(indOid, false);
> > +
> > + /*
> > + * Find the locktag of parent table for this index, we
> need to wait for
> > + * locks on it.
> > + */
> > + foreach(lc2, lockTags)
> > + {
> > + LOCKTAG *localTag = (LOCKTAG *) lfirst(lc2);
> > + if (relOid == localTag->locktag_field2)
> > + heapLockTag = localTag;
> > + }
> > +
> > + Assert(heapLockTag && heapLockTag->locktag_field2 !=
> InvalidOid);
> > +
> > + /* Finish the index invalidation and set it as dead */
> > + index_concurrent_set_dead(indOid, relOid, *heapLockTag);
> > +
> > + /* Commit this transaction to make the update visible. */
> > + CommitTransactionCommand();
> > + }
>
> No waiting here?
>
A wait phase is done inside index_concurrent_set_dead, so no problem.

> > + StartTransactionCommand();
> > +
> > + /* Get fresh snapshot for next step */
> > + PushActiveSnapshot(GetTransactionSnapshot());
> > +
> > + /*
> > + * Phase 6 of REINDEX CONCURRENTLY
> > + *
> > + * Drop the old indexes. This needs to be done through
> performDeletion
> > + * or related dependencies will not be dropped for the old
> indexes. The
> > + * internal mechanism of DROP INDEX CONCURRENTLY is not used as
> here the
> > + * indexes are already considered as dead and invalid, so they
> will not
> > + * be used by other backends.
> > + */
> > + index_concurrent_drop(indexIds);
> > +
> > + /*
> > + * Last thing to do is release the session-level lock on the
> parent table
> > + * and the indexes of table.
> > + */
> > + foreach(lc, relationLocks)
> > + {
> > + LockRelId lockRel = * (LockRelId *) lfirst(lc);
> > + UnlockRelationIdForSession(&lockRel,
> ShareUpdateExclusiveLock);
> > + }
> > +
> > + /* We can do away with our snapshot */
> > + PopActiveSnapshot();
>
> I think I would do the drop in individual transactions as well.
>
Done. Each drop is now done in a single transaction.
--
Michael Paquier
http://michael.otacoo.com

Hi,

Please find attached v7 of this patch, adding support for REINDEX DATABASE
CONCURRENTLY.
When using REINDEX DATABASE with CONCURRENTLY, non-system tables are
reindexed concurrently and system tables are reindexed in the normal way,
ie non-concurrently.

Thanks,
--
Michael Paquier
http://michael.otacoo.com

On 2013-01-15 18:16:59 +0900, Michael Paquier wrote:
> OK. I am back to this patch after a too long time.

Dito ;)

> > > > * would be nice (but thats probably a step #2 thing) to do the
> > > > individual steps of concurrent reindex over multiple relations to
> > > > avoid too much overall waiting for other transactions.
> > > >
> > > I think I did that by now using one transaction per index for each
> > > operation except the drop phase...
> >
> > Without yet having read the new version, I think thats not what I
> > meant. There currently is a wait for concurrent transactions to end
> > after most of the phases for every relation, right? If you have a busy
> > database with somewhat longrunning transactions thats going to slow
> > everything down with waiting quite bit. I wondered whether it would make
> > sense to do PHASE1 for all indexes in all relations, then wait once,
> > then PHASE2...
>
> That obviously has some space and index maintainece overhead issues, but
> > its probably sensible anyway in many cases.
> >
> OK, phase 1 is done with only one transaction for all the indexes. Do you
> mean that we should do that with a single transaction for each index?

Yes.

> > Isn't the following block content thats mostly available somewhere else
> > already?
> > [... doc extract ...]
> >
> Yes, this portion of the docs is pretty similar to what is findable in
> CREATE INDEX CONCURRENTLY. Why not creating a new common documentation
> section that CREATE INDEX CONCURRENTLY and REINDEX CONCURRENTLY could refer
> to? I think we should first work on the code and then do the docs properly
> though.

Agreed. I just noticed it when scrolling through the patch.

> > > - if (concurrent && is_exclusion)
> > > + if (concurrent && is_exclusion && !is_reindex)
> > > ereport(ERROR,
> > > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > > errmsg_internal("concurrent index
> > creation for exclusion constraints is not supported")));
> >
> > This is what I referred to above wrt reindex and CONCURRENTLY. We
> > shouldn't pass concurrently if we don't deem it to be safe for exlusion
> > constraints.
> >
> So does that mean that it is not possible to create an exclusive constraint
> in a concurrent context?

Yes, its currently not safe in the general case.

> Code path used by REINDEX concurrently permits to
> create an index in parallel of an existing one and not a completely new
> index. Shouldn't this work for indexes used by exclusion indexes also?

But that fact might safe things. I don't immediately see any reason that
adding a
if (!indisvalid)
return;
to check_exclusion_constraint wouldn't be sufficient if there's another
index with an equivalent definition.

> > > + /*
> > > + * Phase 2 of REINDEX CONCURRENTLY
> > > + *
> > > + * Build concurrent indexes in a separate transaction for each
> > index to
> > > + * avoid having open transactions for an unnecessary long time.
> > We also
> > > + * need to wait until no running transactions could have the
> > parent table
> > > + * of index open. A concurrent build is done for each concurrent
> > > + * index that will replace the old indexes.
> > > + */
> > > +
> > > + /* Get the first element of concurrent index list */
> > > + lc2 = list_head(concurrentIndexIds);
> > > +
> > > + foreach(lc, indexIds)
> > > + {
> > > + Relation indexRel;
> > > + Oid indOid = lfirst_oid(lc);
> > > + Oid concurrentOid = lfirst_oid(lc2);
> > > + Oid relOid;
> > > + bool primary;
> > > + LOCKTAG *heapLockTag = NULL;
> > > + ListCell *cell;
> > > +
> > > + /* Move to next concurrent item */
> > > + lc2 = lnext(lc2);
> > > +
> > > + /* Start new transaction for this index concurrent build */
> > > + StartTransactionCommand();
> > > +
> > > + /* Get the parent relation Oid */
> > > + relOid = IndexGetRelation(indOid, false);
> > > +
> > > + /*
> > > + * Find the locktag of parent table for this index, we
> > need to wait for
> > > + * locks on it.
> > > + */
> > > + foreach(cell, lockTags)
> > > + {
> > > + LOCKTAG *localTag = (LOCKTAG *) lfirst(cell);
> > > + if (relOid == localTag->locktag_field2)
> > > + heapLockTag = localTag;
> > > + }
> > > +
> > > + Assert(heapLockTag && heapLockTag->locktag_field2 !=
> > InvalidOid);
> > > + WaitForVirtualLocks(*heapLockTag, ShareLock);
> >
> > Why do we have to do the WaitForVirtualLocks here? Shouldn't we do this
> > once for all relations after each phase? Otherwise the waiting time will
> > really start to hit when you do this on a somewhat busy server.
> >
> Each new index is built and set as ready in a separate single transaction,
> so doesn't it make sense to wait for the parent relation each time. It is
> possible to wait for a parent relation only once during this phase but in
> this case all the indexes of the same relation need to be set as ready in
> the same transaction. So here the choice is either to wait for the same
> relation multiple times for a single index or wait once for a parent
> relation but we build all the concurrent indexes within the same
> transaction. Choice 1 makes the code clearer and more robust to my mind as
> the phase 2 is done clearly for each index separately. Thoughts?

As far as I understand that code its purpose is to enforce that all
potential users have an up2date definition available. For that we
acquire a lock on all virtualxids of users using that table thus waiting
for them to finish.
Consider the scenario where you have a workload where most transactions
are fairly long (say 10min) and use the same tables (a,b)/indexes(a_1,
a_2, b_1, b_2). With the current strategy you will do:

WaitForVirtualLocks(a_1) -- wait up to 10min
index_build(a_1)
WaitForVirtualLocks(a_2) -- wait up to 10min
index_build(a_2)
...

So instead of waiting up 10 minutes for that phase you have to wait up
to 40.

> > > + indexRel = index_open(indOid, ShareUpdateExclusiveLock);
> >
> > I wonder we should directly open it exlusive here given its going to
> > opened exclusively in a bit anyway. Not that that will really reduce the
> > deadlock likelihood since we already hold the ShareUpdateExclusiveLock
> > in session mode ...
> >
> I tried to use an AccessExclusiveLock here but it happens that this is not
> compatible with index_set_state_flags. Does taking an exclusive lock
> increments the transaction ID of running transaction? Because what I am
> seeing is that taking AccessExclusiveLock on this index does a transaction
> update.

Yep, it does when wal_level = hot_standby because it logs the exclusive
lock to wal so the startup process on the standby can acquire it.

Imo that Assert needs to be moved to the existing callsites if there
isn't an equivalent one already.

> For those reasons current code sticks with ShareUpdateExclusiveLock. Not a
> big deal btw...

Well, lock upgrades make deadlocks more likely.

Ok, of to v7:
+ */
+void
+index_concurrent_swap(Oid newIndexOid, Oid oldIndexOid)
...
+ /*
+ * Take a lock on the old and new index before switching their names. This
+ * avoids having index swapping relying on relation renaming mechanism to
+ * get a lock on the relations involved.
+ */
+ oldIndexRel = relation_open(oldIndexOid, AccessExclusiveLock);
+ newIndexRel = relation_open(newIndexOid, AccessExclusiveLock);
..
+ /*
+ * If the index swapped is a toast index, take an exclusive lock
on its
+ * parent toast relation and then update reltoastidxid to the
new index Oid
+ * value.
+ */
+ if (get_rel_relkind(parentOid) == RELKIND_TOASTVALUE)
+ {
+ Relation pg_class;
+
+ /* Open pg_class and fetch a writable copy of the relation tuple */
+ pg_class = heap_open(parentOid, RowExclusiveLock);
+
+ /* Update the statistics of this pg_class entry with new toast index Oid */
+ index_update_stats(pg_class, false, false, newIndexOid, -1.0);
+
+ /* Close parent relation */
+ heap_close(pg_class, RowExclusiveLock);
+ }

ISTM the RowExclusiveLock on the toast table should be acquired before
the locks on the indexes.

+index_concurrent_set_dead(Oid indexId, Oid heapId, LOCKTAG locktag)
+{
+ Relation heapRelation;
+ Relation indexRelation;
+
+ /*
+ * Now we must wait until no running transaction could be using the
+ * index for a query. To do this, inquire which xacts currently would
+ * conflict with AccessExclusiveLock on the table -- ie, which ones
+ * have a lock of any kind on the table. Then wait for each of these
+ * xacts to commit or abort. Note we do not need to worry about xacts
+ * that open the table for reading after this point; they will see the
+ * index as invalid when they open the relation.
+ *
+ * Note: the reason we use actual lock acquisition here, rather than
+ * just checking the ProcArray and sleeping, is that deadlock is
+ * possible if one of the transactions in question is blocked trying
+ * to acquire an exclusive lock on our table. The lock code will
+ * detect deadlock and error out properly.
+ *
+ * Note: GetLockConflicts() never reports our own xid, hence we need
+ * not check for that. Also, prepared xacts are not reported, which
+ * is fine since they certainly aren't going to do anything more.
+ */
+ WaitForVirtualLocks(locktag, AccessExclusiveLock);

Most of that comment seems to belong to WaitForVirtualLocks instead of
this specific caller of WaitForVirtualLocks.

A comment in the header that it is doing the waiting would also be good.

In ReindexRelationsConcurrently I suggest s/bypassing/skipping/.

Btw, seing that we have an indisvalid check the toast table's index, do
we have any way to cleanup such a dead index? I don't think its allowed
to drop the index of a toast table. I.e. we possibly need to relax that
check for invalid indexes :/.

I think the usage of list_append_unique_oids in
ReindexRelationsConcurrently might get too expensive in larger
schemas. Its O(n^2) in the current usage and schemas with lots of
relations/indexes aren't unlikely candidates for this feature.
The easist solution probably is to use a hashtable.

ReindexRelationsConcurrently should do a CHECK_FOR_INTERRUPTS() every
once in a while, its currently not gracefully interruptible which
probably is bad in a bigger schema.

Thats all I have for now.

This patch is starting to look seriously cool and it seems realistic to
get into a ready state for 9.3.

I somewhat dislike the fact that CONCURRENTLY isn't really concurrent
here (for the listeners: swapping the indexes acquires exlusive locks) ,
but I don't see any other naming being better.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund escribió:

> I somewhat dislike the fact that CONCURRENTLY isn't really concurrent
> here (for the listeners: swapping the indexes acquires exlusive locks) ,
> but I don't see any other naming being better.

REINDEX ALMOST CONCURRENTLY?

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

On 24/01/13 07:45, Alvaro Herrera wrote:
> Andres Freund escribió:
>
>> I somewhat dislike the fact that CONCURRENTLY isn't really concurrent
>> here (for the listeners: swapping the indexes acquires exlusive locks) ,
>> but I don't see any other naming being better.
> REINDEX ALMOST CONCURRENTLY?
>

REINDEX BEST EFFORT CONCURRENTLY?

On Wed, Jan 23, 2013 at 1:45 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Andres Freund escribió:
>> I somewhat dislike the fact that CONCURRENTLY isn't really concurrent
>> here (for the listeners: swapping the indexes acquires exlusive locks) ,
>> but I don't see any other naming being better.
>
> REINDEX ALMOST CONCURRENTLY?

I'm kind of unconvinced of the value proposition of this patch. I
mean, you can DROP INDEX CONCURRENTLY and CREATE INDEX CONCURRENTLY
today, so ... how is this better?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jan 24, 2013 at 01:29:56PM -0500, Robert Haas wrote:
> On Wed, Jan 23, 2013 at 1:45 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
> > Andres Freund escribió:
> >> I somewhat dislike the fact that CONCURRENTLY isn't really concurrent
> >> here (for the listeners: swapping the indexes acquires exlusive locks) ,
> >> but I don't see any other naming being better.
> >
> > REINDEX ALMOST CONCURRENTLY?
>
> I'm kind of unconvinced of the value proposition of this patch. I
> mean, you can DROP INDEX CONCURRENTLY and CREATE INDEX CONCURRENTLY
> today, so ... how is this better?

This has been on the TODO list for a while, and I don't think the
renaming in a transaction work needed to use drop/create is really
something we want to force on users. In addition, doing that for all
tables in a database is even more work, so I would be disappointed _not_
to get this feature in 9.3.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> On Thu, Jan 24, 2013 at 01:29:56PM -0500, Robert Haas wrote:
>> I'm kind of unconvinced of the value proposition of this patch. I
>> mean, you can DROP INDEX CONCURRENTLY and CREATE INDEX CONCURRENTLY
>> today, so ... how is this better?

> This has been on the TODO list for a while, and I don't think the
> renaming in a transaction work needed to use drop/create is really
> something we want to force on users. In addition, doing that for all
> tables in a database is even more work, so I would be disappointed _not_
> to get this feature in 9.3.

I haven't given the current patch a look, but based on previous
discussions, this isn't going to be more than a macro for things that
users can do already --- that is, it's going to be basically DROP
CONCURRENTLY plus CREATE CONCURRENTLY plus ALTER INDEX RENAME, including
the fact that the RENAME step will transiently need an exclusive lock.
(If that's not what it's doing, it's broken.) So there's some
convenience argument for it, but it's hardly amounting to a stellar
improvement.

I'm kind of inclined to put it off till after we fix the SnapshotNow
race condition problems; at that point it should be possible to do
REINDEX CONCURRENTLY more simply and without any exclusive lock
anywhere.

regards, tom lane

On 2013-01-24 13:29:56 -0500, Robert Haas wrote:
> On Wed, Jan 23, 2013 at 1:45 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
> > Andres Freund escribió:
> >> I somewhat dislike the fact that CONCURRENTLY isn't really concurrent
> >> here (for the listeners: swapping the indexes acquires exlusive locks) ,
> >> but I don't see any other naming being better.
> >
> > REINDEX ALMOST CONCURRENTLY?
>
> I'm kind of unconvinced of the value proposition of this patch. I
> mean, you can DROP INDEX CONCURRENTLY and CREATE INDEX CONCURRENTLY
> today, so ... how is this better?

In the wake of beb850e1d873f8920a78b9b9ee27e9f87c95592f I wrote a script
to do this and it really is harder than one might think:
* you cannot do it in the database as CONCURRENTLY cannot be used in a
TX
* you cannot do it to toast tables (this is currently broken in the
patch but should be fixable)
* you cannot legally do it when foreign key reference your unique key
* you cannot do it to exclusion constraints or non-immediate indexes

All of those are fixable (and most are) within REINDEX CONCURRENLY, so I
find that to be a major feature even if its not as good as it could be.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

All the comments are addressed in version 8 attached, except for the
hashtable part, which requires some heavy changes.

On Thu, Jan 24, 2013 at 3:41 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2013-01-15 18:16:59 +0900, Michael Paquier wrote:
> > Code path used by REINDEX concurrently permits to
> > create an index in parallel of an existing one and not a completely new
> > index. Shouldn't this work for indexes used by exclusion indexes also?
>
> But that fact might safe things. I don't immediately see any reason that
> adding a
> if (!indisvalid)
> return;
> to check_exclusion_constraint wouldn't be sufficient if there's another
> index with an equivalent definition.
>
Indeed, this might be enough as for CREATE INDEX CONCURRENTLY this code
path cannot be taken and only indexes created concurrently can be invalid.
Hence I am adding that in the patch with a comment explaining why.

> > > > + /*
> > > > + * Phase 2 of REINDEX CONCURRENTLY
> > > > + *
> > > > + * Build concurrent indexes in a separate transaction for each
> > > index to
> > > > + * avoid having open transactions for an unnecessary long time.
> > > We also
> > > > + * need to wait until no running transactions could have the
> > > parent table
> > > > + * of index open. A concurrent build is done for each
> concurrent
> > > > + * index that will replace the old indexes.
> > > > + */
> > > > +
> > > > + /* Get the first element of concurrent index list */
> > > > + lc2 = list_head(concurrentIndexIds);
> > > > +
> > > > + foreach(lc, indexIds)
> > > > + {
> > > > + Relation indexRel;
> > > > + Oid indOid = lfirst_oid(lc);
> > > > + Oid concurrentOid =
> lfirst_oid(lc2);
> > > > + Oid relOid;
> > > > + bool primary;
> > > > + LOCKTAG *heapLockTag = NULL;
> > > > + ListCell *cell;
> > > > +
> > > > + /* Move to next concurrent item */
> > > > + lc2 = lnext(lc2);
> > > > +
> > > > + /* Start new transaction for this index concurrent
> build */
> > > > + StartTransactionCommand();
> > > > +
> > > > + /* Get the parent relation Oid */
> > > > + relOid = IndexGetRelation(indOid, false);
> > > > +
> > > > + /*
> > > > + * Find the locktag of parent table for this index, we
> > > need to wait for
> > > > + * locks on it.
> > > > + */
> > > > + foreach(cell, lockTags)
> > > > + {
> > > > + LOCKTAG *localTag = (LOCKTAG *) lfirst(cell);
> > > > + if (relOid == localTag->locktag_field2)
> > > > + heapLockTag = localTag;
> > > > + }
> > > > +
> > > > + Assert(heapLockTag && heapLockTag->locktag_field2 !=
> > > InvalidOid);
> > > > + WaitForVirtualLocks(*heapLockTag, ShareLock);
> > >
> > > Why do we have to do the WaitForVirtualLocks here? Shouldn't we do this
> > > once for all relations after each phase? Otherwise the waiting time
> will
> > > really start to hit when you do this on a somewhat busy server.
> > >
> > Each new index is built and set as ready in a separate single
> transaction,
> > so doesn't it make sense to wait for the parent relation each time. It is
> > possible to wait for a parent relation only once during this phase but in
> > this case all the indexes of the same relation need to be set as ready in
> > the same transaction. So here the choice is either to wait for the same
> > relation multiple times for a single index or wait once for a parent
> > relation but we build all the concurrent indexes within the same
> > transaction. Choice 1 makes the code clearer and more robust to my mind
> as
> > the phase 2 is done clearly for each index separately. Thoughts?
>
> As far as I understand that code its purpose is to enforce that all
> potential users have an up2date definition available. For that we
> acquire a lock on all virtualxids of users using that table thus waiting
> for them to finish.
> Consider the scenario where you have a workload where most transactions
> are fairly long (say 10min) and use the same tables (a,b)/indexes(a_1,
> a_2, b_1, b_2). With the current strategy you will do:
>
> WaitForVirtualLocks(a_1) -- wait up to 10min
> index_build(a_1)
> WaitForVirtualLocks(a_2) -- wait up to 10min
> index_build(a_2)
>
...
>
> So instead of waiting up 10 minutes for that phase you have to wait up
> to 40.
>
This is necessary if you want to process each index entry in a different
transaction as WaitForVirtualLocks needs to wait for the locks held on the
parent table. If you want to fo this wait once per transaction, the
solution would be to group the index builds in the same transaction for all
the indexes of the relation. One index per transaction looks more solid in
this case if there is a failure during a process only one index will be
incorrectly built. Also, when you run a REINDEX CONCURRENTLY, you should
not need to worry about the time it takes. The point is that this operation
is done in background and that the tables are still accessible during this
time.

> > > > + indexRel = index_open(indOid,
> ShareUpdateExclusiveLock);
> > >
> > > I wonder we should directly open it exlusive here given its going to
> > > opened exclusively in a bit anyway. Not that that will really reduce
> the
> > > deadlock likelihood since we already hold the ShareUpdateExclusiveLock
> > > in session mode ...
> > >
> > I tried to use an AccessExclusiveLock here but it happens that this is
> not
> > compatible with index_set_state_flags. Does taking an exclusive lock
> > increments the transaction ID of running transaction? Because what I am
> > seeing is that taking AccessExclusiveLock on this index does a
> transaction
> > update.
>
> Yep, it does when wal_level = hot_standby because it logs the exclusive
> lock to wal so the startup process on the standby can acquire it.
>
> Imo that Assert needs to be moved to the existing callsites if there
> isn't an equivalent one already.
>
OK. Letting the assertion inside index_set_state_flags makethes code more
consistent with CREATE INDEX CONCURRENTLY, so the existing behavior is fine.

>
> > For those reasons current code sticks with ShareUpdateExclusiveLock. Not
> a
> > big deal btw...
>
> Well, lock upgrades make deadlocks more likely.
>
> Ok, of to v7:
> + */
> +void
> +index_concurrent_swap(Oid newIndexOid, Oid oldIndexOid)
> ...
> + /*
> + * Take a lock on the old and new index before switching their
> names. This
> + * avoids having index swapping relying on relation renaming
> mechanism to
> + * get a lock on the relations involved.
> + */
> + oldIndexRel = relation_open(oldIndexOid, AccessExclusiveLock);
> + newIndexRel = relation_open(newIndexOid, AccessExclusiveLock);
> ..
> + /*
> + * If the index swapped is a toast index, take an exclusive lock
> on its
> + * parent toast relation and then update reltoastidxid to the
> new index Oid
> + * value.
> + */
> + if (get_rel_relkind(parentOid) == RELKIND_TOASTVALUE)
> + {
> + Relation pg_class;
> +
> + /* Open pg_class and fetch a writable copy of the relation
> tuple */
> + pg_class = heap_open(parentOid, RowExclusiveLock);
> +
> + /* Update the statistics of this pg_class entry with new
> toast index Oid */
> + index_update_stats(pg_class, false, false, newIndexOid,
> -1.0);
> +
> + /* Close parent relation */
> + heap_close(pg_class, RowExclusiveLock);
> + }
>
> ISTM the RowExclusiveLock on the toast table should be acquired before
> the locks on the indexes.
>
Done.

> +index_concurrent_set_dead(Oid indexId, Oid heapId, LOCKTAG locktag)
> +{
> + Relation heapRelation;
> + Relation indexRelation;
> +
> + /*
> + * Now we must wait until no running transaction could be using the
> + * index for a query. To do this, inquire which xacts currently
> would
> + * conflict with AccessExclusiveLock on the table -- ie, which ones
> + * have a lock of any kind on the table. Then wait for each of
> these
> + * xacts to commit or abort. Note we do not need to worry about
> xacts
> + * that open the table for reading after this point; they will see
> the
> + * index as invalid when they open the relation.
> + *
> + * Note: the reason we use actual lock acquisition here, rather
> than
> + * just checking the ProcArray and sleeping, is that deadlock is
> + * possible if one of the transactions in question is blocked
> trying
> + * to acquire an exclusive lock on our table. The lock code will
> + * detect deadlock and error out properly.
> + *
> + * Note: GetLockConflicts() never reports our own xid, hence we
> need
> + * not check for that. Also, prepared xacts are not reported,
> which
> + * is fine since they certainly aren't going to do anything more.
> + */
> + WaitForVirtualLocks(locktag, AccessExclusiveLock);
>
> Most of that comment seems to belong to WaitForVirtualLocks instead of
> this specific caller of WaitForVirtualLocks.
>
Done.

> A comment in the header that it is doing the waiting would also be good.
>
> In ReindexRelationsConcurrently I suggest s/bypassing/skipping/.
>
Done.

> Btw, seing that we have an indisvalid check the toast table's index, do
> we have any way to cleanup such a dead index? I don't think its allowed
> to drop the index of a toast table. I.e. we possibly need to relax that
> check for invalid indexes :/.
>
For the time being, no I don't think so, except by doing a manual cleanup
and remove the invalid pg_class entry in catalogs. One way to do thath
cleanly could be to have autovacuum remove the invalid toast indexes
automatically, but it is not dedicated to that and this is another
discussion.

> I think the usage of list_append_unique_oids in
> ReindexRelationsConcurrently might get too expensive in larger
> schemas. Its O(n^2) in the current usage and schemas with lots of
> relations/indexes aren't unlikely candidates for this feature.
> The easist solution probably is to use a hashtable.
>
Hum... This requires some thinking that will change the basics inside
ReindexRelationsConcurrently...
Let me play a bit with the hashtable APIs and I'll come back to that later.

ReindexRelationsConcurrently should do a CHECK_FOR_INTERRUPTS() every
> once in a while, its currently not gracefully interruptible which
> probably is bad in a bigger schema.
>
Done. I added some checks at each phase before beginning a new transaction.
--
Michael Paquier
http://michael.otacoo.com

On Thu, Jan 24, 2013 at 3:41 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> I think the usage of list_append_unique_oids in
> ReindexRelationsConcurrently might get too expensive in larger
> schemas. Its O(n^2) in the current usage and schemas with lots of
> relations/indexes aren't unlikely candidates for this feature.
> The easist solution probably is to use a hashtable.
>
I just had a look at the hashtable APIs and I do not think it is adapted to
establish the list of unique index OIDs that need to be built concurrently.
It would be of a better use in case of mapping the indexOids with something
else, like the concurrent Oids, but still even with that the code would be
more readable if let as is.
--
Michael Paquier
http://michael.otacoo.com

On 2013-01-25 14:11:39 +0900, Michael Paquier wrote:
> On Thu, Jan 24, 2013 at 3:41 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
>
> > I think the usage of list_append_unique_oids in
> > ReindexRelationsConcurrently might get too expensive in larger
> > schemas. Its O(n^2) in the current usage and schemas with lots of
> > relations/indexes aren't unlikely candidates for this feature.
> > The easist solution probably is to use a hashtable.
> >
> I just had a look at the hashtable APIs and I do not think it is adapted to
> establish the list of unique index OIDs that need to be built concurrently.
> It would be of a better use in case of mapping the indexOids with something
> else, like the concurrent Oids, but still even with that the code would be
> more readable if let as is.

It sure isn't optimal, but it should do the trick if you use the
hash_seq stuff to iterate the hash afterwards. And you could use it to
map to the respective locks et al.

If you prefer other ways to implement it I guess the other easy solution
is to add the values without preventing duplicates and then sort &
remove duplicates in the end. Probably ends up being slightly more code,
but I am not sure.

I don't think we can leave the quadratic part in there as-is.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-01-25 13:48:50 +0900, Michael Paquier wrote:
> All the comments are addressed in version 8 attached, except for the
> hashtable part, which requires some heavy changes.
>
> On Thu, Jan 24, 2013 at 3:41 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
>
> > On 2013-01-15 18:16:59 +0900, Michael Paquier wrote:
> > > Code path used by REINDEX concurrently permits to
> > > create an index in parallel of an existing one and not a completely new
> > > index. Shouldn't this work for indexes used by exclusion indexes also?
> >
> > But that fact might safe things. I don't immediately see any reason that
> > adding a
> > if (!indisvalid)
> > return;
> > to check_exclusion_constraint wouldn't be sufficient if there's another
> > index with an equivalent definition.
> >
> Indeed, this might be enough as for CREATE INDEX CONCURRENTLY this code
> path cannot be taken and only indexes created concurrently can be invalid.
> Hence I am adding that in the patch with a comment explaining why.

I don't really know anything about those mechanics, so some input from
somebody who does would be very much appreciated.

> > > > > + /*
> > > > > + * Phase 2 of REINDEX CONCURRENTLY
> > > > > + */
> > > > > +
> > > > > + /* Get the first element of concurrent index list */
> > > > > + lc2 = list_head(concurrentIndexIds);
> > > > > +
> > > > > + foreach(lc, indexIds)
> > > > > + {
> > > > > + WaitForVirtualLocks(*heapLockTag, ShareLock);
> > > >
> > > > Why do we have to do the WaitForVirtualLocks here? Shouldn't we do this
> > > > once for all relations after each phase? Otherwise the waiting time will
> > > > really start to hit when you do this on a somewhat busy server.
> > > >
> > > Each new index is built and set as ready in a separate single
> > transaction,
> > > so doesn't it make sense to wait for the parent relation each time. It is
> > > possible to wait for a parent relation only once during this phase but in
> > > this case all the indexes of the same relation need to be set as ready in
> > > the same transaction. So here the choice is either to wait for the same
> > > relation multiple times for a single index or wait once for a parent
> > > relation but we build all the concurrent indexes within the same
> > > transaction. Choice 1 makes the code clearer and more robust to my mind
> > as
> > > the phase 2 is done clearly for each index separately. Thoughts?
> >
> > As far as I understand that code its purpose is to enforce that all
> > potential users have an up2date definition available. For that we
> > acquire a lock on all virtualxids of users using that table thus waiting
> > for them to finish.
> > Consider the scenario where you have a workload where most transactions
> > are fairly long (say 10min) and use the same tables (a,b)/indexes(a_1,
> > a_2, b_1, b_2). With the current strategy you will do:
> >
> > WaitForVirtualLocks(a_1) -- wait up to 10min
> > index_build(a_1)
> > WaitForVirtualLocks(a_2) -- wait up to 10min
> > index_build(a_2)
> >
> ...
> >
> > So instead of waiting up 10 minutes for that phase you have to wait up
> > to 40.
> >
> This is necessary if you want to process each index entry in a different
> transaction as WaitForVirtualLocks needs to wait for the locks held on the
> parent table. If you want to fo this wait once per transaction, the
> solution would be to group the index builds in the same transaction for all
> the indexes of the relation. One index per transaction looks more solid in
> this case if there is a failure during a process only one index will be
> incorrectly built.

I cannot really follow you here. The reason why we need to wait here is
*only* to make sure that nobody still has the old list of indexes
around (which probably could even be relaxed for reindex concurrently,
but thats a separate optimization).
So if we wait for all relevant transactions to end before starting phase
2 proper, we are fine, independent of how many indexes we build in a
single transaction.

> Also, when you run a REINDEX CONCURRENTLY, you should
> not need to worry about the time it takes. The point is that this operation
> is done in background and that the tables are still accessible during this
> time.

I don't think that arguments holds that much water. Having open
transactions for too long *does* incur a rather noticeable overhead. And
you definitely do want such operations to finish as quickly as possible,
even if its just because you can go home only afterwards ;)

Really, imagine doing this too 100 indexes on a system where
transactions regularly take 30 minutes (only needs one at a time). Minus
the actual build-time thats very approx 4h against like half a month.

> > Btw, seing that we have an indisvalid check the toast table's index, do
> > we have any way to cleanup such a dead index? I don't think its allowed
> > to drop the index of a toast table. I.e. we possibly need to relax that
> > check for invalid indexes :/.
> >
> For the time being, no I don't think so, except by doing a manual cleanup
> and remove the invalid pg_class entry in catalogs. One way to do thath
> cleanly could be to have autovacuum remove the invalid toast indexes
> automatically, but it is not dedicated to that and this is another
> discussion.

Hm. Don't think thats acceptable :/

As I mentioned somewhere else, I don't see how to do an concurrent build
of the toast index at all, given there is exactly one index hardcoded in
tuptoaster.c so the second index won't get updated before the switch has
been made.

Haven't yet looked at the new patch - do you plan to provide an updated
version addressing some of the remaining issues soon? Don't want to
review this if you nearly have the next version available.

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Jan 27, 2013 at 1:37 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2013-01-25 14:11:39 +0900, Michael Paquier wrote:
>
> It sure isn't optimal, but it should do the trick if you use the
> hash_seq stuff to iterate the hash afterwards. And you could use it to
> map to the respective locks et al.
>
> If you prefer other ways to implement it I guess the other easy solution
> is to add the values without preventing duplicates and then sort &
> remove duplicates in the end. Probably ends up being slightly more code,
> but I am not sure.
>
Indeed, I began playing with the HTAB functions and it looks that the only
correct way to use that would be to use a hash table using as key the index
OID with as entry:
- the index OID itself
- the concurrent OID
And a second hash table with parent relation OID as key and as output the
LOCKTAG for each parent relation.

> I don't think we can leave the quadratic part in there as-is.
>
Sure, that is understandable.
--
Michael Paquier
http://michael.otacoo.com

On Sun, Jan 27, 2013 at 1:52 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2013-01-25 13:48:50 +0900, Michael Paquier wrote:
> > All the comments are addressed in version 8 attached, except for the
> > hashtable part, which requires some heavy changes.
> >
> > On Thu, Jan 24, 2013 at 3:41 AM, Andres Freund <andres(at)2ndquadrant(dot)com
> >wrote:
> >
> > > On 2013-01-15 18:16:59 +0900, Michael Paquier wrote:
> > > > Code path used by REINDEX concurrently permits to
> > > > create an index in parallel of an existing one and not a completely
> new
> > > > index. Shouldn't this work for indexes used by exclusion indexes
> also?
> > >
> > > But that fact might safe things. I don't immediately see any reason
> that
> > > adding a
> > > if (!indisvalid)
> > > return;
> > > to check_exclusion_constraint wouldn't be sufficient if there's another
> > > index with an equivalent definition.
> > >
> > Indeed, this might be enough as for CREATE INDEX CONCURRENTLY this code
> > path cannot be taken and only indexes created concurrently can be
> invalid.
> > Hence I am adding that in the patch with a comment explaining why.
>
> I don't really know anything about those mechanics, so some input from
> somebody who does would be very much appreciated.
>
> > > > > > + /*
> > > > > > + * Phase 2 of REINDEX CONCURRENTLY
> > > > > > + */
> > > > > > +
> > > > > > + /* Get the first element of concurrent index list */
> > > > > > + lc2 = list_head(concurrentIndexIds);
> > > > > > +
> > > > > > + foreach(lc, indexIds)
> > > > > > + {
> > > > > > + WaitForVirtualLocks(*heapLockTag, ShareLock);
> > > > >
> > > > > Why do we have to do the WaitForVirtualLocks here? Shouldn't we do
> this
> > > > > once for all relations after each phase? Otherwise the waiting
> time will
> > > > > really start to hit when you do this on a somewhat busy server.
> > > > >
> > > > Each new index is built and set as ready in a separate single
> > > transaction,
> > > > so doesn't it make sense to wait for the parent relation each time.
> It is
> > > > possible to wait for a parent relation only once during this phase
> but in
> > > > this case all the indexes of the same relation need to be set as
> ready in
> > > > the same transaction. So here the choice is either to wait for the
> same
> > > > relation multiple times for a single index or wait once for a parent
> > > > relation but we build all the concurrent indexes within the same
> > > > transaction. Choice 1 makes the code clearer and more robust to my
> mind
> > > as
> > > > the phase 2 is done clearly for each index separately. Thoughts?
> > >
> > > As far as I understand that code its purpose is to enforce that all
> > > potential users have an up2date definition available. For that we
> > > acquire a lock on all virtualxids of users using that table thus
> waiting
> > > for them to finish.
> > > Consider the scenario where you have a workload where most transactions
> > > are fairly long (say 10min) and use the same tables (a,b)/indexes(a_1,
> > > a_2, b_1, b_2). With the current strategy you will do:
> > >
> > > WaitForVirtualLocks(a_1) -- wait up to 10min
> > > index_build(a_1)
> > > WaitForVirtualLocks(a_2) -- wait up to 10min
> > > index_build(a_2)
> > >
> > ...
> > >
> > > So instead of waiting up 10 minutes for that phase you have to wait up
> > > to 40.
> > >
> > This is necessary if you want to process each index entry in a different
> > transaction as WaitForVirtualLocks needs to wait for the locks held on
> the
> > parent table. If you want to fo this wait once per transaction, the
> > solution would be to group the index builds in the same transaction for
> all
> > the indexes of the relation. One index per transaction looks more solid
> in
> > this case if there is a failure during a process only one index will be
> > incorrectly built.
>
> I cannot really follow you here.
>
OK let's be more explicit...

> The reason why we need to wait here is
> *only* to make sure that nobody still has the old list of indexes
> around (which probably could even be relaxed for reindex concurrently,
> but thats a separate optimization).
>
In order to do that, you need to wait for the *parent relations* and not
the index themselves, no?
Based on 2 facts:
- each index build is done in a single transaction
- a wait needs to be done on the parent relation before each transaction
You need to wait for the parent relation multiple times depending on the
number of indexes in it. You could optimize that by building all the
indexes of the *same parent relation* in a single transaction.

So, for example in the case of this table:
CREATE TABLE tab (col1 PRIMARY KEY, col2 int);
CREATE INDEX int ON tab (col2);
If the primary key index and the second index on col2 are built in a unique
transaction, you could wait only once for the locks on the parent relation
'tab' only once.

So if we wait for all relevant transactions to end before starting phase
> 2 proper, we are fine, independent of how many indexes we build in a
> single transaction.
>
The reason why all the index builds are done in a single transaction is
that you mentioned in a previous review (v3?) that we should do the builds
in a single transaction for *each* index. What looked fair based on the
fact that the transaction time for each index could be reduced, the
downside being that you wait more on the parent relation.

> > > Btw, seing that we have an indisvalid check the toast table's index, do
> > > we have any way to cleanup such a dead index? I don't think its allowed
> > > to drop the index of a toast table. I.e. we possibly need to relax that
> > > check for invalid indexes :/.
> > >
> > For the time being, no I don't think so, except by doing a manual cleanup
> > and remove the invalid pg_class entry in catalogs. One way to do thath
> > cleanly could be to have autovacuum remove the invalid toast indexes
> > automatically, but it is not dedicated to that and this is another
> > discussion.
>
> Hm. Don't think thats acceptable :/
>
> As I mentioned somewhere else, I don't see how to do an concurrent build
> of the toast index at all, given there is exactly one index hardcoded in
> tuptoaster.c so the second index won't get updated before the switch has
> been made.
>
> Haven't yet looked at the new patch - do you plan to provide an updated
> version addressing some of the remaining issues soon? Don't want to
> review this if you nearly have the next version available.
>
Before providing more effort in coding, I think it is better to be clear
about the strategy to use on the 2 following points:
1) At the index build phase, is it better to build each index in a single
separate transaction? Or group the builds in a transaction for each parent
table? This is solvable but the strategy should be clear.
2) Find a solution for invalid toast indexes, which is not that easy. One
solution could be to use an autovacuum process to clean up the invalid
indexes of toast tables automatically. Another solution is to skip the
reindex for toast indexes, making the feature less usable.

If a solution or an agreement is not found for those 2 points, I think it
will be fair to simply reject the patch.
It looks that this feature has still too many disadvantages compared to the
advantages it could bring in the current infrastructure (SnapshotNow
problems, what to do with invalid toast indexes, etc.), so I would tend to
agree with Tom and postpone this feature once infrastructure is more
mature, one of the main things being the non-MVCC'ed catalogs.
--
Michael Paquier
http://michael.otacoo.com

On 2013-01-27 07:54:43 +0900, Michael Paquier wrote:
> On Sun, Jan 27, 2013 at 1:52 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
> > On 2013-01-25 13:48:50 +0900, Michael Paquier wrote:
> > > > As far as I understand that code its purpose is to enforce that all
> > > > potential users have an up2date definition available. For that we
> > > > acquire a lock on all virtualxids of users using that table thus waiting
> > > > for them to finish.
> > > > Consider the scenario where you have a workload where most transactions
> > > > are fairly long (say 10min) and use the same tables (a,b)/indexes(a_1,
> > > > a_2, b_1, b_2). With the current strategy you will do:
> > > >
> > > > WaitForVirtualLocks(a_1) -- wait up to 10min
> > > > index_build(a_1)
> > > > WaitForVirtualLocks(a_2) -- wait up to 10min
> > > > index_build(a_2)
> > > >
> > > ...
> > > >
> > > > So instead of waiting up 10 minutes for that phase you have to wait up
> > > > to 40.
> > > >
> > > This is necessary if you want to process each index entry in a different
> > > transaction as WaitForVirtualLocks needs to wait for the locks held on the
> > > parent table. If you want to fo this wait once per transaction, the
> > > solution would be to group the index builds in the same transaction for all
> > > the indexes of the relation. One index per transaction looks more solid in
> > > this case if there is a failure during a process only one index will be
> > > incorrectly built.
> >
> > I cannot really follow you here.
> >
> OK let's be more explicit...

> > The reason why we need to wait here is
> > *only* to make sure that nobody still has the old list of indexes
> > around (which probably could even be relaxed for reindex concurrently,
> > but thats a separate optimization).
> >
> In order to do that, you need to wait for the *parent relations* and not
> the index themselves, no?
> Based on 2 facts:
> - each index build is done in a single transaction
> - a wait needs to be done on the parent relation before each transaction
> You need to wait for the parent relation multiple times depending on the
> number of indexes in it. You could optimize that by building all the
> indexes of the *same parent relation* in a single transaction.

I think youre misunderstanding how this part works a bit. We don't
acquire locks on the table itself, but we get a list of all transactions
we would conflict with if we were to acquire a lock of a certain
strength on the table (GetLockConflicts(locktag, mode)). We then wait
for each transaction in the resulting list via the VirtualXact mechanism
(VirtualXactLock(*lockholder)).
It doesn't matter all that waiting happens in the same transaction the
initial index build is done in as long as we keep the session locks
preventing other schema modifications. Nobody can go back and see an
older index list after we've done the above wait once.

So the following should be perfectly fine:

StartTransactionCommand();
BuildListOfIndexes();
foreach(index in indexes)
DefineNewIndex(index);
CommitTransactionCommand();

StartTransactionCommand();
foreach(table in tables)
GetLockConflicts()
foreach(conflict in conflicts)
VirtualXactLocks()
CommitTransactionCommand();

foreach(index in indexes)
StartTransactionCommand();
InitialIndexBuild(index)
CommitTransactionCommand();
...

> It looks that this feature has still too many disadvantages compared to the
> advantages it could bring in the current infrastructure (SnapshotNow
> problems, what to do with invalid toast indexes, etc.), so I would tend to
> agree with Tom and postpone this feature once infrastructure is more
> mature, one of the main things being the non-MVCC'ed catalogs.

I think while catalog mvcc snapshots would make this easier, most
problems, basically all but the switching of relations, are pretty much
independent from that fact. All the waiting etc, will still be there.

I can see an argument for pushing it to the next CF because its not
really there yet...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Jan 28, 2013 at 7:39 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2013-01-27 07:54:43 +0900, Michael Paquier wrote:
> I think you're misunderstanding how this part works a bit. We don't
> acquire locks on the table itself, but we get a list of all transactions
> we would conflict with if we were to acquire a lock of a certain
> strength on the table (GetLockConflicts(locktag, mode)). We then wait
> for each transaction in the resulting list via the VirtualXact mechanism
> (VirtualXactLock(*lockholder)).
> It doesn't matter all that waiting happens in the same transaction the
> initial index build is done in as long as we keep the session locks
> preventing other schema modifications. Nobody can go back and see an
> older index list after we've done the above wait once.
>
Don't worry I got it. I just thought that it was necessary to wait for the
locks taken on the parent relation by other backends just *before* building
the index. It seemed more stable.

So the following should be perfectly fine:
>
> StartTransactionCommand();
> BuildListOfIndexes();
> foreach(index in indexes)
> DefineNewIndex(index);
> CommitTransactionCommand();
>
> StartTransactionCommand();
> foreach(table in tables)
> GetLockConflicts()
> foreach(conflict in conflicts)
> VirtualXactLocks()
> CommitTransactionCommand();
>
> foreach(index in indexes)
> StartTransactionCommand();
> InitialIndexBuild(index)
> CommitTransactionCommand();
>
So you're point is simply to wait for all the locks currently taken on each
table in a different transaction only once and for all, independently from
the build and validation phases. Correct?

> It looks that this feature has still too many disadvantages compared to
> the
> > advantages it could bring in the current infrastructure (SnapshotNow
> > problems, what to do with invalid toast indexes, etc.), so I would tend
> to
> > agree with Tom and postpone this feature once infrastructure is more
> > mature, one of the main things being the non-MVCC'ed catalogs.
>
> I think while catalog mvcc snapshots would make this easier, most
> problems, basically all but the switching of relations, are pretty much
> independent from that fact. All the waiting etc, will still be there.
>
> I can see an argument for pushing it to the next CF because its not
> really there yet...
>
Even if we get this patch in a shape that you think is sufficient to make
it reviewable by a committer within a couple of days, there are still many
doubts from many people regarding this feature, so this is going to take
far more time to put it in a shape that would satisfy a vast majority. So
it is honestly wiser to work on that later.

Another argument that would be enough for a rejection of this patch by a
committer is the problem of invalid toast indexes that cannot be removed up
cleanly by an operator. As long as there is not a clean solution for that...
--
Michael Paquier
http://michael.otacoo.com

Hi,

On 2013-01-28 20:31:48 +0900, Michael Paquier wrote:
> On Mon, Jan 28, 2013 at 7:39 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
>
> > On 2013-01-27 07:54:43 +0900, Michael Paquier wrote:
> > I think you're misunderstanding how this part works a bit. We don't
> > acquire locks on the table itself, but we get a list of all transactions
> > we would conflict with if we were to acquire a lock of a certain
> > strength on the table (GetLockConflicts(locktag, mode)). We then wait
> > for each transaction in the resulting list via the VirtualXact mechanism
> > (VirtualXactLock(*lockholder)).
> > It doesn't matter all that waiting happens in the same transaction the
> > initial index build is done in as long as we keep the session locks
> > preventing other schema modifications. Nobody can go back and see an
> > older index list after we've done the above wait once.
> >
> Don't worry I got it. I just thought that it was necessary to wait for the
> locks taken on the parent relation by other backends just *before* building
> the index. It seemed more stable.

I don't see any need for that. Its really only about making sure their
relcache entry for the indexlist - and by extension rd_indexattr - in
all other transactions that could possibly write to the table is
up2date.

As a relation_open with a lock (which is done for every write) will
always drain the invalidations thats guaranteed if we wait that way.

> So the following should be perfectly fine:
> >
> > StartTransactionCommand();
> > BuildListOfIndexes();
> > foreach(index in indexes)
> > DefineNewIndex(index);
> > CommitTransactionCommand();
> >
> > StartTransactionCommand();
> > foreach(table in tables)
> > GetLockConflicts()
> > foreach(conflict in conflicts)
> > VirtualXactLocks()
> > CommitTransactionCommand();
> >
> > foreach(index in indexes)
> > StartTransactionCommand();
> > InitialIndexBuild(index)
> > CommitTransactionCommand();
> >
> So you're point is simply to wait for all the locks currently taken on each
> table in a different transaction only once and for all, independently from
> the build and validation phases. Correct?

Exactly. That will batch the wait for the transactions together and thus
will greatly decrease the overhead of doing a concurrent reindex
(wall, not cpu-clock wise).

> > > It looks that this feature has still too many disadvantages compared to the
> > > advantages it could bring in the current infrastructure (SnapshotNow
> > > problems, what to do with invalid toast indexes, etc.), so I would tend to
> > > agree with Tom and postpone this feature once infrastructure is more
> > > mature, one of the main things being the non-MVCC'ed catalogs.
> >
> > I think while catalog mvcc snapshots would make this easier, most
> > problems, basically all but the switching of relations, are pretty much
> > independent from that fact. All the waiting etc, will still be there.
> >
> > I can see an argument for pushing it to the next CF because its not
> > really there yet...
> >
> Even if we get this patch in a shape that you think is sufficient to make
> it reviewable by a committer within a couple of days, there are still many
> doubts from many people regarding this feature, so this is going to take
> far more time to put it in a shape that would satisfy a vast majority. So
> it is honestly wiser to work on that later.

I really haven't heard too many arguments from other after the initial
round.
Right now I "only" recall Tom and Robert doubting the usefulness, right?

I think most of the work in this patch is completely independent from
the snapshot stuff, so I really don't see much of an argument to make it
dependent on catalog snapshots.

> Another argument that would be enough for a rejection of this patch by a
> committer is the problem of invalid toast indexes that cannot be removed up
> cleanly by an operator. As long as there is not a clean solution for
> that...

I think that part is relatively easy to fix, I wouldn't worry too
much.
The more complex part is how to get tuptoaster.c to update the
concurrently created index. Thats what I worry about. Its not going
through the normal executor paths but manually updates the toast
index - which means it won't update the indisready && !indisvalid
index...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Jan 28, 2013 at 8:44 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> > Another argument that would be enough for a rejection of this patch by a
> > committer is the problem of invalid toast indexes that cannot be removed
> up
> > cleanly by an operator. As long as there is not a clean solution for
> > that...
>
> I think that part is relatively easy to fix, I wouldn't worry too
> much.
> The more complex part is how to get tuptoaster.c to update the
> concurrently created index. That's what I worry about. Its not going
> through the normal executor paths but manually updates the toast
> index - which means it won't update the indisready && !indisvalid
> index...
>
I included in the patch some stuff to update the reltoastidxid of the
parent relation of the toast index. Have a look at
index.c:index_concurrent_swap. The particular case I had in mind was if
there is a failure of the server during the concurrent reindex of a toast
index. When server restarts, the toast relation will have an invalid index
and this cannot be dropped by an operator via SQL.
--
Michael Paquier
http://michael.otacoo.com