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
