Thread: [Report Bug With Patch] Rel Cache Bug
Hi,
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);
Attachment
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 >