Thread: [PATCH] Skip ALTER x SET SCHEMA if the schema didn't change

Hi list

The attached patch changes the behavior of multiple ALTER x SET SCHEMA
commands, to skip, rather than fail, when the old and new schema is
the same.

The advantage is that it's now easier to write DDL scripts that are indempotent.

This already matches the behavior of ALTER EXTENSION SET SCHEMA in
earlier versions, as well as many other SET-ish commands, e.g. ALTER
TABLE SET TABLESPACE, OWNER TO, CLUSTER ON, SET (storage_parameter...)
etc. I don't see why SET SCHEMA should be treated any differently.

The code is written such that object_access_hook is still called for
each object.

Regression tests included. I couldn't find any documentation that
needs changing.

Regards,
Marti

On Tue, Sep 29, 2015 at 12:17 AM, Marti Raudsepp <marti@juffo.org> wrote:
> Hi list
>
> The attached patch changes the behavior of multiple ALTER x SET SCHEMA
> commands, to skip, rather than fail, when the old and new schema is
> the same.
>
> The advantage is that it's now easier to write DDL scripts that are indempotent.
>
> This already matches the behavior of ALTER EXTENSION SET SCHEMA in
> earlier versions, as well as many other SET-ish commands, e.g. ALTER
> TABLE SET TABLESPACE, OWNER TO, CLUSTER ON, SET (storage_parameter...)
> etc. I don't see why SET SCHEMA should be treated any differently.
>
> The code is written such that object_access_hook is still called for
> each object.
>
> Regression tests included. I couldn't find any documentation that
> needs changing.

I went through the patch, following are my observations,

Patch applied with hunks and compiled with out warnings.
Basic tests are passed.

In AlterTableNamespaceInternal function, if a table or matview called
for set schema,
If the object contains any constraints, the constraint gets updated
with new schema.

In AlterTypeNamespaceInternal function, the InvokeObjectPostAlterHook function
doesn't get called if the type is of composite type, domain and array
types as because
it just returns from top of the function.


Regards,
Hari Babu
Fujitsu Australia

On Thu, Nov 5, 2015 at 6:20 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> I went through the patch, following are my observations,
>
> Patch applied with hunks and compiled with out warnings.
> Basic tests are passed.

I'm interested in hearing opinions from multiple people about the
following two questions:

1. Is the new behavior better than the old behavior?
2. Will breaking backward compatibility make too many people unhappy?

My guess is that the answer to the first question is "yes" and that
the answer to the second one is "no", but this is clearly a
significant incompatibility, so I'd like to hear some more opinions
before concluding that we definitely want to do this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

David G. Johnston wrote:
> On Thu, Nov 5, 2015 at 2:15 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> > I'm interested in hearing opinions from multiple people about the
> > following two questions:
> >
> > 1. Is the new behavior better than the old behavior?
> > 2. Will breaking backward compatibility make too many people unhappy?
> >
> > My guess is that the answer to the first question is "yes" and that
> > the answer to the second one is "no", but this is clearly a
> > significant incompatibility, so I'd like to hear some more opinions
> > before concluding that we definitely want to do this.
> 
> For #2 I'm not that concerned about turning an error case into a non-error.

Yeah, maybe I lack imagination but I don't see how the current behavior
is actually useful.  We already silently do nothing when appropriate in
many other DDL commands.

> The rationale behind #1 makes sense to me.  Given all the recent work on
> "IF NOT EXISTS" we obviously think that this general behavior is desirable
> and we should correct this deviation from that norm.

Agreed.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Nov 5, 2015 at 10:20 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Tue, Sep 29, 2015 at 12:17 AM, Marti Raudsepp <marti@juffo.org> wrote:
>> Hi list
>>
>> The attached patch changes the behavior of multiple ALTER x SET SCHEMA
>> commands, to skip, rather than fail, when the old and new schema is
>> the same.
>>
>> The advantage is that it's now easier to write DDL scripts that are indempotent.
>>
>> This already matches the behavior of ALTER EXTENSION SET SCHEMA in
>> earlier versions, as well as many other SET-ish commands, e.g. ALTER
>> TABLE SET TABLESPACE, OWNER TO, CLUSTER ON, SET (storage_parameter...)
>> etc. I don't see why SET SCHEMA should be treated any differently.
>>
>> The code is written such that object_access_hook is still called for
>> each object.
>>
>> Regression tests included. I couldn't find any documentation that
>> needs changing.
>
> I went through the patch, following are my observations,
>
> Patch applied with hunks and compiled with out warnings.
> Basic tests are passed.
>
> In AlterTableNamespaceInternal function, if a table or matview called
> for set schema,
> If the object contains any constraints, the constraint gets updated
> with new schema.
>
> In AlterTypeNamespaceInternal function, the InvokeObjectPostAlterHook function
> doesn't get called if the type is of composite type, domain and array
> types as because
> it just returns from top of the function.

Most of the community members didn't find any problem in changing the
behavior, so here I attached updated patch with the above two corrections.

Regards,
Hari Babu
Fujitsu Australia

On Mon, Nov 16, 2015 at 4:27 AM, Marti Raudsepp <marti@juffo.org> wrote:
> Thank you so much for the review and patch update. I should have done that
> myself, but I've been really busy for the last few weeks. :(

Maybe I'm having an attack of the stupids today, but it looks to me
like the changes to pg_constraint.c look awfully strange to me.  In
the old code, if object_address_present() returns true, we continue,
skipping the rest of the loop.  In the new code, we instead set
alreadyChanged to true.  That causes both of the following if
statements, as revised, to fall out, so that we skip the rest of the
loop.  Huh?  Wouldn't a one line change to add oldNspId != newNspId to
the criteria for a simple_heap_update be just as good?

Backing up a bit, maybe we should be a bit more vigorous in treating a
same-namespace move as a no-op.  That is, don't worry about calling
the post-alter hook in that case - just have AlterConstraintNamespaces
start by checking whether oldNspId == newNspid right at the top; if
so, return.  The patch seems to have the idea that it is important to
call the post-alter hook even in that case, but I'm not sure whether
that's true.  I'm not sure it's false, but I'm also not sure it's
true.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 18, 2015 at 6:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Nov 16, 2015 at 4:27 AM, Marti Raudsepp <marti@juffo.org> wrote:
>> Thank you so much for the review and patch update. I should have done that
>> myself, but I've been really busy for the last few weeks. :(
>
> Maybe I'm having an attack of the stupids today, but it looks to me
> like the changes to pg_constraint.c look awfully strange to me.  In
> the old code, if object_address_present() returns true, we continue,
> skipping the rest of the loop.  In the new code, we instead set
> alreadyChanged to true.  That causes both of the following if
> statements, as revised, to fall out, so that we skip the rest of the
> loop.  Huh?  Wouldn't a one line change to add oldNspId != newNspId to
> the criteria for a simple_heap_update be just as good?

Yes, that's correct, the above change can be written as you suggested.
Updated patch attached with correction.

> Backing up a bit, maybe we should be a bit more vigorous in treating a
> same-namespace move as a no-op.  That is, don't worry about calling
> the post-alter hook in that case - just have AlterConstraintNamespaces
> start by checking whether oldNspId == newNspid right at the top; if
> so, return.  The patch seems to have the idea that it is important to
> call the post-alter hook even in that case, but I'm not sure whether
> that's true.  I'm not sure it's false, but I'm also not sure it's
> true.

I am also not sure whether calling the post-alter hook in case of constraint is
necessarily required? but it was doing for other objects, so I suggested
that way.

Regards,
Hari Babu
Fujitsu Australia

On Wed, Nov 18, 2015 at 12:32 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Wed, Nov 18, 2015 at 6:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Mon, Nov 16, 2015 at 4:27 AM, Marti Raudsepp <marti@juffo.org> wrote:
>>> Thank you so much for the review and patch update. I should have done that
>>> myself, but I've been really busy for the last few weeks. :(
>>
>> Maybe I'm having an attack of the stupids today, but it looks to me
>> like the changes to pg_constraint.c look awfully strange to me.  In
>> the old code, if object_address_present() returns true, we continue,
>> skipping the rest of the loop.  In the new code, we instead set
>> alreadyChanged to true.  That causes both of the following if
>> statements, as revised, to fall out, so that we skip the rest of the
>> loop.  Huh?  Wouldn't a one line change to add oldNspId != newNspId to
>> the criteria for a simple_heap_update be just as good?
>
> Yes, that's correct, the above change can be written as you suggested.
> Updated patch attached with correction.
>
>> Backing up a bit, maybe we should be a bit more vigorous in treating a
>> same-namespace move as a no-op.  That is, don't worry about calling
>> the post-alter hook in that case - just have AlterConstraintNamespaces
>> start by checking whether oldNspId == newNspid right at the top; if
>> so, return.  The patch seems to have the idea that it is important to
>> call the post-alter hook even in that case, but I'm not sure whether
>> that's true.  I'm not sure it's false, but I'm also not sure it's
>> true.
>
> I am also not sure whether calling the post-alter hook in case of constraint is
> necessarily required? but it was doing for other objects, so I suggested
> that way.

OK, committed with some additional cosmetic improvements.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company