Horton

Thread

Commits

  1. Ensure relcache consistency around generated columns

  1. relcache sometimes initially ignores has_generated_stored

    Andres Freund <andres@anarazel.de> — 2020-01-15T18:11:05Z

    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.

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?

Greetings,

Andres Freund

  2. Re: relcache sometimes initially ignores has_generated_stored

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-01-16T06:10:36Z

    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

  3. Re: relcache sometimes initially ignores has_generated_stored

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-02-06T20:29:58Z

    On 2020-01-15 19:11, Andres Freund wrote:
> 	/*
> 	 * Set up constraint/default info
> 	 */
> 	if (has_not_null || ndef > 0 ||
> 		attrmiss || relation->rd_rel->relchecks)
> test, i.e. there are no defaults.

> 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?

Fixed that way.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  4. Re: relcache sometimes initially ignores has_generated_stored

    Andres Freund <andres@anarazel.de> — 2020-02-06T20:40:22Z

    On 2020-02-06 21:29:58 +0100, Peter Eisentraut wrote:
> On 2020-01-15 19:11, Andres Freund wrote:
> > 	/*
> > 	 * Set up constraint/default info
> > 	 */
> > 	if (has_not_null || ndef > 0 ||
> > 		attrmiss || relation->rd_rel->relchecks)
> > test, i.e. there are no defaults.
> 
> > 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?
> 
> Fixed that way.

Thanks.