Horton

Thread

Commits

  1. Flush relcache entries when their FKs are meddled with

  2. Restore foreign-key-aware estimation of join relation sizes.

  1. should ConstraintRelationId ins/upd cause relcache invals?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-21T19:27:50Z

    Hello

While working on bugfixes for FK problems in partitioned tables, I came
across some behavior that appears to stem from our inclusion of foreign
keys in relcache, without sufficient care for invalidating the relcache
entries when the foreign key set for the table changes.  (Namely, a
partition retains its relcache entry with no FKs when an FK is added to
the parent table, leading a DELETE to skip running action triggers).

At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted
a simplistic for the specific problem I found by calling
CacheInvalidateRelcache in the problem spot.  But I'm wondering if the
correct fix isn't to have CacheInvalidateHeapTuple deal with FK
pg_constraint tuples instead, per the attached patch.  Why does this not
lead to stale cache problems elsewhere?

FKs were added to relcache entries by commit 100340e2dcd0 ("Restore
foreign-key-aware estimation of join relation sizes"), so CCing Tom and
Tomas.

-- 
Álvaro Herrera                PostgreSQL Expert, https://www.2ndQuadrant.com/

  2. Re: should ConstraintRelationId ins/upd cause relcache invals?

    Andres Freund <andres@anarazel.de> — 2019-01-21T19:33:00Z

    Hi,

On 2019-01-21 16:27:50 -0300, Alvaro Herrera wrote:
> While working on bugfixes for FK problems in partitioned tables, I came
> across some behavior that appears to stem from our inclusion of foreign
> keys in relcache, without sufficient care for invalidating the relcache
> entries when the foreign key set for the table changes.  (Namely, a
> partition retains its relcache entry with no FKs when an FK is added to
> the parent table, leading a DELETE to skip running action triggers).
> 
> At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted
> a simplistic for the specific problem I found by calling
> CacheInvalidateRelcache in the problem spot.  But I'm wondering if the
> correct fix isn't to have CacheInvalidateHeapTuple deal with FK
> pg_constraint tuples instead, per the attached patch.  Why does this not
> lead to stale cache problems elsewhere?
> 
> FKs were added to relcache entries by commit 100340e2dcd0 ("Restore
> foreign-key-aware estimation of join relation sizes"), so CCing Tom and
> Tomas.

I wondered about the same in https://www.postgresql.org/message-id/20180628150209.n2qch5jtn3vt2xaa%40alap3.anarazel.de
, just about pg_index, but people didn't like it much.

Greetings,

Andres Freund

  3. Re: should ConstraintRelationId ins/upd cause relcache invals?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-21T20:08:17Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> While working on bugfixes for FK problems in partitioned tables, I came
> across some behavior that appears to stem from our inclusion of foreign
> keys in relcache, without sufficient care for invalidating the relcache
> entries when the foreign key set for the table changes.  (Namely, a
> partition retains its relcache entry with no FKs when an FK is added to
> the parent table, leading a DELETE to skip running action triggers).

Ooops.

> At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted
> a simplistic for the specific problem I found by calling
> CacheInvalidateRelcache in the problem spot.  But I'm wondering if the
> correct fix isn't to have CacheInvalidateHeapTuple deal with FK
> pg_constraint tuples instead, per the attached patch.

+1, this is safer than expecting retail relcache inval calls to be
added in all the right places.

			regards, tom lane

  4. Re: should ConstraintRelationId ins/upd cause relcache invals?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-21T22:40:17Z

    On 2019-Jan-21, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> > At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted
> > a simplistic for the specific problem I found by calling
> > CacheInvalidateRelcache in the problem spot.  But I'm wondering if the
> > correct fix isn't to have CacheInvalidateHeapTuple deal with FK
> > pg_constraint tuples instead, per the attached patch.
> 
> +1, this is safer than expecting retail relcache inval calls to be
> added in all the right places.

Thanks, pushed.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  5. Re: should ConstraintRelationId ins/upd cause relcache invals?

    Andres Freund <andres@anarazel.de> — 2019-01-21T22:42:02Z

    Hi,

On 2019-01-21 19:40:17 -0300, Alvaro Herrera wrote:
> On 2019-Jan-21, Tom Lane wrote:
> 
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> 
> > > At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted
> > > a simplistic for the specific problem I found by calling
> > > CacheInvalidateRelcache in the problem spot.  But I'm wondering if the
> > > correct fix isn't to have CacheInvalidateHeapTuple deal with FK
> > > pg_constraint tuples instead, per the attached patch.
> > 
> > +1, this is safer than expecting retail relcache inval calls to be
> > added in all the right places.
> 
> Thanks, pushed.

Given https://www.postgresql.org/message-id/20190121193300.gknn7p4pmmjg7nqf%40alap3.anarazel.de
and the concerns voiced in the thread quoted therein, I'm a bit
surprised that you just went ahead with this, and backpatched it to boot.

Greetings,

Andres Freund

  6. Re: should ConstraintRelationId ins/upd cause relcache invals?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-21T23:14:31Z

    Andres Freund <andres@anarazel.de> writes:
> Given https://www.postgresql.org/message-id/20190121193300.gknn7p4pmmjg7nqf%40alap3.anarazel.de
> and the concerns voiced in the thread quoted therein, I'm a bit
> surprised that you just went ahead with this, and backpatched it to boot.

I don't think that's relevant.  The issues there were about whether
a pg_index row update ought to cause an invalidation of the relcache
entry for the index's table (not the one for the index, which it
already takes care of).  That seems very questionable to me --- the
potentially-invalidatable info ought to be in the index's relcache entry,
not its parent table's entry, IMO.  Here, however, it's clear which
relcache entry is dependent on those pg_constraint rows (as long as Alvaro
got it right about whether to inval conrelid or confrelid ...), and
that it is indeed so dependent.

			regards, tom lane

  7. Re: should ConstraintRelationId ins/upd cause relcache invals?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-21T23:25:46Z

    Hello

On 2019-Jan-21, Andres Freund wrote:

> On 2019-01-21 19:40:17 -0300, Alvaro Herrera wrote:
> > On 2019-Jan-21, Tom Lane wrote:
> > 
> > > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > 
> > > > At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted
> > > > a simplistic for the specific problem I found by calling
> > > > CacheInvalidateRelcache in the problem spot.  But I'm wondering if the
> > > > correct fix isn't to have CacheInvalidateHeapTuple deal with FK
> > > > pg_constraint tuples instead, per the attached patch.
> > > 
> > > +1, this is safer than expecting retail relcache inval calls to be
> > > added in all the right places.
> > 
> > Thanks, pushed.
> 
> Given https://www.postgresql.org/message-id/20190121193300.gknn7p4pmmjg7nqf%40alap3.anarazel.de
> and the concerns voiced in the thread quoted therein, I'm a bit
> surprised that you just went ahead with this, and backpatched it to boot.

Sigh.

I don't understand why the arguments about a different patch apply to
this one.  The invalidation I added is for pg_constraint, not pg_index.

Tom argues that pg_index can be updated for reasons other than
creation/deletion of the index, and that the relcache entry should not
be invalidated in those cases.  Maybe that's right, particularly given
that the relcache entry only holds a list of index OIDs, not properties
of each index.  For foreign keys the relcache entry keeps a lot more
than the OIDs; and pg_constraint rows are not updated very much anyway.

OTOH I lean towards your side of the argument in the other thread and I
don't quite understand why you gave up on it.  Tom didn't respond to
your last argumentat, and neither did you indicate that you were
convinced by Tom's argumentation.  My conclusion is that you disagreed
but decided not to push the issue any further, to get the thing done.
What I did here was the same: just get the thing done.

I could just as easily revert this commit and push a lone
CacheInvalidateRelcache where it is needed by the other fix I just
pushed, but that seemed to me the wrong thing to do.  Or I could spend a
few hours figuring out test cases that fail in 9.6 with the lack of
invalidation, but I don't have those hours and the bug isn't mine
anyway.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  8. Re: should ConstraintRelationId ins/upd cause relcache invals?

    Andres Freund <andres@anarazel.de> — 2019-01-21T23:33:26Z

    Hi,

On 2019-01-21 18:14:31 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Given https://www.postgresql.org/message-id/20190121193300.gknn7p4pmmjg7nqf%40alap3.anarazel.de
> > and the concerns voiced in the thread quoted therein, I'm a bit
> > surprised that you just went ahead with this, and backpatched it to boot.
> 
> I don't think that's relevant.  The issues there were about whether
> a pg_index row update ought to cause an invalidation of the relcache
> entry for the index's table (not the one for the index, which it
> already takes care of).  That seems very questionable to me --- the
> potentially-invalidatable info ought to be in the index's relcache entry,
> not its parent table's entry, IMO.

Well, we've plenty of information about indexes in the table's
relcache. Among other things, the list of indexes, bitmaps of indexed
attributes, which index is the primary key, etc is all maintained
there...  So I don't really see a material difference between the
constraint and the index case.  You can make some arguments about
superfluous invals, true.  I don't see why rd_indexlist et al is
materially different from rd_fkeylist.

Greetings,

Andres Freund

  9. Re: should ConstraintRelationId ins/upd cause relcache invals?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-21T23:38:25Z

    Andres Freund <andres@anarazel.de> writes:
> On 2019-01-21 18:14:31 -0500, Tom Lane wrote:
>> I don't think that's relevant.  The issues there were about whether
>> a pg_index row update ought to cause an invalidation of the relcache
>> entry for the index's table (not the one for the index, which it
>> already takes care of).  That seems very questionable to me --- the
>> potentially-invalidatable info ought to be in the index's relcache entry,
>> not its parent table's entry, IMO.

> Well, we've plenty of information about indexes in the table's
> relcache. Among other things, the list of indexes, bitmaps of indexed
> attributes, which index is the primary key, etc is all maintained
> there...  So I don't really see a material difference between the
> constraint and the index case.

The difference is that we don't support index redefinitions that could
change any of those properties.

			regards, tom lane

  10. Re: should ConstraintRelationId ins/upd cause relcache invals?

    Andres Freund <andres@anarazel.de> — 2019-01-21T23:42:49Z

    Hi,

On 2019-01-21 20:25:46 -0300, Alvaro Herrera wrote:
> Hello
> 
> On 2019-Jan-21, Andres Freund wrote:
> 
> > On 2019-01-21 19:40:17 -0300, Alvaro Herrera wrote:
> > > On 2019-Jan-21, Tom Lane wrote:
> > > 
> > > > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > > 
> > > > > At https://postgr.es/m/201901182216.nr5clsxrn624@alvherre.pgsql I posted
> > > > > a simplistic for the specific problem I found by calling
> > > > > CacheInvalidateRelcache in the problem spot.  But I'm wondering if the
> > > > > correct fix isn't to have CacheInvalidateHeapTuple deal with FK
> > > > > pg_constraint tuples instead, per the attached patch.
> > > > 
> > > > +1, this is safer than expecting retail relcache inval calls to be
> > > > added in all the right places.
> > > 
> > > Thanks, pushed.
> > 
> > Given https://www.postgresql.org/message-id/20190121193300.gknn7p4pmmjg7nqf%40alap3.anarazel.de
> > and the concerns voiced in the thread quoted therein, I'm a bit
> > surprised that you just went ahead with this, and backpatched it to boot.
> 
> Sigh.

> I don't understand why the arguments about a different patch apply to
> this one.  The invalidation I added is for pg_constraint, not pg_index.

> Tom argues that pg_index can be updated for reasons other than
> creation/deletion of the index, and that the relcache entry should not
> be invalidated in those cases.

So can pg_constraint in some cases, that's not an argument (we'll now
e.g. re-compute the table's relcache entry more times than before after
e.g. renaming an index - arguably we could fix that by fixing the
relcache mechanism to not redundantly re-build).


> OTOH I lean towards your side of the argument in the other thread and I
> don't quite understand why you gave up on it.  Tom didn't respond to
> your last argumentat, and neither did you indicate that you were
> convinced by Tom's argumentation.

I wasn't. My concern isn't that we shouldn't do this, but that we ought
to go about this a bit more systematically than just doing this for an
individual object type, depending on the mood of the day.  Cache
invalidation is hard enough to understand already, the more inconsistent
we make it harder it is to get it right going forward.


> My conclusion is that you disagreed but decided not to push the issue
> any further, to get the thing done.  What I did here was the same:
> just get the thing done.

There's a difference between agreeing to disagree and going ahead with a
majority solution, and not even bothering to see whether we can find
consensus and by not even replying to a message wondering about
consistency.

Greetings,

Andres Freund