On Fri, Aug 4, 2017 at 5:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> So I think that the attached patch is able to do the legwork.
>
> I've pushed this into HEAD. It seems like enough of a behavioral
> change that we wouldn't want to back-patch, but IMO it's not too late
> to be making this type of change in v10. If we wait for the next CF
> then it will take another year for the fix to reach the field.
Thanks for applying the fix. My intention when adding that in a CF is
not to see things lost.
>> While
>> looking at the code, I have bumped into index_check_primary_key() that
>> discarded the case of sending SET NOT NULL to child tables, as
>> introduced by 88452d5b. But that's clearly an oversight IMO, and the
>> comment is wrong to begin with because SET NOT NULL is spread to child
>> tables. Using is_alter_table instead of a plain true in
>> index_check_primary_key() for the call of AlterTableInternal() is
>> defensive, but I don't think that we want to impact any modules
>> relying on this API, so recursing only when ALTER TABLE is used is the
>> safest course of action to me.
>
> I didn't find that persuasive: I think passing "recurse" as just plain
> "true" is safer and more readable. We shouldn't be able to get there
> in a CREATE case, as per the comments; and if we did, there shouldn't
> be any child tables anyway; but if somehow there were, surely the same
> consistency argument would imply that we *must* recurse and fix those
> child tables. So I just made it "true".
Yeah, that matches what I read as well. No complains to switch to a
plain "true" even if it is not reached yet.
--
Michael
