forked frompostgres/postgres
- Notifications
You must be signed in to change notification settings - Fork6
Commit85ccb68
committed
Rearrange pgstat_bestart() to avoid failures within its critical section.
We long ago decided to design the shared PgBackendStatus data structure tominimize the cost of writing status updates, which means that writers justhave to increment the st_changecount field twice. That isn't hooked intoany sort of resource management mechanism, which means that if somethingwere to throw error between the two increments, the st_changecount fieldwould be left odd indefinitely. That would cause readers to lock up.Now, since it's also a bad idea to leave the field odd for longer thanabsolutely necessary (because readers will spin while we have it set),the expectation was that we'd treat these segments like spinlock criticalsections, with only short, more or less straight-line, code in them.That was fine as originally designed, but commit9029f4b broke itby inserting a significant amount of non-straight-line code intopgstat_bestart(), code that is very capable of throwing errors, not tomention taking a significant amount of time during which readers will spin.We have a report from Neeraj Kumar of readers actually locking up, whichI suspect was due to an encoding conversion error in X509_NAME_to_cstring,though conceivably it was just a garden-variety OOM failure.Subsequent commits have loaded even more dubious code into pgstat_bestart'scritical section (and commitfc70a4b deserves some kind of booby prizefor managing to miss the critical section entirely, although the negativeconsequences seem minimal given that the PgBackendStatus entry should beseen by readers as inactive at that point).The right way to fix this mess seems to be to compute all these valuesinto a local copy of the process' PgBackendStatus struct, and then justcopy the data back within the critical section proper. This plan can'tbe implemented completely cleanly because of the struct's heavy relianceon out-of-line strings, which we must initialize separately within thecritical section. But still, the critical section is far smaller andsafer than it was before.In hopes of forestalling future errors of the same ilk, rename themacros for st_changecount management to make it more apparent thatthe writer-side macros create a critical section. And to preventthe worst consequences if we nonetheless manage to mess it up anyway,adjust those macros so that they really are a critical section, iethey now bump CritSectionCount. That doesn't add much overhead, andit guarantees that if we do somehow throw an error while the counteris odd, it will lead to PANIC and a database restart to reset sharedmemory.Back-patch to 9.5 where the problem was introduced.In HEAD, also fix an oversight in commitb0b39f7: it failed to teachpgstat_read_current_status to copy st_gssstatus data from shared memory tolocal memory. Hence, subsequent use of that data within the transactionwould potentially see changing data that it shouldn't see.Discussion:https://postgr.es/m/CAPR3Wj5Z17=+eeyrn_ZDG3NQGYgMEOY6JV6Y-WRRhGgwc16U3Q@mail.gmail.com1 parent4217d15 commit85ccb68