Skip to content

Commit 90be71e

Browse files
authored
Fix memory leak for JSON/LIST type during Postgres table scan (#784)
Fixes #750
1 parent b48de06 commit 90be71e

File tree

6 files changed

+58
-5
lines changed

6 files changed

+58
-5
lines changed

include/pgduckdb/pg/declarations.hpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ typedef double Cardinality;
2525

2626
typedef uintptr_t Datum;
2727

28+
struct MemoryContextData;
29+
typedef MemoryContextData *MemoryContext;
30+
2831
struct FormData_pg_attribute;
2932
typedef FormData_pg_attribute *Form_pg_attribute;
3033

include/pgduckdb/pg/memory.hpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
#pragma once
2+
3+
#include "pgduckdb/pg/declarations.hpp"
4+
5+
namespace pgduckdb::pg {
6+
7+
MemoryContext MemoryContextCreate(MemoryContext parent, const char *name);
8+
MemoryContext MemoryContextSwitchTo(MemoryContext target);
9+
void MemoryContextReset(MemoryContext context);
10+
void MemoryContextDelete(MemoryContext context);
11+
12+
} // namespace pgduckdb::pg

include/pgduckdb/scan/postgres_scan.hpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ struct PostgresScanGlobalState : public duckdb::GlobalTableFunctionState {
3535
std::atomic<std::uint32_t> total_row_count;
3636
std::ostringstream scan_query;
3737
duckdb::shared_ptr<PostgresTableReader> table_reader_global_state;
38+
MemoryContext duckdb_scan_memory_ctx;
3839
};
3940

4041
// Local State

src/pg/memory.cpp

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
#include "pgduckdb/pgduckdb_utils.hpp"
2+
3+
extern "C" {
4+
#include "postgres.h"
5+
#include "utils/memutils.h"
6+
#include "utils/palloc.h"
7+
}
8+
9+
namespace pgduckdb::pg {
10+
11+
MemoryContext
12+
MemoryContextCreate(MemoryContext parent, const char *name) {
13+
return PostgresFunctionGuard(::AllocSetContextCreateInternal, parent, name, ALLOCSET_DEFAULT_MINSIZE,
14+
ALLOCSET_DEFAULT_INITSIZE, ALLOCSET_DEFAULT_MAXSIZE);
15+
}
16+
17+
MemoryContext
18+
MemoryContextSwitchTo(MemoryContext target) {
19+
return PostgresFunctionGuard(::MemoryContextSwitchTo, target);
20+
}
21+
22+
void
23+
MemoryContextReset(MemoryContext context) {
24+
PostgresFunctionGuard(::MemoryContextReset, context);
25+
}
26+
27+
void
28+
MemoryContextDelete(MemoryContext context) {
29+
PostgresFunctionGuard(::MemoryContextDelete, context);
30+
}
31+
32+
} // namespace pgduckdb::pg

src/pgduckdb_types.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1506,10 +1506,10 @@ AppendString(duckdb::Vector &result, Datum value, idx_t offset, bool is_bpchar)
15061506
static void
15071507
AppendJsonb(duckdb::Vector &result, Datum value, idx_t offset) {
15081508
auto jsonb = DatumGetJsonbP(value);
1509-
auto jsonb_str = JsonbToCString(NULL, &jsonb->root, VARSIZE(jsonb));
1510-
duckdb::string_t str(jsonb_str);
1509+
StringInfo str = PostgresFunctionGuard(makeStringInfo);
1510+
auto json_str = PostgresFunctionGuard(JsonbToCString, str, &jsonb->root, VARSIZE(jsonb));
15111511
auto data = duckdb::FlatVector::GetData<duckdb::string_t>(result);
1512-
data[offset] = duckdb::StringVector::AddString(result, str);
1512+
data[offset] = duckdb::StringVector::AddString(result, json_str, str->len);
15131513
}
15141514

15151515
static void
@@ -1820,13 +1820,13 @@ ConvertPostgresToDuckValue(Oid attr_type, Datum value, duckdb::Vector &result, i
18201820
int16 typlen;
18211821
bool typbyval;
18221822
char typalign;
1823-
get_typlenbyvalalign(elem_type, &typlen, &typbyval, &typalign);
1823+
PostgresFunctionGuard(get_typlenbyvalalign, elem_type, &typlen, &typbyval, &typalign);
18241824

18251825
int nelems;
18261826
Datum *elems;
18271827
bool *nulls;
18281828
// Deconstruct the array into Datum elements
1829-
deconstruct_array(array, elem_type, typlen, typbyval, typalign, &elems, &nulls, &nelems);
1829+
PostgresFunctionGuard(deconstruct_array, array, elem_type, typlen, typbyval, typalign, &elems, &nulls, &nelems);
18301830

18311831
if (ndims == -1) {
18321832
throw duckdb::InternalException("Array type has an ndims of -1, so it's actually not an array??");

src/scan/postgres_scan.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#include "pgduckdb/scan/postgres_table_reader.hpp"
55
#include "pgduckdb/pgduckdb_types.hpp"
66
#include "pgduckdb/pgduckdb_utils.hpp"
7+
#include "pgduckdb/pg/memory.hpp"
78
#include "pgduckdb/pg/relations.hpp"
89

910
#include "pgduckdb/pgduckdb_process_lock.hpp"
@@ -170,6 +171,7 @@ PostgresScanGlobalState::PostgresScanGlobalState(Snapshot _snapshot, Relation _r
170171
ConstructTableScanQuery(input);
171172
table_reader_global_state =
172173
duckdb::make_shared_ptr<PostgresTableReader>(scan_query.str().c_str(), count_tuples_only);
174+
duckdb_scan_memory_ctx = pg::MemoryContextCreate(CurrentMemoryContext, "DuckdbScanContext");
173175
pd_log(DEBUG2, "(DuckDB/PostgresSeqScanGlobalState) Running %" PRIu64 " threads -- ", (uint64_t)MaxThreads());
174176
}
175177

@@ -257,6 +259,7 @@ PostgresScanTableFunction::PostgresScanFunction(duckdb::ClientContext &, duckdb:
257259
local_state.output_vector_size = 0;
258260

259261
std::lock_guard<std::recursive_mutex> lock(GlobalProcessLock::GetLock());
262+
MemoryContext old_context = pg::MemoryContextSwitchTo(local_state.global_state->duckdb_scan_memory_ctx);
260263
for (size_t i = 0; i < STANDARD_VECTOR_SIZE; i++) {
261264
TupleTableSlot *slot = local_state.global_state->table_reader_global_state->GetNextTuple();
262265
if (pgduckdb::TupleIsNull(slot)) {
@@ -269,6 +272,8 @@ PostgresScanTableFunction::PostgresScanFunction(duckdb::ClientContext &, duckdb:
269272
InsertTupleIntoChunk(output, local_state, slot);
270273
}
271274

275+
pg::MemoryContextReset(local_state.global_state->duckdb_scan_memory_ctx);
276+
pg::MemoryContextSwitchTo(old_context);
272277
SetOutputCardinality(output, local_state);
273278
}
274279

0 commit comments

Comments
 (0)