pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedRe[2]: Alter index rename concurrently to
7+ messages / 1 participants
[nested] [flat]
Related threads (commitfest entry Allow to rename index in concurrent mode/1726):
Alter index rename concurrently to
* Re[2]: Alter index rename concurrently to
@ 2018-07-17 11:48 ` Andrey Klychkov <aaklychkov@mail.ru>
2018-07-18 09:44 ` Re[2]: Alter index rename concurrently to Andrey Klychkov <aaklychkov@mail.ru>
2018-07-23 13:14 ` Re[2]: Alter index rename concurrently to Andrey Klychkov <aaklychkov@mail.ru>
2 siblings, 2 replies; 7+ messages in thread
From: Andrey Klychkov @ 2018-07-17 11:48 UTC (permalink / raw)
To: Victor Yegorov <vyegorov@gmail.com>; +Cc: pgsql-hackers
>Понедельник, 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 https://www.postgresql.org/message-id/CAB7nPqTys6JUQDxUczbJb0BNW0kPrW8WdZuk11KaxQq6o98PJg@mail.gmail...
>- 2013 https://www.postgresql.org/message-id/CAB7nPqSTFkuc7dZxCDX4HOTU63YXHRroNv2aoUzyD-Zz_8Z_Zg@mail.gmail...
>- 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re[2]: Alter index rename concurrently to
2018-07-17 11:48 ` Re[2]: Alter index rename concurrently to Andrey Klychkov <aaklychkov@mail.ru>
@ 2018-07-18 09:44 ` Andrey Klychkov <aaklychkov@mail.ru>
1 sibling, 0 replies; 7+ messages in thread
From: Andrey Klychkov @ 2018-07-18 09:44 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: pgsql-hackers; Victor Yegorov <vyegorov@gmail.com>
>Среда, 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re[2]: Alter index rename concurrently to
2018-07-17 11:48 ` Re[2]: Alter index rename concurrently to Andrey Klychkov <aaklychkov@mail.ru>
@ 2018-07-23 13:14 ` Andrey Klychkov <aaklychkov@mail.ru>
1 sibling, 0 replies; 7+ messages in thread
From: Andrey Klychkov @ 2018-07-23 13:14 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: pgsql-hackers; Victor Yegorov <vyegorov@gmail.com>
>Среда, 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re[2]: Alter index rename concurrently to
@ 2018-07-17 12:36 ` Andrey Klychkov <aaklychkov@mail.ru>
2 siblings, 0 replies; 7+ messages in thread
From: Andrey Klychkov @ 2018-07-17 12:36 UTC (permalink / raw)
To: Andrey Borodin <x4mmm@yandex-team.ru>; +Cc: pgsql-hackers
> Понедельник, 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re[2]: Alter index rename concurrently to
@ 2018-10-03 11:51 ` Andrey Klychkov <aaklychkov@mail.ru>
2018-10-08 07:48 ` Re[2]: Alter index rename concurrently to Andrey Klychkov <aaklychkov@mail.ru>
2 siblings, 1 reply; 7+ messages in thread
From: Andrey Klychkov @ 2018-10-03 11:51 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Victor Yegorov <vyegorov@gmail.com>; pgsql-hackers
> Based on these assertions, here is my proposed patch. It lowers the
> lock level for index renaming to ShareUpdateExclusiveLock.
Hi, Peter
I made small review for your patch:
Server source code got from https://github.com/postgres/postgres.git
1. Patch was applied without any errors except a part related to documentation:
error: patch failed: doc/src/sgml/ref/alter_index.sgml:50
error: doc/src/sgml/ref/alter_index.sgml: patch does not apply
2. The code has been compiled successfully, configured by:
# ./configure CFLAGS="-O0" --enable-debug --enable-cassert --enable-depend --without-zlib
3. 'make' / 'make install' successfully made and complete.
4. The compiled instance has been started without any errors.
5. I realized several tests by the pgbench (with -c 4 -j 4 -T 360) that modified data into columns, indexed by pk and common btree.
In the same time there was a running script that was making renaming indexes multiple times in transactions with pg_sleep(1).
After several tests no errors were found.
6. pg_upgrade from 10 to 12 (patched) has been done without any errors / warnings
7. Code style:
+RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal, bool is_index)
This line is longer than 80 chars.
Thank you
>Вторник, 14 августа 2018, 9:33 +03:00 от Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:
>
>On 31/07/2018 23:10, Peter Eisentraut wrote:
>> 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.
>
>Based on these assertions, here is my proposed patch. It lowers the
>lock level for index renaming to ShareUpdateExclusiveLock.
>
>--
>Peter Eisentraut http://www.2ndQuadrant.com/
>PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Regards,
Andrey Klychkov
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re[2]: Alter index rename concurrently to
2018-10-03 11:51 ` Re[2]: Alter index rename concurrently to Andrey Klychkov <aaklychkov@mail.ru>
@ 2018-10-08 07:48 ` Andrey Klychkov <aaklychkov@mail.ru>
0 siblings, 0 replies; 7+ messages in thread
From: Andrey Klychkov @ 2018-10-08 07:48 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Victor Yegorov <vyegorov@gmail.com>; pgsql-hackers
> Attached is an updated patch.
That's OK now, the patch applying is without any errors.
I have no more remarks.
>Пятница, 5 октября 2018, 13:04 +03:00 от Peter Eisentraut <peter.eisentraut@2ndquadrant.com>:
>
>On 03/10/2018 13:51, Andrey Klychkov wrote:
>> 1. Patch was applied without any errors except a part related to
>> documentation:
>> error: patch failed: doc/src/sgml/ref/alter_index.sgml:50
>> error: doc/src/sgml/ref/alter_index.sgml: patch does not apply
>
>Attached is an updated patch.
>
>> 2. The code has been compiled successfully, configured by:
>> # ./configure CFLAGS="-O0" --enable-debug --enable-cassert
>> --enable-depend --without-zlib
>
>Not sure why you use -O0 here. It's not a good idea for development,
>because it might miss interesting warnings.
>
>> 7. Code style:
>> +RenameRelationInternal(Oid myrelid, const char *newrelname, bool
>> is_internal, bool is_index)
>> This line is longer than 80 chars.
>
>pgindent leaves this line alone.
>
>--
>Peter Eisentraut http://www.2ndQuadrant.com/
>PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Regards,
Andrey Klychkov
^ permalink raw reply [nested|flat] 7+ messages in thread
* Fwd: Re[2]: Alter index rename concurrently to
@ 2018-07-25 11:46 Andrey Klychkov <aaklychkov@mail.ru>
0 siblings, 0 replies; 7+ messages in thread
From: Andrey Klychkov @ 2018-07-25 11:46 UTC (permalink / raw)
To: pgsql-hackers
Понедельник, 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
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2018-10-08 07:48 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-07-17 11:48 ` Andrey Klychkov <aaklychkov@mail.ru>
2018-07-18 09:44 ` Andrey Klychkov <aaklychkov@mail.ru>
2018-07-23 13:14 ` Andrey Klychkov <aaklychkov@mail.ru>
2018-07-17 12:36 ` Andrey Klychkov <aaklychkov@mail.ru>
2018-10-03 11:51 ` Andrey Klychkov <aaklychkov@mail.ru>
2018-10-08 07:48 ` Andrey Klychkov <aaklychkov@mail.ru>
2018-07-25 11:46 Fwd: Re[2]: Alter index rename concurrently to Andrey Klychkov <aaklychkov@mail.ru>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox