pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Fix resource leaks in PL/Python error reporting, redux. 7+ messages / 1 participants [nested] [flat]
* pgsql: Fix resource leaks in PL/Python error reporting, redux. @ 2025-10-23 15:48 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2025-10-23 15:48 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix resource leaks in PL/Python error reporting, redux. Commit c6f7f11d8 intended to prevent leaking any PyObject reference counts in edge cases (such as out-of-memory during string construction), but actually it introduced a leak in the normal case. Repeating an error-trapping operation often enough would lead to session-lifespan memory bloat. The problem is that I failed to think about the fact that PyObject_GetAttrString() increments the refcount of the returned PyObject, so that simply walking down the list of error frame objects causes all but the first one to have their refcount incremented. I experimented with several more-or-less-complex ways around that, and eventually concluded that the right fix is simply to drop the newly-obtained refcount as soon as we walk to the next frame object in PLy_traceback. This sounds unsafe, but it's perfectly okay because the caller holds a refcount on the first frame object and each frame object holds a refcount on the next one; so the current frame object can't disappear underneath us. By the same token, we can simplify the caller's cleanup back to simply dropping its refcount on the first object. Cleanup of each frame object will lead in turn to the refcount of the next one going to zero. I also added a couple of comments explaining why PLy_elog_impl() doesn't try to free the strings acquired from PLy_get_spi_error_data() or PLy_get_error_data(). That's because I got here by looking at a Coverity complaint about how those strings might get leaked. They are not leaked, but in testing that I discovered this other leak. Back-patch, as c6f7f11d8 was. It's a bit nervous-making to be putting such a fix into v13, which is only a couple weeks from its final release; but I can't see that leaving a recently-introduced leak in place is a better idea. Author: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/1203918.1761184159@sss.pgh.pa.us Backpatch-through: 13 Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/798b19d27b28e6dec3ae5134d396882d3197dfe8 Modified Files -------------- src/pl/plpython/plpy_elog.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Fix resource leaks in PL/Python error reporting, redux. @ 2025-10-23 15:48 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2025-10-23 15:48 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix resource leaks in PL/Python error reporting, redux. Commit c6f7f11d8 intended to prevent leaking any PyObject reference counts in edge cases (such as out-of-memory during string construction), but actually it introduced a leak in the normal case. Repeating an error-trapping operation often enough would lead to session-lifespan memory bloat. The problem is that I failed to think about the fact that PyObject_GetAttrString() increments the refcount of the returned PyObject, so that simply walking down the list of error frame objects causes all but the first one to have their refcount incremented. I experimented with several more-or-less-complex ways around that, and eventually concluded that the right fix is simply to drop the newly-obtained refcount as soon as we walk to the next frame object in PLy_traceback. This sounds unsafe, but it's perfectly okay because the caller holds a refcount on the first frame object and each frame object holds a refcount on the next one; so the current frame object can't disappear underneath us. By the same token, we can simplify the caller's cleanup back to simply dropping its refcount on the first object. Cleanup of each frame object will lead in turn to the refcount of the next one going to zero. I also added a couple of comments explaining why PLy_elog_impl() doesn't try to free the strings acquired from PLy_get_spi_error_data() or PLy_get_error_data(). That's because I got here by looking at a Coverity complaint about how those strings might get leaked. They are not leaked, but in testing that I discovered this other leak. Back-patch, as c6f7f11d8 was. It's a bit nervous-making to be putting such a fix into v13, which is only a couple weeks from its final release; but I can't see that leaving a recently-introduced leak in place is a better idea. Author: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/1203918.1761184159@sss.pgh.pa.us Backpatch-through: 13 Branch ------ REL_17_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/fbc41a145ae00166b8cdd22c236566e76864732c Modified Files -------------- src/pl/plpython/plpy_elog.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Fix resource leaks in PL/Python error reporting, redux. @ 2025-10-23 15:48 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2025-10-23 15:48 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix resource leaks in PL/Python error reporting, redux. Commit c6f7f11d8 intended to prevent leaking any PyObject reference counts in edge cases (such as out-of-memory during string construction), but actually it introduced a leak in the normal case. Repeating an error-trapping operation often enough would lead to session-lifespan memory bloat. The problem is that I failed to think about the fact that PyObject_GetAttrString() increments the refcount of the returned PyObject, so that simply walking down the list of error frame objects causes all but the first one to have their refcount incremented. I experimented with several more-or-less-complex ways around that, and eventually concluded that the right fix is simply to drop the newly-obtained refcount as soon as we walk to the next frame object in PLy_traceback. This sounds unsafe, but it's perfectly okay because the caller holds a refcount on the first frame object and each frame object holds a refcount on the next one; so the current frame object can't disappear underneath us. By the same token, we can simplify the caller's cleanup back to simply dropping its refcount on the first object. Cleanup of each frame object will lead in turn to the refcount of the next one going to zero. I also added a couple of comments explaining why PLy_elog_impl() doesn't try to free the strings acquired from PLy_get_spi_error_data() or PLy_get_error_data(). That's because I got here by looking at a Coverity complaint about how those strings might get leaked. They are not leaked, but in testing that I discovered this other leak. Back-patch, as c6f7f11d8 was. It's a bit nervous-making to be putting such a fix into v13, which is only a couple weeks from its final release; but I can't see that leaving a recently-introduced leak in place is a better idea. Author: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/1203918.1761184159@sss.pgh.pa.us Backpatch-through: 13 Branch ------ REL_14_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/7fb8c924042ea918ab95cfe757d60f8953b6c61c Modified Files -------------- src/pl/plpython/plpy_elog.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Fix resource leaks in PL/Python error reporting, redux. @ 2025-10-23 15:48 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2025-10-23 15:48 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix resource leaks in PL/Python error reporting, redux. Commit c6f7f11d8 intended to prevent leaking any PyObject reference counts in edge cases (such as out-of-memory during string construction), but actually it introduced a leak in the normal case. Repeating an error-trapping operation often enough would lead to session-lifespan memory bloat. The problem is that I failed to think about the fact that PyObject_GetAttrString() increments the refcount of the returned PyObject, so that simply walking down the list of error frame objects causes all but the first one to have their refcount incremented. I experimented with several more-or-less-complex ways around that, and eventually concluded that the right fix is simply to drop the newly-obtained refcount as soon as we walk to the next frame object in PLy_traceback. This sounds unsafe, but it's perfectly okay because the caller holds a refcount on the first frame object and each frame object holds a refcount on the next one; so the current frame object can't disappear underneath us. By the same token, we can simplify the caller's cleanup back to simply dropping its refcount on the first object. Cleanup of each frame object will lead in turn to the refcount of the next one going to zero. I also added a couple of comments explaining why PLy_elog_impl() doesn't try to free the strings acquired from PLy_get_spi_error_data() or PLy_get_error_data(). That's because I got here by looking at a Coverity complaint about how those strings might get leaked. They are not leaked, but in testing that I discovered this other leak. Back-patch, as c6f7f11d8 was. It's a bit nervous-making to be putting such a fix into v13, which is only a couple weeks from its final release; but I can't see that leaving a recently-introduced leak in place is a better idea. Author: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/1203918.1761184159@sss.pgh.pa.us Backpatch-through: 13 Branch ------ REL_18_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/447a794f6473f86457279f4a21673042271724d0 Modified Files -------------- src/pl/plpython/plpy_elog.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Fix resource leaks in PL/Python error reporting, redux. @ 2025-10-23 15:48 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2025-10-23 15:48 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix resource leaks in PL/Python error reporting, redux. Commit c6f7f11d8 intended to prevent leaking any PyObject reference counts in edge cases (such as out-of-memory during string construction), but actually it introduced a leak in the normal case. Repeating an error-trapping operation often enough would lead to session-lifespan memory bloat. The problem is that I failed to think about the fact that PyObject_GetAttrString() increments the refcount of the returned PyObject, so that simply walking down the list of error frame objects causes all but the first one to have their refcount incremented. I experimented with several more-or-less-complex ways around that, and eventually concluded that the right fix is simply to drop the newly-obtained refcount as soon as we walk to the next frame object in PLy_traceback. This sounds unsafe, but it's perfectly okay because the caller holds a refcount on the first frame object and each frame object holds a refcount on the next one; so the current frame object can't disappear underneath us. By the same token, we can simplify the caller's cleanup back to simply dropping its refcount on the first object. Cleanup of each frame object will lead in turn to the refcount of the next one going to zero. I also added a couple of comments explaining why PLy_elog_impl() doesn't try to free the strings acquired from PLy_get_spi_error_data() or PLy_get_error_data(). That's because I got here by looking at a Coverity complaint about how those strings might get leaked. They are not leaked, but in testing that I discovered this other leak. Back-patch, as c6f7f11d8 was. It's a bit nervous-making to be putting such a fix into v13, which is only a couple weeks from its final release; but I can't see that leaving a recently-introduced leak in place is a better idea. Author: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/1203918.1761184159@sss.pgh.pa.us Backpatch-through: 13 Branch ------ REL_15_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/4cde7325924e84fbaaa4e404944af7753aa88174 Modified Files -------------- src/pl/plpython/plpy_elog.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Fix resource leaks in PL/Python error reporting, redux. @ 2025-10-23 15:48 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2025-10-23 15:48 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix resource leaks in PL/Python error reporting, redux. Commit c6f7f11d8 intended to prevent leaking any PyObject reference counts in edge cases (such as out-of-memory during string construction), but actually it introduced a leak in the normal case. Repeating an error-trapping operation often enough would lead to session-lifespan memory bloat. The problem is that I failed to think about the fact that PyObject_GetAttrString() increments the refcount of the returned PyObject, so that simply walking down the list of error frame objects causes all but the first one to have their refcount incremented. I experimented with several more-or-less-complex ways around that, and eventually concluded that the right fix is simply to drop the newly-obtained refcount as soon as we walk to the next frame object in PLy_traceback. This sounds unsafe, but it's perfectly okay because the caller holds a refcount on the first frame object and each frame object holds a refcount on the next one; so the current frame object can't disappear underneath us. By the same token, we can simplify the caller's cleanup back to simply dropping its refcount on the first object. Cleanup of each frame object will lead in turn to the refcount of the next one going to zero. I also added a couple of comments explaining why PLy_elog_impl() doesn't try to free the strings acquired from PLy_get_spi_error_data() or PLy_get_error_data(). That's because I got here by looking at a Coverity complaint about how those strings might get leaked. They are not leaked, but in testing that I discovered this other leak. Back-patch, as c6f7f11d8 was. It's a bit nervous-making to be putting such a fix into v13, which is only a couple weeks from its final release; but I can't see that leaving a recently-introduced leak in place is a better idea. Author: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/1203918.1761184159@sss.pgh.pa.us Backpatch-through: 13 Branch ------ REL_13_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/d90c92d1c16dbd01026bbd3bca5d9e7b24dae0e1 Modified Files -------------- src/pl/plpython/plpy_elog.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
* pgsql: Fix resource leaks in PL/Python error reporting, redux. @ 2025-10-23 15:48 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2025-10-23 15:48 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Fix resource leaks in PL/Python error reporting, redux. Commit c6f7f11d8 intended to prevent leaking any PyObject reference counts in edge cases (such as out-of-memory during string construction), but actually it introduced a leak in the normal case. Repeating an error-trapping operation often enough would lead to session-lifespan memory bloat. The problem is that I failed to think about the fact that PyObject_GetAttrString() increments the refcount of the returned PyObject, so that simply walking down the list of error frame objects causes all but the first one to have their refcount incremented. I experimented with several more-or-less-complex ways around that, and eventually concluded that the right fix is simply to drop the newly-obtained refcount as soon as we walk to the next frame object in PLy_traceback. This sounds unsafe, but it's perfectly okay because the caller holds a refcount on the first frame object and each frame object holds a refcount on the next one; so the current frame object can't disappear underneath us. By the same token, we can simplify the caller's cleanup back to simply dropping its refcount on the first object. Cleanup of each frame object will lead in turn to the refcount of the next one going to zero. I also added a couple of comments explaining why PLy_elog_impl() doesn't try to free the strings acquired from PLy_get_spi_error_data() or PLy_get_error_data(). That's because I got here by looking at a Coverity complaint about how those strings might get leaked. They are not leaked, but in testing that I discovered this other leak. Back-patch, as c6f7f11d8 was. It's a bit nervous-making to be putting such a fix into v13, which is only a couple weeks from its final release; but I can't see that leaving a recently-introduced leak in place is a better idea. Author: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/1203918.1761184159@sss.pgh.pa.us Backpatch-through: 13 Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/cbfd4d0f883d5214d06a112912227fa41fc60fb6 Modified Files -------------- src/pl/plpython/plpy_elog.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2025-10-23 15:48 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-10-23 15:48 pgsql: Fix resource leaks in PL/Python error reporting, redux. Tom Lane <tgl@sss.pgh.pa.us> 2025-10-23 15:48 pgsql: Fix resource leaks in PL/Python error reporting, redux. Tom Lane <tgl@sss.pgh.pa.us> 2025-10-23 15:48 pgsql: Fix resource leaks in PL/Python error reporting, redux. Tom Lane <tgl@sss.pgh.pa.us> 2025-10-23 15:48 pgsql: Fix resource leaks in PL/Python error reporting, redux. Tom Lane <tgl@sss.pgh.pa.us> 2025-10-23 15:48 pgsql: Fix resource leaks in PL/Python error reporting, redux. Tom Lane <tgl@sss.pgh.pa.us> 2025-10-23 15:48 pgsql: Fix resource leaks in PL/Python error reporting, redux. Tom Lane <tgl@sss.pgh.pa.us> 2025-10-23 15:48 pgsql: Fix resource leaks in PL/Python error reporting, redux. 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