From 24a6a7a11b33e72f368343613ce442c50e1391ae Mon Sep 17 00:00:00 2001 From: Greg Sabino Mullane Date: Wed, 1 Jul 2026 14:06:02 -0400 Subject: [PATCH 1/3] Use new string buffer system, to enable arrays rather than linked list. Based on the work from https://github.com/bucardo/dbdpg/pull/21 Author: Matt Tyson Co-authored-by: Greg Sabino Mullane --- MANIFEST | 2 + Makefile.PL | 2 +- Pg.h | 1 + README.dev | 4 + dbdimp.c | 626 ++++++++++++++++++++++++---------------------- dbdimp.h | 43 ++-- strbuf.c | 110 ++++++++ strbuf.h | 17 ++ t/03smethod.t | 11 +- t/99_spellcheck.t | 3 + 10 files changed, 501 insertions(+), 318 deletions(-) create mode 100644 strbuf.c create mode 100644 strbuf.h diff --git a/MANIFEST b/MANIFEST index 9dc6c422..80f99763 100644 --- a/MANIFEST +++ b/MANIFEST @@ -25,6 +25,8 @@ Pg.xs dbivport.h dbdimp.c dbdimp.h +strbuf.h +strbuf.c types.c types.h quote.c diff --git a/Makefile.PL b/Makefile.PL index f80478b7..3168cc33 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -225,7 +225,7 @@ my %opts = NAME => 'DBD::Pg', VERSION_FROM => 'Pg.pm', INC => qq{-I"$POSTGRES_INCLUDE" -I"$dbi_arch_dir"}, - OBJECT => 'Pg$(OBJ_EXT) dbdimp$(OBJ_EXT) quote$(OBJ_EXT) types$(OBJ_EXT)', + OBJECT => 'Pg$(OBJ_EXT) dbdimp$(OBJ_EXT) quote$(OBJ_EXT) types$(OBJ_EXT) strbuf$(OBJ_EXT)', LIBS => ["-L\"$POSTGRES_LIB\" -lpq -lm"], AUTHOR => 'Greg Sabino Mullane', ABSTRACT => 'PostgreSQL database driver for the DBI module', diff --git a/Pg.h b/Pg.h index cf6bb0d0..2a04f817 100644 --- a/Pg.h +++ b/Pg.h @@ -66,6 +66,7 @@ DBISTATE_DECLARE; #include "types.h" #include "dbdimp.h" #include "quote.h" +#include "strbuf.h" #define TLEVEL_slow (DBIS->debug & DBIc_TRACE_LEVEL_MASK) #define TFLAGS_slow (DBIS->debug & DBIc_TRACE_FLAGS_MASK) diff --git a/README.dev b/README.dev index 156692bc..73ac855e 100644 --- a/README.dev +++ b/README.dev @@ -149,6 +149,10 @@ quote.c - Various methods to help quote and dequote variables. Some of this is quote.h - Header file for quote.c +strbuf.c - Small library for string buffer handling + +strbuf.h - Header file for strbuf.c + types.c - Lists all known data types for PostgreSQL. Can be run as a perl script to check for new types; rewrites the following: types.h types.c Pg.xs Pg.pm t/01constants.t 99_pod.t diff --git a/dbdimp.c b/dbdimp.c index 5585a07c..81eb3cf6 100644 --- a/dbdimp.c +++ b/dbdimp.c @@ -125,6 +125,100 @@ static int pg_db_start_txn (pTHX_ SV *dbh, imp_dbh_t *imp_dbh); static int handle_old_async(pTHX_ SV * handle, imp_dbh_t * imp_dbh, const int asyncflag); static void pg_db_detect_client_encoding_utf8(pTHX_ imp_dbh_t *imp_dbh); +static void ph_array_init(imp_sth_t *imp_sth) +{ + imp_sth->ph_array.length = 15; + imp_sth->ph_array.elements = 0; + Newz(0, imp_sth->ph_array.array, imp_sth->ph_array.length, ph_t); +} + +static void ph_array_append(imp_sth_t *imp_sth, ph_t *data) +{ + if (imp_sth->ph_array.length == imp_sth->ph_array.elements) { + /* The array is full, realloc the array to make it bigger */ + size_t new_length = imp_sth->ph_array.length; + if (new_length > SIZE_MAX / 2) + croak("ph_array_append: array too large"); + new_length *= 2; + Renew(imp_sth->ph_array.array, new_length, ph_t); + imp_sth->ph_array.length = (int)new_length; + } + + imp_sth->ph_array.array[imp_sth->ph_array.elements++] = *data; +} + +static ph_t* ph_array_element(imp_sth_t *imp_sth, int idx) +{ + return &(imp_sth->ph_array.array[idx]); +} + +static int ph_array_count(imp_sth_t *imp_sth) +{ + return imp_sth->ph_array.elements; +} + +static void ph_array_destroy(imp_sth_t *imp_sth) +{ + for (int i = 0; i < imp_sth->ph_array.elements; i++) { + ph_t *elem = &(imp_sth->ph_array.array[i]); + + Safefree(elem->fooname); + Safefree(elem->value); + Safefree(elem->quoted); + elem->bind_type = NULL; + } + + Safefree(imp_sth->ph_array.array); + imp_sth->ph_array.array = NULL; + imp_sth->ph_array.length = 0; + imp_sth->ph_array.elements = 0; +} + +static void seg_array_init(imp_sth_t *imp_sth) +{ + imp_sth->seg_array.length = 15; + imp_sth->seg_array.elements = 0; + Newz(0, imp_sth->seg_array.array, imp_sth->seg_array.length, seg_t); +} + +static void seg_array_append(imp_sth_t *imp_sth, seg_t *data) +{ + if (imp_sth->seg_array.length == imp_sth->seg_array.elements) { + /* The array is full, realloc the array to make it bigger */ + size_t new_length = imp_sth->seg_array.length; + if (new_length > SIZE_MAX / 2) + croak("seg_array_append: array too large"); + new_length *= 2; + Renew(imp_sth->seg_array.array, new_length, seg_t); + imp_sth->seg_array.length = (int)new_length; + } + + imp_sth->seg_array.array[imp_sth->seg_array.elements++] = *data; +} + +static seg_t* seg_array_element(imp_sth_t *imp_sth, int idx) +{ + return &(imp_sth->seg_array.array[idx]); +} + +static int seg_array_count(imp_sth_t *imp_sth) +{ + return imp_sth->seg_array.elements; +} + +static void seg_array_destroy(imp_sth_t *imp_sth) +{ + for (int i = 0; i < imp_sth->seg_array.elements; i++) { + seg_t *elem = &(imp_sth->seg_array.array[i]); + Safefree(elem->segment); + } + + Safefree(imp_sth->seg_array.array); + imp_sth->seg_array.array = NULL; + imp_sth->seg_array.length = 0; + imp_sth->seg_array.elements = 0; +} + static int do_send_cancel(SV *h, imp_dbh_t *imp_dbh, char const *caller) { dTHX; @@ -252,7 +346,7 @@ int dbd_db_login6 (SV * dbh, imp_dbh_t * imp_dbh, char * dbname, char * uid, cha connect_string_size += strlen("user='' ") + 2*strlen(uid); if (*pwd) connect_string_size += strlen("password='' ") + 2*strlen(pwd); - New(0, conn_str, connect_string_size+1, char); /* freed below */ + New(0, conn_str, connect_string_size+1, char); /* freed in dbd_st_destroy */ /* Change all semi-colons in dbname to a space, unless single-quoted */ dest = conn_str; @@ -1243,16 +1337,14 @@ SV * dbd_st_FETCH_attrib (SV * sth, imp_sth_t * imp_sth, SV * keysv) if (strEQ("pg_bound", key)) { HV *pvhv = newHV(); - ph_t *currph; - int i; - for (i=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph,i++) { - SV *key, *val; - key = pg_st_placeholder_key(imp_sth, currph, i); - val = newSViv(NULL == currph->bind_type ? 0 : 1); - if (! hv_store_ent(pvhv, key, val, 0)) { + for (int p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); + SV *phkey = pg_st_placeholder_key(imp_sth, currph, p); + SV *val = newSViv(NULL == currph->bind_type ? 0 : 1); + if (! hv_store_ent(pvhv, phkey, val, 0)) { SvREFCNT_dec(val); } - SvREFCNT_dec(key); + SvREFCNT_dec(phkey); } retsv = newRV_noinc((SV*)pvhv); } @@ -1271,14 +1363,13 @@ SV * dbd_st_FETCH_attrib (SV * sth, imp_sth_t * imp_sth, SV * keysv) if (strEQ("ParamTypes", key)) { HV *pvhv = newHV(); - ph_t *currph; - int i; - for (i=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph,i++) { - SV *key, *val; - key = pg_st_placeholder_key(imp_sth, currph, i); + for (int p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); + SV *phkey = pg_st_placeholder_key(imp_sth, currph, p); + SV *val; if (NULL == currph->bind_type) { val = newSV(0); - if (! hv_store_ent(pvhv, key, val, 0)) { + if (! hv_store_ent(pvhv, phkey, val, 0)) { SvREFCNT_dec(val); } } @@ -1291,11 +1382,11 @@ SV * dbd_st_FETCH_attrib (SV * sth, imp_sth_t * imp_sth, SV * keysv) (void)hv_store(pvhv2, "pg_type", 7, newSViv(currph->bind_type->type_id), 0); } val = newRV_noinc((SV*)pvhv2); - if (! hv_store_ent(pvhv, key, val, 0)) { + if (! hv_store_ent(pvhv, phkey, val, 0)) { SvREFCNT_dec(val); } } - SvREFCNT_dec(key); + SvREFCNT_dec(phkey); } retsv = newRV_noinc((SV*)pvhv); } @@ -1305,43 +1396,41 @@ SV * dbd_st_FETCH_attrib (SV * sth, imp_sth_t * imp_sth, SV * keysv) if (strEQ("ParamValues", key)) { HV *pvhv = newHV(); - ph_t *currph; - int i; - for (i=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph,i++) { - SV *key, *val; - key = pg_st_placeholder_key(imp_sth, currph, i); + for (int p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); + SV *phkey = pg_st_placeholder_key(imp_sth, currph, p); + SV *val; if (NULL == currph->value) { val = newSV(0); - if (!hv_store_ent(pvhv, key, val, 0)) { + if (!hv_store_ent(pvhv, phkey, val, 0)) { SvREFCNT_dec(val); } } else { val = newSVpv(currph->value,currph->valuelen); - if (!hv_store_ent(pvhv, key, val, 0)) { + if (!hv_store_ent(pvhv, phkey, val, 0)) { SvREFCNT_dec(val); } } - SvREFCNT_dec(key); + SvREFCNT_dec(phkey); } retsv = newRV_noinc((SV*)pvhv); } else if (strEQ("pg_segments", key)) { AV *arr = newAV(); - seg_t *currseg; - int i; - for (i=0,currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg,i++) { + for (int s=0; s < seg_array_count(imp_sth); s++) { + seg_t *currseg = seg_array_element(imp_sth, s); av_push(arr, newSVpv(currseg->segment ? currseg->segment : "NULL",0)); } retsv = newRV_noinc((SV*)arr); } else if (strEQ("pg_numbound", key)) { - ph_t *currph; - int i = 0; - for (currph=imp_sth->ph; NULL != currph; currph=currph->nextph) { - i += NULL == currph->bind_type ? 0 : 1; + int p, num = 0; + for (p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); + num += NULL == currph->bind_type ? 0 : 1; } - retsv = newSViv(i); + retsv = newSViv(num); } break; @@ -1497,10 +1586,10 @@ SV * dbd_st_FETCH_attrib (SV * sth, imp_sth_t * imp_sth, SV * keysv) TRACE_PQFTABLECOL; y = PQftablecol(imp_sth->result, fields); if (InvalidOid != o && y > 0) { /* We know what table and column this came from */ - char statement[128]; - sprintf(statement, "SELECT attnotnull FROM pg_catalog.pg_attribute WHERE attrelid=%u AND attnum=%d", o, y); + char sqlstring[128]; + sprintf(sqlstring, "SELECT attnotnull FROM pg_catalog.pg_attribute WHERE attrelid=%u AND attnum=%d", o, y); TRACE_PQEXEC; - result = PQexec(imp_dbh->conn, statement); + result = PQexec(imp_dbh->conn, sqlstring); TRACE_PQRESULTSTATUS; status = PQresultStatus(result); if (PGRES_TUPLES_OK == status) { @@ -1808,8 +1897,6 @@ int dbd_st_prepare_sv (SV * sth, imp_sth_t * imp_sth, SV * statement_sv, SV * at imp_sth->firstword = NULL; imp_sth->result = NULL; imp_sth->type_info = NULL; - imp_sth->seg = NULL; - imp_sth->ph = NULL; imp_sth->PQvals = NULL; imp_sth->PQlens = NULL; imp_sth->PQfmts = NULL; @@ -1824,6 +1911,10 @@ int dbd_st_prepare_sv (SV * sth, imp_sth_t * imp_sth, SV * statement_sv, SV * at imp_sth->all_bound = DBDPG_FALSE; /* Have all placeholders been bound? */ imp_sth->number_iterations = 0; + /* Create the array of placeholders and array of segments */ + ph_array_init(imp_sth); + seg_array_init(imp_sth); + /* We inherit some preferences from the database handle */ imp_sth->server_prepare = imp_dbh->server_prepare; imp_sth->switch_prepared = imp_dbh->switch_prepared; @@ -1945,20 +2036,19 @@ static void pg_st_split_statement (pTHX_ imp_sth_t * imp_sth, char * statement) PGPlaceholderType placeholder_type; /* Which type we are in: one of none,?,$,: */ - unsigned char ch; /* The current character being checked */ + unsigned char ch; /* The current character being checked */ unsigned char oldch; /* The previous character */ signed char non_standard_strings = -1; /* Status 0=standard 1=non_standard -1=unknown */ - int xint; + seg_t newseg; - seg_t *newseg, *currseg = NULL; /* Segment structures to help build linked lists */ - - ph_t *newph, *thisph, *currph = NULL; /* Placeholder structures to help build ll */ + int xint; bool statement_rewritten = DBDPG_FALSE; char * original_statement = NULL; /* Copy as needed so we can restore the original */ + char * statement_start = statement; /* Safe point to rewind to */ if (TSTART_slow) TRC(DBILOGFP, "%sBegin pg_st_split_statement\n", THEADER_slow); if (TRACE6_slow) TRC(DBILOGFP, "%spg_st_split_statement: (%s)\n", THEADER_slow, statement); @@ -1972,24 +2062,24 @@ static void pg_st_split_statement (pTHX_ imp_sth_t * imp_sth, char * statement) TRC(DBILOGFP, "%snot splitting due to %s\n", THEADER_slow, imp_sth->direct ? "pg_direct" : "empty string"); } - imp_sth->numsegs = 1; imp_sth->numphs = 0; imp_sth->totalsize = strlen(statement); - New(0, imp_sth->seg, 1, seg_t); /* freed in dbd_st_destroy */ - imp_sth->seg->placeholder = 0; - imp_sth->seg->nextseg = NULL; - imp_sth->seg->ph = NULL; + newseg.placeholder = 0; if (imp_sth->totalsize > 0) { - New(0, imp_sth->seg->segment, imp_sth->totalsize+1, char); /* freed in dbd_st_destroy */ - Copy(statement, imp_sth->seg->segment, imp_sth->totalsize+1, char); + New(0, newseg.segment, imp_sth->totalsize+1, char); /* freed in dbd_st_destroy */ + Copy(statement, newseg.segment, imp_sth->totalsize+1, char); + } else { - imp_sth->seg->segment = NULL; + newseg.segment = NULL; } + seg_array_append(imp_sth, &newseg); + imp_sth->numsegs = 1; + if (TRACE6_slow) TRC(DBILOGFP, "%sdirect split = (%s) length=(%d)\n", - THEADER_slow, imp_sth->seg->segment, (int)imp_sth->totalsize); + THEADER_slow, newseg.segment, (int)imp_sth->totalsize); if (TEND_slow) TRC(DBILOGFP, "%sEnd pg_st_split_statement (direct)\n", THEADER_slow); return; } @@ -2221,8 +2311,8 @@ static void pg_st_split_statement (pTHX_ imp_sth_t * imp_sth, char * statement) */ if ('\\' == oldch && imp_dbh->ph_escaped) { if (! statement_rewritten) { - New(0, original_statement, strlen(statement-currpos)+1, char); - Copy(statement-currpos, original_statement, strlen(statement-currpos)+1, char); + New(0, original_statement, strlen(statement_start)+1, char); + Copy(statement-currpos, original_statement, strlen(statement_start)+1, char); statement_rewritten = DBDPG_TRUE; } @@ -2304,81 +2394,72 @@ static void pg_st_split_statement (pTHX_ imp_sth_t * imp_sth, char * statement) continue; /* If we got here, we have a segment that needs to be saved */ - New(0, newseg, 1, seg_t); /* freed in dbd_st_destroy */ - newseg->nextseg = NULL; - newseg->placeholder = 0; - newseg->ph = NULL; + newseg.placeholder = 0; if (PLACEHOLDER_QUESTIONMARK == placeholder_type) { - newseg->placeholder = ++imp_sth->numphs; + newseg.placeholder = ++imp_sth->numphs; } else if (PLACEHOLDER_DOLLAR == placeholder_type) { - newseg->placeholder = atoi(statement-(currpos-sectionstop-1)); + newseg.placeholder = atoi(statement-(currpos-sectionstop-1)); } else if (PLACEHOLDER_COLON == placeholder_type) { STRLEN phsectionsize = currpos-sectionstop; /* Have we seen this placeholder yet? */ - for (xint=1,thisph=imp_sth->ph; NULL != thisph; thisph=thisph->nextph,xint++) { + for (int p=0; p < ph_array_count(imp_sth); p++) { + ph_t *thisph = ph_array_element(imp_sth, p); + STRLEN fooname_len = strlen(thisph->fooname); + STRLEN best_len = fooname_len > phsectionsize ? fooname_len : phsectionsize; /* Because we need to make sure :foobar does not match as a previous hit when seeing :foobar2, we always use the greater of the two lengths: the length of the old name or the current name we are scanning */ - if (0==strncmp(thisph->fooname, statement-phsectionsize, - strlen(thisph->fooname) > phsectionsize ? strlen(thisph->fooname) : phsectionsize)) { - newseg->placeholder = xint; - newseg->ph = thisph; + if (0==strncmp(thisph->fooname, statement-phsectionsize, best_len)) { + newseg.placeholder = p+1; break; } } - if (0==newseg->placeholder) { + if (0==newseg.placeholder) { + ph_t newph; + imp_sth->numphs++; - newseg->placeholder = imp_sth->numphs; - New(0, newph, 1, ph_t); /* freed in dbd_st_destroy */ - newseg->ph = newph; - newph->nextph = NULL; - newph->bind_type = NULL; - newph->value = NULL; - newph->quoted = NULL; - newph->referenced = DBDPG_FALSE; - newph->defaultval = DBDPG_TRUE; - newph->isdefault = DBDPG_FALSE; - newph->iscurrent = DBDPG_FALSE; - newph->isinout = DBDPG_FALSE; - New(0, newph->fooname, phsectionsize+1, char); /* freed in dbd_st_destroy */ - Copy(statement-phsectionsize, newph->fooname, phsectionsize, char); - newph->fooname[phsectionsize] = '\0'; - if (NULL==currph) { - imp_sth->ph = newph; - } - else { - currph->nextph = newph; - } - currph = newph; + newseg.placeholder = imp_sth->numphs; + newph.bind_type = NULL; + newph.value = NULL; + newph.quoted = NULL; + newph.referenced = DBDPG_FALSE; + newph.defaultval = DBDPG_TRUE; + newph.isdefault = DBDPG_FALSE; + newph.iscurrent = DBDPG_FALSE; + newph.isinout = DBDPG_FALSE; + newph.valuelen = 0; + newph.quotedlen = 0; + + New(0, newph.fooname, phsectionsize+1, char); /* freed in dbd_st_destroy */ + Copy(statement-phsectionsize, newph.fooname, phsectionsize, char); + newph.fooname[phsectionsize] = '\0'; + + ph_array_append(imp_sth, &newph); + } } /* end if placeholder_type */ sectionsize = sectionstop-sectionstart; /* 4-0 for "ABCD" */ if (sectionsize>0) { - New(0, newseg->segment, sectionsize+1, char); /* freed in dbd_st_destroy */ - Copy(statement-(currpos-sectionstart), newseg->segment, sectionsize, char); - newseg->segment[sectionsize] = '\0'; + New(0, newseg.segment, sectionsize+1, char); /* freed in dbd_st_destroy */ + Copy(statement-(currpos-sectionstart), newseg.segment, sectionsize, char); + newseg.segment[sectionsize] = '\0'; imp_sth->totalsize += sectionsize; } else { - newseg->segment = NULL; + newseg.segment = NULL; } if (TRACE6_slow) - TRC(DBILOGFP, "%sCreated segment (%s)\n", THEADER_slow, newseg->segment); + TRC(DBILOGFP, "%sCreated segment (%s)\n", THEADER_slow, newseg.segment); + + /* Add to the list of segments */ + seg_array_append(imp_sth, &newseg); - /* Tie it in to the previous one */ - if (NULL==currseg) { - imp_sth->seg = newseg; - } - else { - currseg->nextseg = newseg; - } - currseg = newseg; sectionstart = currpos; imp_sth->numsegs++; @@ -2402,14 +2483,16 @@ static void pg_st_split_statement (pTHX_ imp_sth_t * imp_sth, char * statement) numbers must be sequential. We change numphs if repeats found */ int topdollar = 0; - for (currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg) { + for (int s=0; s < seg_array_count(imp_sth); s++) { + seg_t *currseg = seg_array_element(imp_sth, s); if (currseg->placeholder > topdollar) topdollar = currseg->placeholder; } /* Make sure every placeholder from 1 to topdollar is used at least once */ for (xint=1; xint <= topdollar; xint++) { bool found = DBDPG_FALSE; - for (currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg) { + for (int s=0; s < seg_array_count(imp_sth); s++) { + seg_t *currseg = seg_array_element(imp_sth, s); if (currseg->placeholder==xint) { found = DBDPG_TRUE; break; @@ -2425,30 +2508,22 @@ static void pg_st_split_statement (pTHX_ imp_sth_t * imp_sth, char * statement) /* Create sequential placeholders */ if (PLACEHOLDER_COLON != imp_sth->placeholder_type) { for (xint=1; xint <= imp_sth->numphs; xint++) { - New(0, newph, 1, ph_t); /* freed in dbd_st_destroy */ - newph->nextph = NULL; - newph->bind_type = NULL; - newph->value = NULL; - newph->quoted = NULL; - newph->fooname = NULL; - newph->referenced = DBDPG_FALSE; - newph->defaultval = DBDPG_TRUE; - newph->isdefault = DBDPG_FALSE; - newph->iscurrent = DBDPG_FALSE; - newph->isinout = DBDPG_FALSE; - /* Let the correct segment(s) point to it */ - for (currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg) { - if (currseg->placeholder==xint) { - currseg->ph = newph; - } - } - if (NULL==currph) { - imp_sth->ph = newph; - } - else { - currph->nextph = newph; - } - currph = newph; + ph_t newph; + + newph.bind_type = NULL; + newph.value = NULL; + newph.quoted = NULL; + newph.fooname = NULL; + newph.inout = NULL; + newph.referenced = DBDPG_FALSE; + newph.defaultval = DBDPG_TRUE; + newph.isdefault = DBDPG_FALSE; + newph.iscurrent = DBDPG_FALSE; + newph.isinout = DBDPG_FALSE; + newph.valuelen = 0; + newph.quotedlen = 0; + + ph_array_append(imp_sth, &newph); } } @@ -2457,16 +2532,17 @@ static void pg_st_split_statement (pTHX_ imp_sth_t * imp_sth, char * statement) THEADER_slow, imp_sth->placeholder_type, imp_sth->numsegs, imp_sth->numphs); TRC(DBILOGFP, "%sPlaceholder numbers and segments:\n", THEADER_slow); - for (currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg) { + for (int s=0; s < seg_array_count(imp_sth); s++) { + seg_t *currseg = seg_array_element(imp_sth, s); TRC(DBILOGFP, "%sPH: (%d) SEG: (%s)\n", THEADER_slow, currseg->placeholder, currseg->segment); } if (imp_sth->numphs) { TRC(DBILOGFP, "%sPlaceholder number, fooname, id:\n", THEADER_slow); STRLEN xlen = 1; - for (currph=imp_sth->ph; NULL != currph; currph=currph->nextph,xlen++) { - TRC(DBILOGFP, "%s#%d FOONAME: (%s)\n", - THEADER_slow, (int)xlen, currph->fooname); + for (int p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); + TRC(DBILOGFP, "%s#%d FOONAME: (%s)\n", THEADER_slow, p+1, currph->fooname); } } } @@ -2474,7 +2550,7 @@ static void pg_st_split_statement (pTHX_ imp_sth_t * imp_sth, char * statement) DBIc_NUM_PARAMS(imp_sth) = imp_sth->numphs; if (statement_rewritten) { - Copy(original_statement, statement-currpos, strlen(original_statement)+1, char); + Copy(original_statement, statement_start, strlen(original_statement)+1, char); } Safefree(original_statement); @@ -2491,15 +2567,9 @@ static void pg_st_split_statement (pTHX_ imp_sth_t * imp_sth, char * statement) static int pg_st_prepare_statement (pTHX_ SV * sth, imp_sth_t * imp_sth) { D_imp_dbh_from_sth; - char * statement; - unsigned int placeholder_digits; - int x, params; - STRLEN execsize; - ExecStatusType status; - seg_t * currseg; - ph_t * currph; - long power_of_ten; + strbuf_t *statement = NULL; int send_prepare_status; + ExecStatusType prepare_status; if (TSTART_slow) TRC(DBILOGFP, "%sBegin pg_st_prepare_statement\n", THEADER_slow); @@ -2515,56 +2585,37 @@ static int pg_st_prepare_statement (pTHX_ SV * sth, imp_sth_t * imp_sth) if (TRACE5_slow) TRC(DBILOGFP, "%sNew statement name (%s)\n", THEADER_slow, imp_sth->prepare_name); - execsize = imp_sth->totalsize; - if (imp_sth->numphs!=0) { - for (currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg) { - if (0==currseg->placeholder) - continue; - /* The parameter itself: dollar sign plus digit(s) */ - power_of_ten = 10; - for (placeholder_digits=1; placeholder_digits<7; placeholder_digits++, power_of_ten *= 10) { - if (currseg->placeholder < power_of_ten) - break; - } - if (placeholder_digits >= 7) - croak("Too many placeholders!"); - execsize += placeholder_digits+1; - } - } - - New(0, statement, execsize+1, char); /* freed below */ - - statement[0] = '\0'; /* Construct the statement, with proper placeholders */ - for (currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg) { - if (currseg->segment != NULL) - strcat(statement, currseg->segment); - if (currseg->placeholder) { - sprintf(strchr(statement, '\0'), "$%d", currseg->placeholder); + statement = strbuf_create(1024); + for (int s = 0; s < seg_array_count(imp_sth); s++) { + seg_t *currseg = seg_array_element(imp_sth, s); + if (currseg->segment) { + strbuf_append_text(statement, currseg->segment); + } + if (currseg->placeholder != 0) { + strbuf_append_dollar_placeholder(statement, currseg->placeholder); } } - statement[execsize] = '\0'; - - params = 0; - if (imp_sth->numbound!=0) { - params = imp_sth->numphs; - if (NULL == imp_sth->PQoids) { - Newz(0, imp_sth->PQoids, (unsigned int)imp_sth->numphs, Oid); + /* If the user has bound anything, send the entire array of oids */ + if (imp_sth->numbound) { + if (!imp_sth->PQoids) { + Newz(0, imp_sth->PQoids, ph_array_count(imp_sth), Oid); /* freed in dbd_st_destroy */ } - for (x=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph) { - imp_sth->PQoids[x++] = (currph->defaultval) ? 0 : (Oid)currph->bind_type->type_id; + for (int p = 0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); + imp_sth->PQoids[p] = (currph->defaultval) ? 0 : (Oid)currph->bind_type->type_id; } } if (TSQL) - TRC(DBILOGFP, "PREPARE %s AS %s;\n\n", imp_sth->prepare_name, statement); + TRC(DBILOGFP, "PREPARE %s AS %s;\n\n", imp_sth->prepare_name, strbuf_get(statement)); if (imp_sth->async_flag & PG_ASYNC) { TRACE_PQSENDPREPARE; send_prepare_status = - PQsendPrepare(imp_dbh->conn, imp_sth->prepare_name, statement, params, imp_sth->PQoids); - Safefree(statement); + PQsendPrepare(imp_dbh->conn, imp_sth->prepare_name, strbuf_get(statement), imp_sth->numphs, imp_sth->PQoids); + strbuf_destroy(statement); if (send_prepare_status) { imp_sth->async_status = STH_ASYNC_PREPARE; @@ -2589,13 +2640,13 @@ static int pg_st_prepare_statement (pTHX_ SV * sth, imp_sth_t * imp_sth) TRACE_PQPREPARE; imp_dbh->last_result = imp_sth->result = - PQprepare(imp_dbh->conn, imp_sth->prepare_name, statement, params, imp_sth->PQoids); + PQprepare(imp_dbh->conn, imp_sth->prepare_name, strbuf_get(statement), imp_sth->numphs, imp_sth->PQoids); imp_dbh->result_shared = DBDPG_TRUE; - Safefree(statement); + strbuf_destroy(statement); - status = _sqlstate(aTHX_ imp_dbh, imp_sth->result); + prepare_status = _sqlstate(aTHX_ imp_dbh, imp_sth->result); - if (PGRES_COMMAND_OK == status) { + if (PGRES_COMMAND_OK == prepare_status) { imp_sth->prepared_by_us = DBDPG_TRUE; /* Done here so deallocate is not called spuriously */ imp_dbh->prepare_number++; if (TEND_slow) TRC(DBILOGFP, "%sEnd pg_st_prepare_statement\n", THEADER_slow); @@ -2605,7 +2656,7 @@ static int pg_st_prepare_statement (pTHX_ SV * sth, imp_sth_t * imp_sth) Safefree(imp_sth->prepare_name); imp_sth->prepare_name = NULL; TRACE_PQERRORMESSAGE; - pg_error(aTHX_ sth, status, PQerrorMessage(imp_dbh->conn)); + pg_error(aTHX_ sth, prepare_status, PQerrorMessage(imp_dbh->conn)); if (TEND_slow) TRC(DBILOGFP, "%sEnd pg_st_prepare_statement (error)\n", THEADER_slow); return -2; @@ -2624,6 +2675,7 @@ int dbd_bind_ph (SV * sth, imp_sth_t * imp_sth, SV * ph_name, SV * newvalue, IV int x, phnum; SV ** svp; bool reprepare = DBDPG_FALSE; + bool found; int pg_type = 0; char * value_string = NULL; bool is_array = DBDPG_FALSE; @@ -2656,15 +2708,16 @@ int dbd_bind_ph (SV * sth, imp_sth_t * imp_sth, SV * ph_name, SV * newvalue, IV } /* Find the placeholder in question */ - if (PLACEHOLDER_COLON == imp_sth->placeholder_type) { - for (x=0,currph=imp_sth->ph; NULL != currph; currph = currph->nextph) { + found = 0; + for (int p=0; p < ph_array_count(imp_sth); p++) { + currph = ph_array_element(imp_sth, p); if (0==strcmp(currph->fooname, name)) { - x=1; + found = 1; break; } } - if (0==x) + if (!found) croak("Cannot bind unknown placeholder '%s'", name); } else { /* We have a number */ @@ -2673,10 +2726,17 @@ int dbd_bind_ph (SV * sth, imp_sth_t * imp_sth, SV * ph_name, SV * newvalue, IV phnum = atoi(name); if (phnum < 1 || phnum > imp_sth->numphs) croak("Cannot bind unknown placeholder %d (%s)", phnum, neatsvpv(ph_name,0)); - for (x=1,currph=imp_sth->ph; NULL != currph; currph = currph->nextph,x++) { - if (x==phnum) + found = 0; + for (int p=0; p < ph_array_count(imp_sth); p++) { + if ((p+1)==phnum) { + found = 1; + currph = ph_array_element(imp_sth, p); break; + } } + + if (!found) + croak("Cannot bind unknown placeholder %d", phnum); } /* Check the value */ @@ -3388,16 +3448,11 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) { dTHX; D_imp_dbh_from_sth; - ph_t * currph; - int status; - STRLEN execsize, x; - unsigned int placeholder_digits; - seg_t * currseg; - char * statement = NULL; - int num_fields; + int status, p, s; + STRLEN execsize; + strbuf_t *statement = NULL; long ret; PQExecType pqtype; - long power_of_ten; if (TSTART_slow) TRC(DBILOGFP, "%sBegin dbd_st_execute\n", THEADER_slow); @@ -3412,7 +3467,8 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) /* Ensure that all the placeholders have been bound */ if (!imp_sth->all_bound && imp_sth->numphs!=0) { - for (currph=imp_sth->ph; NULL != currph; currph=currph->nextph) { + for (p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); if (NULL == currph->bind_type) { pg_error(aTHX_ sth, PGRES_FATAL_ERROR, "execute called with an unbound placeholder"); if (TEND_slow) TRC(DBILOGFP, "%sEnd dbd_st_execute (error: unbound placeholder)\n", THEADER_slow); @@ -3538,7 +3594,8 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) /* If using plain old PQexec, we need to quote each value ourselves */ if (PQTYPE_EXEC == pqtype) { - for (currph=imp_sth->ph; NULL != currph; currph=currph->nextph) { + for (p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); if (currph->isdefault) { Renew(currph->quoted, 8, char); /* freed in dbd_st_destroy */ strncpy(currph->quoted, "DEFAULT", 8); @@ -3571,27 +3628,29 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) /* Put all values into an array to pass to one of the above */ if (NULL == imp_sth->PQvals) { - Newz(0, imp_sth->PQvals, (unsigned int)imp_sth->numphs, const char *); /* freed in dbd_st_destroy */ + Newz(0, imp_sth->PQvals, (size_t)imp_sth->numphs, const char *); /* freed in dbd_st_destroy */ } - for (x=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph) { - imp_sth->PQvals[x++] = currph->value; + for (p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); + imp_sth->PQvals[p] = currph->value; } /* Binary or regular? */ if (imp_sth->has_binary) { if (NULL == imp_sth->PQlens) { - Newz(0, imp_sth->PQlens, (unsigned int)imp_sth->numphs, int); /* freed in dbd_st_destroy */ - Newz(0, imp_sth->PQfmts, (unsigned int)imp_sth->numphs, int); /* freed below */ + Newz(0, imp_sth->PQlens, (size_t)imp_sth->numphs, int); /* freed in dbd_st_destroy */ + Newz(0, imp_sth->PQfmts, (size_t)imp_sth->numphs, int); /* freed below */ } - for (x=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph,x++) { + for (p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); if (PG_BYTEA==currph->bind_type->type_id) { - imp_sth->PQlens[x] = (int)currph->valuelen; - imp_sth->PQfmts[x] = 1; + imp_sth->PQlens[p] = (int)currph->valuelen; + imp_sth->PQfmts[p] = 1; } else { - imp_sth->PQlens[x] = 0; - imp_sth->PQfmts[x] = 0; + imp_sth->PQlens[p] = 0; + imp_sth->PQfmts[p] = 0; } } } @@ -3606,33 +3665,35 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) imp_sth->async_flag & PG_ASYNC ? "PQsendQuery" : "PQexec"); /* Go through and quote each value, then turn into a giant statement */ - for (currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg) { - if (currseg->placeholder!=0) - execsize += currseg->ph->quotedlen; + for (s=0; s < seg_array_count(imp_sth); s++) { + seg_t *currseg = seg_array_element(imp_sth, s); + if (currseg->placeholder!=0) { + execsize += ph_array_element(imp_sth, currseg->placeholder-1)->quotedlen; + } } - New(0, statement, execsize+1, char); /* freed below at end of this 'if' block */ - statement[0] = '\0'; - for (currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg) { + statement = strbuf_create(execsize + 1); + + for (s=0; s < seg_array_count(imp_sth); s++) { + seg_t *currseg = seg_array_element(imp_sth, s); if (currseg->segment != NULL) - strcat(statement, currseg->segment); + strbuf_append_text(statement, currseg->segment); if (currseg->placeholder!=0) - strcat(statement, currseg->ph->quoted); + strbuf_append_text(statement, ph_array_element(imp_sth, currseg->placeholder-1)->quoted); } - statement[execsize] = '\0'; if (TRACE5_slow) TRC(DBILOGFP, "%sRunning %s with (%s)\n", THEADER_slow, imp_sth->async_flag & PG_ASYNC ? "PQsendQuery" : "PQexec", - statement); + strbuf_get(statement)); if (TSQL) - TRC(DBILOGFP, "%s;\n\n", statement); + TRC(DBILOGFP, "%s;\n\n", strbuf_get(statement)); if (imp_sth->async_flag & PG_ASYNC) { TRACE_PQSENDQUERY; - if (!PQsendQuery(imp_dbh->conn, statement)) { - Safefree(statement); + if (!PQsendQuery(imp_dbh->conn, strbuf_get(statement))) { + strbuf_destroy(statement); _fatal_sqlstate(aTHX_ imp_dbh); TRACE_PQERRORMESSAGE; pg_error(aTHX_ sth, PGRES_FATAL_ERROR, PQerrorMessage(imp_dbh->conn)); @@ -3647,11 +3708,11 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) CLEAR_STH_RESULT(imp_sth); TRACE_PQEXEC; - imp_dbh->last_result = imp_sth->result = PQexec(imp_dbh->conn, statement); + imp_dbh->last_result = imp_sth->result = PQexec(imp_dbh->conn, strbuf_get(statement)); imp_dbh->result_shared = DBDPG_TRUE; } - Safefree(statement); + strbuf_destroy(statement); } else if (PQTYPE_PARAMS == pqtype) { /* PQexecParams or PQsendQueryParams */ @@ -3660,54 +3721,41 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) THEADER_slow, imp_sth->async_flag & PG_ASYNC ? "PQsendQueryParams" : "PQexecParams"); - /* Figure out how big the statement plus placeholders will be */ - for (currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg) { - if (0==currseg->placeholder) - continue; - /* The parameter itself: dollar sign plus digit(s) */ - power_of_ten = 10; - for (placeholder_digits=1; placeholder_digits<7; placeholder_digits++, power_of_ten *= 10) { - if (currseg->placeholder < power_of_ten) - break; - } - if (placeholder_digits >= 7) - croak("Too many placeholders!"); - execsize += placeholder_digits+1; - } - /* Create the statement */ - New(0, statement, execsize+1, char); /* freed below at end of this 'if' block */ - statement[0] = '\0'; - for (currseg=imp_sth->seg; NULL != currseg; currseg=currseg->nextseg) { + statement = strbuf_create(1024); + + for (s=0; s < seg_array_count(imp_sth); s++) { + seg_t *currseg = seg_array_element(imp_sth, s); if (currseg->segment != NULL) - strcat(statement, currseg->segment); + strbuf_append_text(statement, currseg->segment); if (currseg->placeholder!=0) - sprintf(strchr(statement, '\0'), "$%d", currseg->placeholder); + strbuf_append_dollar_placeholder(statement, currseg->placeholder); } - statement[execsize] = '\0'; /* Populate PQoids */ if (NULL == imp_sth->PQoids) { - Newz(0, imp_sth->PQoids, (unsigned int)imp_sth->numphs, Oid); + Newz(0, imp_sth->PQoids, (size_t)imp_sth->numphs, Oid); /* freed in dbd_st_destroy */ } - for (x=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph) { - imp_sth->PQoids[x++] = (currph->defaultval) ? 0 : (Oid)currph->bind_type->type_id; + for (p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); + imp_sth->PQoids[p] = (currph->defaultval) ? 0 : (Oid)currph->bind_type->type_id; } if (TRACE7_slow) { - for (x=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph,x++) { - TRC(DBILOGFP, "%sPQexecParams item #%d\n", THEADER_slow, (int)x); - TRC(DBILOGFP, "%s-> Type: (%d)\n", THEADER_slow, imp_sth->PQoids[x]); - TRC(DBILOGFP, "%s-> Value: (%s)\n", THEADER_slow, imp_sth->PQvals[x]); - TRC(DBILOGFP, "%s-> Length: (%d)\n", THEADER_slow, imp_sth->PQlens ? imp_sth->PQlens[x] : 0); - TRC(DBILOGFP, "%s-> Format: (%d)\n", THEADER_slow, imp_sth->PQfmts ? imp_sth->PQfmts[x] : 0); + for (p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); + TRC(DBILOGFP, "%sPQexecParams item #%d\n", THEADER_slow, (int)p); + TRC(DBILOGFP, "%s-> Type: (%d)\n", THEADER_slow, imp_sth->PQoids[p]); + TRC(DBILOGFP, "%s-> Value: (%s)\n", THEADER_slow, imp_sth->PQvals[p]); + TRC(DBILOGFP, "%s-> Length: (%d)\n", THEADER_slow, imp_sth->PQlens ? imp_sth->PQlens[p] : 0); + TRC(DBILOGFP, "%s-> Format: (%d)\n", THEADER_slow, imp_sth->PQfmts ? imp_sth->PQfmts[p] : 0); } } if (TSQL) { - TRC(DBILOGFP, "EXECUTE %s (\n", statement); - for (x=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph,x++) { - TRC(DBILOGFP, "$%d: %s\n", (int)x+1, imp_sth->PQvals[x]); + TRC(DBILOGFP, "EXECUTE %s (\n", strbuf_get(statement)); + for (p=0; p < ph_array_count(imp_sth); p++) { + TRC(DBILOGFP, "$%d: %s\n", p+1, imp_sth->PQvals[p]); } TRC(DBILOGFP, ");\n\n"); } @@ -3715,12 +3763,12 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) if (TRACE5_slow) TRC(DBILOGFP, "%sRunning %s with (%s)\n", THEADER_slow, imp_sth->async_flag & PG_ASYNC ? "PQsendQueryParams" : "PQexecParams", - statement); + strbuf_get(statement)); if (imp_sth->async_flag & PG_ASYNC) { TRACE_PQSENDQUERYPARAMS; if (!PQsendQueryParams - (imp_dbh->conn, statement, imp_sth->numphs, + (imp_dbh->conn, strbuf_get(statement), imp_sth->numphs, imp_sth->PQoids, imp_sth->PQvals, imp_sth->PQlens, imp_sth->PQfmts, 0)) { Safefree(statement); _fatal_sqlstate(aTHX_ imp_dbh); @@ -3739,13 +3787,13 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) TRACE_PQEXECPARAMS; imp_dbh->last_result = imp_sth->result = PQexecParams ( - imp_dbh->conn, statement, imp_sth->numphs, + imp_dbh->conn, strbuf_get(statement), imp_sth->numphs, imp_sth->PQoids, imp_sth->PQvals, imp_sth->PQlens, imp_sth->PQfmts, 0 ); imp_dbh->result_shared = DBDPG_TRUE; } - Safefree(statement); + strbuf_destroy(statement); } else if (PQTYPE_PREPARED == pqtype) { /* PQexecPrepared or PQsendQueryPrepared */ @@ -3773,13 +3821,13 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) if (STH_ASYNC_PREPARE != imp_sth->async_status) { if (TRACE7_slow) { - for (x=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph,x++) { - TRC(DBILOGFP, "%sPQexecPrepared item #%d\n", THEADER_slow, (int)x); + for (p=0; p < ph_array_count(imp_sth); p++) { + TRC(DBILOGFP, "%sPQexecPrepared item #%d\n", THEADER_slow, p); TRC(DBILOGFP, "%s-> Value: (%s)\n", - THEADER_slow, (imp_sth->PQfmts && imp_sth->PQfmts[x]==1) ? "(binary, not shown)" - : imp_sth->PQvals[x]); - TRC(DBILOGFP, "%s-> Length: (%d)\n", THEADER_slow, imp_sth->PQlens ? imp_sth->PQlens[x] : 0); - TRC(DBILOGFP, "%s-> Format: (%d)\n", THEADER_slow, imp_sth->PQfmts ? imp_sth->PQfmts[x] : 0); + THEADER_slow, (imp_sth->PQfmts && imp_sth->PQfmts[p]==1) ? "(binary, not shown)" + : imp_sth->PQvals[p]); + TRC(DBILOGFP, "%s-> Length: (%d)\n", THEADER_slow, imp_sth->PQlens ? imp_sth->PQlens[p] : 0); + TRC(DBILOGFP, "%s-> Format: (%d)\n", THEADER_slow, imp_sth->PQfmts ? imp_sth->PQfmts[p] : 0); } } @@ -3789,8 +3837,8 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) if (TSQL) { TRC(DBILOGFP, "EXECUTE %s (\n", imp_sth->prepare_name); - for (x=0,currph=imp_sth->ph; NULL != currph; currph=currph->nextph,x++) { - TRC(DBILOGFP, "$%d: %s\n", (int)x+1, imp_sth->PQvals[x]); + for (p=0; p < ph_array_count(imp_sth); p++) { + TRC(DBILOGFP, "$%d: %s\n", p+1, imp_sth->PQvals[p]); } TRC(DBILOGFP, ");\n\n"); } @@ -3841,9 +3889,9 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) imp_dbh->copystate = 0; /* Assume not in copy mode until told otherwise */ if (PGRES_TUPLES_OK == status) { - TRACE_PQNFIELDS; - num_fields = PQnfields(imp_sth->result); imp_sth->cur_tuple = 0; + TRACE_PQNFIELDS; + int num_fields = PQnfields(imp_sth->result); DBIc_NUM_FIELDS(imp_sth) = num_fields; DBIc_ACTIVE_on(imp_sth); TRACE_PQNTUPLES; @@ -3945,7 +3993,7 @@ AV * dbd_st_fetch (SV * sth, imp_sth_t * imp_sth) /* Set up the type_info array if we have not seen it yet */ if (NULL == imp_sth->type_info) { - Newz(0, imp_sth->type_info, (unsigned int)num_fields, sql_type_info_t*); /* freed in dbd_st_destroy */ + Newz(0, imp_sth->type_info, (size_t)num_fields, sql_type_info_t*); /* freed in dbd_st_destroy */ for (i = 0; i < num_fields; ++i) { TRACE_PQFTYPE; imp_sth->type_info[i] = pg_type_data((int)PQftype(imp_sth->result, i)); @@ -4058,10 +4106,10 @@ AV * dbd_st_fetch (SV * sth, imp_sth_t * imp_sth) /* Experimental inout support */ if (imp_sth->use_inout) { - ph_t *currph; - for (i=0,currph=imp_sth->ph; NULL != currph && i < num_fields; currph=currph->nextph,i++) { + for (int p=0; p < ph_array_count(imp_sth); p++) { + ph_t *currph = ph_array_element(imp_sth, p); if (currph->isinout) - sv_copypv(currph->inout, AvARRAY(av)[i]); + sv_copypv(currph->inout, AvARRAY(av)[p]); } } @@ -4247,20 +4295,16 @@ void dbd_st_destroy (SV * sth, imp_sth_t * imp_sth) { dTHX; D_imp_dbh_from_sth; - seg_t * currseg; - seg_t * nextseg; - ph_t * currph; - ph_t * nextph; imp_dbh->do_tmp_sth = NULL; if (TSTART_slow) TRC(DBILOGFP, "%sBegin dbd_st_destroy\n", THEADER_slow); - if (NULL == imp_sth->seg) /* Already been destroyed! */ + if (0 == seg_array_count(imp_sth)) /* Already been destroyed! */ croak("dbd_st_destroy called twice!"); /* If the AutoInactiveDestroy flag has been set, we go no further */ - if ((DBIc_AIADESTROY(imp_dbh)) && ((U32)PerlProc_getpid() != (unsigned int)imp_dbh->pid_number)) { + if ((DBIc_AIADESTROY(imp_dbh)) && ((U32)PerlProc_getpid() != (size_t)imp_dbh->pid_number)) { if (TRACE4_slow) { TRC(DBILOGFP, "%sskipping sth destroy due to AutoInactiveDestroy\n", THEADER_slow); } @@ -4317,28 +4361,10 @@ void dbd_st_destroy (SV * sth, imp_sth_t * imp_sth) imp_sth->result = NULL; /* Free all the segments */ - currseg = imp_sth->seg; - while (NULL != currseg) { - Safefree(currseg->segment); - currseg->ph = NULL; - nextseg = currseg->nextseg; - Safefree(currseg); - currseg = nextseg; - } - imp_sth->seg = NULL; + seg_array_destroy(imp_sth); /* Free all the placeholders */ - currph = imp_sth->ph; - while (NULL != currph) { - Safefree(currph->fooname); - Safefree(currph->value); - Safefree(currph->quoted); - currph->bind_type = NULL; - nextph = currph->nextph; - Safefree(currph); - currph = nextph; - } - imp_sth->ph = NULL; + ph_array_destroy(imp_sth); if (NULL != imp_dbh->async_sth && imp_dbh->async_sth == imp_sth) imp_dbh->async_sth = NULL; diff --git a/dbdimp.h b/dbdimp.h index abf60ed0..503a2a8a 100644 --- a/dbdimp.h +++ b/dbdimp.h @@ -54,17 +54,7 @@ struct imp_dbh_st { imp_sth_t *do_tmp_sth; /* temporary sth to refer inside a do() call */ }; - -/* Each statement is broken up into segments */ -struct seg_st { - char *segment; /* non-placeholder string segment */ - int placeholder; /* which placeholder this points to, 0=none */ - struct ph_st *ph; /* points to the relevant ph structure */ - struct seg_st *nextseg; /* linked lists are fun */ -}; -typedef struct seg_st seg_t; - -/* The placeholders are also a linked list */ +/* The placeholder structure. Used as array elements in the ph_array_t structure */ struct ph_st { char *fooname; /* name if using :foo style */ char *value; /* the literal passed-in value, may be binary */ @@ -76,12 +66,35 @@ struct ph_st { bool iscurrent; /* do we want to use a literal CURRENT_TIMESTAMP? */ bool isdefault; /* are we passing a literal 'DEFAULT'? */ bool isinout; /* is this a bind_param_inout value? */ - SV *inout; /* what variable we are updating via inout magic */ + SV *inout; /* what variable we are updating via inout magic (do not Safefree!) */ sql_type_info_t* bind_type; /* type information for this placeholder */ - struct ph_st *nextph; /* more linked list goodness */ }; typedef struct ph_st ph_t; +/* The array container for the placeholders */ +struct ph_array_st { + int length; /* length of the array */ + int elements; /* num of elements in the array */ + ph_t *array; /* the array of placeholders */ +}; +typedef struct ph_array_st ph_array_t; + +/* Each statement is broken up into segments */ +struct seg_st { + char *segment; /* non-placeholder string segment */ + int placeholder; /* which placeholder this points to, 0=none */ +}; +typedef struct seg_st seg_t; + +/* The array container for the segments */ +struct seg_array_st { + int length; + int elements; + seg_t *array; +}; +typedef struct seg_array_st seg_array_t; + + typedef enum { PLACEHOLDER_NONE, @@ -119,8 +132,8 @@ struct imp_sth_st { PGresult *result; /* result structure from the executed query */ sql_type_info_t **type_info; /* type of each column in result */ - seg_t *seg; /* linked list of segments */ - ph_t *ph; /* linked list of placeholders */ + ph_array_t ph_array; /* array of placeholders */ + seg_array_t seg_array; /* array of segments */ bool prepare_now; /* prepare this statement right away, even if it has placeholders */ bool prepared_by_us; /* false if {prepare_name} set directly */ diff --git a/strbuf.c b/strbuf.c new file mode 100644 index 00000000..0895ac8d --- /dev/null +++ b/strbuf.c @@ -0,0 +1,110 @@ +#include +#include "Pg.h" +#include "strbuf.h" + +/* + * String buffer that automatically grows as needed + */ + +struct strbuf_s { + size_t length; /* number of characters in string (excluding null byte) */ + size_t memory; /* amount of allocated memory */ + char *string; +}; + +typedef struct strbuf_s strbuf_t; + +/* Returns a pointer to the null byte */ +static char* strbuf_end(strbuf_t *str) +{ + return str->string + str->length; +} + +/* Reallocate string buffer to at least the given size in bytes */ +static void strbuf_realloc(strbuf_t *str, size_t needed) +{ + if (needed == 0) + needed = 1; + + if (needed <= str->memory) + return; + + size_t newsize = (needed > SIZE_MAX / 2) ? needed : needed * 2; + + Renew(str->string, newsize, char); + str->memory = newsize; +} + +/* Returns pointer to the string itself */ +const char* strbuf_get(const strbuf_t *str) +{ + return str->string; +} + +/* Create a new strbuf object */ +strbuf_t* strbuf_create(size_t size) +{ + strbuf_t *str; + + if (size == 0) + size = 1; + + New(0, str, 1, strbuf_t); + New(0, str->string, size, char); + + str->length = 0; + str->memory = size; + str->string[0] = '\0'; + + return str; +} + +void strbuf_destroy(strbuf_t *str) +{ + if (!str) + return; + + Safefree(str->string); + Safefree(str); +} + +void strbuf_append_text(strbuf_t *str, const char *text) +{ + if (!text) + croak("strbuf_append_text: text is NULL"); + + const size_t textlen = strlen(text); + + if (textlen > SIZE_MAX - str->length - 1) + croak("strbuf_append_text: text too large"); + + const size_t needed = str->length + textlen + 1; + + if (needed > str->memory) + strbuf_realloc(str, needed); + + Copy(text, strbuf_end(str), textlen, char); + str->length += textlen; + str->string[str->length] = '\0'; +} + +/* Append a number to a string, prefixed with a dollar sign */ +void strbuf_append_dollar_placeholder(strbuf_t *str, int phnum) +{ + const size_t num_buf_size = 32; /* 10 digits + '$' + sign + null + lots of extra */ + + if (str->length >= SIZE_MAX - num_buf_size) + croak("strbuf_append_dollar_placeholder: string too large"); + + if (str->memory < str->length + num_buf_size) + strbuf_realloc(str, str->length + num_buf_size); + + const size_t avail = str->memory - str->length; + const int written = snprintf(strbuf_end(str), avail, "$%d", phnum); + + if (written < 0 || (size_t)written >= avail) + croak("strbuf_append_dollar_placeholder: problem writing string"); + + str->length += (size_t) written; + str->string[str->length] = '\0'; +} diff --git a/strbuf.h b/strbuf.h new file mode 100644 index 00000000..3bdd301f --- /dev/null +++ b/strbuf.h @@ -0,0 +1,17 @@ +#ifndef STRBUF_H +#define STRBUF_H + +#include + +typedef struct strbuf_s strbuf_t; + +strbuf_t* strbuf_create(size_t characters); +void strbuf_destroy(strbuf_t *string); + +/* Returns a pointer to the internal char*. Do not free */ +const char* strbuf_get(const strbuf_t *str); + +void strbuf_append_text(strbuf_t *str, const char *text); +void strbuf_append_dollar_placeholder(strbuf_t *str, int num); + +#endif diff --git a/t/03smethod.t b/t/03smethod.t index aa0361a6..1b6c0feb 100644 --- a/t/03smethod.t +++ b/t/03smethod.t @@ -23,7 +23,7 @@ my $dbh = connect_database(); if (! $dbh) { plan skip_all => 'Connection to database failed, cannot continue testing'; } -plan tests => 161; +plan tests => 163; isnt ($dbh, undef, 'Connect to database for statement handle testing'); @@ -127,8 +127,15 @@ $sth = $dbh->prepare($SQL, {pg_server_prepare => 1}); $sth->execute(1); ok ($sth->execute, $t); +$t='Statement handle attribute pg_prepare_now returns default of 0'; +is ($sth->{pg_prepare_now}, 0, $t); + +$t='Statement handle attribute pg_prepare_now returns correct value after explicit set'; +$sth = $dbh->prepare($SQL); +$sth->{pg_prepare_now} = 1; +is ($sth->{pg_prepare_now}, 1, $t); + $t='Prepare/execute with pg_prepare_now on at database handle works'; -$dbh->{pg_prepare_now} = 1; $sth = $dbh->prepare($SQL); $sth->execute(1); ok ($sth->execute, $t); diff --git a/t/99_spellcheck.t b/t/99_spellcheck.t index f488a774..64983612 100644 --- a/t/99_spellcheck.t +++ b/t/99_spellcheck.t @@ -731,6 +731,7 @@ RDBMS README ReadOnly realclean +realloc recv'd RedHat Refactor @@ -753,6 +754,7 @@ RowCacheSize RowsInCache rowtypes Sabino +Safefree safemalloc sandia savepoint @@ -802,6 +804,7 @@ STDERR STDIN STDOUT sth +strbuf strcmp strcpy strdup From 5f37c08a90d6663a0695192ae0c877ca26c23095 Mon Sep 17 00:00:00 2001 From: Greg Sabino Mullane Date: Wed, 1 Jul 2026 21:50:18 -0400 Subject: [PATCH 2/3] Fixes as noted by Ed Sabol on code review --- dbdimp.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/dbdimp.c b/dbdimp.c index 81eb3cf6..777521b9 100644 --- a/dbdimp.c +++ b/dbdimp.c @@ -346,7 +346,7 @@ int dbd_db_login6 (SV * dbh, imp_dbh_t * imp_dbh, char * dbname, char * uid, cha connect_string_size += strlen("user='' ") + 2*strlen(uid); if (*pwd) connect_string_size += strlen("password='' ") + 2*strlen(pwd); - New(0, conn_str, connect_string_size+1, char); /* freed in dbd_st_destroy */ + New(0, conn_str, connect_string_size+1, char); /* freed below */ /* Change all semi-colons in dbname to a space, unless single-quoted */ dest = conn_str; @@ -1425,8 +1425,8 @@ SV * dbd_st_FETCH_attrib (SV * sth, imp_sth_t * imp_sth, SV * keysv) retsv = newRV_noinc((SV*)arr); } else if (strEQ("pg_numbound", key)) { - int p, num = 0; - for (p=0; p < ph_array_count(imp_sth); p++) { + int num = 0; + for (int p=0; p < ph_array_count(imp_sth); p++) { ph_t *currph = ph_array_element(imp_sth, p); num += NULL == currph->bind_type ? 0 : 1; } @@ -2726,17 +2726,7 @@ int dbd_bind_ph (SV * sth, imp_sth_t * imp_sth, SV * ph_name, SV * newvalue, IV phnum = atoi(name); if (phnum < 1 || phnum > imp_sth->numphs) croak("Cannot bind unknown placeholder %d (%s)", phnum, neatsvpv(ph_name,0)); - found = 0; - for (int p=0; p < ph_array_count(imp_sth); p++) { - if ((p+1)==phnum) { - found = 1; - currph = ph_array_element(imp_sth, p); - break; - } - } - - if (!found) - croak("Cannot bind unknown placeholder %d", phnum); + currph = ph_array_element(imp_sth, phnum - 1); } /* Check the value */ @@ -3640,7 +3630,7 @@ long dbd_st_execute (SV * sth, imp_sth_t * imp_sth) if (imp_sth->has_binary) { if (NULL == imp_sth->PQlens) { Newz(0, imp_sth->PQlens, (size_t)imp_sth->numphs, int); /* freed in dbd_st_destroy */ - Newz(0, imp_sth->PQfmts, (size_t)imp_sth->numphs, int); /* freed below */ + Newz(0, imp_sth->PQfmts, (size_t)imp_sth->numphs, int); /* freed in dbd_st_destroy */ } for (p=0; p < ph_array_count(imp_sth); p++) { ph_t *currph = ph_array_element(imp_sth, p); From 73e21a95b5092add9feae1c507198323a97c5104 Mon Sep 17 00:00:00 2001 From: Greg Sabino Mullane Date: Thu, 2 Jul 2026 11:53:51 -0400 Subject: [PATCH 3/3] Merge in recent memory leak fixes. --- dbdimp.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/dbdimp.c b/dbdimp.c index 777521b9..1d1c3636 100644 --- a/dbdimp.c +++ b/dbdimp.c @@ -299,7 +299,8 @@ static int after_connect_init(pTHX_ SV *dbh, imp_dbh_t * imp_dbh) PQfinish(imp_dbh->conn); imp_dbh->conn = NULL; sv_free((SV *)imp_dbh->savepoints); - if (TEND_slow) TRC(DBILOGFP, "%sEnd dbd_db_login (error)\n", THEADER_slow); + Safefree(imp_dbh->sqlstate); + if (TEND_slow) TRC(DBILOGFP, "%sEnd dbd_db_login6 (error)\n", THEADER_slow); return 1; } } @@ -420,7 +421,7 @@ int dbd_db_login6 (SV * dbh, imp_dbh_t * imp_dbh, char * dbname, char * uid, cha Safefree(conn_str); /* Set the initial sqlstate */ - Renew(imp_dbh->sqlstate, 6, char); /* freed in dbd_db_destroy */ + Renew(imp_dbh->sqlstate, 6, char); /* freed below, or in dbd_db_destroy */ strncpy(imp_dbh->sqlstate, "25P01", 6); /* "NO ACTIVE SQL TRANSACTION" */ /* Check to see that the backend connection was successfully made */ @@ -436,7 +437,8 @@ int dbd_db_login6 (SV * dbh, imp_dbh_t * imp_dbh, char * dbname, char * uid, cha PQfinish(imp_dbh->conn); imp_dbh->conn = NULL; sv_free((SV *)imp_dbh->savepoints); - if (TEND_slow) TRC(DBILOGFP, "%sEnd dbd_db_login (error)\n", THEADER_slow); + Safefree(imp_dbh->sqlstate); + if (TEND_slow) TRC(DBILOGFP, "%sEnd dbd_db_login6 (error)\n", THEADER_slow); return 0; } @@ -476,11 +478,11 @@ int dbd_db_login6 (SV * dbh, imp_dbh_t * imp_dbh, char * dbname, char * uid, cha else retval = ! after_connect_init(aTHX_ dbh, imp_dbh); - if (TEND_slow) TRC(DBILOGFP, "%sEnd dbd_db_login\n", THEADER_slow); + if (TEND_slow) TRC(DBILOGFP, "%sEnd dbd_db_login6\n", THEADER_slow); return retval; -} /* end of dbd_db_login */ +} /* end of dbd_db_login6 */ int pg_db_continue_connect(SV *dbh) {