Обсуждение: ALTER INDEX .. RENAME allows to rename tables/views as well

I can confirm this bug in git head, and I think it should be fixed.

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

On Mon, Oct  4, 2021 at 10:23:23AM +0000, Onder Kalaci wrote:
> Hi hackers,
> 
> I realized a subtle behavior with ALTER INDEX .. RENAME. It seems like a bug to
> me, please see the steps below.
> 
>  
> 
> Test 1: Rename table via RENAME .. INDEX
> 
> CREATE TABLE test_table (a int);
> 
> SELECT 'test_table'::regclass::oid;
> 
>   oid 
> 
> -------
> 
> 34470
> 
> (1 row)
> 
> -- rename table using ALTER INDEX ..
> 
> ALTER INDEX test_table RENAME TO test_table_2;
> 
> 
> -- see that table is rename
> 
> SELECT 34470::regclass;
> 
>    regclass  
> 
> --------------
> 
> test_table_2
> 
> (1 row)
> 
> 
> Test 2: Rename view via RENAME .. INDEX
> CREATE VIEW test_view AS SELECT * FROM pg_class;
> 
> SELECT 'test_view'::regclass::oid;
> 
>   oid 
> 
> -------
> 
> 34473
> 
> (1 row)
> 
>  
> 
> ALTER INDEX test_view RENAME TO test_view_2;
> 
> ELECT 34473::regclass;
> 
>   regclass  
> 
> -------------
> 
> test_view_2
> 
> (1 row)
> 
>  
> 
> 
> It seems like an oversight in ExecRenameStmt(), and probably applies to
> sequences, mat. views and foreign tables as well.                    
> 
>  
> 
> I can reproduce this on both 13.2 and 14.0. Though haven’t checked earlier
> versions.
> 
>  
> 
> Thanks,
> 
> Onder
> 

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

On 10/6/21, 1:52 PM, "Bruce Momjian" <bruce@momjian.us> wrote:
> I can confirm this bug in git head, and I think it should be fixed.

Here's a patch that ERRORs if the object type and statement type do
not match.  Interestingly, some of the regression tests were relying
on this behavior.  I considered teaching RenameRelation() how to
handle such mismatches, but we have to choose the lock level before we
know the object type, so that might be more trouble than it's worth.

I'm not too happy with the error message format, but I'm not sure we
can do much better without listing all the object types or doing some
more invasive refactoring.

Nathan

"Bossart, Nathan" <bossartn@amazon.com> writes:
> On 10/6/21, 1:52 PM, "Bruce Momjian" <bruce@momjian.us> wrote:
>> I can confirm this bug in git head, and I think it should be fixed.

> Here's a patch that ERRORs if the object type and statement type do
> not match.  Interestingly, some of the regression tests were relying
> on this behavior.

... as, no doubt, are a lot of applications that this will gratuitously
break.  We've long had a policy that ALTER TABLE will work on relations
that aren't tables, so long as the requested operation is sensible.

The situation for "ALTER some-other-relation-kind" is a bit more
confused, because some cases throw errors and some don't; but I really
doubt that tightening things up here will earn you anything but
brickbats.  I *definitely* don't agree with discarding the policy
about ALTER TABLE, especially if it's only done for RENAME.

In short: no, I do not agree that this is a bug to be fixed.  Perhaps
we should have done things differently years ago, but it's too late to
redefine it.

            regards, tom lane

On 10/6/21, 3:44 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> "Bossart, Nathan" <bossartn@amazon.com> writes:
>> Here's a patch that ERRORs if the object type and statement type do
>> not match.  Interestingly, some of the regression tests were relying
>> on this behavior.
>
> ... as, no doubt, are a lot of applications that this will gratuitously
> break.  We've long had a policy that ALTER TABLE will work on relations
> that aren't tables, so long as the requested operation is sensible.

Right.

> The situation for "ALTER some-other-relation-kind" is a bit more
> confused, because some cases throw errors and some don't; but I really
> doubt that tightening things up here will earn you anything but
> brickbats.  I *definitely* don't agree with discarding the policy
> about ALTER TABLE, especially if it's only done for RENAME.

I think we should at least consider adding this check for ALTER INDEX
since we choose a different lock level in that case.

Nathan

On 2021-Oct-06, Bossart, Nathan wrote:

> On 10/6/21, 3:44 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:

> > The situation for "ALTER some-other-relation-kind" is a bit more
> > confused, because some cases throw errors and some don't; but I really
> > doubt that tightening things up here will earn you anything but
> > brickbats.  I *definitely* don't agree with discarding the policy
> > about ALTER TABLE, especially if it's only done for RENAME.
> 
> I think we should at least consider adding this check for ALTER INDEX
> since we choose a different lock level in that case.

I agree -- letting ALTER INDEX process relations that aren't indexes is
dangerous, with its current coding that uses a reduced lock level.  But
maybe erroring out is not necessary; can we instead loop, locking the
object with ShareUpdateExclusive first, assuming it *is* an index, and
if it isn't then we release and restart using the stronger lock this
time?

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

On 10/6/21, 4:45 PM, "Alvaro Herrera" <alvherre@alvh.no-ip.org> wrote:
> On 2021-Oct-06, Bossart, Nathan wrote:
>> I think we should at least consider adding this check for ALTER INDEX
>> since we choose a different lock level in that case.
>
> I agree -- letting ALTER INDEX process relations that aren't indexes is
> dangerous, with its current coding that uses a reduced lock level.  But
> maybe erroring out is not necessary; can we instead loop, locking the
> object with ShareUpdateExclusive first, assuming it *is* an index, and
> if it isn't then we release and restart using the stronger lock this
> time?

Good idea.  Patch attached.

Nathan

On Wed, Oct 06, 2021 at 06:43:25PM -0400, Tom Lane wrote:
> ... as, no doubt, are a lot of applications that this will gratuitously
> break.  We've long had a policy that ALTER TABLE will work on relations
> that aren't tables, so long as the requested operation is sensible.

Yeah, that was my first thought after seeing this thread.  There is a
risk in breaking something that was working previously.  Perhaps it
was just working by accident, but that could be surprising if an
application relied on the existing behavior.
--
Michael

On 2021-Oct-07, Bossart, Nathan wrote:

> Good idea.  Patch attached.

Yeah, that sounds exactly what I was thinking.

Now, what is the worst that can happen if we rename a table under SUE
and somebody else is using the table concurrently?  Is there any way to
cause a backend crash or something like that?  As far as I can see,
because we grab a fresh catalog snapshot for each query, you can't cause
anything worse than reading from a different table.  I do lack
imagination for creating attacks, though.

So my inclination would be to apply this to master only.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)

On 10/18/21, 4:56 PM, "Alvaro Herrera" <alvherre@alvh.no-ip.org> wrote:
> Now, what is the worst that can happen if we rename a table under SUE
> and somebody else is using the table concurrently?  Is there any way to
> cause a backend crash or something like that?  As far as I can see,
> because we grab a fresh catalog snapshot for each query, you can't cause
> anything worse than reading from a different table.  I do lack
> imagination for creating attacks, though.

This message [0] in the thread for lowering the lock level for
renaming indexes seems to indicate that there may be some risk of
crashing.

Nathan

[0] https://postgr.es/m/CA%2BTgmobtmFT5g-0dA%3DvEFFtogjRAuDHcYPw%2BqEdou5dZPnF%3Dpg%40mail.gmail.com

I was about to push this when it occurred to me that it seems a bit
pointless to release AEL in order to retry with the lighter lock; once
we have AEL, let's just keep it and proceed.  So how about the attached?

I'm now thinking that this is to back-patch all the way to 12.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/

On 10/19/21, 1:36 PM, "Alvaro Herrera" <alvherre@alvh.no-ip.org> wrote:
> I was about to push this when it occurred to me that it seems a bit
> pointless to release AEL in order to retry with the lighter lock; once
> we have AEL, let's just keep it and proceed.  So how about the attached?

I did consider this, but I figured it might be better to keep the lock
level consistent for a given object type no matter what the statement
type is.  I don't have a strong opinion about this, though.

> I'm now thinking that this is to back-patch all the way to 12.

+1.  The patch LGTM.  I like the test additions to check the lock
level.

Nathan

On 2021-Oct-19, Bossart, Nathan wrote:

> I did consider this, but I figured it might be better to keep the lock
> level consistent for a given object type no matter what the statement
> type is.  I don't have a strong opinion about this, though.

Yeah, the problem is that if there is a concurrent process waiting on
your lock, we'll release ours and they'll grab theirs, so we'll be
waiting on them afterwards, which is worse.

BTW I noticed that the case of partitioned indexes was wrong too.  I
fixed that, added it to the tests, and pushed.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"People get annoyed when you try to debug them."  (Larry Wall)

On 10/19/21, 3:13 PM, "Alvaro Herrera" <alvherre@alvh.no-ip.org> wrote:
> On 2021-Oct-19, Bossart, Nathan wrote:
>
>> I did consider this, but I figured it might be better to keep the lock
>> level consistent for a given object type no matter what the statement
>> type is.  I don't have a strong opinion about this, though.
>
> Yeah, the problem is that if there is a concurrent process waiting on
> your lock, we'll release ours and they'll grab theirs, so we'll be
> waiting on them afterwards, which is worse.

Makes sense.

> BTW I noticed that the case of partitioned indexes was wrong too.  I
> fixed that, added it to the tests, and pushed.

Ah, good catch.  Thanks!

Nathan