Thread: should ConstraintRelationId ins/upd cause relcache invals?
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/
Attachment
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
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
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
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
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
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
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
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
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