Thread: Inval reliability, especially for inplace updates
https://postgr.es/m/20240512232923.aa.nmisch@google.com wrote: > Separable, nontrivial things not fixed in the attached patch stack: > > - Inplace update uses transactional CacheInvalidateHeapTuple(). ROLLBACK of > CREATE INDEX wrongly discards the inval, leading to the relhasindex=t loss > still seen in inplace-inval.spec. CacheInvalidateRelmap() does this right. I plan to fix that like CacheInvalidateRelmap(): send the inval immediately, inside the critical section. Send it in heap_xlog_inplace(), too. The interesting decision is how to handle RelationCacheInitFilePreInvalidate(), which has an unlink_initfile() that can fail with e.g. EIO. Options: 1. Unlink during critical section, and accept that EIO becomes PANIC. Replay may reach the same EIO, and the system won't reopen to connections until the storage starts cooperating.a Interaction with checkpoints is not ideal. If we checkpoint and then crash between inplace XLogInsert() and inval, we'd be relying on StartupXLOG() -> RelationCacheInitFileRemove(). That uses elevel==LOG, so replay would neglect to PANIC on EIO. 2. Unlink before critical section, so normal xact abort suffices. This would hold RelCacheInitLock and a buffer content lock at the same time. In RecordTransactionCommit(), it would hold RelCacheInitLock and e.g. slru locks at the same time. The PANIC risk of (1) seems similar to the risk of PANIC at RecordTransactionCommit() -> XLogFlush(), which hasn't been a problem. The checkpoint-related risk bothers me more, and (1) generally makes it harder to reason about checkpoint interactions. The lock order risk of (2) feels tolerable. I'm leaning toward (2), but that might change. Other preferences? Another decision is what to do about LogLogicalInvalidations(). Currently, inplace update invalidations do reach WAL via LogLogicalInvalidations() at the next CCI. Options: a. Within logical decoding, cease processing invalidations for inplace updates. Inplace updates don't affect storage or interpretation of table rows, so they don't affect logicalrep_write_tuple() outcomes. If they did, invalidations wouldn't make it work. Decoding has no way to retrieve a snapshot-appropriate version of the inplace-updated value. b. Make heap_decode() of XLOG_HEAP_INPLACE recreate the invalidation. This would be, essentially, cheap insurance against invalidations having a benefit I missed in (a). I plan to pick (a). > - AtEOXact_Inval(true) is outside the RecordTransactionCommit() critical > section, but it is critical. We must not commit transactional DDL without > other backends receiving an inval. (When the inplace inval becomes > nontransactional, it will face the same threat.) This faces the same RelationCacheInitFilePreInvalidate() decision, and I think the conclusion should be the same as for inplace update. Thanks, nm
On Wed, May 22, 2024 at 05:05:48PM -0700, Noah Misch wrote: > https://postgr.es/m/20240512232923.aa.nmisch@google.com wrote: > > Separable, nontrivial things not fixed in the attached patch stack: > > > > - Inplace update uses transactional CacheInvalidateHeapTuple(). ROLLBACK of > > CREATE INDEX wrongly discards the inval, leading to the relhasindex=t loss > > still seen in inplace-inval.spec. CacheInvalidateRelmap() does this right. > > I plan to fix that like CacheInvalidateRelmap(): send the inval immediately, > inside the critical section. Send it in heap_xlog_inplace(), too. > a. Within logical decoding, cease processing invalidations for inplace I'm attaching the implementation. This applies atop the v3 patch stack from https://postgr.es/m/20240614003549.c2.nmisch@google.com, but the threads are mostly orthogonal and intended for independent review. Translating a tuple into inval messages uses more infrastructure than relmapper, which needs just a database ID. Hence, this ended up more like a miniature of inval.c's participation in the transaction commit sequence. I waffled on whether to back-patch inplace150-inval-durability-atcommit. The consequences of that bug are plenty bad, but reaching them requires an error between TransactionIdCommitTree() and AtEOXact_Inval(). I've not heard reports of that, and I don't have a recipe for making it happen on demand. For now, I'm leaning toward back-patch. The main risk would be me overlooking an LWLock deadlock scenario reachable from the new, earlier RelCacheInitLock timing. Alternatives for RelCacheInitLock: - RelCacheInitLock before PreCommit_Notify(), because notify concurrency matters more than init file concurrency. I chose this. - RelCacheInitLock after PreCommit_Notify(), because PreCommit_Notify() uses a heavyweight lock, giving it less risk of undetected deadlock. - Replace RelCacheInitLock with a heavyweight lock, and keep it before PreCommit_Notify(). - Fold PreCommit_Inval() back into AtCommit_Inval(), accepting that EIO in unlink_initfile() will PANIC. Opinions on that? The patch changes xl_heap_inplace of XLOG_HEAP_INPLACE. For back branches, we could choose between: - Same change, no WAL version bump. Standby must update before primary. This is best long-term, but the transition is more disruptive. I'm leaning toward this one, but the second option isn't bad: - heap_xlog_inplace() could set the shared-inval-queue overflow signal on every backend. This is more wasteful, but inplace updates might be rare enough (~once per VACUUM) to make it tolerable. - Use LogStandbyInvalidations() just after XLOG_HEAP_INPLACE. This isn't correct if one ends recovery between the two records, but you'd need to be unlucky to notice. Noticing would need a procedure like the following. A hot standby backend populates a relcache entry, then does DDL on the rel after recovery ends. Future cleanup work could eliminate LogStandbyInvalidations() and the case of !markXidCommitted && nmsgs != 0. Currently, the src/test/regress suite still reaches that case: - AlterDomainDropConstraint() queues an inval even if !found; it can stop that. - ON COMMIT DELETE ROWS nontransactionally rebuilds an index, which sends a relcache inval. The point of that inval is, I think, to force access methods like btree and hash to reload the metapage copy that they store in rd_amcache. Since no assigned XID implies no changes to the temp index, the no-XID case could simply skip the index rebuild. (temp.sql reaches this with a read-only transaction that selects from an ON COMMIT DELETE ROWS table. Realistic usage will tend not to do that.) ON COMMIT DELETE ROWS has another preexisting problem for indexes, mentioned in a code comment. Thanks, nm
Attachment
On Sat, Jun 15, 2024 at 03:37:18PM -0700, Noah Misch wrote: > I'm attaching the implementation. I'm withdrawing inplace150-inval-durability-atcommit-v1.patch, having found two major problems so far: 1. It sends transactional invalidation messages before ProcArrayEndTransaction(), so other backends can read stale data. 2. It didn't make the equivalent changes for COMMIT PREPARED.
On Sat, Jun 15, 2024 at 03:37:18PM -0700, Noah Misch wrote: > On Wed, May 22, 2024 at 05:05:48PM -0700, Noah Misch wrote: > > https://postgr.es/m/20240512232923.aa.nmisch@google.com wrote: > > > Separable, nontrivial things not fixed in the attached patch stack: > > > > > > - Inplace update uses transactional CacheInvalidateHeapTuple(). ROLLBACK of > > > CREATE INDEX wrongly discards the inval, leading to the relhasindex=t loss > > > still seen in inplace-inval.spec. CacheInvalidateRelmap() does this right. > > > > I plan to fix that like CacheInvalidateRelmap(): send the inval immediately, > > inside the critical section. Send it in heap_xlog_inplace(), too. > > > a. Within logical decoding, cease processing invalidations for inplace > > I'm attaching the implementation. This applies atop the v3 patch stack from > https://postgr.es/m/20240614003549.c2.nmisch@google.com, but the threads are > mostly orthogonal and intended for independent review. Translating a tuple > into inval messages uses more infrastructure than relmapper, which needs just > a database ID. Hence, this ended up more like a miniature of inval.c's > participation in the transaction commit sequence. > > I waffled on whether to back-patch inplace150-inval-durability-atcommit That inplace150 patch turned out to be unnecessary. Contrary to the "noncritical resource releasing" comment some lines above AtEOXact_Inval(true), the actual behavior is already to promote ERROR to PANIC. An ERROR just before or after sending invals becomes PANIC, "cannot abort transaction %u, it was already committed". Since inplace130-AtEOXact_RelationCache-comments existed to clear the way for inplace150, inplace130 also becomes unnecessary. I've removed both from the attached v2 patch stack. > The patch changes xl_heap_inplace of XLOG_HEAP_INPLACE. For back branches, we > could choose between: > > - Same change, no WAL version bump. Standby must update before primary. This > is best long-term, but the transition is more disruptive. I'm leaning > toward this one, but the second option isn't bad: > > - heap_xlog_inplace() could set the shared-inval-queue overflow signal on > every backend. This is more wasteful, but inplace updates might be rare > enough (~once per VACUUM) to make it tolerable. > > - Use LogStandbyInvalidations() just after XLOG_HEAP_INPLACE. This isn't > correct if one ends recovery between the two records, but you'd need to be > unlucky to notice. Noticing would need a procedure like the following. A > hot standby backend populates a relcache entry, then does DDL on the rel > after recovery ends. That still holds.
Attachment
Hi, On 2024-06-17 16:58:54 -0700, Noah Misch wrote: > On Sat, Jun 15, 2024 at 03:37:18PM -0700, Noah Misch wrote: > > On Wed, May 22, 2024 at 05:05:48PM -0700, Noah Misch wrote: > > > https://postgr.es/m/20240512232923.aa.nmisch@google.com wrote: > > > > Separable, nontrivial things not fixed in the attached patch stack: > > > > > > > > - Inplace update uses transactional CacheInvalidateHeapTuple(). ROLLBACK of > > > > CREATE INDEX wrongly discards the inval, leading to the relhasindex=t loss > > > > still seen in inplace-inval.spec. CacheInvalidateRelmap() does this right. > > > > > > I plan to fix that like CacheInvalidateRelmap(): send the inval immediately, > > > inside the critical section. Send it in heap_xlog_inplace(), too. I'm worried this might cause its own set of bugs, e.g. if there are any places that, possibly accidentally, rely on the invalidation from the inplace update to also cover separate changes. Have you considered instead submitting these invalidations during abort as well? > > > a. Within logical decoding, cease processing invalidations for inplace > > > > I'm attaching the implementation. This applies atop the v3 patch stack from > > https://postgr.es/m/20240614003549.c2.nmisch@google.com, but the threads are > > mostly orthogonal and intended for independent review. Translating a tuple > > into inval messages uses more infrastructure than relmapper, which needs just > > a database ID. Hence, this ended up more like a miniature of inval.c's > > participation in the transaction commit sequence. > > > > I waffled on whether to back-patch inplace150-inval-durability-atcommit > > That inplace150 patch turned out to be unnecessary. Contrary to the > "noncritical resource releasing" comment some lines above > AtEOXact_Inval(true), the actual behavior is already to promote ERROR to > PANIC. An ERROR just before or after sending invals becomes PANIC, "cannot > abort transaction %u, it was already committed". Relying on that, instead of explicit critical sections, seems fragile to me. IIRC some of the behaviour around errors around transaction commit/abort has changed a bunch of times. Tying correctness into something that could be changed for unrelated reasons doesn't seem great. I'm not sure it holds true even today - what if the transaction didn't have an xid? Then RecordTransactionAbort() wouldn't trigger "cannot abort transaction %u, it was already committed" I think? > > - Same change, no WAL version bump. Standby must update before primary. This > > is best long-term, but the transition is more disruptive. I'm leaning > > toward this one, but the second option isn't bad: Hm. The inplace record doesn't use the length of the "main data" record segment for anything, from what I can tell. If records by an updated primary were replayed by an old standby, it'd just ignore the additional data, afaict? I think with the code as-is, the situation with an updated standby replaying an old primary's record would actually be worse - it'd afaict just assume the now-longer record contained valid fields, despite those just pointing into uninitialized memory. I think the replay routine would have to check the length of the main data and execute the invalidation conditionally. > > - heap_xlog_inplace() could set the shared-inval-queue overflow signal on > > every backend. This is more wasteful, but inplace updates might be rare > > enough (~once per VACUUM) to make it tolerable. We already set that surprisingly frequently, as a) The size of the sinval queue is small b) If a backend is busy, it does not process catchup interrupts (i.e. executing queries, waiting for a lock prevents processing) c) There's no deduplication of invals, we often end up sending the same inval over and over. So I suspect this might not be too bad, compared to the current badness. At least for core code. I guess there could be extension code triggering inplace updates more frequently? But I'd hope they'd do it not on catalog tables... Except that we wouldn't know that that's the case during replay, it's not contained in the record. > > - Use LogStandbyInvalidations() just after XLOG_HEAP_INPLACE. This isn't > > correct if one ends recovery between the two records, but you'd need to be > > unlucky to notice. Noticing would need a procedure like the following. A > > hot standby backend populates a relcache entry, then does DDL on the rel > > after recovery ends. Hm. The problematic cases presumably involves an access exclusive lock? If so, could we do LogStandbyInvalidations() *before* logging the WAL record for the inplace update? The invalidations can't be processed by other backends until the exclusive lock has been released, which should avoid the race? Greetings, Andres Freund
On Mon, Jun 17, 2024 at 06:57:30PM -0700, Andres Freund wrote: > On 2024-06-17 16:58:54 -0700, Noah Misch wrote: > > On Sat, Jun 15, 2024 at 03:37:18PM -0700, Noah Misch wrote: > > > On Wed, May 22, 2024 at 05:05:48PM -0700, Noah Misch wrote: > > > > https://postgr.es/m/20240512232923.aa.nmisch@google.com wrote: > > > > > Separable, nontrivial things not fixed in the attached patch stack: > > > > > > > > > > - Inplace update uses transactional CacheInvalidateHeapTuple(). ROLLBACK of > > > > > CREATE INDEX wrongly discards the inval, leading to the relhasindex=t loss > > > > > still seen in inplace-inval.spec. CacheInvalidateRelmap() does this right. > > > > > > > > I plan to fix that like CacheInvalidateRelmap(): send the inval immediately, > > > > inside the critical section. Send it in heap_xlog_inplace(), too. > > I'm worried this might cause its own set of bugs, e.g. if there are any places > that, possibly accidentally, rely on the invalidation from the inplace update > to also cover separate changes. Good point. I do have index_update_stats() still doing an ideally-superfluous relcache update for that reason. Taking that further, it would be cheap insurance to have the inplace update do a transactional inval in addition to its immediate inval. Future master-only work could remove the transactional one. How about that? > Have you considered instead submitting these invalidations during abort as > well? I had not. Hmmm. If the lock protocol in README.tuplock (after patch inplace120) told SearchSysCacheLocked1() to do systable scans instead of syscache reads, that could work. Would need to ensure a PANIC if transaction abort doesn't reach the inval submission. Overall, it would be harder to reason about the state of caches, but I suspect the patch would be smaller. How should we choose between those strategies? > > > > a. Within logical decoding, cease processing invalidations for inplace > > > > > > I'm attaching the implementation. This applies atop the v3 patch stack from > > > https://postgr.es/m/20240614003549.c2.nmisch@google.com, but the threads are > > > mostly orthogonal and intended for independent review. Translating a tuple > > > into inval messages uses more infrastructure than relmapper, which needs just > > > a database ID. Hence, this ended up more like a miniature of inval.c's > > > participation in the transaction commit sequence. > > > > > > I waffled on whether to back-patch inplace150-inval-durability-atcommit > > > > That inplace150 patch turned out to be unnecessary. Contrary to the > > "noncritical resource releasing" comment some lines above > > AtEOXact_Inval(true), the actual behavior is already to promote ERROR to > > PANIC. An ERROR just before or after sending invals becomes PANIC, "cannot > > abort transaction %u, it was already committed". > > Relying on that, instead of explicit critical sections, seems fragile to me. > IIRC some of the behaviour around errors around transaction commit/abort has > changed a bunch of times. Tying correctness into something that could be > changed for unrelated reasons doesn't seem great. Fair enough. It could still be a good idea for master, but given I missed a bug in inplace150-inval-durability-atcommit-v1.patch far worse than the ones $SUBJECT fixes, let's not risk it in back branches. > I'm not sure it holds true even today - what if the transaction didn't have an > xid? Then RecordTransactionAbort() wouldn't trigger > "cannot abort transaction %u, it was already committed" > I think? I think that's right. As the inplace160-inval-durability-inplace-v2.patch edits to xact.c say, the concept of invals in XID-less transactions is buggy at its core. Fortunately, after that patch, we use them only for two things that could themselves stop with something roughly as simple as the attached. > > > - Same change, no WAL version bump. Standby must update before primary. This > > > is best long-term, but the transition is more disruptive. I'm leaning > > > toward this one, but the second option isn't bad: > > Hm. The inplace record doesn't use the length of the "main data" record > segment for anything, from what I can tell. If records by an updated primary > were replayed by an old standby, it'd just ignore the additional data, afaict? Agreed, but ... > I think with the code as-is, the situation with an updated standby replaying > an old primary's record would actually be worse - it'd afaict just assume the > now-longer record contained valid fields, despite those just pointing into > uninitialized memory. I think the replay routine would have to check the > length of the main data and execute the invalidation conditionally. I anticipated back branches supporting a new XLOG_HEAP_INPLACE_WITH_INVAL alongside the old XLOG_HEAP_INPLACE. Updated standbys would run both fine, and old binaries consuming new WAL would PANIC, "heap_redo: unknown op code". > > > - heap_xlog_inplace() could set the shared-inval-queue overflow signal on > > > every backend. This is more wasteful, but inplace updates might be rare > > > enough (~once per VACUUM) to make it tolerable. > > We already set that surprisingly frequently, as > a) The size of the sinval queue is small > b) If a backend is busy, it does not process catchup interrupts > (i.e. executing queries, waiting for a lock prevents processing) > c) There's no deduplication of invals, we often end up sending the same inval > over and over. > > So I suspect this might not be too bad, compared to the current badness. That is good. We might be able to do the overflow signal once at end of recovery, like RelationCacheInitFileRemove() does for the init file. That's mildly harder to reason about, but it would be cheaper. Hmmm. > At least for core code. I guess there could be extension code triggering > inplace updates more frequently? But I'd hope they'd do it not on catalog > tables... Except that we wouldn't know that that's the case during replay, > it's not contained in the record. For what it's worth, from a grep of PGXN, only citus does inplace updates. > > > - Use LogStandbyInvalidations() just after XLOG_HEAP_INPLACE. This isn't > > > correct if one ends recovery between the two records, but you'd need to be > > > unlucky to notice. Noticing would need a procedure like the following. A > > > hot standby backend populates a relcache entry, then does DDL on the rel > > > after recovery ends. > > Hm. The problematic cases presumably involves an access exclusive lock? If so, > could we do LogStandbyInvalidations() *before* logging the WAL record for the > inplace update? The invalidations can't be processed by other backends until > the exclusive lock has been released, which should avoid the race? A lock forces a backend to drain the inval queue before using the locked object, but it doesn't stop the backend from draining the queue and repopulating cache entries earlier. For example, pg_describe_object() can query many syscaches without locking underlying objects. Hence, the inval system relies on the buffer change getting fully visible to catcache queries before the sinval message enters the shared queue. Thanks, nm
On Tue, Jun 18, 2024 at 08:23:49AM -0700, Noah Misch wrote: > On Mon, Jun 17, 2024 at 06:57:30PM -0700, Andres Freund wrote: > > On 2024-06-17 16:58:54 -0700, Noah Misch wrote: > > > On Sat, Jun 15, 2024 at 03:37:18PM -0700, Noah Misch wrote: > > > > On Wed, May 22, 2024 at 05:05:48PM -0700, Noah Misch wrote: > > > > > https://postgr.es/m/20240512232923.aa.nmisch@google.com wrote: > > > > > > Separable, nontrivial things not fixed in the attached patch stack: > > > > > > > > > > > > - Inplace update uses transactional CacheInvalidateHeapTuple(). ROLLBACK of > > > > > > CREATE INDEX wrongly discards the inval, leading to the relhasindex=t loss > > > > > > still seen in inplace-inval.spec. CacheInvalidateRelmap() does this right. > > > > > > > > > > I plan to fix that like CacheInvalidateRelmap(): send the inval immediately, > > > > > inside the critical section. Send it in heap_xlog_inplace(), too. > > > > I'm worried this might cause its own set of bugs, e.g. if there are any places > > that, possibly accidentally, rely on the invalidation from the inplace update > > to also cover separate changes. > > Good point. I do have index_update_stats() still doing an ideally-superfluous > relcache update for that reason. Taking that further, it would be cheap > insurance to have the inplace update do a transactional inval in addition to > its immediate inval. Future master-only work could remove the transactional > one. How about that? > > > Have you considered instead submitting these invalidations during abort as > > well? > > I had not. Hmmm. If the lock protocol in README.tuplock (after patch > inplace120) told SearchSysCacheLocked1() to do systable scans instead of > syscache reads, that could work. Would need to ensure a PANIC if transaction > abort doesn't reach the inval submission. Overall, it would be harder to > reason about the state of caches, but I suspect the patch would be smaller. > How should we choose between those strategies? > > > > > > a. Within logical decoding, cease processing invalidations for inplace > > > > > > > > I'm attaching the implementation. This applies atop the v3 patch stack from > > > > https://postgr.es/m/20240614003549.c2.nmisch@google.com, but the threads are > > > > mostly orthogonal and intended for independent review. Translating a tuple > > > > into inval messages uses more infrastructure than relmapper, which needs just > > > > a database ID. Hence, this ended up more like a miniature of inval.c's > > > > participation in the transaction commit sequence. > > > > > > > > I waffled on whether to back-patch inplace150-inval-durability-atcommit > > > > > > That inplace150 patch turned out to be unnecessary. Contrary to the > > > "noncritical resource releasing" comment some lines above > > > AtEOXact_Inval(true), the actual behavior is already to promote ERROR to > > > PANIC. An ERROR just before or after sending invals becomes PANIC, "cannot > > > abort transaction %u, it was already committed". > > > > Relying on that, instead of explicit critical sections, seems fragile to me. > > IIRC some of the behaviour around errors around transaction commit/abort has > > changed a bunch of times. Tying correctness into something that could be > > changed for unrelated reasons doesn't seem great. > > Fair enough. It could still be a good idea for master, but given I missed a > bug in inplace150-inval-durability-atcommit-v1.patch far worse than the ones > $SUBJECT fixes, let's not risk it in back branches. > > > I'm not sure it holds true even today - what if the transaction didn't have an > > xid? Then RecordTransactionAbort() wouldn't trigger > > "cannot abort transaction %u, it was already committed" > > I think? > > I think that's right. As the inplace160-inval-durability-inplace-v2.patch > edits to xact.c say, the concept of invals in XID-less transactions is buggy > at its core. Fortunately, after that patch, we use them only for two things > that could themselves stop with something roughly as simple as the attached. Now actually attached. > > > > - Same change, no WAL version bump. Standby must update before primary. This > > > > is best long-term, but the transition is more disruptive. I'm leaning > > > > toward this one, but the second option isn't bad: > > > > Hm. The inplace record doesn't use the length of the "main data" record > > segment for anything, from what I can tell. If records by an updated primary > > were replayed by an old standby, it'd just ignore the additional data, afaict? > > Agreed, but ... > > > I think with the code as-is, the situation with an updated standby replaying > > an old primary's record would actually be worse - it'd afaict just assume the > > now-longer record contained valid fields, despite those just pointing into > > uninitialized memory. I think the replay routine would have to check the > > length of the main data and execute the invalidation conditionally. > > I anticipated back branches supporting a new XLOG_HEAP_INPLACE_WITH_INVAL > alongside the old XLOG_HEAP_INPLACE. Updated standbys would run both fine, > and old binaries consuming new WAL would PANIC, "heap_redo: unknown op code". > > > > > - heap_xlog_inplace() could set the shared-inval-queue overflow signal on > > > > every backend. This is more wasteful, but inplace updates might be rare > > > > enough (~once per VACUUM) to make it tolerable. > > > > We already set that surprisingly frequently, as > > a) The size of the sinval queue is small > > b) If a backend is busy, it does not process catchup interrupts > > (i.e. executing queries, waiting for a lock prevents processing) > > c) There's no deduplication of invals, we often end up sending the same inval > > over and over. > > > > So I suspect this might not be too bad, compared to the current badness. > > That is good. We might be able to do the overflow signal once at end of > recovery, like RelationCacheInitFileRemove() does for the init file. That's > mildly harder to reason about, but it would be cheaper. Hmmm. > > > At least for core code. I guess there could be extension code triggering > > inplace updates more frequently? But I'd hope they'd do it not on catalog > > tables... Except that we wouldn't know that that's the case during replay, > > it's not contained in the record. > > For what it's worth, from a grep of PGXN, only citus does inplace updates. > > > > > - Use LogStandbyInvalidations() just after XLOG_HEAP_INPLACE. This isn't > > > > correct if one ends recovery between the two records, but you'd need to be > > > > unlucky to notice. Noticing would need a procedure like the following. A > > > > hot standby backend populates a relcache entry, then does DDL on the rel > > > > after recovery ends. > > > > Hm. The problematic cases presumably involves an access exclusive lock? If so, > > could we do LogStandbyInvalidations() *before* logging the WAL record for the > > inplace update? The invalidations can't be processed by other backends until > > the exclusive lock has been released, which should avoid the race? > > A lock forces a backend to drain the inval queue before using the locked > object, but it doesn't stop the backend from draining the queue and > repopulating cache entries earlier. For example, pg_describe_object() can > query many syscaches without locking underlying objects. Hence, the inval > system relies on the buffer change getting fully visible to catcache queries > before the sinval message enters the shared queue. > > Thanks, > nm
Attachment
On Mon, Jun 17, 2024 at 04:58:54PM -0700, Noah Misch wrote: > attached v2 patch stack. Rebased. This applies on top of three patches from https://postgr.es/m/20240629024251.03.nmisch@google.com. I'm attaching those to placate cfbot, but this thread is for review of the last patch only.
Attachment
On Tue, Jun 18, 2024 at 08:23:49AM -0700, Noah Misch wrote: > On Mon, Jun 17, 2024 at 06:57:30PM -0700, Andres Freund wrote: > > On 2024-06-17 16:58:54 -0700, Noah Misch wrote: > > > That inplace150 patch turned out to be unnecessary. Contrary to the > > > "noncritical resource releasing" comment some lines above > > > AtEOXact_Inval(true), the actual behavior is already to promote ERROR to > > > PANIC. An ERROR just before or after sending invals becomes PANIC, "cannot > > > abort transaction %u, it was already committed". > > > > Relying on that, instead of explicit critical sections, seems fragile to me. > > IIRC some of the behaviour around errors around transaction commit/abort has > > changed a bunch of times. Tying correctness into something that could be > > changed for unrelated reasons doesn't seem great. > > Fair enough. It could still be a good idea for master, but given I missed a > bug in inplace150-inval-durability-atcommit-v1.patch far worse than the ones > $SUBJECT fixes, let's not risk it in back branches. What are your thoughts on whether a change to explicit critical sections should be master-only vs. back-patched? I have a feeling your comment pointed to something I'm still missing, but I don't know where to look next.
On Sat, Oct 12, 2024 at 5:47 PM Noah Misch <noah@leadboat.com> wrote: > > Rebased. Hi, I have a couple of questions : 1. In heap_inplace_update_and_unlock, currently both buffer and tuple are unlocked outside the critical section. Why do we have to move the buffer unlock within the critical section here? My guess is that it needs to be unlocked for the inplace invals to be processed. But what is the reasoning behind that? 2. Is there any benefit in CacheInvalidateHeapTupleCommon taking the preapre_callback argument? Wouldn't it be simpler to just pass an InvalidationInfo* to the function? Also is inval-requires-xid-v0.patch planned to be fixed up to inplace160? Thanks
On Sat, Oct 12, 2024 at 06:05:06PM +0530, Nitin Motiani wrote:
> 1. In heap_inplace_update_and_unlock, currently both buffer and tuple
> are unlocked outside the critical section. Why do we have to move the
> buffer unlock within the critical section here? My guess is that it
> needs to be unlocked for the inplace invals to be processed. But what
> is the reasoning behind that?
AtInplace_Inval() acquires SInvalWriteLock. There are two reasons to want to
release the buffer lock before acquiring SInvalWriteLock:
1. Otherwise, we'd need to maintain the invariant that no other part of the
system tries to lock the buffer while holding SInvalWriteLock. (That would
cause an undetected deadlock.)
2. Concurrency is better if we release a no-longer-needed LWLock before doing
something time-consuming, like acquiring another LWLock potentially is.
Inplace invals do need to happen in the critical section, because we've
already written the change to shared buffers, making it the new authoritative
value. If we fail to invalidate, other backends may continue operating with
stale caches.
> 2. Is there any benefit in CacheInvalidateHeapTupleCommon taking the
> preapre_callback argument? Wouldn't it be simpler to just pass an
> InvalidationInfo* to the function?
CacheInvalidateHeapTupleCommon() has three conditions that cause it to return
without invoking the callback. Every heap_update() calls
CacheInvalidateHeapTuple(). In typical performance-critical systems, non-DDL
changes dwarf DDL. Hence, the overwhelming majority of heap_update() calls
involve !IsCatalogRelation(). I wouldn't want to allocate InvalidationInfo in
DDL-free transactions. To pass in InvalidationInfo*, I suppose I'd move those
three conditions to a function and make the callers look like:
CacheInvalidateHeapTuple(Relation relation,
HeapTuple tuple,
HeapTuple newtuple)
{
if (NeedsInvalidateHeapTuple(relation))
CacheInvalidateHeapTupleCommon(relation, tuple, newtuple,
PrepareInvalidationState());
}
I don't have a strong preference between that and the callback way.
> Also is inval-requires-xid-v0.patch planned to be fixed up to inplace160?
I figure I'll pursue that on a different thread, after inplace160 and
inplace180. If there's cause to pursue it earlier, let me know.
Thanks,
nm
On Sun, Oct 13, 2024 at 6:15 AM Noah Misch <noah@leadboat.com> wrote:
>
> On Sat, Oct 12, 2024 at 06:05:06PM +0530, Nitin Motiani wrote:
> > 1. In heap_inplace_update_and_unlock, currently both buffer and tuple
> > are unlocked outside the critical section. Why do we have to move the
> > buffer unlock within the critical section here? My guess is that it
> > needs to be unlocked for the inplace invals to be processed. But what
> > is the reasoning behind that?
>
> AtInplace_Inval() acquires SInvalWriteLock. There are two reasons to want to
> release the buffer lock before acquiring SInvalWriteLock:
>
> 1. Otherwise, we'd need to maintain the invariant that no other part of the
> system tries to lock the buffer while holding SInvalWriteLock. (That would
> cause an undetected deadlock.)
>
> 2. Concurrency is better if we release a no-longer-needed LWLock before doing
> something time-consuming, like acquiring another LWLock potentially is.
>
> Inplace invals do need to happen in the critical section, because we've
> already written the change to shared buffers, making it the new authoritative
> value. If we fail to invalidate, other backends may continue operating with
> stale caches.
>
Thanks for the clarification.
> > 2. Is there any benefit in CacheInvalidateHeapTupleCommon taking the
> > preapre_callback argument? Wouldn't it be simpler to just pass an
> > InvalidationInfo* to the function?
>
> CacheInvalidateHeapTupleCommon() has three conditions that cause it to return
> without invoking the callback. Every heap_update() calls
> CacheInvalidateHeapTuple(). In typical performance-critical systems, non-DDL
> changes dwarf DDL. Hence, the overwhelming majority of heap_update() calls
> involve !IsCatalogRelation(). I wouldn't want to allocate InvalidationInfo in
> DDL-free transactions. To pass in InvalidationInfo*, I suppose I'd move those
> three conditions to a function and make the callers look like:
>
> CacheInvalidateHeapTuple(Relation relation,
> HeapTuple tuple,
> HeapTuple newtuple)
> {
> if (NeedsInvalidateHeapTuple(relation))
> CacheInvalidateHeapTupleCommon(relation, tuple, newtuple,
> PrepareInvalidationState());
> }
>
> I don't have a strong preference between that and the callback way.
>
Thanks. I would have probably done it using the
NeedsInvalidateHeapTuple. But I don't have a strong enough preference
to change it from the callback way. So the current approach seems
good.
> > Also is inval-requires-xid-v0.patch planned to be fixed up to inplace160?
>
> I figure I'll pursue that on a different thread, after inplace160 and
> inplace180. If there's cause to pursue it earlier, let me know.
>
Sure. Can be done in a different thread.
Thanks,
Nitin Motiani
Google
On Mon, Oct 14, 2024 at 3:15 PM Nitin Motiani <nitinmotiani@google.com> wrote:
>
> On Sun, Oct 13, 2024 at 6:15 AM Noah Misch <noah@leadboat.com> wrote:
> >
> > On Sat, Oct 12, 2024 at 06:05:06PM +0530, Nitin Motiani wrote:
> > > 1. In heap_inplace_update_and_unlock, currently both buffer and tuple
> > > are unlocked outside the critical section. Why do we have to move the
> > > buffer unlock within the critical section here? My guess is that it
> > > needs to be unlocked for the inplace invals to be processed. But what
> > > is the reasoning behind that?
> >
> > AtInplace_Inval() acquires SInvalWriteLock. There are two reasons to want to
> > release the buffer lock before acquiring SInvalWriteLock:
> >
> > 1. Otherwise, we'd need to maintain the invariant that no other part of the
> > system tries to lock the buffer while holding SInvalWriteLock. (That would
> > cause an undetected deadlock.)
> >
> > 2. Concurrency is better if we release a no-longer-needed LWLock before doing
> > something time-consuming, like acquiring another LWLock potentially is.
> >
> > Inplace invals do need to happen in the critical section, because we've
> > already written the change to shared buffers, making it the new authoritative
> > value. If we fail to invalidate, other backends may continue operating with
> > stale caches.
> >
>
> Thanks for the clarification.
>
> > > 2. Is there any benefit in CacheInvalidateHeapTupleCommon taking the
> > > preapre_callback argument? Wouldn't it be simpler to just pass an
> > > InvalidationInfo* to the function?
> >
> > CacheInvalidateHeapTupleCommon() has three conditions that cause it to return
> > without invoking the callback. Every heap_update() calls
> > CacheInvalidateHeapTuple(). In typical performance-critical systems, non-DDL
> > changes dwarf DDL. Hence, the overwhelming majority of heap_update() calls
> > involve !IsCatalogRelation(). I wouldn't want to allocate InvalidationInfo in
> > DDL-free transactions. To pass in InvalidationInfo*, I suppose I'd move those
> > three conditions to a function and make the callers look like:
> >
> > CacheInvalidateHeapTuple(Relation relation,
> > HeapTuple tuple,
> > HeapTuple newtuple)
> > {
> > if (NeedsInvalidateHeapTuple(relation))
> > CacheInvalidateHeapTupleCommon(relation, tuple, newtuple,
> > PrepareInvalidationState());
> > }
> >
> > I don't have a strong preference between that and the callback way.
> >
>
> Thanks. I would have probably done it using the
> NeedsInvalidateHeapTuple. But I don't have a strong enough preference
> to change it from the callback way. So the current approach seems
> good.
>
> > > Also is inval-requires-xid-v0.patch planned to be fixed up to inplace160?
> >
> > I figure I'll pursue that on a different thread, after inplace160 and
> > inplace180. If there's cause to pursue it earlier, let me know.
> >
> Sure. Can be done in a different thread.
>
I tested the patch locally and it works. And I have no other question
regarding the structure. So this patch looks good to me to commit.
Thanks,
Nitin Motiani
Google
On Thu, Oct 24, 2024 at 8:24 AM Noah Misch <noah@leadboat.com> wrote: > > With the releases wrapping in 2.5 weeks, I'm ambivalent about pushing this > before the release or after. Pushing before means fewer occurrences of > corruption, but pushing after gives more bake time to discover these changes > were defective. It's hard to predict which helps users more, on a > risk-adjusted basis. I'm leaning toward pushing this week. Opinions? > I lean towards pushing after the release. This is based on my assumption that since this bug has been around for a while, it is (probably) not hit often. And a few weeks delay is better than introducing a new defect. Thanks
On Mon, Oct 28, 2024 at 02:27:03PM +0530, Nitin Motiani wrote: > On Thu, Oct 24, 2024 at 8:24 AM Noah Misch <noah@leadboat.com> wrote: > > With the releases wrapping in 2.5 weeks, I'm ambivalent about pushing this > > before the release or after. Pushing before means fewer occurrences of > > corruption, but pushing after gives more bake time to discover these changes > > were defective. It's hard to predict which helps users more, on a > > risk-adjusted basis. I'm leaning toward pushing this week. Opinions? > > I lean towards pushing after the release. This is based on my > assumption that since this bug has been around for a while, it is > (probably) not hit often. And a few weeks delay is better than > introducing a new defect. I had pushed this during the indicated week, before your mail. Reverting it is an option. Let's see if more opinions arrive.
On Thu, Oct 31, 2024 at 01:01:39PM -0700, Noah Misch wrote:
> On Thu, Oct 31, 2024 at 05:00:02PM +0300, Alexander Lakhin wrote:
> > I've accidentally discovered an incorrect behaviour caused by commit
> > 4eac5a1fa. Running this script:
>
> Thanks. This looks important.
>
> > parallel -j40 --linebuffer --tag .../reproi.sh ::: `seq 40`
>
> This didn't reproduce it for me, at -j20, -j40, or -j80. I tested at commit
> fb7e27a. At what commit(s) does it reproduce for you? At what commits, if
> any, did your test not reproduce this?
I reproduced this using a tmpfs current working directory.
> > All three autovacuum workers (1143263, 1143320, 1143403) are also waiting
> > for the same buffer lock:
> > #5 0x0000561dd715f1fe in PGSemaphoreLock (sema=0x7fed9a817338) at pg_sema.c:327
> > #6 0x0000561dd722fe02 in LWLockAcquire (lock=0x7fed9ad9b4e4, mode=LW_SHARED) at lwlock.c:1318
> > #7 0x0000561dd71f8423 in LockBuffer (buffer=36, mode=1) at bufmgr.c:4182
>
> Can you share the full backtrace for the autovacuum workers?
Here, one of the autovacuum workers had the guilty stack trace, appearing at
the end of this message. heap_inplace_update_and_unlock() calls
CacheInvalidateHeapTupleInplace() while holding BUFFER_LOCK_EXCLUSIVE on a
buffer of pg_class. CacheInvalidateHeapTupleInplace() may call
CatalogCacheInitializeCache(), which opens the cache's rel. If there's not a
valid relcache entry for the catcache's rel, we scan pg_class to make a valid
relcache entry. The ensuing hang makes sense.
Tomorrow, I'll think more about fixes. Two that might work:
1. Call CacheInvalidateHeapTupleInplace() before locking the buffer. Each
time we need to re-find the tuple, discard the previous try's inplace
invals and redo CacheInvalidateHeapTupleInplace(). That's because
concurrent activity may have changed cache key fields like relname.
2. Add some function that we call before locking the buffer. Its charter is
to ensure PrepareToInvalidateCacheTuple() won't have to call
CatalogCacheInitializeCache(). I think nothing resets catcache to the
extent that CatalogCacheInitializeCache() must happen again, so this should
suffice regardless of concurrent sinval traffic, debug_discard_caches, etc.
What else is worth considering? Any preferences among those?
#6 0x0000555e8a3c3afc in LWLockAcquire (lock=0x7f1683a65424, mode=LW_SHARED) at lwlock.c:1287
#7 0x0000555e8a08cec8 in heap_prepare_pagescan (sscan=sscan@entry=0x7f168e99ba48) at heapam.c:512
#8 0x0000555e8a08d6f1 in heapgettup_pagemode (scan=scan@entry=0x7f168e99ba48, dir=<optimized out>, nkeys=<optimized
out>,key=<optimized out>) at heapam.c:979
#9 0x0000555e8a08dc9e in heap_getnextslot (sscan=0x7f168e99ba48, direction=<optimized out>, slot=0x7f168e9a58c0) at
heapam.c:1299
#10 0x0000555e8a0aad3d in table_scan_getnextslot (direction=ForwardScanDirection, slot=<optimized out>,
sscan=<optimizedout>)
at ../../../../src/include/access/tableam.h:1080
#11 systable_getnext (sysscan=sysscan@entry=0x7f168e9a79c8) at genam.c:538
#12 0x0000555e8a501391 in ScanPgRelation (targetRelId=<optimized out>, indexOK=false,
force_non_historic=force_non_historic@entry=false)at relcache.c:388
#13 0x0000555e8a507ea4 in RelationReloadIndexInfo (relation=0x7f168f1b8808) at relcache.c:2272
#14 RelationRebuildRelation (relation=0x7f168f1b8808) at relcache.c:2573
#15 0x0000555e8a5083e5 in RelationFlushRelation (relation=0x7f168f1b8808) at relcache.c:2848
#16 RelationCacheInvalidateEntry (relationId=<optimized out>) at relcache.c:2910
#17 0x0000555e8a4fafaf in LocalExecuteInvalidationMessage (msg=0x7ffec99ff8a0) at inval.c:795
#18 0x0000555e8a3b711a in ReceiveSharedInvalidMessages (invalFunction=invalFunction@entry=0x555e8a4faf10
<LocalExecuteInvalidationMessage>,
resetFunction=resetFunction@entry=0x555e8a4fa650 <InvalidateSystemCaches>) at sinval.c:88
#19 0x0000555e8a4fa677 in AcceptInvalidationMessages () at inval.c:865
#20 0x0000555e8a3bc809 in LockRelationOid (relid=1259, lockmode=1) at lmgr.c:135
#21 0x0000555e8a05155d in relation_open (relationId=relationId@entry=1259, lockmode=lockmode@entry=1) at relation.c:55
#22 0x0000555e8a0dd809 in table_open (relationId=relationId@entry=1259, lockmode=lockmode@entry=1) at table.c:44
#23 0x0000555e8a501359 in ScanPgRelation (targetRelId=<optimized out>, indexOK=indexOK@entry=true,
force_non_historic=force_non_historic@entry=false)
at relcache.c:371
#24 0x0000555e8a507f9a in RelationReloadNailed (relation=0x7f168f1738c8) at relcache.c:2380
#25 RelationRebuildRelation (relation=0x7f168f1738c8) at relcache.c:2579
#26 0x0000555e8a5083e5 in RelationFlushRelation (relation=0x7f168f1738c8) at relcache.c:2848
#27 RelationCacheInvalidateEntry (relationId=<optimized out>) at relcache.c:2910
#28 0x0000555e8a4fafaf in LocalExecuteInvalidationMessage (msg=0x7ffec99ffc80) at inval.c:795
#29 0x0000555e8a3b71aa in ReceiveSharedInvalidMessages (invalFunction=invalFunction@entry=0x555e8a4faf10
<LocalExecuteInvalidationMessage>,
resetFunction=resetFunction@entry=0x555e8a4fa650 <InvalidateSystemCaches>) at sinval.c:118
#30 0x0000555e8a4fa677 in AcceptInvalidationMessages () at inval.c:865
#31 0x0000555e8a3bc809 in LockRelationOid (relid=1259, lockmode=1) at lmgr.c:135
#32 0x0000555e8a05155d in relation_open (relationId=1259, lockmode=lockmode@entry=1) at relation.c:55
#33 0x0000555e8a0dd809 in table_open (relationId=<optimized out>, lockmode=lockmode@entry=1) at table.c:44
#34 0x0000555e8a4f7211 in CatalogCacheInitializeCache (cache=cache@entry=0x555eb26d2180) at catcache.c:1045
#35 0x0000555e8a4f9a68 in PrepareToInvalidateCacheTuple (relation=relation@entry=0x7f168f1738c8,
tuple=tuple@entry=0x7f168e9a74e8,newtuple=newtuple@entry=0x0,
function=function@entry=0x555e8a4fa220 <RegisterCatcacheInvalidation>, context=context@entry=0x7f168e9a7428) at
catcache.c:2326
#36 0x0000555e8a4fa500 in CacheInvalidateHeapTupleCommon (relation=relation@entry=0x7f168f1738c8,
tuple=tuple@entry=0x7f168e9a74e8,newtuple=newtuple@entry=0x0,
prepare_callback=prepare_callback@entry=0x555e8a4fa320 <PrepareInplaceInvalidationState>) at inval.c:1391
#37 0x0000555e8a4fabcb in CacheInvalidateHeapTupleCommon (prepare_callback=0x555e8a4fa320
<PrepareInplaceInvalidationState>,newtuple=newtuple@entry=0x0,
tuple=tuple@entry=0x7f168e9a74e8, relation=relation@entry=0x7f168f1738c8) at inval.c:1504
#38 0x0000555e8a094f7c in heap_inplace_update_and_unlock (relation=0x7f168f1738c8, oldtup=0x555eb277ea58,
tuple=0x7f168e9a74e8,buffer=3) at heapam.c:6354
#39 0x0000555e8a0ab35a in systable_inplace_update_finish (state=0x7f168e9a72c8, tuple=<optimized out>) at genam.c:891
#40 0x0000555e8a1fbdc4 in vac_update_relstats (relation=relation@entry=0x7f168f1beae8, num_pages=num_pages@entry=19,
num_tuples=<optimizedout>,
num_all_visible_pages=<optimized out>, hasindex=hasindex@entry=true, frozenxid=frozenxid@entry=0,
minmulti=minmulti@entry=0,frozenxid_updated=0x0,
minmulti_updated=0x0, in_outer_xact=false) at vacuum.c:1545
#41 0x0000555e8a18c97c in do_analyze_rel (onerel=onerel@entry=0x7f168f1beae8, params=params@entry=0x555eb272e2e4,
va_cols=va_cols@entry=0x0,
acquirefunc=<optimized out>, relpages=19, inh=inh@entry=false, in_outer_xact=false, elevel=13) at analyze.c:643
#42 0x0000555e8a18e122 in analyze_rel (relid=<optimized out>, relation=<optimized out>,
params=params@entry=0x555eb272e2e4,va_cols=0x0,
in_outer_xact=<optimized out>, bstrategy=bstrategy@entry=0x555eb2741238) at analyze.c:249
#43 0x0000555e8a1fc883 in vacuum (relations=0x555eb274b370, relations@entry=0x555eb2749390,
params=params@entry=0x555eb272e2e4,
bstrategy=bstrategy@entry=0x555eb2741238, vac_context=vac_context@entry=0x555eb274b220,
isTopLevel=isTopLevel@entry=true)at vacuum.c:635
#44 0x0000555e8a330e69 in autovacuum_do_vac_analyze (bstrategy=<optimized out>, tab=<optimized out>) at
autovacuum.c:3108
#45 do_autovacuum () at autovacuum.c:2425
#46 0x0000555e8a33132f in AutoVacWorkerMain (startup_data=<optimized out>, startup_data_len=<optimized out>) at
autovacuum.c:1571
...
(gdb) p num_held_lwlocks
$2 = 1
On Sun, Nov 03, 2024 at 10:29:25AM -0800, Noah Misch wrote:
> Pushed as 0bada39.
>
> Buildfarm member hornet REL_15_STABLE was in the same hang. Other buildfarm
> runs 2024-10-25T13:51:02Z - 2024-11-02T16:04:56Z may hang the same way. It's
> early to make a comprehensive list of hung buildfarm members, since many
> reported infrequently even before this period. I'll wait a week or two and
> then contact the likely-hung member owners. I regret the damage.
Buildfarm members plover and (less likely) sevengill might be hung in this
way. Owners (bcc'd): if convenient, please check whether a buildfarm run that
started before 2024-11-02T16:04:56Z is still ongoing. If it is, please use
"kill -QUIT" on one of the long-running autovacuum workers.
If it's the same hang, one autovacuum worker will have stack frames like:
#37 0x0000555e8a4fabcb in CacheInvalidateHeapTupleCommon (prepare_callback=0x555e8a4fa320
<PrepareInplaceInvalidationState>,newtuple=newtuple@entry=0x0,
tuple=tuple@entry=0x7f168e9a74e8, relation=relation@entry=0x7f168f1738c8) at inval.c:1504
#38 0x0000555e8a094f7c in heap_inplace_update_and_unlock (relation=0x7f168f1738c8, oldtup=0x555eb277ea58,
tuple=0x7f168e9a74e8,buffer=3) at heapam.c:6354