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]/messages/by-id/CA+TgmobtmFT5g-0dA=vEFFtogjRAuDHcYPw+qEdou5dZPnF=pg@mail.gmail.com in the thread for lowering the lock level for
renaming indexes seems to indicate that there may be some risk of
crashing.

Nathan

[0]: /messages/by-id/CA+TgmobtmFT5g-0dA=vEFFtogjRAuDHcYPw+qEdou5dZPnF=pg@mail.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