Alter index rename concurrently to

Hi!

16 июля 2018 г., в 22:58, Andrey Klychkov <aaklychkov@mail.ru> написал(а):
Dear hackers!

I have an idea to facilitate work with index rebuilding.
....
"ALTER INDEX ... RENAME CONCURRENTLY TO ..."

The idea seems useful. I'm not an expert in CIC, but your post do not cover one very important topic. When we use CREATE INDEX CONCURRENTLY we pay for less intrusive lock by scanning data twice. What is the price of RENAME CONCURRENTLY? Should this mode be default?

Best regards, Andrey Borodin.

пн, 16 июл. 2018 г. в 21:58, Andrey Klychkov <aaklychkov@mail.ru>:

I made a patch to solve this issue (see the attachment).
It allows to avoid locks by a query like this:
“alter index <new_index> rename CONCURRENTLY to <old_index_name>”.

Please, have a look at previous discussions on the subject:
- 2012
/messages/by-id/CAB7nPqTys6JUQDxUczbJb0BNW0kPrW8WdZuk11KaxQq6o98PJg@mail.gmail.com
- 2013
/messages/by-id/CAB7nPqSTFkuc7dZxCDX4HOTU63YXHRroNv2aoUzyD-Zz_8Z_Zg@mail.gmail.com
- https://commitfest.postgresql.org/16/1276/

--
Victor Yegorov

Понедельник, 16 июля 2018, 22:40 +03:00 от Victor Yegorov <vyegorov@gmail.com>:

пн, 16 июл. 2018 г. в 21:58, Andrey Klychkov < aaklychkov@mail.ru >:

I made a patch to solve this issue (see the attachment).
It allows to avoid locks by a query like this:
“alter index <new_index> rename CONCURRENTLY to <old_index_name>”.

Please, have a look at previous discussions on the subject:
- 2012 /messages/by-id/CAB7nPqTys6JUQDxUczbJb0BNW0kPrW8WdZuk11KaxQq6o98PJg@mail.gmail.com
- 2013  /messages/by-id/CAB7nPqSTFkuc7dZxCDX4HOTU63YXHRroNv2aoUzyD-Zz_8Z_Zg@mail.gmail.com
-  https://commitfest.postgresql.org/16/1276/

Hello,
Thank you for this information!

In the first discussion the concurrent alter was mentioned.
In the next link and commitfest info I only saw "Reindex concurrently 2.0".
It sounds great!
If this component will be added to core it certainly facilitates index rebuilding.

What about "alter index ... rename to" in the concurrent mode?
Does "Reindex concurrently 2.0" add it?
From time to time we need just to rename some indexes.
Without concurrent mode this "alter index" makes queues.
It may be a problem on high load databases.

--
Kind regards,
Andrey Klychkov

Понедельник, 16 июля 2018, 22:19 +03:00 от Andrey Borodin <x4mmm@yandex-team.ru>:

Hi!

16 июля 2018 г., в 22:58, Andrey Klychkov < aaklychkov@mail.ru > написал(а):
Dear hackers!

I have an idea to facilitate work with index rebuilding.
....
"ALTER INDEX ... RENAME CONCURRENTLY TO ..."

The idea seems useful. I'm not an expert in CIC, but your post do not cover one very important topic. When we use CREATE INDEX CONCURRENTLY we pay for less intrusive lock by scanning data twice. What is the price of RENAME CONCURRENTLY? Should this mode be default?

Hello and thank you for the answer!

I don't think "alter index ... rename concurrently to ..." has large overheads
cause it just locks table and copies a new name instead of an old name
to the field relform->relname of the FormData_pg_class struct.

./src/include/catalog/pg_class.h: typedef of FormData_pg_class
./src/backend/commands/tablecmds.c: RenameRelation() and RenameRelationInternal()

As I wrote before, in my patch I've added the opportunity to change a locking
AccessShareLock -> ShareUpdateExclusiveLock
by passing "concurrently" in "alter", it's similar to the way of index_drop()/index_create()
functions.

It changes only one name field and nothing more.
I believe it is safe.
Also I ran tests again: select/insert queries in loops in several sessions and changed
an index name concurrently in parallel many times.
After that, index was valid and its index_scan was increased.

However, I don't have much experience and maybe I am wrong.

--
Kind regards,
Andrey Klychkov

On 17.07.18 13:48, Andrey Klychkov wrote:

Please, have a look at previous discussions on the subject:
- 2012
/messages/by-id/CAB7nPqTys6JUQDxUczbJb0BNW0kPrW8WdZuk11KaxQq6o98PJg@mail.gmail.com
-
2013 /messages/by-id/CAB7nPqSTFkuc7dZxCDX4HOTU63YXHRroNv2aoUzyD-Zz_8Z_Zg@mail.gmail.com
- https://commitfest.postgresql.org/16/1276/

[the above are discussions on REINDEX CONCURRENTLY]

In the first discussion the concurrent alter was mentioned.
In the next link and commitfest info I only saw "Reindex concurrently 2.0".
It sounds great!
If this component will be added to core it certainly facilitates index
rebuilding.

What about "alter index ... rename to" in the concurrent mode?
Does "Reindex concurrently 2.0" add it?

The latest posted REINDEX CONCURRENTLY patch implements swapping index
dependencies and name with a ShareUpdateExclusiveLock, which is
effectively what your patch is doing also, for renaming only.

In the 2012 thread, it was mentioned several times that some thought
that renaming without an exclusive lock was unsafe, but without any
further reasons. I think that was before MVCC catalog snapshots were
implemented, so at that time, any catalog change without an exclusive
lock would have been risky. After that was changed, the issue hasn't
been mentioned again in the 2013 and beyond thread, and it seems that
everyone assumed that it was OK.

It we think that it is safe, then I think we should just lower the lock
level of RENAME by default.

In your patch, the effect of the CONCURRENTLY keyword is just to change
the lock level, without any further changes. That doesn't make much
sense. If we think the lower lock level is OK, then we should just use
it always.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Среда, 18 июля 2018, 12:21 +03:00 от Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:

If we think the lower lock level is OK, then we should just use
it always.

Hi, I absolutely agree with you.
If lower locking is safe and possible to be used by default in renaming it will be great.
What stage is solving of this issue? Does anybody do it?

Thank you!

--
Kind regards,
Andrey Klychkov

On 18.07.18 11:44, Andrey Klychkov wrote:

If lower locking is safe and possible to be used by default in renaming
it will be great.
What stage is solving of this issue? Does anybody do it?

We wait to see if anyone has any concerns about this change.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Среда, 18 июля 2018, 12:21 +03:00 от Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:

In your patch, the effect of the CONCURRENTLY keyword is just to change
the lock level, without any further changes. That doesn't make much
sense. If we think the lower lock level is OK, then we should just use
it always.

I was thinking about it and I seem that AccessShareExclusiveLock by default is a better way than
the lower lock level because it corresponds to the principle of more strict implementation
of changing database schema (by default).
I think if some users need to rename a relation without query locking they should
say it definitely by using "concurrently" keyword like in the drop/create index cases.

Otherwise, to set the lower lock level we must also embody new keyword for "alter.. rename" to allow user
to set up stronger lock level for this action (not only indexes but tables and so on).

Moreover, if you rename Table without query locking, it may crushes your services that
do queries at the same time, therefore , this is unlikely that someone will be do it
with concurrent queries to renamed table, in other words, with running production.
So, I think it doesn't have real sense to use the lower lock for example for tables (at least by default).
However, renaming Indexes with the lower lock is safe for database consumers
because they don't know anything about them.

As I wrote early, in the patch I added the 'concurrent' field in the RenameStmt structure.
However, the "concurrently" keyword is realized there for index renaming only
because I only see need to rename indexes in concurrent mode.
It also may be implemented for tables, etc if it will be really needed for someone in the future.

--
Kind regards,
Andrey Klychkov

On 23.07.18 15:14, Andrey Klychkov wrote:

Moreover, if you rename Table without query locking, it may crushes your
services that
do queries at the same time, therefore, this is unlikely that someone
will be do it
with concurrent queries to renamed table, in other words, with running
production.
So, I think it doesn't have real sense to use the lower lock for example
for tables (at least by default).
However, renaming Indexes with the lower lock is safe for database consumers
because they don't know anything about them.

You appear to be saying that you think that renaming an index
concurrently is not safe. In that case, this patch should be rejected.
However, I don't think it necessarily is unsafe. What we need is some
reasoning about the impact, not a bunch of different options that we
don't understand.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Понедельник, 23 июля 2018, 18:06 +03:00 от Peter Eisentraut < peter.eisentraut@2ndquadrant.com >:

You appear to be saying that you think that renaming an index
concurrently is not safe

No, I didn't say it about renaming indexes.
I tried to say that it does not make sense exactly to rename a table (view, functions) concurrently
(and implement the lower lock level respectively) because probably no one will do this on production.
Because names of that objects are used in application's configuration.
I don't remember any cases I need to do this for tables but for indexes very often.
Renaming indexes is safe for client's applications because their names are not used in texts of SQL queries.
I think it's safe for relations too because, as we know, the database server works with indexes by using identifiers, not names.
Moreover, I tested it.

Ok, Peter, thanks for your answer!

--
Kind regards,
Andrey Klychkov

----------------------------------------------------------------------

--
Kind regards,
Andrey Klychkov

You appear to be saying that you think that renaming an index
concurrently is not safe. In that case, this patch should be rejected.
However, I don't think it necessarily is unsafe. What we need is some
reasoning about the impact, not a bunch of different options that we
don't understand.

I've had this same need, and dreamed this same solution before. I also
thought about a syntax like ALTER INDEX foo RENAME TO
WHATEVER-IT-WOULD-HAVE-BEEN-NAMED-BY-DEFAULT to aid this situation.

But all of those needs fade if we have REINDEX CONCURRENTLY. I think that's
where we should focus our efforts.

A possible side effort into something like a VACUUM FULL CONCURRENTLY,
which would essentially do what pg_repack does, but keeping the same oid
and the stats that go with it, but even that's a nice-to-have add-on to
REINDEX CONCURRENTLY.

On Wed, Jul 18, 2018 at 5:20 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

In the 2012 thread, it was mentioned several times that some thought
that renaming without an exclusive lock was unsafe, but without any
further reasons. I think that was before MVCC catalog snapshots were
implemented, so at that time, any catalog change without an exclusive
lock would have been risky. After that was changed, the issue hasn't
been mentioned again in the 2013 and beyond thread, and it seems that
everyone assumed that it was OK.

It we think that it is safe, then I think we should just lower the lock
level of RENAME by default.

In your patch, the effect of the CONCURRENTLY keyword is just to change
the lock level, without any further changes. That doesn't make much
sense. If we think the lower lock level is OK, then we should just use
it always.

Right. I think that, in the world of MVCC catalog scans, there are
basically two main concerns around reducing the lock levels for DDL.
The first is the way that the shared invalidation queue works. If
people reading a certain piece of information take a lock X, and
people changing it take a lock Y which conflicts with X, then the
shared invalidation machinery guarantees that everyone always sees the
latest version. If not, some backends may continue to use the old
version for an unbounded period of time -- there doesn't seem to be a
guarantee that AcceptInvalidationMessages() even runs once per
command, if say the transaction already holds all the locks the query
needs. Moreover, there is no predictability to when the new
information may become visible -- it may happen just by chance that
the changes become visible almost at once. I think it would be
worthwhile for somebody to consider adjusting or maybe even
significantly rewriting the invalidation code to provide some
less-unpredictable behavior in this area. It would be much more
palatable to make non-critical changes under lesser lock levels if you
could describe in an understandable way when those changes would take
effect. If you could say something like "normally within a second" or
"no later than the start of the next query" it would be a lot better.

The second problem is the relation cache. Lots of things cache
pointers to the RelationData structure or its subsidiary structures,
and if any of that gets rebuild, the pointer addresses may change,
leaving those pointers pointing to garbage. This is handled in
different ways in different places. One approach is -- if we do a
rebuild of something, say the partition descriptor, and find that the
new one is identical to the old one, then free the new one and keep
the old one. This means that it's safe to rely on the pointer not
changing underneath you provided you hold a lock strong enough to
prevent any DDL that might change the partition descriptor. But note
that something like this is essential for any concurrent DDL to work
at all. Without this, concurrent DDL that changes some completely
unrelated property of the table (e.g. the reloptions) would cause a
relcache rebuild that would invalidate cached pointers to the
partition descriptor, leading to crashes.

With respect to this particular patch, I don't know whether there are
any hazards of the second type. What I'd check, if it were me, is
what structures in the index's relcache entry are going to get rebuilt
when the index name changes. If any of those look like things that
something that somebody could hold a pointer to during the course of
query execution, or more generally be relying on not to change during
the course of query execution, then you've got a problem. As to the
first category, I suspect it's possible to construct cases where the
fact that the index's name hasn't change fails to get noticed for an
alarmingly long period of time after the change has happened. I also
suspect that an appropriate fix might be to ensure that
AcceptInvalidationMessages() is run at least once at the beginning of
parse analysis. I don't think that will make this kind of thing
race-free, but if you can't demonstrate a case where the new name
fails to be noticed immediately without resorting to setting
breakpoints with a debugger, it's probably good enough. We might need
to worry about whether the extra call to AcceptInvalidationMessages()
costs enough to be noticeable, but I hope it doesn't.

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

On 27/07/2018 16:16, Robert Haas wrote:

With respect to this particular patch, I don't know whether there are
any hazards of the second type. What I'd check, if it were me, is
what structures in the index's relcache entry are going to get rebuilt
when the index name changes. If any of those look like things that
something that somebody could hold a pointer to during the course of
query execution, or more generally be relying on not to change during
the course of query execution, then you've got a problem.

I have investigated this, and I think it's safe. Relcache reloads for
open indexes are already handled specially in RelationReloadIndexInfo().
The only pointers that get replaced there are rd_amcache and
rd_options. I have checked where those are used, and it looks like they
are not used across possible relcache reloads. The code structure in
those two cases make this pretty unlikely even in the future. Also,
violations would probably be caught by CLOBBER_CACHE_ALWAYS.

As to the
first category, I suspect it's possible to construct cases where the
fact that the index's name hasn't change fails to get noticed for an
alarmingly long period of time after the change has happened. I also
suspect that an appropriate fix might be to ensure that
AcceptInvalidationMessages() is run at least once at the beginning of
parse analysis.

Why don't we just do that? I have run some performance tests and there
was no impact (when no invalidation events are generated). The code
path in the case where there are no events queued up looks pretty
harmless, certainly compared to all the other stuff that goes on per
command.

Then again, I don't know why you consider this such a big problem in the
first place. If a concurrent transaction doesn't get the new index name
until the next transaction, what's the problem?

Then again, I don't know why we don't just call
AcceptInvalidationMessages() before each command or at least before each
top-level command? That would make way too much sense.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 27/07/2018 16:16, Robert Haas wrote:

I also suspect that an appropriate fix might be to ensure that
AcceptInvalidationMessages() is run at least once at the beginning of
parse analysis.

Why don't we just do that?

Don't we do that already? Certainly it should get run in advance of
any relation name lookup. There is one at transaction start also,
if memory serves.

regards, tom lane

On 31/07/2018 23:25, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 27/07/2018 16:16, Robert Haas wrote:

I also suspect that an appropriate fix might be to ensure that
AcceptInvalidationMessages() is run at least once at the beginning of
parse analysis.

Why don't we just do that?

Don't we do that already? Certainly it should get run in advance of
any relation name lookup. There is one at transaction start also,
if memory serves.

Right, we do it at transaction start and when opening a relation with a
lock that you don't already have. Which I suppose in practice is almost
equivalent to at least once per command, but you can construct cases
where subsequent commands in a transaction use the all same tables as
the previous commands, in which case they don't run AIM() again.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Aug 1, 2018 at 3:04 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 31/07/2018 23:25, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 27/07/2018 16:16, Robert Haas wrote:

I also suspect that an appropriate fix might be to ensure that
AcceptInvalidationMessages() is run at least once at the beginning of
parse analysis.

Why don't we just do that?

Don't we do that already? Certainly it should get run in advance of
any relation name lookup. There is one at transaction start also,
if memory serves.

Right, we do it at transaction start and when opening a relation with a
lock that you don't already have. Which I suppose in practice is almost
equivalent to at least once per command, but you can construct cases
where subsequent commands in a transaction use the all same tables as
the previous commands, in which case they don't run AIM() again.

Right. If nobody sees a reason not to change that, I think we should.
It would make the behavior more predictable with, I hope, no real
loss.

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

On 2018-08-01 15:33:09 -0400, Robert Haas wrote:

On Wed, Aug 1, 2018 at 3:04 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 31/07/2018 23:25, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 27/07/2018 16:16, Robert Haas wrote:

I also suspect that an appropriate fix might be to ensure that
AcceptInvalidationMessages() is run at least once at the beginning of
parse analysis.

Why don't we just do that?

Don't we do that already? Certainly it should get run in advance of
any relation name lookup. There is one at transaction start also,
if memory serves.

Right, we do it at transaction start and when opening a relation with a
lock that you don't already have. Which I suppose in practice is almost
equivalent to at least once per command, but you can construct cases
where subsequent commands in a transaction use the all same tables as
the previous commands, in which case they don't run AIM() again.

Right. If nobody sees a reason not to change that, I think we should.
It would make the behavior more predictable with, I hope, no real
loss.

What precisely are you proposing?

Greetings,

Andres Freund

On Wed, Aug 1, 2018 at 3:36 PM, Andres Freund <andres@anarazel.de> wrote:

Right. If nobody sees a reason not to change that, I think we should.
It would make the behavior more predictable with, I hope, no real
loss.

What precisely are you proposing?

Inserting AcceptInvalidationMessages() in some location that
guarantees it will be executed at least once per SQL statement. I
tentatively propose the beginning of parse_analyze(), but I am open to
suggestions.

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

Hi,

On 2018-08-02 15:57:13 -0400, Robert Haas wrote:

On Wed, Aug 1, 2018 at 3:36 PM, Andres Freund <andres@anarazel.de> wrote:

Right. If nobody sees a reason not to change that, I think we should.
It would make the behavior more predictable with, I hope, no real
loss.

What precisely are you proposing?

Inserting AcceptInvalidationMessages() in some location that
guarantees it will be executed at least once per SQL statement. I
tentatively propose the beginning of parse_analyze(), but I am open to
suggestions.

I'm inclined to think that that doesn't really actually solve anything,
but makes locking issues harder to find, because the window is smaller,
but decidedly non-zero. Can you describe why this'd make things more
"predictable" precisely?

Greetings,

Andres Freund