[PATCH] rename column if exists

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed

Thank you for your contribution.

This is a useful feature. Although, there are so many places we alter a column which don't support IF EXISTS. For example: ALTER COLUMN IF EXISTS. Why don't we include the necessary changes across different use cases to this patch?

+ | ALTER TABLE IF_P EXISTS relation_expr RENAME opt_column IF_P EXISTS name TO name

Since this is my first review patch, can you help me understand why some keywords are written with "_P" suffix?

+     | ALTER TABLE relation_expr RENAME opt_column IF_P EXISTS name TO name
+       {
+         RenameStmt *n = makeNode(RenameStmt);
+         n->renameType = OBJECT_COLUMN;
+         n->relationType = OBJECT_TABLE;
+         n->relation = $3;
+         n->subname = $8;
+         n->newname = $10;
+         n->missing_ok = false;
+         n->sub_missing_ok = true;
+         $$ = (Node *)n;
+       }

Copying alter table combinations (with and without IF EXISTS statements) makes this patch hard to review and bloats the gram. Instead of copying, perhaps we can use an optional syntax, like opt_if_not_exists of ALTER TYPE.

+ if (attnum == InvalidAttrNumber)
+ {
+   if (!stmt->sub_missing_ok)
+     ereport(ERROR,
+         (errcode(ERRCODE_UNDEFINED_COLUMN),
+          errmsg("column \"%s\" does not exist",
+             stmt->subname)));
+   else
+   {
+     ereport(NOTICE,
+         (errmsg("column \"%s\" does not exist, skipping",
+             stmt->subname)));
+     return InvalidObjectAddress;
+   }
+ }
+

Other statements in gram.y includes sub_missing_ok = true and missing_ok = false. Why don't we add sub_missing_ok = false to existing declarations where IF EXISTS is not used?

-    <term><literal>RENAME ATTRIBUTE</literal></term>
+    <term><literal>RENAME ATTRIBUTE [ IF EXISTS ]</literal></term>

It seems that ALTER VIEW, ALTER TYPE, and ALTER MATERIALIZED VIEW does not have any tests for this feature.

On 22 Mar 2021, at 20:40, David Oksman <oksman.dav@gmail.com> wrote:

Added the ability to specify IF EXISTS when renaming a column of an object (table, view, etc.).
For example: ALTER TABLE distributors RENAME COLUMN IF EXISTS address TO city;
If the column does not exist, a notice is issued instead of throwing an error.

What is the intended use-case for RENAME COLUMN IF EXISTS? I'm struggling to
see when that would be helpful to users but I might not be imaginative enough.

--
Daniel Gustafsson https://vmware.com/

On Thursday, November 4, 2021, Daniel Gustafsson <daniel@yesql.se> wrote:

On 22 Mar 2021, at 20:40, David Oksman <oksman.dav@gmail.com> wrote:

Added the ability to specify IF EXISTS when renaming a column of an

object (table, view, etc.).

For example: ALTER TABLE distributors RENAME COLUMN IF EXISTS address TO

city;

If the column does not exist, a notice is issued instead of throwing an

error.

What is the intended use-case for RENAME COLUMN IF EXISTS? I'm struggling
to
see when that would be helpful to users but I might not be imaginative
enough.

Same reasoning as for all the other if exists we have, idempotence. Being
able to run the command on an object that is already in the desired state
without provoking an error.

David J.

On 4 Nov 2021, at 14:26, David G. Johnston <david.g.johnston@gmail.com> wrote:

On Thursday, November 4, 2021, Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> wrote:

On 22 Mar 2021, at 20:40, David Oksman <oksman.dav@gmail.com <mailto:oksman.dav@gmail.com>> wrote:

Added the ability to specify IF EXISTS when renaming a column of an object (table, view, etc.).
For example: ALTER TABLE distributors RENAME COLUMN IF EXISTS address TO city;
If the column does not exist, a notice is issued instead of throwing an error.

What is the intended use-case for RENAME COLUMN IF EXISTS? I'm struggling to
see when that would be helpful to users but I might not be imaginative enough.

Same reasoning as for all the other if exists we have, idempotence. Being able to run the command on an object that is already in the desired state without provoking an error.

If the object is known to be in the desired state, there is no need to use IF
EXISTS. Personally I think IF EXISTS commands are useful when they provide a
transition to a known end state, but in this case it's an unknown end state.

--
Daniel Gustafsson https://vmware.com/

On Fri, 5 Nov 2021 at 05:21, Daniel Gustafsson <daniel@yesql.se> wrote:

Same reasoning as for all the other if exists we have, idempotence.

Being able to run the command on an object that is already in the desired
state without provoking an error.

If the object is known to be in the desired state, there is no need to use
IF
EXISTS. Personally I think IF EXISTS commands are useful when they
provide a
transition to a known end state, but in this case it's an unknown end
state.

The whole point of IF EXISTS, not to mention IF NOT EXISTS and OR REPLACE,
is that the same script can run without error on a variety of existing
schemas. They aren't (primarily) for typing directly at the psql prompt. At
the time the script is written, the state of the object when the script is
run is unknown.

On 5 Nov 2021, at 13:03, Isaac Morland <isaac.morland@gmail.com> wrote:

On Fri, 5 Nov 2021 at 05:21, Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> wrote:

Same reasoning as for all the other if exists we have, idempotence. Being able to run the command on an object that is already in the desired state without provoking an error.

If the object is known to be in the desired state, there is no need to use IF
EXISTS. Personally I think IF EXISTS commands are useful when they provide a
transition to a known end state, but in this case it's an unknown end state.

The whole point of IF EXISTS, not to mention IF NOT EXISTS and OR REPLACE, is that the same script can run without error on a variety of existing schemas. They aren't (primarily) for typing directly at the psql prompt. At the time the script is written, the state of the object when the script is run is unknown.

I know that, I'm just not convinced that it's a feature (in the case at hand).

--
Daniel Gustafsson https://vmware.com/

On Friday, November 5, 2021, Daniel Gustafsson <daniel@yesql.se> wrote:

I know that, I'm just not convinced that it's a feature (in the case at
hand)

I don’t see how this one should be expected to meet a higher bar than drop
table or other existing commands. I get why in the nearby discussion
create role if not exists is treated differently based upon its unique
security concerns. Does column renaming have a hidden concern I’m not
thinking of?

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:

On Friday, November 5, 2021, Daniel Gustafsson <daniel@yesql.se> wrote:

I know that, I'm just not convinced that it's a feature (in the case at
hand)

I don’t see how this one should be expected to meet a higher bar than drop
table or other existing commands. I get why in the nearby discussion
create role if not exists is treated differently based upon its unique
security concerns. Does column renaming have a hidden concern I’m not
thinking of?

IMV, the best forms of these options are the ones that produce a known
end state of the object. DROP IF EXISTS meets that test: upon successful
completion, the object doesn't exist. CREATE OR REPLACE meets that test:
upon successful completion, the object exists and has exactly the
properties stated in the command. CREATE IF NOT EXISTS fails that test,
because while you know that the object will exist afterwards, you've got
next to no information about its properties. We've put up with C.I.N.E.
semantics in some limited cases like CREATE TABLE, where C.O.R.
semantics would be too drastic; but IMO it's generally best avoided.

In this framework, RENAME IF EXISTS is in sort of a weird place.
You'd know that afterwards there is no longer any column with the
source name. But you are not entitled to draw any conclusions
about whether a column exists with the target name, nor what its
properties are. So that makes me feel like the semantics are more
on the poorly-defined than well-defined side of the fence.

I'd be more willing to overlook that if a clear use-case had been
given, but AFAICS no concrete case has been offered.

regards, tom lane

On Friday, November 5, 2021, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'd be more willing to overlook that if a clear use-case had been
given, but AFAICS no concrete case has been offered.

The use case is the exact same one for all of these - indempotence,
especially in the face of being able to run migration scripts starting at a
point in the past and still having the final outcome be the same (or error
if there is a fundamental conflict in the history). It’s the same argument
used for why our current implementation of create [type] if not exists is
broken in how it deals with schemas.

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:

On Friday, November 5, 2021, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'd be more willing to overlook that if a clear use-case had been
given, but AFAICS no concrete case has been offered.

The use case is the exact same one for all of these - indempotence,

... except that, as I explained, it's NOT really idempotent.
It's a sort of half-baked idempotence, which is exactly the kind
of underspecification you complain about in your next sentence.
Do we really want to go there?

The perspective I'm coming from is that it's not terribly hard
to write whatever sort of conditional DDL you want using plpgsql
DO blocks, so it's not like we lack the capability. I think we
should only provide pre-fab conditional DDL for the most basic,
solidly-defined cases; and it seems to me that RENAME IF EXISTS
isn't solid enough.

regards, tom lane

On Fri, Nov 5, 2021 at 10:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

In this framework, RENAME IF EXISTS is in sort of a weird place.
You'd know that afterwards there is no longer any column with the
source name. But you are not entitled to draw any conclusions
about whether a column exists with the target name, nor what its
properties are. So that makes me feel like the semantics are more
on the poorly-defined than well-defined side of the fence.

I have mixed feelings about this proposal. As you may recall, I was a
big fan of CREATE IF NOT EXISTS because it's a super-useful thing in
DDL upgrade scripts. You have an application and every time you deploy
it you CREATE IF NOT EXISTS all the tables that are supposed to be
there. As long as the application is careful not to modify any table
definitions, and nothing else is changing the database, this works
great. You can add functionality by adding new tables, so the schema
can be upgraded before the app. Life is good.

Making renaming work in the same kind of context is harder. You're
definitely going to have to upgrade the application and the schema in
lock step, unless the application is smart enough to work with the
column having either name. You're also going to end up with some
trouble if you ever reuse a column name, because then the next time
you run the script it might rename the successor of the original
column by that name rather than the column you intended to rename. So
it seems more finnicky to use.

But I'm also not sure that it's useless. People don't usually ask for
a feature unless they have a use in mind for it. Also, adding an
option to skip failures when the object is missing is a popular kind
of thing. The backend is full of functions that have a missingOK or
noError flag, for example. Maybe the fact that I don't quite see how
to use this effectively is just lack of imagination on my part....

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Nov 5, 2021 at 8:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

"David G. Johnston" <david.g.johnston@gmail.com> writes:

On Friday, November 5, 2021, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'd be more willing to overlook that if a clear use-case had been
given, but AFAICS no concrete case has been offered.

The use case is the exact same one for all of these - indempotence,

... except that, as I explained, it's NOT really idempotent.
It's a sort of half-baked idempotence, which is exactly the kind
of underspecification you complain about in your next sentence.
Do we really want to go there?

It may not be self-contained idempotence but so long as the user is using
the command in the expected manner the end result will appear that way.

I disagree with the premise that we have to meet the known end state
requirement. In the imagined use case either, but not both, the original
column or the result column are going to exist. A RIE will behave as
expected and desired in that case. If someone executes RIE in a case where
neither column exists the end result is no error and neither column still
exists. This is exactly what the command RIE promises will happen in that
case. It works reliably and as one would expect and from there it is up to
the user, not us, to decide when it is appropriate to use or not.

The perspective I'm coming from is that it's not terribly hard

to write whatever sort of conditional DDL you want using plpgsql
DO blocks, so it's not like we lack the capability. I think we
should only provide pre-fab conditional DDL for the most basic,
solidly-defined cases; and it seems to me that RENAME IF EXISTS
isn't solid enough.

IOW, it doesn't actually matter what the use case is. And the definition
of solid basically precludes anything except CREATE and DROP commands
(including the create version written ALTER TABLE IF EXISTS <do something>).

If this is indeed the agreed upon standard for this kind of thing an FAQ or
documentation entry formalizing it would help, because lots of people are
just going to see that we meet this migration use case only partially and
will continue to request and even develop the missing pieces.

David J.

On Fri, Nov 5, 2021 at 8:37 AM Robert Haas <robertmhaas@gmail.com> wrote:

Making renaming work in the same kind of context is harder. You're
definitely going to have to upgrade the application and the schema in
lock step, unless the application is smart enough to work with the
column having either name. You're also going to end up with some
trouble if you ever reuse a column name, because then the next time
you run the script it might rename the successor of the original
column by that name rather than the column you intended to rename. So
it seems more finnicky to use.

This I understand fully, and am fine with leaving it to the user to
handle. They can choose whether rewriting the table (column add with
non-null values) in order to have an easier application migration is better
or worse than doing a rename and just ensuring that the old name is fully
retired (which seems likely).

It can be used profitably and that is good enough for me given that we've
already set a precedent of having IF EXISTS conditionals. That people need
to understand exactly what the command will do, and test their scripts when
using it, is a reasonable expectation. The possibility that some may not
and might have issues shouldn't prevent us from providing a useful feature
to others who will use it appropriately and with care.

David J.

Hi,

This patch has been around for about 10 months. There seems to be some support
for the feature but 3 committers raised concerns about the patch, and the OP
never answered, or clarified the intended use case until now.

At that point I don't see this patch getting committed at all so I'm marking it
as Rejected.