Обсуждение: [PATCH] rename column if exists
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.
Вложения
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 =
falseto 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 isalready 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 isalready 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 erroron a variety of existing schemas. They aren't (primarily) for typing directly at the psql prompt. At the time the scriptis 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.