Skip to content

fix: Ensure MessageContext and its children are actually cleared between queries#783

Merged
tdrz merged 8 commits into
electric-sql:mainfrom
marcus-pousette:chore/submodule-bump-postgres-pglite-ed3ed3de5c
Sep 8, 2025
Merged

fix: Ensure MessageContext and its children are actually cleared between queries#783
tdrz merged 8 commits into
electric-sql:mainfrom
marcus-pousette:chore/submodule-bump-postgres-pglite-ed3ed3de5c

Conversation

@marcus-pousette

Copy link
Copy Markdown
Contributor

Fixes a memory leak

#779

@marcus-pousette marcus-pousette changed the title Ensure MessageContext and its children are actually cleared between queries fix: ensure MessageContext and its children are actually cleared between queries Sep 6, 2025
@marcus-pousette marcus-pousette changed the title fix: ensure MessageContext and its children are actually cleared between queries fix: Ensure MessageContext and its children are actually cleared between queries Sep 6, 2025
@tdrz

tdrz commented Sep 8, 2025

Copy link
Copy Markdown
Collaborator

Hey @marcus-pousette

Thanks a lot for this!

Looking through PostgreSQL's git log:

commit 6a72c42fd5af7ada49584694f543eb06dddb4a87
Author: Nathan Bossart <nathan@postgresql.org>
Date:   Wed Nov 15 13:42:30 2023 -0600

    Retire MemoryContextResetAndDeleteChildren() macro.
    
    As of commit eaa5808e8e, MemoryContextResetAndDeleteChildren() is
    just a backwards compatibility macro for MemoryContextReset().  Now
    that some time has passed, this macro seems more likely to create
    confusion.
    
    This commit removes the macro and replaces all remaining uses with
    calls to MemoryContextReset().  Any third-party code that use this
    macro will need to be adjusted to call MemoryContextReset()
    instead.  Since the two have behaved the same way since v9.5, such
    adjustments won't produce any behavior changes for all
    currently-supported versions of PostgreSQL.
    
    Reviewed-by: Amul Sul, Tom Lane, Alvaro Herrera, Dagfinn Ilmari Mannsåker
    Discussion: https://postgr.es/m/20231113185950.GA1668018%40nathanxps13

I believe best would be to use the MemoryContextReset and completely remove MemoryContextResetAndDeleteChildren.

@marcus-pousette

Copy link
Copy Markdown
Contributor Author

@tdrz good find. I did another commit and all the tests seems to pass after rebuild

@tdrz tdrz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move the changeset to the top folder of this repo. You can run the changeset command at the top and remove this local .changeset folder.

@marcus-pousette Almost there! Just this one left.

@tdrz

tdrz commented Sep 8, 2025

Copy link
Copy Markdown
Collaborator

@marcus-pousette Please run a pnpm -r stylecheck --write at the top level and push again.

@marcus-pousette

marcus-pousette commented Sep 8, 2025

Copy link
Copy Markdown
Contributor Author

I maybe got it alright now. I was initially little confused with that we are working with a git submodule and the testing is mainly performed in the parent module. I tried initially to create all the testing in the submodule, and isolate it there but that feels like a bigger task. (setting up the env etc)

@marcus-pousette

Copy link
Copy Markdown
Contributor Author

hmm maybe bump waitForPort to 15s?

@marcus-pousette

Copy link
Copy Markdown
Contributor Author

Did a quick PR. #786

@tdrz tdrz merged commit f12a582 into electric-sql:main Sep 8, 2025
18 checks passed
@tdrz

tdrz commented Sep 8, 2025

Copy link
Copy Markdown
Collaborator

@marcus-pousette My bad, the changeset should have been "patch" instead of "minor"

@marcus-pousette

marcus-pousette commented Sep 8, 2025

Copy link
Copy Markdown
Contributor Author

ok no problem! I did another commit to my PR/Fork branch marcus-pousette@26d432a @tdrz

@pjrule

pjrule commented Sep 8, 2025

Copy link
Copy Markdown

Thanks so much for this! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants