At Wed, 15 Jan 2020 10:11:05 -0800, Andres Freund <andres@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
