Thread: [Report Bug With Patch] Rel Cache Bug

Hi,

On 2015-03-25 21:59:17 +0800, Jaimin Pan wrote:
> I think there is a bug in rel cache rebuild of release 9.4.1.
> it also exists on some other release.
>
> the issue was introduced by 491dd4a97daa6b4de9ee8401ada10ad5da76af46
>
> -- a/src/backend/utils/cache/relcache.c
> +++ b/src/backend/utils/cache/relcache.c
> @@ -2166,9 +2166,9 @@ RelationClearRelation(Relation relation, bool rebuild)
>                 /* ... but actually, we don't have to update newrel->rd_rel
> */
>                 memcpy(relation->rd_rel, newrel->rd_rel, CLASS_TUPLE_SIZE);
>                 /* preserve old tupledesc and rules if no logical change */
> -               if (keep_tupdesc)
> +               if (!keep_tupdesc)
>                         SWAPFIELD(TupleDesc, rd_att);
> -               if (keep_rules)
> +               if (!keep_rules)
>                 {
>                         SWAPFIELD(RuleLock *, rd_rules);
>                         SWAPFIELD(MemoryContext, rd_rulescxt);

> diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
> index f6520a0..733860c 100644
> --- a/src/backend/utils/cache/relcache.c
> +++ b/src/backend/utils/cache/relcache.c
> @@ -2166,9 +2166,9 @@ RelationClearRelation(Relation relation, bool rebuild)
>          /* ... but actually, we don't have to update newrel->rd_rel */
>          memcpy(relation->rd_rel, newrel->rd_rel, CLASS_TUPLE_SIZE);
>          /* preserve old tupledesc and rules if no logical change */
> -        if (keep_tupdesc)
> +        if (!keep_tupdesc)
>              SWAPFIELD(TupleDesc, rd_att);
> -        if (keep_rules)
> +        if (!keep_rules)
>          {
>              SWAPFIELD(RuleLock *, rd_rules);
>              SWAPFIELD(MemoryContext, rd_rulescxt);

Why do you think this is broken? While certainly not the prettiest code
around it looks ok to me. What it does is to keep the other entries data
around. Which we want if it's possibly referenced.

Greetings,

Andres Freund

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Jaimin Pan <jaimin.pan@gmail.com> writes:
> I think there is a bug in rel cache rebuild of release 9.4.1.
> it also exists on some other release.

> the issue was introduced by 491dd4a97daa6b4de9ee8401ada10ad5da76af46

> -- a/src/backend/utils/cache/relcache.c
> +++ b/src/backend/utils/cache/relcache.c
> @@ -2166,9 +2166,9 @@ RelationClearRelation(Relation relation, bool rebuild)
>                 /* ... but actually, we don't have to update newrel->rd_rel
> */
>                 memcpy(relation->rd_rel, newrel->rd_rel, CLASS_TUPLE_SIZE);
>                 /* preserve old tupledesc and rules if no logical change */
> -               if (keep_tupdesc)
> +               if (!keep_tupdesc)
>                         SWAPFIELD(TupleDesc, rd_att);
> -               if (keep_rules)
> +               if (!keep_rules)
>                 {
>                         SWAPFIELD(RuleLock *, rd_rules);
>                         SWAPFIELD(MemoryContext, rd_rulescxt);


You would need to provide a fairly convincing argument why you think that
five-year-old code is backwards.  Asserting there's a problem with
absolutely zero evidence is not likely to impress anyone.

(For the record, it did and does still look right to me.  As per the
comment about thirty lines up, what we're doing is un-swapping the
fields we don't want to change.  If keep_tupdesc is true, we don't
want to change rd_att, so we need to swap its old value back.
Likewise for the other thing.)

            regards, tom lane

Thanks you for your replay.

I realized that i am wrong. I just miss the whole struct swap at the
beginning.
I am so sorry for bothering you with my fault.

2015-03-26 1:38 GMT+08:00 Tom Lane <tgl@sss.pgh.pa.us>:

> Jaimin Pan <jaimin.pan@gmail.com> writes:
> > I think there is a bug in rel cache rebuild of release 9.4.1.
> > it also exists on some other release.
>
> > the issue was introduced by 491dd4a97daa6b4de9ee8401ada10ad5da76af46
>
> > -- a/src/backend/utils/cache/relcache.c
> > +++ b/src/backend/utils/cache/relcache.c
> > @@ -2166,9 +2166,9 @@ RelationClearRelation(Relation relation, bool
> rebuild)
> >                 /* ... but actually, we don't have to update
> newrel->rd_rel
> > */
> >                 memcpy(relation->rd_rel, newrel->rd_rel,
> CLASS_TUPLE_SIZE);
> >                 /* preserve old tupledesc and rules if no logical change
> */
> > -               if (keep_tupdesc)
> > +               if (!keep_tupdesc)
> >                         SWAPFIELD(TupleDesc, rd_att);
> > -               if (keep_rules)
> > +               if (!keep_rules)
> >                 {
> >                         SWAPFIELD(RuleLock *, rd_rules);
> >                         SWAPFIELD(MemoryContext, rd_rulescxt);
>
>
> You would need to provide a fairly convincing argument why you think that
> five-year-old code is backwards.  Asserting there's a problem with
> absolutely zero evidence is not likely to impress anyone.
>
> (For the record, it did and does still look right to me.  As per the
> comment about thirty lines up, what we're doing is un-swapping the
> fields we don't want to change.  If keep_tupdesc is true, we don't
> want to change rd_att, so we need to swap its old value back.
> Likewise for the other thing.)
>
>                         regards, tom lane
>