Fix resource leaks#514
Conversation
A few functions failed to release resources in early returns. I noticed the one with row filters while working on snapshot management changes in PostgreSQL when I added an assertion that when a historic snapshot is free'd, it must not still be marked as active. The leaks in row filter processing are otherwise harmlesss AFAICS, but if a lot of rows are filtered out, the resource owner holding all the resources can grow very large. The other leak is with multi-inserts to a table. I'm not sure what the effect of that is, I found it by searching for other uses of PushActiveSnapshot.
There was a problem hiding this comment.
Pull request overview
This PR fixes resource leaks in two locations where functions failed to release resources (specifically, active snapshots) before early returns. The leaks could cause resource owners to grow very large when many rows are filtered out or when multi-insert operations return early.
Changes:
- Fixed snapshot leak in row filter processing by deferring the return until after cleanup
- Fixed snapshot leak in multi-insert early return by adding proper resource cleanup
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pglogical_output_plugin.c | Refactored row filter logic to ensure PopActiveSnapshot is called before early return when filter evaluation fails |
| pglogical_apply.c | Added missing resource cleanup (relation close, snapshot pop, command counter increment) before early return in multi-insert path |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| apply_api.multi_insert_add_tuple(rel, &newtup); | ||
| last_insert_rel_cnt++; | ||
|
|
||
| pglogical_relation_close(rel, NoLock); |
There was a problem hiding this comment.
This does:
table_close(rel->rel, lockmode);
rel->rel = NULL;
A subsequent call chain of handle_relation() -> multi_insert_finish() ->
pglogical_apply_heap_mi_finish() -> pglogical_apply_heap_mi_flush() assumes
rel->rel is still valid.
| last_insert_rel_cnt++; | ||
|
|
||
| pglogical_relation_close(rel, NoLock); | ||
| PopActiveSnapshot(); |
There was a problem hiding this comment.
pglogical_apply_heap_mi_add() may have acquired the buffered tuple from
ExecBRInsertTriggers(). It may contain TOAST pointers that rely on the
snapshot. If that's right, we need to delay this till multi_insert_finish().
Given the leak of this snapshot ref, I'm surprised we're not seeing WARNING
"snapshot %p still active". Do you have a test case that did see it?
|
|
||
| pglogical_relation_close(rel, NoLock); | ||
| PopActiveSnapshot(); | ||
| CommandCounterIncrement(); |
There was a problem hiding this comment.
ExecInsert() doesn't CCI between rows, so I think refraining here is more
defensible.
There was a problem hiding this comment.
I agree with this change. Here too, do you have a test case that saw WARNING
"snapshot %p still active"?
A few functions failed to release resources in early returns. I noticed the one with row filters while working on snapshot management changes in PostgreSQL when I added an assertion that when a historic snapshot is free'd, it must not still be marked as active. The leaks in row filter processing are otherwise harmlesss AFAICS, but if a lot of rows are filtered out, the resource owner holding all the resources can grow very large.
The other leak is with multi-inserts to a table. I'm not sure what the effect of that is, I found it by searching for other uses of PushActiveSnapshot.