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
