Re: pointless check in RelationBuildPartitionDesc

On 2018/09/04 21:51, Alvaro Herrera wrote:
> On 2018-Sep-04, Amit Langote wrote:
> 
>> On 2018/09/04 13:08, Alvaro Herrera wrote:
> 
>>> I think it'd be pointless noise.  If we really want to protect against
>>> that, I think we should promote the Assert for relpartbound's isnull
>>> flag into an if test.
>>
>> So that we can elog(ERROR, ...) if isnull is true?  If so, I agree,
>> because that's what should've been done here anyway (for a value read from
>> the disk that is).
> 
> Yes.
> 
> I wonder now what's the value of using an Assert for this, BTW: if those
> are enabled, then we'll crash saying "this column is null and shouldn't!".
> If they are disabled, we'll instead crash (possibly in production)
> trying to decode the field.  What was achieved?

I can see how that the Assert was pointless all along.

> With an elog(ERROR) the reaction is the expected one: the current
> transaction is aborted, but the server continues to run.
> 
>> I think we should check relispartition then too.
> 
> Well, we don't check every single catalog flag in every possible code
> path just to ensure it's sane.  I don't see any reason to do differently
> here.  We rely heavily on the catalog contents to be correct, and
> install defenses against user-induced corruption that would cause us to
> crash; but going much further than that would be excessive IMO.

Okay, sure anyway.

Thanks,
Amit