On Thu, Sep 26, 2013 at 8:43 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-09-26 20:40:40 +0900, Michael Paquier wrote:
>> On Thu, Sep 26, 2013 at 7:34 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > On 2013-09-26 12:13:30 +0900, Michael Paquier wrote:
>> >> > 2) I don't think the drop algorithm used now is correct. Your
>> >> > index_concurrent_set_dead() sets both indisvalid = false and indislive =
>> >> > false at the same time. It does so after doing a WaitForVirtualLocks() -
>> >> > but that's not sufficient. Between waiting and setting indisvalid =
>> >> > false another transaction could start which then would start using that
>> >> > index. Which will not get updated anymore by other concurrent backends
>> >> > because of inislive = false.
>> >> > You really need to follow index_drop's lead here and first unset
>> >> > indisvalid then wait till nobody can use the index for querying anymore
>> >> > and only then unset indislive.
>> >
>> >> Sorry, I do not follow you here. index_concurrent_set_dead calls
>> >> index_set_state_flags that sets indislive and *indisready* to false,
>> >> not indisvalid. The concurrent index never uses indisvalid = true so
>> >> it can never be called by another backend for a read query. The drop
>> >> algorithm is made to be consistent with DROP INDEX CONCURRENTLY btw.
>> >
>> > That makes it even worse... You can do the concurrent drop only in the
>> > following steps:
>> > 1) set indisvalid = false, no future relcache lookups will have it as valid
>
>> indisvalid is never set to true for the concurrent index. Swap is done
>> with concurrent index having indisvalid = false and former index with
>> indisvalid = true. The concurrent index is validated with
>> index_validate in a transaction before swap transaction.
>
> Yes. I've described how it *has* to be done, not how it's done.
>
> The current method of going straight to isready = false for the original
> index will result in wrong results because it's not updated anymore
> while it's still being used.
The index being dropped at the end of process is not the former index,
but the concurrent index. The index used after REINDEX CONCURRENTLY is
the old index but with the new relfilenode.
Am I lacking of caffeine? It looks so...
--
Michael