Fixes #3573

• Fix: Avoid connection leaks by ensuring parse closure on error.
• Refactor: Move Writer instantiation to improve isolation and prevent state reuse issues.

Choose a reason for hiding this comment

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

@charmander

I would really appreciate to get a feedback first, that those changes are in right direction. Those two changes are tight together as they both are related to serialization issue. Just because I solved global writer issue first, zombie client appeared next in PgPool, because without fixing writers you just write sync/close in next query

I'm not an expert of pg protocol and just investigated this issue few hours.
Maybe there were a reasons to use shared writers? If not and those changes makes sence, would be happy to do what you request

Yes, these changes make sense. The shared writer was probably an attempt at improving performance.

• Refactor: Move Writer instantiation to improve isolation and prevent state reuse issues.

If this is something which can be acomplished without reinstantiating the writer I would greatly prefer it. This is extremely hot path code. Is there a way to fix this w/o? Also: needs tests before merging.

Thanks for the eyes on this tho! I'll leave it open for a bit as it might be something I can quickly fix (IF there is a repoducable test case on how to trigger the issue)

@charmander @brianc

Sorry, I missed these comments, my bad.

I'll try to take a look on this back in a next few weeks (split to different commits, adding tests)

Re

If this is something which can be acomplished without reinstantiating the writer I would greatly prefer it.

I think it should be doable without reinstantiating the writer, however the fix will be fragile then for any future changes. Also code will be a bit ugly (each time when you have some error you should not forget to clean it properly). I'll try to play around it and come back with something

I think it should be doable without reinstantiating the writer, however the fix will be fragile then for any future changes. Also code will be a bit ugly (each time when you have some error you should not forget to clean it properly). I'll try to play around it and come back with something

sounds good - i think we can just re-instantiate it then. allocating something is pretty cheap.

Is it possible to write a couple tests for this? I'd like to get it merged.