diff --git a/FU.xs b/FU.xs index d700b3f..5496c46 100644 --- a/FU.xs +++ b/FU.xs @@ -77,11 +77,9 @@ void _load_libpq() CODE: if (!PQconnectdb) fupg_load(); -int lib_version() +void lib_version() CODE: - RETVAL = PQlibVersion(); - OUTPUT: - RETVAL + XSRETURN_IV(PQlibVersion()); void connect(const char *pkg, const char *conninfo) CODE: @@ -91,11 +89,9 @@ void connect(const char *pkg, const char *conninfo) MODULE = FU PACKAGE = FU::PG::conn -int server_version(fupg_conn *c) +void server_version(fupg_conn *c) CODE: - RETVAL = PQserverVersion(c->conn); - OUTPUT: - RETVAL + XSRETURN_IV(PQserverVersion(c->conn)); void _debug_trace(fupg_conn *c, bool on) CODE: diff --git a/c/common.c b/c/common.c index dd47578..4468f39 100644 --- a/c/common.c +++ b/c/common.c @@ -81,9 +81,10 @@ static void fustr_init_(pTHX_ fustr *s, SV *mortal, size_t maxlen) { } #define fustr_start(s) (((s)->sv ? SvPVX((s)->sv) : (s)->sbuf)) +#define fustr_len(s) ((s)->cur - fustr_start(s)) static void fustr_grow(pTHX_ fustr *s, size_t add) { - size_t off = s->cur - (s->sv ? SvPVX(s->sv) : s->sbuf); + size_t off = fustr_len(s); size_t newlen = sizeof s->sbuf; char *buf; add += off; diff --git a/c/pgconn.c b/c/pgconn.c index a08fef3..ee672a0 100644 --- a/c/pgconn.c +++ b/c/pgconn.c @@ -10,21 +10,22 @@ typedef struct { UV cookie_counter; UV cookie; /* currently active transaction object; 0 = none active */ int stflags; + fustr buf; /* Scratch space for query params */ } fupg_conn; +typedef struct fupg_txn fupg_txn; struct fupg_txn { SV *self; - struct fupg_txn *parent; + fupg_txn *parent; fupg_conn *conn; UV cookie; /* 0 means done */ int stflags; char rollback_cmd[64]; }; -typedef struct fupg_txn fupg_txn; typedef struct { - /* Set in $conn->q() */ + /* Set on creation */ SV *self; /* (unused, but whatever) */ fupg_conn *conn; /* has a refcnt on conn->self */ UV cookie; @@ -32,16 +33,18 @@ typedef struct { SV **bind; int nbind; int stflags; + /* Set during prepare */ int prepared; char name[32]; PGresult *describe; + /* Set during execute */ - int nparam; int nfields; - char **param; - const fupg_type **recv; - void **recvctx; + const char **param_values; /* Points into conn->buf or st->bind SVs, may be invalid after exec */ + int *param_lengths; + int *param_formats; + fupg_recv *recv; PGresult *result; } fupg_st; @@ -147,8 +150,11 @@ static SV *fupg_connect(pTHX_ const char *str) { croak_sv(sv); } - fupg_conn *c = safecalloc(1, sizeof(fupg_conn)); + fupg_conn *c = safemalloc(sizeof(fupg_conn)); c->conn = conn; + c->prep_counter = c->cookie_counter = c->cookie = 0; + c->stflags = 0; + fustr_init(&c->buf, NULL, SIZE_MAX); return fu_selfobj(c, "FU::PG::conn"); } @@ -277,8 +283,8 @@ static SV *fupg_q(pTHX_ fupg_conn *c, int stflags, const char *query, I32 ax, I3 st->bind = safemalloc((argc-2) * sizeof(SV *)); I32 i; for (i=2; i < argc; i++) { - st->bind[st->nbind] = newSV(0); - sv_setsv(st->bind[st->nbind], ST(i)); + SvGETMAGIC(ST(i)); + st->bind[st->nbind] = SvREFCNT_inc(ST(i)); st->nbind++; } } @@ -352,6 +358,68 @@ static void fupg_st_check_dupcols(pTHX_ PGresult *r) { SvREFCNT_dec((SV *)hv); } +static void fupg_params_setup(pTHX_ fupg_st *st) { + int i; + st->param_values = safecalloc(st->nbind, sizeof(*st->param_values)); + if (st->stflags & FUPG_TEXT_PARAMS) { + for (i=0; inbind; i++) + st->param_values[i] = !SvOK(st->bind[i]) ? NULL : SvPVutf8_nolen(st->bind[i]); + return; + } + + fustr *buf = &st->conn->buf; + buf->cur = fustr_start(buf); + st->param_lengths = safecalloc(st->nbind, sizeof(*st->param_lengths)); + st->param_formats = safecalloc(st->nbind, sizeof(*st->param_formats)); + size_t off = 0; + for (i=0; inbind; i++) { + if (!SvOK(st->bind[i])) { + st->param_values[i] = NULL; + continue; + } + fupg_send send; + send.oid = PQparamtype(st->describe, i); + const fupg_core_type *t = fupg_core_type_byoid(send.oid); + if (!t) + fu_confess("Unable to use type oid %u as bind parameter", send.oid); + send.name = t->name; + send.fn = t->send; + off = fustr_len(buf); + send.fn(aTHX_ &send, st->bind[i], buf); + st->param_lengths[i] = fustr_len(buf) - off; + st->param_formats[i] = 1; + st->param_values[i] = ""; + /* Don't write param_values here, the buffer may be invalidated when writing the next param */ + } + off = 0; + buf->cur = fustr_start(buf); + for (i=0; inbind; i++) { + if (st->param_values[i]) { + st->param_values[i] = buf->cur + off; + off += st->param_lengths[i]; + } + } +} + +static void fupg_results_setup(pTHX_ fupg_st *st) { + int i; + st->recv = safecalloc(st->nfields, sizeof(*st->recv)); + if (st->stflags & FUPG_TEXT_RESULTS) { + for (i=0; infields; i++) + st->recv[i].fn = fupg_recv_textfmt; + return; + } + + for (i=0; infields; i++) { + fupg_recv *r = st->recv + i; + r->oid = PQftype(st->result, i); + const fupg_core_type *t = fupg_core_type_byoid(r->oid); + if (!t) fu_confess("Unable to receive query results of type oid %u", r->oid); + r->name = t->name; + r->fn = t->recv; + } +} + static void fupg_st_execute(pTHX_ fupg_st *st) { /* Disallow fetching the results more than once. I don't see a reason why * someone would need that and disallowing it leaves room for fetching the @@ -359,14 +427,10 @@ static void fupg_st_execute(pTHX_ fupg_st *st) { if (st->result) fu_confess("Invalid attempt to execute statement multiple times"); /* TODO: prepare can be skipped when prepared statement caching is disabled and (text-format queries or no bind params) */ - /* TODO: support binary format params */ fupg_st_prepare(aTHX_ st); - st->param = safemalloc(st->nbind * sizeof(char *)); - int i; - for (i=0; inbind; i++) { - st->param[i] = SvPVutf8_nolen(st->bind[i]); - st->nparam++; - } + if (PQnparams(st->describe) != st->nbind) + fu_confess("Statement expects %d bind parameters but %d were given", PQnparams(st->describe), st->nbind); + fupg_params_setup(aTHX_ st); /* I'm not super fond of this approach. Storing the full query results in a * PGresult involves unnecessary parsing, memory allocation and copying. @@ -379,8 +443,12 @@ static void fupg_st_execute(pTHX_ fupg_st *st) { * malloc()/free()'s. Performance-wise, it probably won't be much of an * improvement */ PGresult *r = PQexecPrepared(st->conn->conn, - st->name, st->nparam, (const char * const*)st->param, - NULL, NULL, st->stflags & FUPG_TEXT_RESULTS ? 0 : 1); + st->name, + st->nbind, + (const char * const *)st->param_values, + st->param_lengths, + st->param_formats, + st->stflags & FUPG_TEXT_RESULTS ? 0 : 1); if (!r) fupg_conn_croak(st->conn , "exec"); switch (PQresultStatus(r)) { case PGRES_COMMAND_OK: @@ -390,22 +458,14 @@ static void fupg_st_execute(pTHX_ fupg_st *st) { st->result = r; st->nfields = PQnfields(r); - st->recv = safecalloc(st->nfields, sizeof(*st->recv)); - st->recvctx = safecalloc(st->nfields, sizeof(*st->recvctx)); - for (i=0; infields; i++) { - st->recv[i] = fupg_type_lookup(st->stflags & FUPG_TEXT_RESULTS ? 0 : PQftype(r, i)); - if (!st->recv[i]) - fu_confess("Unable to receive query results of type %u", PQftype(r, i)); - } + fupg_results_setup(aTHX_ st); } static SV *fupg_st_getval(pTHX_ fupg_st *st, int row, int col) { PGresult *r = st->result; if (PQgetisnull(r, row, col)) return newSV(0); - int len = PQgetlength(st->result, row, col); - const fupg_type *t = st->recv[col]; - if (t->len && len != t->len) fu_confess("invalid length for type %s: %d\n", t->name, len); - return t->recv(aTHX_ PQgetvalue(r, row, col), len, st->recvctx[col]); + const fupg_recv *ctx = st->recv+col; + return ctx->fn(aTHX_ ctx, PQgetvalue(r, row, col), PQgetlength(r, row, col)); } static SV *fupg_st_exec(pTHX_ fupg_st *st) { @@ -474,11 +534,10 @@ static void fupg_st_destroy(fupg_st *st) { safefree(st->query); for (i=0; i < st->nbind; i++) SvREFCNT_dec(st->bind[i]); safefree(st->bind); - /* XXX: These point into bind SVs (for now): - * for (i=0; i < st->nparam; i++) safefree(st->param[i]); */ - safefree(st->param); + safefree(st->param_values); + safefree(st->param_lengths); + safefree(st->param_formats); safefree(st->recv); - safefree(st->recvctx); /* XXX: Needs type-specific free() for the individual pointers */ PQclear(st->describe); PQclear(st->result); SvREFCNT_dec(st->conn->self); diff --git a/c/pgtypes.c b/c/pgtypes.c index 6cf40e4..160c3e0 100644 --- a/c/pgtypes.c +++ b/c/pgtypes.c @@ -1,73 +1,137 @@ +typedef struct fupg_send fupg_send; +typedef struct fupg_recv fupg_recv; + /* Send function, takes a Perl value and should write the binary encoded * format into the given fustr. */ -typedef void (*fupg_send_fn)(pTHX_ SV *, fustr *, void *); +typedef void (*fupg_send_fn)(pTHX_ const fupg_send *, SV *, fustr *); /* Receive function, takes a binary string and should return a Perl value. * libpq guarantees that the given buffer is aligned to MAXIMUM_ALIGNOF. - * For fixed-length types, the recv function is only called after verifying - * that the input buffer has the correct length. */ -typedef SV *(*fupg_recv_fn)(pTHX_ const char *, int, void *); + */ +typedef SV *(*fupg_recv_fn)(pTHX_ const fupg_recv *, const char *, int); + +struct fupg_send { + Oid oid; + const char *name; + fupg_send_fn fn; +}; + +struct fupg_recv { + Oid oid; + const char *name; + fupg_recv_fn fn; +}; typedef struct { Oid oid; - int len; - const char *name; + char name[16]; /* Postgres has a 64 byte limit on names, but this is sufficient for the core types listed here */ fupg_send_fn send; fupg_recv_fn recv; -} fupg_type; +} fupg_core_type; -#define RECVFN(name) static SV *fupg_recv_##name(pTHX_ const char *buf, int buflen __attribute__((unused)), void *data __attribute__((unused))) +#define RECVFN(name) static SV *fupg_recv_##name(pTHX_ const fupg_recv *ctx __attribute__((unused)), const char *buf, int len) +#define SENDFN(name) static void fupg_send_##name(pTHX_ const fupg_send *ctx __attribute__((unused)), SV *val, fustr *out) +#define RLEN(l) if (l != len) fu_confess("Invalid length for type '%s' (oid %u), expected %d but got %d", ctx->name, ctx->oid, l, len) + +/* Perl likes to play loose with SV-to-integer conversions, but that's not + * very fun when trying to store values in a database. Text-based bind + * parameters get stricter validation by Postgres, so let's emulate some of + * that for binary parameters as well. */ +#define SIV(min, max) IV iv;\ + if (SvIOK(val)) iv = SvIV(val); \ + else if (SvNOK(val)) { \ + NV nv = SvNV(val); \ + if (nv < IV_MIN || nv > IV_MAX || fabs(nv - floor(nv)) > 0.0000000001) \ + fu_confess("Type '%s' (oid %u) expects an integer but got a floating point", ctx->name, ctx->oid); \ + iv = SvIV(val); \ + } else if (SvPOK(val)) {\ + STRLEN sl; \ + UV uv; \ + char *s = SvPV(val, sl); \ + if (*s == '-' && grok_atoUV(s+1, &uv, NULL) && uv <= ((UV)IV_MAX)+1) iv = SvIV(val);\ + else if (grok_atoUV(s, &uv, NULL) && uv <= IV_MAX) iv = SvIV(val);\ + else fu_confess("Type '%s' (oid %u) expects an integer", ctx->name, ctx->oid); \ + } else fu_confess("Type '%s' (oid %u) expects an integer", ctx->name, ctx->oid);\ + if (iv < min || iv > max) fu_confess("Integer %"IVdf" out of range for type '%s' (oid %u)", iv, ctx->name, ctx->oid) + RECVFN(textfmt) { - return newSVpvn_utf8(buf, buflen, 1); + return newSVpvn_utf8(buf, len, 1); } RECVFN(bool) { + RLEN(1); return *buf ? &PL_sv_yes : &PL_sv_no; } +SENDFN(bool) { + fustr_write_ch(out, SvTRUE(val) ? 1 : 0); +} + RECVFN(int2) { + RLEN(2); return newSViv((I16)__builtin_bswap16(*((U16 *)buf))); } +SENDFN(int2) { + SIV(-32768, 32767); + U16 v = __builtin_bswap16((U16)iv); + fustr_write(out, (const char *)&v, 2); +} + RECVFN(int4) { + RLEN(4); return newSViv((I32)__builtin_bswap32(*((U32 *)buf))); } +SENDFN(int4) { + SIV(-2147483648, 2147483647); + U32 v = __builtin_bswap32((U32)iv); + fustr_write(out, (const char *)&v, 4); +} + RECVFN(int8) { + RLEN(8); return newSViv((I64)__builtin_bswap64(*((U64 *)buf))); } +SENDFN(int8) { + SIV(IV_MIN, IV_MAX); + U64 v = __builtin_bswap64((U64)SvIV(val)); + fustr_write(out, (const char *)&v, 8); +} + +#undef RLEN #undef RECVFN +#undef SENDFN #define R(name) fupg_recv_##name +#define S(name) fupg_send_##name -/* Sorted by oid to support binary search. - * (XXX: hash lookup might be faster, but requires codegen) */ -static const fupg_type fupg_types[] = { - { 0, 0, NULL, NULL, R(textfmt) }, /* Invalid Oid, abused for text format */ - { 16, 1, "bool", NULL, R(bool) }, - { 20, 8, "int8", NULL, R(int8) }, - { 21, 2, "int2", NULL, R(int2) }, - { 23, 4, "int4", NULL, R(int4) }, +/* Sorted by oid to support binary search. */ +static const fupg_core_type fupg_core_types[] = { + { 16, "bool", S(bool), R(bool) }, + { 20, "int8", S(int8), R(int8) }, + { 21, "int2", S(int2), R(int2) }, + { 23, "int4", S(int4), R(int4) }, }; /* TODO: A LOT MORE TYPES */ #undef R -#define FUPG_TYPES (sizeof(fupg_types) / sizeof(fupg_type)) +#define FUPG_CORE_TYPES (sizeof(fupg_core_types) / sizeof(fupg_core_type)) -static const fupg_type *fupg_type_lookup(Oid oid) { - int i, b = 0, e = FUPG_TYPES-1; +static const fupg_core_type *fupg_core_type_byoid(Oid oid) { + int i, b = 0, e = FUPG_CORE_TYPES-1; while (b <= e) { i = b + (e - b)/2; - if (fupg_types[i].oid == oid) return fupg_types+i; - if (fupg_types[i].oid < oid) b = i+1; + if (fupg_core_types[i].oid == oid) return fupg_core_types+i; + if (fupg_core_types[i].oid < oid) b = i+1; else e = i-1; } return NULL; diff --git a/t/pgconnect.t b/t/pgconnect.t index ccd848e..351f56c 100644 --- a/t/pgconnect.t +++ b/t/pgconnect.t @@ -66,10 +66,10 @@ subtest '$st prepare & exec', sub { is $conn->exec('SELECT 1 FROM pg_prepared_statements'), 0; ok !eval { $conn->q('SELECT 1', 1)->exec; 1 }; - okerr ERROR => exec => qr/bind message supplies 1 parameters, but prepared statement/; + like $@, qr/Statement expects 0 bind parameters but 1 were given/; ok !eval { $conn->q('SELECT $1')->exec; 1 }; - okerr ERROR => exec => qr/bind message supplies 0 parameters, but prepared statement/; + like $@, qr/Statement expects 1 bind parameters but 0 were given/; # prepare + describe won't let us detect empty queries, hmm... is_deeply $conn->q('')->params, []; @@ -107,7 +107,9 @@ subtest '$st->rowl', sub { is scalar $conn->q('SELECT')->rowl, 0; is scalar $conn->q('SELECT 1, 2')->rowl, 2; is_deeply [$conn->q('SELECT')->rowl], []; - is_deeply [$conn->q('SELECT 1, 2')->rowl], [1, 2]; + is_deeply [$conn->q('SELECT 1, null')->rowl], [1, undef]; + is_deeply [$conn->q('SELECT 1, $1', undef)->rowl], [1, undef]; + is_deeply [$conn->q('SELECT 1, $1::int', undef)->text_params(0)->rowl], [1, undef]; }; subtest '$st->rowa', sub { @@ -120,6 +122,9 @@ subtest '$st->rowa', sub { ok !eval { $conn->q('SELEXT')->rowa; 1; }; is_deeply $conn->q('SELECT')->rowa, []; is_deeply $conn->q('SELECT 1, 2')->rowa, [1, 2]; + is_deeply $conn->q('SELECT 1, null')->rowa, [1, undef]; + is_deeply $conn->q('SELECT 1, $1', undef)->rowa, [1, undef]; + is_deeply $conn->q('SELECT 1, $1::int', undef)->text_params(0)->rowa, [1, undef]; }; subtest '$st->rowh', sub { @@ -135,6 +140,8 @@ subtest '$st->rowh', sub { ok !eval { $conn->q('SELEXT')->rowh; 1; }; is_deeply $conn->q('SELECT')->rowh, {}; is_deeply $conn->q('SELECT 1 as a, 2 as b')->rowh, {a => 1, b => 2}; + is_deeply $conn->q('SELECT 1 as a, null as b')->rowh, {a => 1, b => undef}; + is_deeply $conn->q('SELECT 1 as a, $1::int as b', undef)->rowh, {a => 1, b => undef}; }; subtest 'txn', sub { diff --git a/t/pgtypes.t b/t/pgtypes.t index 1a8a183..51a1111 100644 --- a/t/pgtypes.t +++ b/t/pgtypes.t @@ -10,19 +10,40 @@ plan skip_all => 'Please set FU_TEST_DB to a PostgreSQL connection string to run my $conn = FU::PG->connect($ENV{FU_TEST_DB}); $conn->_debug_trace(0); -sub v($type, $v, $sql=$v) { - $sql = "($sql)::$type"; - my $res = $conn->q("SELECT $sql")->text_results(0)->val; - ok is_bool($res), "recv bool $sql" if $type eq 'bool'; - ok created_as_number($res), "recv number $sql" if $type =~ /^int/; - is $res, $v, "recv value $sql"; +# TODO: Test behavior of magic bind params + +sub v($type, $p_in, @args) { + my $p_out = @args > 0 && ref $args[0] ne 'SCALAR' ? $args[0] : $p_in; + my $s_in = @args > 1 && ref $args[1] ne 'SCALAR' ? $args[1] : $p_in; + my $s_out = @args > 2 && ref $args[2] ne 'SCALAR' ? $args[2] : $s_in; + + { + my $res = $conn->q("SELECT \$1::$type", $s_in)->text_params->val; + ok is_bool($res), "$type $s_in is bool" if $type eq 'bool'; + ok created_as_number($res), "$type $s_in is number" if $type =~ /^int/; + is_deeply $res, $p_out, "$type $s_in text->bin"; + } + { + my $res = $conn->q("SELECT \$1::$type", $p_in)->text_results->val; + is $res, $s_out, "$type $s_out bin->text"; + } + { + my $res = $conn->q("SELECT \$1::$type", $p_in)->val; + is_deeply $res, $p_out, "$type $s_in bin->bin"; + } +} +sub f($type, $p_in) { + ok !eval { $conn->q("SELECT \$1::$type", $p_in)->val; 1 }, "$type $p_in fail"; } -v bool => true, 'true'; -v bool => false, 'false'; +v bool => true, 1, 'true', 't'; +v bool => false, '', 'false', 'f'; -v int2 => $_ for (1, -1, -32768, 32767, 12345, -12345); +v int2 => $_ for (1, -1, -32768, 32767, '12345', -12345, 123.0); +f int2 => $_ for (-32769, 32768, [], '', 'a', 1.5); v int4 => $_ for (1, -1, -2147483648, 2147483647, 1234567890, -1234567890); -v int8 => $_ for (1, -1, -9223372036854775808, 9223372036854775807, 1234567890123456789, -1234567890123456789); +f int4 => $_ for (-2147483649, 2147483648, []); +v int8 => $_ for (1, -1, -9223372036854775808, 9223372036854775807, 1234567890123456789, -1234567890123456789, 1e10); +f int8 => $_ for ('aaa', '-9223372036854775809', '9223372036854775808', 1e20); done_testing;