Обсуждение: pointless check in RelationBuildPartitionDesc

I noticed this strange hack in RelationBuildPartitionDesc:

       /*
        * It is possible that the pg_class tuple of a partition has not been
        * updated yet to set its relpartbound field.  The only case where
        * this happens is when we open the parent relation to check using its
        * partition descriptor that a new partition's bound does not overlap
        * some existing partition.
        */
       if (!((Form_pg_class) GETSTRUCT(tuple))->relispartition)
       {
           ReleaseSysCache(tuple);
           continue;
       }

After looking, it seems that this is just self-inflicted pain: for some
reason, we store the pg_inherits row for a partition, and immediately
afterwards compute and store its partition bound, which requires the
above hack.  But if we do things in the opposite order, this is no
longer needed.  I propose to remove it, as in the attached patch.

-- 
Álvaro Herrera                         Developer, https://www.PostgreSQL.org/

On 2018/09/04 6:39, Alvaro Herrera wrote:
> I noticed this strange hack in RelationBuildPartitionDesc:
> 
>        /*
>         * It is possible that the pg_class tuple of a partition has not been
>         * updated yet to set its relpartbound field.  The only case where
>         * this happens is when we open the parent relation to check using its
>         * partition descriptor that a new partition's bound does not overlap
>         * some existing partition.
>         */
>        if (!((Form_pg_class) GETSTRUCT(tuple))->relispartition)
>        {
>            ReleaseSysCache(tuple);
>            continue;
>        }
> 
> After looking, it seems that this is just self-inflicted pain: for some
> reason, we store the pg_inherits row for a partition, and immediately
> afterwards compute and store its partition bound, which requires the
> above hack.  But if we do things in the opposite order, this is no
> longer needed.  I propose to remove it, as in the attached patch.

+1.  I remember having facepalmed at this before and had also written a
patch but never got around to submitting it.

Thanks,
Amit

On Tue, Sep 04, 2018 at 09:47:07AM +0900, Amit Langote wrote:
> On 2018/09/04 6:39, Alvaro Herrera wrote:
>> After looking, it seems that this is just self-inflicted pain: for some
>> reason, we store the pg_inherits row for a partition, and immediately
>> afterwards compute and store its partition bound, which requires the
>> above hack.  But if we do things in the opposite order, this is no
>> longer needed.  I propose to remove it, as in the attached patch.
>
> +1.  I remember having facepalmed at this before and had also written a
> patch but never got around to submitting it.

Ok, I see.  It seems to me that this could be replaced by an
elog(ERROR), as relispartition ought to be set anyway.  This way any
future callers would get things done in the correct order.
--
Michael

On 2018/09/04 10:19, Michael Paquier wrote:
> On Tue, Sep 04, 2018 at 09:47:07AM +0900, Amit Langote wrote:
>> On 2018/09/04 6:39, Alvaro Herrera wrote:
>>> After looking, it seems that this is just self-inflicted pain: for some
>>> reason, we store the pg_inherits row for a partition, and immediately
>>> afterwards compute and store its partition bound, which requires the
>>> above hack.  But if we do things in the opposite order, this is no
>>> longer needed.  I propose to remove it, as in the attached patch.
>>
>> +1.  I remember having facepalmed at this before and had also written a
>> patch but never got around to submitting it.
> 
> Ok, I see.  It seems to me that this could be replaced by an
> elog(ERROR), as relispartition ought to be set anyway.  This way any
> future callers would get things done in the correct order.

Converting it to elog(ERROR, ...) might be a good idea.

Thanks,
Amit

On 2018-Sep-04, Amit Langote wrote:

> On 2018/09/04 10:19, Michael Paquier wrote:
> > On Tue, Sep 04, 2018 at 09:47:07AM +0900, Amit Langote wrote:
> >> On 2018/09/04 6:39, Alvaro Herrera wrote:
> >>> After looking, it seems that this is just self-inflicted pain: for some
> >>> reason, we store the pg_inherits row for a partition, and immediately
> >>> afterwards compute and store its partition bound, which requires the
> >>> above hack.  But if we do things in the opposite order, this is no
> >>> longer needed.  I propose to remove it, as in the attached patch.
> >>
> >> +1.  I remember having facepalmed at this before and had also written a
> >> patch but never got around to submitting it.
> > 
> > Ok, I see.  It seems to me that this could be replaced by an
> > elog(ERROR), as relispartition ought to be set anyway.  This way any
> > future callers would get things done in the correct order.
> 
> Converting it to elog(ERROR, ...) might be a good idea.

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.

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

On 2018/09/04 13:08, Alvaro Herrera wrote:
> On 2018-Sep-04, Amit Langote wrote:
> 
>> On 2018/09/04 10:19, Michael Paquier wrote:
>>> On Tue, Sep 04, 2018 at 09:47:07AM +0900, Amit Langote wrote:
>>>> On 2018/09/04 6:39, Alvaro Herrera wrote:
>>>>> After looking, it seems that this is just self-inflicted pain: for some
>>>>> reason, we store the pg_inherits row for a partition, and immediately
>>>>> afterwards compute and store its partition bound, which requires the
>>>>> above hack.  But if we do things in the opposite order, this is no
>>>>> longer needed.  I propose to remove it, as in the attached patch.
>>>>
>>>> +1.  I remember having facepalmed at this before and had also written a
>>>> patch but never got around to submitting it.
>>>
>>> Ok, I see.  It seems to me that this could be replaced by an
>>> elog(ERROR), as relispartition ought to be set anyway.  This way any
>>> future callers would get things done in the correct order.
>>
>> Converting it to elog(ERROR, ...) might be a good idea.
> 
> 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).  I think we should check relispartition then too.

Thanks,
Amit

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?

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.

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

Proposed patch.  Checking isnull in a elog(ERROR) is important, because
the column is not marked NOT NULL.  This is not true for other columns
where we simply do Assert(!isnull).

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

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

On 2018/09/05 1:50, Alvaro Herrera wrote:
> Proposed patch.  Checking isnull in a elog(ERROR) is important, because
> the column is not marked NOT NULL.  This is not true for other columns
> where we simply do Assert(!isnull).

Looks good.  Thanks for taking care of other sites as well.

@@ -14705,7 +14705,9 @@ ATExecDetachPartition(Relation rel, RangeVar *name)

     (void) SysCacheGetAttr(RELOID, tuple, Anum_pg_class_relpartbound,
                            &isnull);
-    Assert(!isnull);
+    if (isnull)
+        elog(ERROR, "null relpartbound for relation %u",
+             RelationGetRelid(partRel));

In retrospect, I'm not sure why this piece of code is here at all; maybe
just remove the SycCacheGetAttr and Assert?

Regards,
Amit

On 2018-Sep-05, Amit Langote wrote:

> On 2018/09/05 1:50, Alvaro Herrera wrote:
> > Proposed patch.  Checking isnull in a elog(ERROR) is important, because
> > the column is not marked NOT NULL.  This is not true for other columns
> > where we simply do Assert(!isnull).
> 
> Looks good.  Thanks for taking care of other sites as well.
> 
> @@ -14705,7 +14705,9 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
> 
>      (void) SysCacheGetAttr(RELOID, tuple, Anum_pg_class_relpartbound,
>                             &isnull);
> -    Assert(!isnull);
> +    if (isnull)
> +        elog(ERROR, "null relpartbound for relation %u",
> +             RelationGetRelid(partRel));
> 
> In retrospect, I'm not sure why this piece of code is here at all; maybe
> just remove the SycCacheGetAttr and Assert?

Yeah, good idea, will do.

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