Thread: 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 <old index>
3. alter index <new_index> rename to <like_old_index>

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 <new_index> rename CONCURRENTLY to <old_index_name>”.

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

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
CREATEINDEX 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, 22:40 +03:00 от Victor Yegorov <vyegorov@gmail.com>:

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>:

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
>     https://www.postgresql.org/message-id/CAB7nPqTys6JUQDxUczbJb0BNW0kPrW8WdZuk11KaxQq6o98PJg@mail.gmail.com
>     -
>     2013 https://www.postgresql.org/message-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
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

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Aug 1, 2018 at 3:36 PM, Andres Freund <andres@anarazel.de> 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

On Thu, Aug 2, 2018 at 4:02 PM, Andres Freund <andres@anarazel.de> 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

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

Robert Haas <robertmhaas@gmail.com> 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

On 2018-08-02 16:51:10 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> 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

On Thu, Aug 2, 2018 at 4:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> 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

On Thu, Aug 2, 2018 at 4:44 PM, Andres Freund <andres@anarazel.de> 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

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

On 03/08/2018 15:00, Robert Haas wrote:
> On Thu, Aug 2, 2018 at 4:44 PM, Andres Freund <andres@anarazel.de> 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

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 <andres@anarazel.de> 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

> 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>:

--
Regards,
Andrey Klychkov

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

> 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>:

--
Regards,
Andrey Klychkov

Hi,

On 2018-10-05 12:03:54 +0200, Peter Eisentraut wrote:
> From 37591a06901e2d694e3928b7e1cddfcfd84f6267 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e@gmx.net>
> 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

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

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

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