public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/3] bootstrap: convert Typ to a List* 2+ messages / 2 participants [nested] [flat]
* [PATCH 1/3] bootstrap: convert Typ to a List* @ 2020-11-20 02:48 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Justin Pryzby @ 2020-11-20 02:48 UTC (permalink / raw) --- src/backend/bootstrap/bootstrap.c | 69 ++++++++++++++----------------- 1 file changed, 31 insertions(+), 38 deletions(-) diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 6f615e6622..18eb62ca47 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -159,7 +159,7 @@ struct typmap FormData_pg_type am_typ; }; -static struct typmap **Typ = NULL; +static List *Typ = NIL; /* List of struct typmap* */ static struct typmap *Ap = NULL; static Datum values[MAXATTR]; /* current row's attribute values */ @@ -597,7 +597,7 @@ boot_openrel(char *relname) * pg_type must be filled before any OPEN command is executed, hence we * can now populate the Typ array if we haven't yet. */ - if (Typ == NULL) + if (Typ == NIL) populate_typ_array(); if (boot_reldesc != NULL) @@ -688,7 +688,7 @@ DefineAttr(char *name, char *type, int attnum, int nullness) typeoid = gettype(type); - if (Typ != NULL) + if (Typ != NIL) { attrtypes[attnum]->atttypid = Ap->am_oid; attrtypes[attnum]->attlen = Ap->am_typ.typlen; @@ -877,36 +877,25 @@ populate_typ_array(void) Relation rel; TableScanDesc scan; HeapTuple tup; - int nalloc; - int i; - - Assert(Typ == NULL); - nalloc = 512; - Typ = (struct typmap **) - MemoryContextAlloc(TopMemoryContext, nalloc * sizeof(struct typmap *)); + Assert(Typ == NIL); rel = table_open(TypeRelationId, NoLock); scan = table_beginscan_catalog(rel, 0, NULL); - i = 0; while ((tup = heap_getnext(scan, ForwardScanDirection)) != NULL) { Form_pg_type typForm = (Form_pg_type) GETSTRUCT(tup); + struct typmap *newtyp; + MemoryContext old; - /* make sure there will be room for a trailing NULL pointer */ - if (i >= nalloc - 1) - { - nalloc *= 2; - Typ = (struct typmap **) - repalloc(Typ, nalloc * sizeof(struct typmap *)); - } - Typ[i] = (struct typmap *) - MemoryContextAlloc(TopMemoryContext, sizeof(struct typmap)); - Typ[i]->am_oid = typForm->oid; - memcpy(&(Typ[i]->am_typ), typForm, sizeof(Typ[i]->am_typ)); - i++; + old = MemoryContextSwitchTo(TopMemoryContext); + newtyp = (struct typmap *) palloc(sizeof(struct typmap)); + Typ = lappend(Typ, newtyp); + MemoryContextSwitchTo(old); + + newtyp->am_oid = typForm->oid; + memcpy(&newtyp->am_typ, typForm, sizeof(newtyp->am_typ)); } - Typ[i] = NULL; /* Fill trailing NULL pointer */ table_endscan(scan); table_close(rel, NoLock); } @@ -925,16 +914,17 @@ populate_typ_array(void) static Oid gettype(char *type) { - if (Typ != NULL) + if (Typ != NIL) { - struct typmap **app; + ListCell *lc; - for (app = Typ; *app != NULL; app++) + foreach (lc, Typ) { - if (strncmp(NameStr((*app)->am_typ.typname), type, NAMEDATALEN) == 0) + struct typmap *app = lfirst(lc); + if (strncmp(NameStr(app->am_typ.typname), type, NAMEDATALEN) == 0) { - Ap = *app; - return (*app)->am_oid; + Ap = app; + return app->am_oid; } } } @@ -980,14 +970,17 @@ boot_get_type_io_data(Oid typid, if (Typ != NULL) { /* We have the boot-time contents of pg_type, so use it */ - struct typmap **app; - struct typmap *ap; - - app = Typ; - while (*app && (*app)->am_oid != typid) - ++app; - ap = *app; - if (ap == NULL) + struct typmap *ap = NULL; + ListCell *lc; + + foreach (lc, Typ) + { + ap = lfirst(lc); + if (ap->am_oid == typid) + break; + } + + if (!ap || ap->am_oid != typid) elog(ERROR, "type OID %u not found in Typ list", typid); *typlen = ap->am_typ.typlen; -- 2.26.2 --------------EED78D4A67CFF327F6B880D2 Content-Type: text/x-patch; charset=UTF-8; name="0002-Allow-composite-types-in-bootstrap-20210122b.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0002-Allow-composite-types-in-bootstrap-20210122b.patch" ^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: [oauth] Stabilize the libpq-oauth ABI (and allow alternative implementations?) @ 2026-02-26 21:56 Jacob Champion <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Jacob Champion @ 2026-02-26 21:56 UTC (permalink / raw) To: Zsolt Parragi <[email protected]>; +Cc: pgsql-hackers; Chao Li <[email protected]> On Wed, Feb 25, 2026 at 9:16 AM Zsolt Parragi <[email protected]> wrote: > How would this work with library clients exactly (and even multiple > library clients)? The global setting? Not very well at all. The eventual solution needs to be per-connection. I don't think any version of v3-0006 is likely to make it for PG19, for the record. I just need to ensure that the use case we're moving towards continues to work with the code that's actually committed, and there still might be some pieces that should be cherry-picked backwards in the set. As I said in my original post: > That brings us to patch [v1-]0007, which experimentally promotes the stable > API to a public header, and introduces a really janky environment > variable so that people don't have to play games. It will be obvious > from the code that this is not well-baked yet. My hope was to continue to take small steps in the direction of the end goal with every release, but I think PGOAUTHMODULE is a step too far. I don't want to expose an oauth_module connection option, and an environment variable that can't be subsumed by a future connection option will just be cruft eventually. For 19, bleeding-edge developers who want a global override ASAP can implement the public API and tell their application's linker to find a different libpq-oauth. If a global override isn't good enough, PGOAUTHMODULE wouldn't have helped anyway. > * Language bindings: there's already ongoing work on adding support > for the hooks into these libraries, so that's not really a use case (The hooks are probably insufficient in the general case [1], but a global envvar isn't intended to solve that.) > * postgres CLI tools: That's one use case, but it could work without > generic user-facing plugin support, limited to these tools. Yes. > * postgres_fwd/dblink and similar: I'm not sure how that would work, > what if different extensions require different plugins? This already > seems like a tricky question with the hooks We don't support proxied OAuth at all yet (i.e. both of those extensions outright prohibit oauth_* settings). > I can't just download > a binary application, and a separate oauth plugin, and use them > together, I need admin permission - that seems strange to me. Why does that seem strange? If you don't have the ability to install libpq to begin with, you shouldn't be able to modify that libpq or get it to run arbitrary code. If you control the application, on the other hand, you control both the global hook and the link behavior (you don't have to link against system libpq, after all), so it doesn't seem like you've lost any functionality. > An easy solution could be using secure_getenv instead of getenv, which > would at least improve the situation? I don't think we claim setuid-safety for libpq. (Our own code aside, we'd have to vet all of our transitive dependencies in perpetuity.) > And a few specific comments about the patches: Thank you for the review! > +oom: > + request->error = libpq_gettext("out of memory"); > + return -1; > > Shouldn't this also free conninfo if it is allocated? Yep, good catch. > +/* Features added in PostgreSQL v19: */ > +/* Indicates presence of PQgetThreadLock */ > +#define LIBPQ_HAS_GET_THREAD_LOCK > > Should this be defined to 1? Yes. One of my weirder copy-paste errors... > + if ((lockerr = pthread_mutex_lock(&init_mutex)) != 0) > + { > + /* Should not happen... but don't continue if it does. */ > + Assert(false); > > - libpq_append_conn_error(conn, "failed to lock mutex (%d)", lockerr); > - return 0; > - } > + libpq_append_conn_error(conn, "failed to lock mutex (%d)", lockerr); > + return 0; > + } > > Shouldn't this path return -1, We could. I chose zero to try to retain the PG18 behavior, but I could expand this error message and set request->error instead. If that'd be less confusing to you as a reader, it's probably worth the change. > and also dlclose the library? I don't think we gain anything by that, do we? It's named correctly, it has the right symbols, and the shouldn't-happen-failure-that-happened had nothing to do with the library implementation. > Shouldn't dlcloses also reset state->flow_module after? That'd probably be kinder to anyone expanding this logic in the future, yeah. Maybe not worth a backpatch though (there's no users outside of this function; it's only retained to assist with debugging at the moment). > -free_async_ctx(PGconn *conn, struct async_ctx *actx) > +free_async_ctx(PGoauthBearerRequestV2 *req, struct async_ctx *actx) > > req (or conn) doesn't seem to be used in this function, does it need > that parameter? Ah, right, it's dead now that req->error isn't set. Thanks! > + env = strchr(env, '\x01'); > + *env++ = '\0'; > > Isn't mutating environment variables UB? Kinda, or at least it invites UB later on according to POSIX. I should just make a copy. Thanks, --Jacob [1] https://github.com/ged/ruby-pg/pull/693#issuecomment-3867178201 ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-02-26 21:56 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-20 02:48 [PATCH 1/3] bootstrap: convert Typ to a List* Justin Pryzby <[email protected]> 2026-02-26 21:56 Re: [oauth] Stabilize the libpq-oauth ABI (and allow alternative implementations?) Jacob Champion <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox