Re: Alter index rename concurrently to
On 25/10/2018 19:35, Fabrízio de Royes Mello wrote: >> > OK, I can refine those descriptions/comments. Do you have any concerns >> > about the underlying principle of this patch? >> >> Patch with updated comments to reflect your input. > I didn't found any issue, so the patch looks in a very good shape. Committed, thanks all. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Alter index rename concurrently to
On Thu, Oct 25, 2018 at 4:41 AM Peter Eisentraut < peter.eisentr...@2ndquadrant.com> wrote: > > On 17/10/2018 23:11, Peter Eisentraut wrote: > > On 13/10/2018 04:01, Andres Freund wrote: > >> I don't see how this could be argued. It has to be a self-conflicting > >> lockmode, otherwise you'd end up doing renames of tables where you > >> cannot see the previous state. And you'd get weird errors about updating > >> invisible rows etc. > > > >> I don't buy this description. Imo it's a fundamental correctness > >> thing. Without it concurrent DDL would potentially overwrite the rename, > >> because they could start updating while still seeing the old version. > > > > OK, I can refine those descriptions/comments. Do you have any concerns > > about the underlying principle of this patch? > > Patch with updated comments to reflect your input. > Great... this patch will help a lot so I took the liberty to perform some review: - the doc and code (simple) looks good - the patch apply cleanly against current master - all tests (check and check-world) past without any issue I also perform some test using DDLs and DMLs in various sessions: - renaming indexes / dml against same table - creating and renaming indexes / altering tables in other session - renaming indexes / dropping indexes I didn't found any issue, so the patch looks in a very good shape. Regards, -- Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Re: Alter index rename concurrently to
On 17/10/2018 23:11, Peter Eisentraut wrote: > On 13/10/2018 04:01, Andres Freund wrote: >> I don't see how this could be argued. It has to be a self-conflicting >> lockmode, otherwise you'd end up doing renames of tables where you >> cannot see the previous state. And you'd get weird errors about updating >> invisible rows etc. > >> I don't buy this description. Imo it's a fundamental correctness >> thing. Without it concurrent DDL would potentially overwrite the rename, >> because they could start updating while still seeing the old version. > > OK, I can refine those descriptions/comments. Do you have any concerns > about the underlying principle of this patch? Patch with updated comments to reflect your input. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services From 3188b76cb4a2851b87361193addb6cbe326d4a64 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Thu, 25 Oct 2018 08:33:17 +0100 Subject: [PATCH v3] Lower lock level for renaming indexes Change lock level for renaming index (either ALTER INDEX or implicitly via some other commands) from AccessExclusiveLock to ShareUpdateExclusiveLock. One reason we need a strong lock for relation renaming is that the name change causes a rebuild of the relcache entry. Concurrent sessions that have the relation open might not be able to handle the relcache entry changing underneath them. Therefore, we need to lock the relation in a way that no one can have the relation open concurrently. But for indexes, the relcache handles reloads specially in RelationReloadIndexInfo() in a way that keeps changes in the relcache entry to a minimum. As long as no one keeps pointers to rd_amcache and rd_options around across possible relcache flushes, which is the case, this ought to be safe. We also want to use a self-exclusive lock for correctness, so that concurrent DDL doesn't overwrite the rename if they start updating while still seeing the old version. Therefore, we use ShareUpdateExclusiveLock, which is already used by other DDL commands that want to operate in a concurrent manner. The reason this is interesting at all is that renaming an index is a typical part of a concurrent reindexing workflow (CREATE INDEX CONCURRENTLY new + DROP INDEX CONCURRENTLY old + rename back). And indeed a future built-in REINDEX CONCURRENTLY might rely on the ability to do concurrent renames as well. --- doc/src/sgml/mvcc.sgml| 12 ++-- doc/src/sgml/ref/alter_index.sgml | 9 - src/backend/commands/cluster.c| 4 ++-- src/backend/commands/tablecmds.c | 27 +-- src/backend/commands/typecmds.c | 2 +- src/include/commands/tablecmds.h | 3 ++- 6 files changed, 36 insertions(+), 21 deletions(-) diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml index 73934e5cf3..bedd9a008d 100644 --- a/doc/src/sgml/mvcc.sgml +++ b/doc/src/sgml/mvcc.sgml @@ -926,10 +926,10 @@ Table-level Lock Modes Acquired by VACUUM (without FULL), ANALYZE, CREATE INDEX CONCURRENTLY, - CREATE STATISTICS and - ALTER TABLE VALIDATE and other - ALTER TABLE variants (for full details see - ). + CREATE STATISTICS, and certain ALTER + INDEX and ALTER TABLE variants (for full + details see and ). @@ -970,7 +970,7 @@ Table-level Lock Modes - Acquired by CREATE TRIGGER and many forms of + Acquired by CREATE TRIGGER and some forms of ALTER TABLE (see ). @@ -1020,7 +1020,7 @@ Table-level Lock Modes CLUSTER, VACUUM FULL, and REFRESH MATERIALIZED VIEW (without CONCURRENTLY) - commands. Many forms of ALTER TABLE also acquire + commands. Many forms of ALTER INDEX and ALTER TABLE also acquire a lock at this level. This is also the default lock mode for LOCK TABLE statements that do not specify a mode explicitly. diff --git a/doc/src/sgml/ref/alter_index.sgml b/doc/src/sgml/ref/alter_index.sgml index d0a6212358..6d34dbb74e 100644 --- a/doc/src/sgml/ref/alter_index.sgml +++ b/doc/src/sgml/ref/alter_index.sgml @@ -39,7 +39,10 @@ Description ALTER INDEX changes the definition of an existing index. - There are several subforms: + There are several subforms described below. Note that the lock level required + may differ for each subform. An ACCESS EXCLUSIVE lock is held + unless explicitly noted. When multiple subcommands are listed, the lock + held will be the strictest one required from any subcommand. @@ -53,6 +56,10 @@ Description or EXCLUDE), the constraint is renamed as well. There is no effect on the stored data. + + Renaming an index acquires a SHARE UPDATE EXCLUSIVE + lock. + diff --git a/src/backend/commands/cluster.c b/src/backend/c
Re: Alter index rename concurrently to
On 13/10/2018 04:01, Andres Freund wrote: > I don't see how this could be argued. It has to be a self-conflicting > lockmode, otherwise you'd end up doing renames of tables where you > cannot see the previous state. And you'd get weird errors about updating > invisible rows etc. > I don't buy this description. Imo it's a fundamental correctness > thing. Without it concurrent DDL would potentially overwrite the rename, > because they could start updating while still seeing the old version. OK, I can refine those descriptions/comments. Do you have any concerns about the underlying principle of this patch? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Alter index rename concurrently to
Hi, On 2018-10-05 12:03:54 +0200, Peter Eisentraut wrote: > From 37591a06901e2d694e3928b7e1cddfcfd84f6267 Mon Sep 17 00:00:00 2001 > From: Peter Eisentraut > Date: Mon, 13 Aug 2018 22:38:36 +0200 > Subject: [PATCH v2] Lower lock level for renaming indexes > > Change lock level for renaming index (either ALTER INDEX or implicitly > via some other commands) from AccessExclusiveLock to > ShareUpdateExclusiveLock. > > The reason we need a strong lock at all for relation renaming is that > the name change causes a rebuild of the relcache entry. Concurrent > sessions that have the relation open might not be able to handle the > relcache entry changing underneath them. Therefore, we need to lock the > relation in a way that no one can have the relation open concurrently. > But for indexes, the relcache handles reloads specially in > RelationReloadIndexInfo() in a way that keeps changes in the relcache > entry to a minimum. As long as no one keeps pointers to rd_amcache and > rd_options around across possible relcache flushes, which is the case, > this ought to be safe. > > One could perhaps argue that this could all be done with an > AccessShareLock then. But we standardize here for consistency on > ShareUpdateExclusiveLock, which is already used by other DDL commands > that want to operate in a concurrent manner. For example, renaming an > index concurrently with CREATE INDEX CONCURRENTLY might be trouble. I don't see how this could be argued. It has to be a self-conflicting lockmode, otherwise you'd end up doing renames of tables where you cannot see the previous state. And you'd get weird errors about updating invisible rows etc. > @@ -3155,11 +3157,13 @@ RenameRelationInternal(Oid myrelid, const char > *newrelname, bool is_internal) > Oid namespaceId; > > /* > - * Grab an exclusive lock on the target table, index, sequence, view, > - * materialized view, or foreign table, which we will NOT release until > - * end of transaction. > + * Grab a lock on the target relation, which we will NOT release until > end > + * of transaction. The lock here guards mainly against triggering > + * relcache reloads in concurrent sessions, which might not handle this > + * information changing under them. For indexes, we can use a reduced > + * lock level because RelationReloadIndexInfo() handles indexes > specially. >*/ I don't buy this description. Imo it's a fundamental correctness thing. Without it concurrent DDL would potentially overwrite the rename, because they could start updating while still seeing the old version. Greetings, Andres Freund
Re[2]: Alter index rename concurrently to
> 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 >: > >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
Re: Alter index rename concurrently to
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 From 37591a06901e2d694e3928b7e1cddfcfd84f6267 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 13 Aug 2018 22:38:36 +0200 Subject: [PATCH v2] Lower lock level for renaming indexes Change lock level for renaming index (either ALTER INDEX or implicitly via some other commands) from AccessExclusiveLock to ShareUpdateExclusiveLock. The reason we need a strong lock at all for relation renaming is that the name change causes a rebuild of the relcache entry. Concurrent sessions that have the relation open might not be able to handle the relcache entry changing underneath them. Therefore, we need to lock the relation in a way that no one can have the relation open concurrently. But for indexes, the relcache handles reloads specially in RelationReloadIndexInfo() in a way that keeps changes in the relcache entry to a minimum. As long as no one keeps pointers to rd_amcache and rd_options around across possible relcache flushes, which is the case, this ought to be safe. One could perhaps argue that this could all be done with an AccessShareLock then. But we standardize here for consistency on ShareUpdateExclusiveLock, which is already used by other DDL commands that want to operate in a concurrent manner. For example, renaming an index concurrently with CREATE INDEX CONCURRENTLY might be trouble. The reason this is interesting at all is that renaming an index is a typical part of a concurrent reindexing workflow (CREATE INDEX CONCURRENTLY new + DROP INDEX CONCURRENTLY old + rename back). And indeed a future built-in REINDEX CONCURRENTLY might rely on the ability to do concurrent renames as well. --- doc/src/sgml/mvcc.sgml| 12 ++-- doc/src/sgml/ref/alter_index.sgml | 9 - src/backend/commands/cluster.c| 4 ++-- src/backend/commands/tablecmds.c | 24 ++-- src/backend/commands/typecmds.c | 2 +- src/include/commands/tablecmds.h | 3 ++- 6 files changed, 33 insertions(+), 21 deletions(-) diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml index 73934e5cf3..bedd9a008d 100644 --- a/doc/src/sgml/mvcc.sgml +++ b/doc/src/sgml/mvcc.sgml @@ -926,10 +926,10 @@ Table-level Lock Modes Acquired by VACUUM (without FULL), ANALYZE, CREATE INDEX CONCURRENTLY, - CREATE STATISTICS and - ALTER TABLE VALIDATE and other - ALTER TABLE variants (for full details see - ). + CREATE STATISTICS, and certain ALTER + INDEX and ALTER TABLE variants (for full + details see and ). @@ -970,7 +970,7 @@ Table-level Lock Modes - Acquired by CREATE TRIGGER and many forms of + Acquired by CREATE TRIGGER and some forms of ALTER TABLE (see ). @@ -1020,7 +1020,7 @@ Table-level Lock Modes CLUSTER, VACUUM FULL, and REFRESH MATERIALIZED VIEW (without CONCURRENTLY) - commands. Many forms of ALTER TABLE also acquire + commands. Many forms of ALTER INDEX and ALTER TABLE also acquire a lock at this level. This is also the default lock mode for LOCK TABLE statements that do not specify a mode explicitly. diff --git a/doc/src/sgml/ref/alter_index.sgml b/doc/src/sgml/ref/alter_index.sgml index d0a6212358..6d34dbb74e 100644 --- a/doc/src/sgml/ref/alter_index.sgml +++ b/doc/src/sgml/ref/alter_index.sgml @@ -39,7 +39,10 @@ Description ALTER INDEX changes the definition of an existing index. - There are several subforms: + There are several subforms described below. Note that the lock level required + may differ for each subform. An ACCESS EXCLUSIVE lock is held + unless explicitly noted. When multiple subcommands are listed, the lock + held will be the strictest one required from any subcommand. @@ -53,6 +56,10 @@ Description or EXCLUDE), the constraint is renamed as well. There is no effect on the stored data. + + Renaming an index acquires a SHARE UPDATE EXCLUSIVE + lock. + diff --git
Re[2]: Alter index rename concurrently to
> 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 >: > >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
Re: Alter index rename concurrently to
Hi, On 2018-08-14 08:44:46 +0200, Peter Eisentraut wrote: > On 03/08/2018 15:00, Robert Haas wrote: > > On Thu, Aug 2, 2018 at 4:44 PM, Andres Freund wrote: > >> ISTM, if you want to increase consistency in this area, you've to go > >> further. E.g. processing invalidations in StartTransactionCommand() in > >> all states, which'd give you a lot more consistency. > > > > Hmm, that seems like a pretty good idea. > > That would only affect top-level commands, not things like SPI. Is that > what we want? Or we could sprinkle additional > AcceptInvalidationMessages() calls in spi.c. I'd say it's not unreasonable to *not* to guarantee spi (and various other functions) immediately take concurrent activity into account, unless locking rules guarantee that independently. Definining it as "commands sent by the client see effects of previously issued non-conflicting DDL, others might see them earlier" doesn't seem insane. If you want to take account of SPI and PLs it gets more comoplicated quickly, because we don't always parse functions afresh. So you'd need invalidations in a lot more places. Greetings, Andres Freund
Re: Alter index rename concurrently to
On 03/08/2018 15:00, Robert Haas wrote: > On Thu, Aug 2, 2018 at 4:44 PM, Andres Freund wrote: >> ISTM, if you want to increase consistency in this area, you've to go >> further. E.g. processing invalidations in StartTransactionCommand() in >> all states, which'd give you a lot more consistency. > > Hmm, that seems like a pretty good idea. That would only affect top-level commands, not things like SPI. Is that what we want? Or we could sprinkle additional AcceptInvalidationMessages() calls in spi.c. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Alter index rename concurrently to
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 From febe62543115a99eae00747a864cd922fde71875 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 13 Aug 2018 22:38:36 +0200 Subject: [PATCH v1] Lower lock level for renaming indexes Change lock level for renaming index (either ALTER INDEX or implicitly via some other commands) from AccessExclusiveLock to ShareUpdateExclusiveLock. The reason we need a strong lock at all for relation renaming is that the name change causes a rebuild of the relcache entry. Concurrent sessions that have the relation open might not be able to handle the relcache entry changing underneath them. Therefore, we need to lock the relation in a way that no one can have the relation open concurrently. But for indexes, the relcache handles reloads specially in RelationReloadIndexInfo() in a way that keeps changes in the relcache entry to a minimum. As long as no one keeps pointers to rd_amcache and rd_options around across possible relcache flushes, which is the case, this ought to be safe. One could perhaps argue that this could all be done with an AccessShareLock then. But we standardize here for consistency on ShareUpdateExclusiveLock, which is already used by other DDL commands that want to operate in a concurrent manner. For example, renaming an index concurrently with CREATE INDEX CONCURRENTLY might be trouble. The reason this is interesting at all is that renaming an index is a typical part of a concurrent reindexing workflow (CREATE INDEX CONCURRENTLY new + DROP INDEX CONCURRENTLY old + rename back). And indeed a future built-in REINDEX CONCURRENTLY might rely on the ability to do concurrent renames as well. --- doc/src/sgml/mvcc.sgml| 12 ++-- doc/src/sgml/ref/alter_index.sgml | 9 - src/backend/commands/cluster.c| 4 ++-- src/backend/commands/tablecmds.c | 24 ++-- src/backend/commands/typecmds.c | 2 +- src/include/commands/tablecmds.h | 3 ++- 6 files changed, 33 insertions(+), 21 deletions(-) diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml index 73934e5cf3..bedd9a008d 100644 --- a/doc/src/sgml/mvcc.sgml +++ b/doc/src/sgml/mvcc.sgml @@ -926,10 +926,10 @@ Table-level Lock Modes Acquired by VACUUM (without FULL), ANALYZE, CREATE INDEX CONCURRENTLY, - CREATE STATISTICS and - ALTER TABLE VALIDATE and other - ALTER TABLE variants (for full details see - ). + CREATE STATISTICS, and certain ALTER + INDEX and ALTER TABLE variants (for full + details see and ). @@ -970,7 +970,7 @@ Table-level Lock Modes - Acquired by CREATE TRIGGER and many forms of + Acquired by CREATE TRIGGER and some forms of ALTER TABLE (see ). @@ -1020,7 +1020,7 @@ Table-level Lock Modes CLUSTER, VACUUM FULL, and REFRESH MATERIALIZED VIEW (without CONCURRENTLY) - commands. Many forms of ALTER TABLE also acquire + commands. Many forms of ALTER INDEX and ALTER TABLE also acquire a lock at this level. This is also the default lock mode for LOCK TABLE statements that do not specify a mode explicitly. diff --git a/doc/src/sgml/ref/alter_index.sgml b/doc/src/sgml/ref/alter_index.sgml index 7290d9a5bd..73094fd179 100644 --- a/doc/src/sgml/ref/alter_index.sgml +++ b/doc/src/sgml/ref/alter_index.sgml @@ -39,7 +39,10 @@ Description ALTER INDEX changes the definition of an existing index. - There are several subforms: + There are several subforms described below. Note that the lock level required +
Re: Alter index rename concurrently to
On Thu, Aug 2, 2018 at 4:44 PM, Andres Freund wrote: > ISTM, if you want to increase consistency in this area, you've to go > further. E.g. processing invalidations in StartTransactionCommand() in > all states, which'd give you a lot more consistency. Hmm, that seems like a pretty good idea. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Alter index rename concurrently to
On Thu, Aug 2, 2018 at 4:51 PM, Tom Lane wrote: > Robert Haas writes: >> [ reasons why DDL under less than AEL sucks ] > > Unfortunately, none of these problems are made to go away with an > AcceptInvalidationMessages at statement start. That just makes the > window smaller. But DDL effects could still be seen - or not - > partway through a statement, with just as much ensuing hilarity > as in your example. Maybe more. Making the window a lot smaller -- so that it's a statement rather than a transaction -- has a lot of value, I think. What's particularly obnoxious about the transaction thing is that in many cases the next statement will see the changes, but sometimes (if it only references previously-referenced tables in previously-acquired lock modes) it won't. I can't see any good reason to keep that inconsistency around. It's true that the changes might or might not see by a concurrently-running transaction, and it would be nice to do something about that, too, but it's not clear that there is any straightforward change that would accomplish that. OTOH, ensuring that AcceptInvalidationMessages() runs at least once per statement seems pretty straightforward. > The real issue here, and the reason why I'm very unhappy with the mad rush > in certain quarters to try to reduce locking levels for DDL, is exactly > that it generally results in uncertainty about when the effects will be > seen. I do not think your proposal does much more than put a fig leaf > over that problem. Let's not treat wanting to reduce locking levels for DDL as a character flaw. As Andres says, it's a huge operational problem. I don't think we should be irresponsible about lowering DDL lock levels and accept random breakage along the way, but I do think we need, rather desperately actually, to reduce lock levels. We need to figure out which things are problems and what can be done about them, not just say "no" to everything. > Barring somebody having a great idea about resolving that, I think we > just need to be very clear that any DDL done with less than AEL has > exactly this issue. In the case at hand, couldn't we just document > that "the effects of RENAME CONCURRENTLY may not be seen in other > sessions right away; at worst, not till they begin a new transaction"? > If you don't like that, don't use CONCURRENTLY. Sure. On the other hand, we could ALSO insert an AcceptInvalidationMessages() call once per statement and document that they might not be seen until you begin a new statement, which seems like it has basically no downside but is much better for users, particularly because the name of an object tends -- in the great majority of cases -- to matter only at the beginning of statement processing. You can construct SQL queries that perform name lookups midway through, but that's not typical. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Alter index rename concurrently to
On 2018-08-02 16:51:10 -0400, Tom Lane wrote: > Robert Haas writes: > > [ reasons why DDL under less than AEL sucks ] > > Unfortunately, none of these problems are made to go away with an > AcceptInvalidationMessages at statement start. That just makes the > window smaller. But DDL effects could still be seen - or not - > partway through a statement, with just as much ensuing hilarity > as in your example. Maybe more. I think it's a lot easier to explain that every newly issued statement sees the effect of DDL, and already running statements may see it, than something else. I don't agree that parse analysis is a good hook to force that, as I've written. > The real issue here, and the reason why I'm very unhappy with the mad rush > in certain quarters to try to reduce locking levels for DDL, is exactly > that it generally results in uncertainty about when the effects will be > seen. I do not think your proposal does much more than put a fig leaf > over that problem. I think it's a significant issue operationally, which is why that pressure exists. I don't know what makes it a "mad rush", given these discussions have been going on for *years*? Greetings, Andres Freund
Re: Alter index rename concurrently to
Robert Haas writes: > [ reasons why DDL under less than AEL sucks ] Unfortunately, none of these problems are made to go away with an AcceptInvalidationMessages at statement start. That just makes the window smaller. But DDL effects could still be seen - or not - partway through a statement, with just as much ensuing hilarity as in your example. Maybe more. The real issue here, and the reason why I'm very unhappy with the mad rush in certain quarters to try to reduce locking levels for DDL, is exactly that it generally results in uncertainty about when the effects will be seen. I do not think your proposal does much more than put a fig leaf over that problem. Barring somebody having a great idea about resolving that, I think we just need to be very clear that any DDL done with less than AEL has exactly this issue. In the case at hand, couldn't we just document that "the effects of RENAME CONCURRENTLY may not be seen in other sessions right away; at worst, not till they begin a new transaction"? If you don't like that, don't use CONCURRENTLY. regards, tom lane
Re: Alter index rename concurrently to
Hi, On 2018-08-02 16:30:42 -0400, Robert Haas wrote: > With this change, we'd be able to say that new statements will > definitely see the results of concurrent DDL. I don't think it really gets you that far. Because you're suggesting to do it at the parse stage, you suddenly have issues with prepared statements. Some client libraries, and a lot more frameworks, automatically use prepared statements when they see the same query text multiple times. And this'll not improve anything on that. ISTM, if you want to increase consistency in this area, you've to go further. E.g. processing invalidations in StartTransactionCommand() in all states, which'd give you a lot more consistency. Greetings, Andres Freund
Re: Alter index rename concurrently to
On Thu, Aug 2, 2018 at 4:02 PM, Andres Freund wrote: >> 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? Sure. I'd like to be able to explain in the documentation in simple words when a given DDL change is likely to take effect. For changes made under AccessExclusiveLock, there's really nothing to say: the change will definitely take effect immediately. If you're able to do anything at all with the relevant table, you must have got some kind of lock on it, and that means you must have done AcceptInvalidationMessages(), and so you will definitely see the change. With DDL changes made under less than AccessExclusiveLock, the situation is more complicated. Right now, we can say that a new transaction will definitely see the changes, because it will have to acquire a lock on that relation which it doesn't already have and will thus have to do AcceptInvalidationMessages(). A new statement within the same transaction may see the changes, or it may not. If it mentions any relations not previously mentioned or if it does something like UPDATE a relation where we previously only did a SELECT, thus triggering a new lock acquisition, it will see the changes. If a catchup interrupt gets sent to it, it will see the changes. Otherwise, it won't. It's even possible that we'll start to see the changes in the middle of a statement, because of a sinval reset or something. To summarize, at present, all we can really say is that changes made by concurrent DDL which doesn't take AEL may or may not affect already-running queries, may or may not affect new queries, and will affect new transactions. With this change, we'd be able to say that new statements will definitely see the results of concurrent DDL. That's a clear, easy to understand rule which I think users will like. It would be even better if we could say something stronger, e.g. "Concurrent DDL will affect new SQL statements, but not those already in progress." Or "Concurrent DDL will affect new SQL statements, but SQL statements that are already running may take up to 10 seconds to react to the changes". Or whatever. I'm not sure there's really a way to get to a really solid guarantee, but being able to tell users that, at a minimum, the next SQL statement will notice that things have changed would be good. Otherwise, we'll have conversations like this: User: I have a usage pattern where I run a DDL command to rename an object, and then in another session I tried to refer to it by the new name, and it sometimes it works and sometimes it doesn't. Why does that happen? Postgres Expert: Well, there are several possible reasons. If you already had a transaction in progress in the second window and it already had a lock on the object strong enough for the operation you attempted to perform and no sinval resets occurred and nothing else triggered invalidation processing, then it would still know that object under the old name. Otherwise it would know about it under the new name. User: Well, I did have a transaction open and it may have had some kind of lock on that object already, but how do I know whether invalidation processing happened? Postgres Expert: There's really know way to know. If something else on the system were doing a lot of DDL operations, then it might fill up the invalidation queue enough to trigger catchup interrupts or sinval resets, but if not, it could be deferred for an arbitrarily long period of time. User: So you're saying that if I have two PostgreSQL sessions, and I execute the same commands in those sessions in the same order, just like the isolationtester does, I can get different answers depending on whether some third session creates a lot of unrelated temporary tables in a different database while that's happening? Postgres Expert: Yes. I leave it to your imagination to fill in what this imaginary user will say next, but I bet it will be snarky. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Alter index rename concurrently to
Robert Haas writes: > On Wed, Aug 1, 2018 at 3:36 PM, Andres Freund wrote: >> 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. What will that accomplish that the existing call in transaction start doesn't? It might make the window for concurrency issues a bit narrower, but it certainly doesn't close it. regards, tom lane
Re: Alter index rename concurrently to
Hi, On 2018-08-02 15:57:13 -0400, Robert Haas wrote: > On Wed, Aug 1, 2018 at 3:36 PM, Andres Freund 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
Re: Alter index rename concurrently to
On Wed, Aug 1, 2018 at 3:36 PM, Andres Freund 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
Re: Alter index rename concurrently to
On 2018-08-01 15:33:09 -0400, Robert Haas wrote: > On Wed, Aug 1, 2018 at 3:04 AM, Peter Eisentraut > wrote: > > On 31/07/2018 23:25, Tom Lane wrote: > >> Peter Eisentraut 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
Re: Alter index rename concurrently to
On Wed, Aug 1, 2018 at 3:04 AM, Peter Eisentraut wrote: > On 31/07/2018 23:25, Tom Lane wrote: >> Peter Eisentraut 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
Re: Alter index rename concurrently to
On 31/07/2018 23:25, Tom Lane wrote: > Peter Eisentraut 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
Re: Alter index rename concurrently to
Peter Eisentraut 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
Re: Alter index rename concurrently to
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
Re: Alter index rename concurrently to
On Wed, Jul 18, 2018 at 5:20 AM, Peter Eisentraut 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
Re: Alter index rename concurrently to
> > 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.
Fwd: Re[2]: Alter index rename concurrently to
Понедельник, 23 июля 2018, 18:06 +03:00 от Peter Eisentraut < peter.eisentr...@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
Re: Alter index rename concurrently to
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
Re[2]: Alter index rename concurrently to
>Среда, 18 июля 2018, 12:21 +03:00 от Peter Eisentraut >: > > >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
Re: Alter index rename concurrently to
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
Re[2]: Alter index rename concurrently to
>Среда, 18 июля 2018, 12:21 +03:00 от Peter Eisentraut >: > >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
Re: Alter index rename concurrently to
On 17.07.18 13:48, Andrey Klychkov wrote: > Please, have a look at previous discussions on the subject: > - 2012 > > https://www.postgresql.org/message-id/cab7npqtys6juqdxuczbjb0bnw0kprw8wdzuk11kaxqq6o98...@mail.gmail.com > - > 2013 > https://www.postgresql.org/message-id/cab7npqstfkuc7dzxcdx4hotu63yxhrronv2aouzyd-zz_8z...@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
Re[2]: Alter index rename concurrently to
> Понедельник, 16 июля 2018, 22:19 +03:00 от Andrey Borodin > : > >Hi! > >> 16 июля 2018 г., в 22:58, Andrey Klychkov < aaklych...@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
Re[2]: Alter index rename concurrently to
>Понедельник, 16 июля 2018, 22:40 +03:00 от Victor Yegorov : > >пн, 16 июл. 2018 г. в 21:58, Andrey Klychkov < aaklych...@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 rename CONCURRENTLY to ”. > >Please, have a look at previous discussions on the subject: >- 2012 >https://www.postgresql.org/message-id/cab7npqtys6juqdxuczbjb0bnw0kprw8wdzuk11kaxqq6o98...@mail.gmail.com >- 2013 >https://www.postgresql.org/message-id/cab7npqstfkuc7dzxcdx4hotu63yxhrronv2aouzyd-zz_8z...@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
Re: Alter index rename concurrently to
пн, 16 июл. 2018 г. в 21:58, Andrey Klychkov : > I made a patch to solve this issue (see the attachment). > It allows to avoid locks by a query like this: > “alter index rename CONCURRENTLY to ”. > Please, have a look at previous discussions on the subject: - 2012 https://www.postgresql.org/message-id/cab7npqtys6juqdxuczbjb0bnw0kprw8wdzuk11kaxqq6o98...@mail.gmail.com - 2013 https://www.postgresql.org/message-id/cab7npqstfkuc7dzxcdx4hotu63yxhrronv2aouzyd-zz_8z...@mail.gmail.com - https://commitfest.postgresql.org/16/1276/ -- Victor Yegorov
Re: Alter index rename concurrently to
Hi! > 16 июля 2018 г., в 22:58, Andrey Klychkov написал(а): > 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.
Alter index rename concurrently to
Dear hackers!
I have an idea to facilitate work with index rebuilding.
Usually if we want to rebuild an index without table locks we should do the
queries below:
1. create index concurrently... (with different name on the same columns)
2. drop index concurrently
3. alter index rename to
As you can see, only the last query in this simple algorithm locks table.
Commonly this steps are included to a more complex script and run by cron or
manually.
When you have high load databases, maybe some problems appear:
1. it’s dangerous and unacceptable for production to wait unknown
number of minutes or even hours while a table is locked
2. we must write a complex script that sets statement timeout to rename and
implement several checks there to prevent stopping of production at nights
3. it’s uncomfortable to find indexes that couldn’t be renamed during script
executing
Then we must execute renaming manually and interrupt it again and
again if it can’t execute in allowable time.
I made a patch to solve this issue (see the attachment).
It allows to avoid locks by a query like this:
“alter index rename CONCURRENTLY to ”.
Briefly, I did it by using similar way in the index_create() and index_drop()
functions
that set ShareUpdateExclusiveLock instead of AccessShareLock
when the structure DropStmt/CreateStmt has “true” in the stmt->concurrent field.
(In other words, when it "see" "concurretly" in query).
In all other cases (alter table, sequence, etc) I initialized this field as
“false” respectively.
I ran it when another transactions to the same table are started but not
finished.
Also I used a script that made SELECT\DML queries in a loop to that test tables
and
"ALTER INDEX ... RENAME CONCURRENTLY TO ..." works without any errors and
indexes were valid after renaming.
Maybe it’s interesting for someone.
I’ll be very thankful for any ideas!
--
Kind regards,
Andrew Klychkov# Author: Andrew Klychkov
# 16-07-2018
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 482d463..47e20d5 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1658,14 +1658,14 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u",
OIDOldHeap);
RenameRelationInternal(newrel->rd_rel->reltoastrelid,
- NewToastName, true);
+ NewToastName, true, false);
/* ... and its valid index too. */
snprintf(NewToastName, NAMEDATALEN, "pg_toast_%u_index",
OIDOldHeap);
RenameRelationInternal(toastidx,
- NewToastName, true);
+ NewToastName, true, false);
}
relation_close(newrel, NoLock);
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 22e81e7..ead4f26 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3012,7 +3012,7 @@ rename_constraint_internal(Oid myrelid,
|| con->contype == CONSTRAINT_UNIQUE
|| con->contype == CONSTRAINT_EXCLUSION))
/* rename the index; this renames the constraint as well */
- RenameRelationInternal(con->conindid, newconname, false);
+ RenameRelationInternal(con->conindid, newconname, false, false);
else
RenameConstraintById(constraintOid, newconname);
@@ -3082,7 +3082,9 @@ RenameRelation(RenameStmt *stmt)
{
Oid relid;
ObjectAddress address;
+LOCKMODElockmode;
+lockmode = stmt->concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock;
/*
* Grab an exclusive lock on the target table, index, sequence, view,
* materialized view, or foreign table, which we will NOT release until
@@ -3091,7 +3093,7 @@ RenameRelation(RenameStmt *stmt)
* Lock level used here should match RenameRelationInternal, to avoid lock
* escalation.
*/
- relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
+ relid = RangeVarGetRelidExtended(stmt->relation, lockmode,
stmt->missing_ok ? RVR_MISSING_OK : 0,
RangeVarCallbackForAlterRelation,
(void *) stmt);
@@ -3105,7 +3107,7 @@ RenameRelation(RenameStmt *stmt)
}
/* Do the work */
- RenameRelationInternal(relid, stmt->newname, false);
+ RenameRelationInternal(relid, stmt->newname, false, stmt->concurrent);
ObjectAddressSet(address, RelationRelationId, relid);
@@ -3122,20 +3124,22 @@ RenameRelation(RenameStmt *stmt)
* sequence, AFAIK there's no need for it to be there.
*/
void
-RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal)
+RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal, bool concurrent)
{
Relation targetrelation;
Relation relrelation; /* for RELATION relation */
HeapTuple reltup;
Form_pg_class relform;
Oid namespaceId;
+LOCKMODElockmode;
/*
* Grab an exclusive lock on the target table, index, sequence, view,
* materialized view,
