Fix memory management for postgres in edge cases

[mcraveiro]

Hi sqlgen developers,

As discussed in [1], I encountered some issues with memory management for Postgres in v0.3.0 and v0.4.0. If you recall, I had created a patch [2] to fix those problems. When I tried to raise a PR I noticed main has moved on a fair bit since then, so some parts of the patch are no longer needed and those that still apply needed some changes. At any rate, I updated the patch to fix the current state of main, which this PR contains for your perusal.

With regards to RAII: the code is a tad complex and I am by no means an expert in libpq, so I'm afraid I was not brave enough to attempt implementing this as you had suggested in the discussion (a-la [3]). At any rate, even if you embark on such a clean up, I think it's still best to apply some version of this patch since it at least solves the immediate problems and makes the "correct fix" slightly less urgent.

One final point: You may find the ROLLBACK a bit puzzling. I added it on the advice of Gemini, as follows (verbatim):

2. Missing Transaction Cleanup in insert_impl()

If insert_impl() is called within a PostgreSQL transaction, an error during the PREPARE or the row insertions should cause the transaction to be rolled back. The current implementation only deallocates the prepared statement (DEALLOCATE).

// On error (e.g., lines 72, 80, 98), you should consider:
execute("ROLLBACK;");
execute("DEALLOCATE sqlgen_insert_into_table;"); // If applicable

While not strictly a memory leak, it's a critical resource/state management issue. If the function fails, the caller is left with a half-executed and possibly failed transaction, which could lock resources or lead to inconsistent data unless the caller explicitly handles the rollback (which good RAII wrappers often try to handle internally).

[1] #88
[2] Proposed fix to this round of leaks
[3] DuckDBResult.hpp

[mcraveiro]

Copy command

For what it's worth, here is the Gemini advice on the COPY command:

When using the COPY protocol functions like PQputCopyData(), if an error occurs (success != 1), the documentation recommends closing the copy stream with PQputCopyEnd() and then checking for results to clear any potential errors queued up on the connection.

While you call PQputCopyEnd(), you don't retrieve and clear the result that follows the PQputCopyEnd() call (similar to what you correctly do in end_write()).

Function	Line(s)	Issue Description
write_impl()	179-183	If PQputCopyData() fails, you call PQputCopyEnd(), but then immediately return. The PQputCopyEnd() operation will place a final status/error result on the connection, which is never retrieved and cleared, leading to an unhandled result object remaining on the connection.

[liuzicheng1987]

@mcraveiro , cool, thank you so much.

I will merge this and over the next couple of days I will implement a more robust RAII-based strategy for all database connectors, to make sure sure we cover all potential edge cases that we might see in the future.

But thank you so much for raising this and for providing a fix.