From 7c8473533df2f8b4d5ef8243a5aece3a0501cd2c Mon Sep 17 00:00:00 2001 From: Yorhel Date: Fri, 7 Feb 2025 10:59:27 +0100 Subject: [PATCH] pg: More verbose error traces Partly because some errors currently appeared to come from within FU::PG itself, which is useless, and partly because it's common to wrap database access methods, while that's exactly the kind of operation where you *really* want to know where the error originated from. (Source: too much time wasted debugging VNDB errors) --- FU.xs | 8 ++++---- FU/PG.pm | 12 +++++------- c/common.c | 23 ++++++++++++++++++++++- c/pgconn.c | 30 +++++++++++++++--------------- 4 files changed, 46 insertions(+), 27 deletions(-) diff --git a/FU.xs b/FU.xs index 0abce30..e54795a 100644 --- a/FU.xs +++ b/FU.xs @@ -15,10 +15,10 @@ #define FUPG_CONN_COOKIE \ - if (c->cookie) croak("Invalid attempt to run a query on the top-level connection while a transaction object exists") + if (c->cookie) fu_confess("Invalid attempt to run a query on the top-level connection while a transaction object exists") #define FUPG_ST_COOKIE \ - if (st->cookie != st->conn->cookie) croak("Invalid cross-transaction operation on statement object") + if (st->cookie != st->conn->cookie) fu_confess("Invalid cross-transaction operation on statement object") typedef fupg_conn *fupg_txn; @@ -37,14 +37,14 @@ fupg_st * FUPG_ST INPUT FUPG_CONN if (sv_derived_from($arg, \"FU::PG::conn\")) $var = (fupg_conn *)SvIVX(SvRV($arg)); - else croak(\"invalid connection object\"); + else fu_confess(\"invalid connection object\"); FUPG_TXN $var = fupg_get_transaction(aTHX_ $arg); FUPG_ST if (sv_derived_from($arg, \"FU::PG::st\")) $var = (fupg_st *)SvIVX(SvRV($arg)); - else croak(\"invalid statement object\"); + else fu_confess(\"invalid statement object\"); #" EOT diff --git a/FU/PG.pm b/FU/PG.pm index bfadbd4..d7cd69d 100644 --- a/FU/PG.pm +++ b/FU/PG.pm @@ -15,7 +15,7 @@ package FU::PG::conn { }; package FU::PG::txn { - use Carp 'croak'; + use Carp 'confess'; my $COUNTER = 0; @@ -27,19 +27,19 @@ package FU::PG::txn { # 2: $parent, undef if this is a top-level transaction. sub commit($t) { - croak "Unable to commit transaction that has already finished" if !$t->[1]; + confess "Unable to commit transaction that has already finished" if !$t->[1]; $t->exec($t->[2] ? "RELEASE SAVEPOINT fupg_$t->[1]" : 'COMMIT'); $t->[1] = undef; } sub rollback($t) { - croak "Unable to rollback transaction that has already finished" if !$t->[1]; + confess "Unable to rollback transaction that has already finished" if !$t->[1]; $t->exec($t->[2] ? "ROLLBACK TO SAVEPOINT fupg_$t->[1]" : 'ROLLBACK'); $t->[1] = undef; } sub txn($t) { - croak "Unable to create sub-transaction when current transaction has already finished" if !$t->[1]; + confess "Unable to create sub-transaction when current transaction has already finished" if !$t->[1]; $COUNTER++; $t->exec("SAVEPOINT fupg_$COUNTER"); $t->[0]->_set_cookie($COUNTER); @@ -80,7 +80,7 @@ FU::PG - Another PostgreSQL client module $conn->q('INSERT INTO books (title) VALUES ($1)', 'Revelation Space')->exec; - for my ($id, $title) ($conn->q('SELECT * FROM books')->flat) { + for my ($id, $title) ($conn->q('SELECT * FROM books')->flat->@*) { print "$id: $title\n"; } @@ -365,8 +365,6 @@ handling, that is still available: Just don't try to use transaction objects and manual transaction commands at the same time, that won't end well. -=back - =head2 Errors diff --git a/c/common.c b/c/common.c index a0063e9..0379ed0 100644 --- a/c/common.c +++ b/c/common.c @@ -26,7 +26,7 @@ static SV *fu_croak_hv(HV *hv, const char *klass, const char *message, ...) { ENTER; SAVETMPS; PUSHMARK(SP); - XPUSHs(sv_2mortal(sv)); + mXPUSHs(sv); PUTBACK; call_pv("Carp::longmess", G_SCALAR); hv_stores(hv, "full_message", SvREFCNT_inc(POPs)); @@ -36,6 +36,27 @@ static SV *fu_croak_hv(HV *hv, const char *klass, const char *message, ...) { return sv_bless(sv_2mortal(newRV_noinc((SV *)hv)), gv_stashpv(klass, GV_ADD)); } +__attribute__((noreturn, format (printf, 1, 2))) +static void fu_confess(const char *message, ...) { + va_list args; + SV *sv; + dTHX; + dSP; + + va_start(args, message); + sv = vnewSVpvf(message, &args); + va_end(args); + + ENTER; + SAVETMPS; + PUSHMARK(SP); + mXPUSHs(sv); + PUTBACK; + call_pv("Carp::confess", G_DISCARD); + /* Won't happen, but a safe fallback */ + croak("%s", SvPV_nolen(sv)); +} + /* Custom string builder, should be slightly faster than using Sv* macros directly. */ diff --git a/c/pgconn.c b/c/pgconn.c index 887a016..c43a87f 100644 --- a/c/pgconn.c +++ b/c/pgconn.c @@ -142,11 +142,11 @@ static fupg_conn *fupg_get_transaction(pTHX_ SV *sv) { v = av_fetch(av, 1, 0); if (!v || !*v) goto invalid; - if (!SvOK(*v)) croak("Invalid attempt to run a query on a transaction that has already finished"); - if (c->cookie != SvUV(*v)) croak("Invalid cross-transaction operation"); + if (!SvOK(*v)) fu_confess("Invalid attempt to run a query on a transaction that has already finished"); + if (c->cookie != SvUV(*v)) fu_confess("Invalid cross-transaction operation"); return c; invalid: - croak("invalid transaction object"); + fu_confess("invalid transaction object"); } /* Read a Perl value from a PGresult. @@ -196,7 +196,7 @@ static SV *fupg_q(pTHX_ fupg_conn *c, const char *query, I32 ax, I32 argc) { static void fupg_st_prepare(pTHX_ fupg_st *st) { if (st->describe) return; - if (st->prepared) croak("invalid attempt to re-prepare invalid statement"); + if (st->prepared) fu_confess("invalid attempt to re-prepare invalid statement"); /* TODO: This is where we check for any cached prepared statements */ @@ -253,7 +253,7 @@ static void fupg_st_check_dupcols(pTHX_ PGresult *r) { int len = -strlen(key); if (hv_exists(hv, key, len)) { SvREFCNT_dec((SV *)hv); - croak("Query returns multiple columns with the same name ('%s')", key); + fu_confess("Query returns multiple columns with the same name ('%s')", key); } hv_store(hv, key, len, &PL_sv_yes, 0); } @@ -264,7 +264,7 @@ 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 * results in a streaming fashion without breaking API compat. */ - if (st->result) croak("Invalid attempt to execute statement multiple times"); + 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) */ fupg_st_prepare(aTHX_ st); @@ -302,10 +302,10 @@ static SV *fupg_st_exec(pTHX_ fupg_st *st) { static SV *fupg_st_val(pTHX_ fupg_st *st) { fupg_st_prepare(aTHX_ st); - if (PQnfields(st->describe) > 1) croak("Invalid use of $st->val() on query returning more than one column"); - if (PQnfields(st->describe) == 0) croak("Invalid use of $st->val() on query returning no data"); + if (PQnfields(st->describe) > 1) fu_confess("Invalid use of $st->val() on query returning more than one column"); + if (PQnfields(st->describe) == 0) fu_confess("Invalid use of $st->val() on query returning no data"); fupg_st_execute(aTHX_ st); - if (PQntuples(st->result) > 1) croak("Invalid use of $st->val() on query returning more than one row"); + if (PQntuples(st->result) > 1) fu_confess("Invalid use of $st->val() on query returning more than one row"); SV *sv = PQntuples(st->result) == 0 ? newSV(0) : fupg_val(aTHX_ st->result, 0, 0); return sv_2mortal(sv); } @@ -313,8 +313,8 @@ static SV *fupg_st_val(pTHX_ fupg_st *st) { static I32 fupg_st_rowl(pTHX_ fupg_st *st, I32 ax) { dSP; fupg_st_execute(aTHX_ st); - if (PQntuples(st->result) == 0) croak("Invalid use of $st->rowl() on query returning zero rows"); - if (PQntuples(st->result) > 1) croak("Invalid use of $st->rowl() on query returning more than one row"); + if (PQntuples(st->result) == 0) fu_confess("Invalid use of $st->rowl() on query returning zero rows"); + if (PQntuples(st->result) > 1) fu_confess("Invalid use of $st->rowl() on query returning more than one row"); if (GIMME_V != G_LIST) { ST(0) = sv_2mortal(newSViv(PQnfields(st->result))); return 1; @@ -328,8 +328,8 @@ static I32 fupg_st_rowl(pTHX_ fupg_st *st, I32 ax) { static SV *fupg_st_rowa(pTHX_ fupg_st *st) { fupg_st_execute(aTHX_ st); - if (PQntuples(st->result) == 0) croak("Invalid use of $st->rowl() on query returning zero rows"); - if (PQntuples(st->result) > 1) croak("Invalid use of $st->rowl() on query returning more than one row"); + if (PQntuples(st->result) == 0) fu_confess("Invalid use of $st->rowl() on query returning zero rows"); + if (PQntuples(st->result) > 1) fu_confess("Invalid use of $st->rowl() on query returning more than one row"); int i, nfields = PQnfields(st->result); AV *av = newAV_alloc_x(nfields); SV *sv = sv_2mortal(newRV_noinc((SV *)av)); @@ -341,8 +341,8 @@ static SV *fupg_st_rowh(pTHX_ fupg_st *st) { fupg_st_prepare(aTHX_ st); fupg_st_check_dupcols(aTHX_ st->describe); fupg_st_execute(aTHX_ st); - if (PQntuples(st->result) == 0) croak("Invalid use of $st->rowh() on query returning zero rows"); - if (PQntuples(st->result) > 1) croak("Invalid use of $st->rowh() on query returning more than one row"); + if (PQntuples(st->result) == 0) fu_confess("Invalid use of $st->rowh() on query returning zero rows"); + if (PQntuples(st->result) > 1) fu_confess("Invalid use of $st->rowh() on query returning more than one row"); int i, nfields = PQnfields(st->result); HV *hv = newHV(); SV *sv = sv_2mortal(newRV_noinc((SV *)hv));