pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Preserve CurrentMemoryContext across Start/CommitTransactionComm 7+ messages / 1 participants [nested] [flat]
* pgsql: Preserve CurrentMemoryContext across Start/CommitTransactionComm @ 2024-07-01 15:55 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-07-01 15:55 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Preserve CurrentMemoryContext across Start/CommitTransactionCommand. Up to now, committing a transaction has caused CurrentMemoryContext to get set to TopMemoryContext. Most callers did not pay any particular heed to this, which is problematic because TopMemoryContext is a long-lived context that never gets reset. If the caller assumes it can leak memory because it's running in a limited-lifespan context, that behavior translates into a session-lifespan memory leak. The first-reported instance of this involved ProcessIncomingNotify, which is called from the main processing loop that normally runs in MessageContext. That outer-loop code assumes that whatever it allocates will be cleaned up when we're done processing the current client message --- but if we service a notify interrupt, then whatever gets allocated before the next switch to MessageContext will be permanently leaked in TopMemoryContext. sinval catchup interrupts have a similar problem, and I strongly suspect that some places in logical replication do too. To fix this in a generic way, let's redefine the behavior as "CommitTransactionCommand restores the memory context that was current at entry to StartTransactionCommand". This clearly fixes the issue for the notify and sinval cases, and it seems to match the mental model that's in use in the logical replication code, to the extent that anybody thought about it there at all. For consistency, likewise make subtransaction exit restore the context that was current at subtransaction start (rather than always selecting the CurTransactionContext of the parent transaction level). This case has less risk of resulting in a permanent leak than the outer-level behavior has, but it would not meet the principle of least surprise for some CommitTransactionCommand calls to restore the previous context while others don't. While we're here, also change xact.c so that we reset TopTransactionContext at transaction exit and then re-use it in later transactions, rather than dropping and recreating it in each cycle. This probably doesn't save a lot given the context recycling mechanism in aset.c, but it should save a little bit. Per suggestion from David Rowley. (Parenthetically, the text in src/backend/utils/mmgr/README implies that this is how I'd planned to implement it as far back as commit 1aebc3618 --- but the code actually added in that commit just drops and recreates it each time.) This commit also cleans up a few places outside xact.c that were needlessly making TopMemoryContext current, and thus risking more leaks of the same kind. I don't think any of them represent significant leak risks today, but let's deal with them while the issue is top-of-mind. Per bug #18512 from wizardbrony. Commit to HEAD only, as this change seems to have some risk of breaking things for some callers. We'll apply a narrower fix for the known-broken cases in the back branches. Discussion: https://postgr.es/m/3478884.1718656625@sss.pgh.pa.us Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/1afe31f03cd268a0bbb7a340d56b8eef6419bcb0 Modified Files -------------- src/backend/access/transam/xact.c | 101 +++++++++++++++++++++++++----------- src/backend/replication/walsender.c | 6 +-- src/backend/tcop/backend_startup.c | 8 ++- src/backend/tcop/postgres.c | 2 +- src/backend/utils/mmgr/mcxt.c | 3 +- 5 files changed, 80 insertions(+), 40 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt @ 2024-07-01 16:21 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-07-01 16:21 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Preserve CurrentMemoryContext across notify and sinval interrupts. ProcessIncomingNotify is called from the main processing loop that normally runs in MessageContext. That outer-loop code assumes that whatever it allocates will be cleaned up when we're done processing the current client message --- but if we service a notify interrupt, then whatever gets allocated before the next switch into MessageContext will be permanently leaked in TopMemoryContext, because CommitTransactionCommand sets CurrentMemoryContext to TopMemoryContext. There are observable leaks associated with (at least) encoding conversion of incoming queries and parameters attached to Bind messages. sinval catchup interrupts have a similar problem. There might be others, but I've not identified any other clear cases. To fix, take care to save and restore CurrentMemoryContext across the Start/CommitTransactionCommand calls in these functions. Per bug #18512 from wizardbrony. Commit to back branches only; in HEAD, this was dealt with by the riskier but more thoroughgoing approach in commit 1afe31f03. Discussion: https://postgr.es/m/3478884.1718656625@sss.pgh.pa.us Branch ------ REL_12_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/8565fb6fbb6908b6af0ad2035820cb7578f9bcb8 Modified Files -------------- src/backend/commands/async.c | 11 ++++++++++- src/backend/storage/ipc/sinval.c | 7 +++++++ 2 files changed, 17 insertions(+), 1 deletion(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt @ 2024-07-01 16:21 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-07-01 16:21 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Preserve CurrentMemoryContext across notify and sinval interrupts. ProcessIncomingNotify is called from the main processing loop that normally runs in MessageContext. That outer-loop code assumes that whatever it allocates will be cleaned up when we're done processing the current client message --- but if we service a notify interrupt, then whatever gets allocated before the next switch into MessageContext will be permanently leaked in TopMemoryContext, because CommitTransactionCommand sets CurrentMemoryContext to TopMemoryContext. There are observable leaks associated with (at least) encoding conversion of incoming queries and parameters attached to Bind messages. sinval catchup interrupts have a similar problem. There might be others, but I've not identified any other clear cases. To fix, take care to save and restore CurrentMemoryContext across the Start/CommitTransactionCommand calls in these functions. Per bug #18512 from wizardbrony. Commit to back branches only; in HEAD, this was dealt with by the riskier but more thoroughgoing approach in commit 1afe31f03. Discussion: https://postgr.es/m/3478884.1718656625@sss.pgh.pa.us Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/54a7b21b3a9dc4c8be5f7fec47e435ce5b1772bb Modified Files -------------- src/backend/commands/async.c | 11 ++++++++++- src/backend/storage/ipc/sinval.c | 7 +++++++ 2 files changed, 17 insertions(+), 1 deletion(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt @ 2024-07-01 16:21 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-07-01 16:21 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Preserve CurrentMemoryContext across notify and sinval interrupts. ProcessIncomingNotify is called from the main processing loop that normally runs in MessageContext. That outer-loop code assumes that whatever it allocates will be cleaned up when we're done processing the current client message --- but if we service a notify interrupt, then whatever gets allocated before the next switch into MessageContext will be permanently leaked in TopMemoryContext, because CommitTransactionCommand sets CurrentMemoryContext to TopMemoryContext. There are observable leaks associated with (at least) encoding conversion of incoming queries and parameters attached to Bind messages. sinval catchup interrupts have a similar problem. There might be others, but I've not identified any other clear cases. To fix, take care to save and restore CurrentMemoryContext across the Start/CommitTransactionCommand calls in these functions. Per bug #18512 from wizardbrony. Commit to back branches only; in HEAD, this was dealt with by the riskier but more thoroughgoing approach in commit 1afe31f03. Discussion: https://postgr.es/m/3478884.1718656625@sss.pgh.pa.us Branch ------ REL_14_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/1608902fc3a73b598dbc09922e877aee5da55cf5 Modified Files -------------- src/backend/commands/async.c | 11 ++++++++++- src/backend/storage/ipc/sinval.c | 7 +++++++ 2 files changed, 17 insertions(+), 1 deletion(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt @ 2024-07-01 16:21 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-07-01 16:21 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Preserve CurrentMemoryContext across notify and sinval interrupts. ProcessIncomingNotify is called from the main processing loop that normally runs in MessageContext. That outer-loop code assumes that whatever it allocates will be cleaned up when we're done processing the current client message --- but if we service a notify interrupt, then whatever gets allocated before the next switch into MessageContext will be permanently leaked in TopMemoryContext, because CommitTransactionCommand sets CurrentMemoryContext to TopMemoryContext. There are observable leaks associated with (at least) encoding conversion of incoming queries and parameters attached to Bind messages. sinval catchup interrupts have a similar problem. There might be others, but I've not identified any other clear cases. To fix, take care to save and restore CurrentMemoryContext across the Start/CommitTransactionCommand calls in these functions. Per bug #18512 from wizardbrony. Commit to back branches only; in HEAD, this was dealt with by the riskier but more thoroughgoing approach in commit 1afe31f03. Discussion: https://postgr.es/m/3478884.1718656625@sss.pgh.pa.us Branch ------ REL_17_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/31f8d620b287258a34a35c25868896a46deea14f Modified Files -------------- src/backend/commands/async.c | 11 ++++++++++- src/backend/storage/ipc/sinval.c | 7 +++++++ 2 files changed, 17 insertions(+), 1 deletion(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt @ 2024-07-01 16:21 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-07-01 16:21 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Preserve CurrentMemoryContext across notify and sinval interrupts. ProcessIncomingNotify is called from the main processing loop that normally runs in MessageContext. That outer-loop code assumes that whatever it allocates will be cleaned up when we're done processing the current client message --- but if we service a notify interrupt, then whatever gets allocated before the next switch into MessageContext will be permanently leaked in TopMemoryContext, because CommitTransactionCommand sets CurrentMemoryContext to TopMemoryContext. There are observable leaks associated with (at least) encoding conversion of incoming queries and parameters attached to Bind messages. sinval catchup interrupts have a similar problem. There might be others, but I've not identified any other clear cases. To fix, take care to save and restore CurrentMemoryContext across the Start/CommitTransactionCommand calls in these functions. Per bug #18512 from wizardbrony. Commit to back branches only; in HEAD, this was dealt with by the riskier but more thoroughgoing approach in commit 1afe31f03. Discussion: https://postgr.es/m/3478884.1718656625@sss.pgh.pa.us Branch ------ REL_13_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/5f86cd70d8010ae4162051d08112d6e1bbb6b0da Modified Files -------------- src/backend/commands/async.c | 11 ++++++++++- src/backend/storage/ipc/sinval.c | 7 +++++++ 2 files changed, 17 insertions(+), 1 deletion(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt @ 2024-07-01 16:21 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2024-07-01 16:21 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Preserve CurrentMemoryContext across notify and sinval interrupts. ProcessIncomingNotify is called from the main processing loop that normally runs in MessageContext. That outer-loop code assumes that whatever it allocates will be cleaned up when we're done processing the current client message --- but if we service a notify interrupt, then whatever gets allocated before the next switch into MessageContext will be permanently leaked in TopMemoryContext, because CommitTransactionCommand sets CurrentMemoryContext to TopMemoryContext. There are observable leaks associated with (at least) encoding conversion of incoming queries and parameters attached to Bind messages. sinval catchup interrupts have a similar problem. There might be others, but I've not identified any other clear cases. To fix, take care to save and restore CurrentMemoryContext across the Start/CommitTransactionCommand calls in these functions. Per bug #18512 from wizardbrony. Commit to back branches only; in HEAD, this was dealt with by the riskier but more thoroughgoing approach in commit 1afe31f03. Discussion: https://postgr.es/m/3478884.1718656625@sss.pgh.pa.us Branch ------ REL_15_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/4df767cf906d4406306f5853fea0e8133803c0f1 Modified Files -------------- src/backend/commands/async.c | 11 ++++++++++- src/backend/storage/ipc/sinval.c | 7 +++++++ 2 files changed, 17 insertions(+), 1 deletion(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-07-01 16:21 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-07-01 15:55 pgsql: Preserve CurrentMemoryContext across Start/CommitTransactionComm Tom Lane <tgl@sss.pgh.pa.us> 2024-07-01 16:21 pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt Tom Lane <tgl@sss.pgh.pa.us> 2024-07-01 16:21 pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt Tom Lane <tgl@sss.pgh.pa.us> 2024-07-01 16:21 pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt Tom Lane <tgl@sss.pgh.pa.us> 2024-07-01 16:21 pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt Tom Lane <tgl@sss.pgh.pa.us> 2024-07-01 16:21 pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt Tom Lane <tgl@sss.pgh.pa.us> 2024-07-01 16:21 pgsql: Preserve CurrentMemoryContext across notify and sinval interrupt Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox