At Wed, 15 Jan 2020 10:11:05 -0800, Andres Freund <and...@anarazel.de> wrote in > Hi, > > I think it's probably not relevant, but it confused me for a moment > that RelationBuildTupleDesc() might set constr->has_generated_stored to > true, but then throw away the constraint at the end, because nothing > matches the > /* > * Set up constraint/default info > */ > if (has_not_null || ndef > 0 || > attrmiss || relation->rd_rel->relchecks) > test, i.e. there are no defaults.
It was as follows before 16828d5c02. - if (constr->has_not_null || ndef > 0 ||relation->rd_rel->relchecks) At that time TupleConstr has only members defval, check and has_not_null other than subsidiary members. The condition apparently checked all of the members. Then the commit adds attrmiss to the condition since the corresponding member to TupleConstr. + if (constr->has_not_null || ndef > 0 || + attrmiss || relation->rd_rel->relchecks) Later fc22b6623b introduced has_generated_stored to TupleConstr but didn't add the corresponding check. > A quick assert confirms we do indeed pfree() constr in cases where > has_generated_stored == true. > > I suspect that's just an intermediate catalog, however, e.g. when > DefineRelation() does > heap_create_with_catalog(); > CommandCounterIncrement(); > relation_open(); > AddRelationNewConstraints(). > > It does still strike me as not great that we can get a different > relcache entry, even if transient, depending on whether there are other > reasons to create a TupleConstr. Say a NOT NULL column. > > I'm inclined to think we should just also check has_generated_stored in > the if quoted above? I agree to that. We could have a local boolean "has_any_constraint" to merge them but it would be an overkill. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
