public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/5] bootstrap: convert Typ to a List* 5+ messages / 3 participants [nested] [flat]
* [PATCH 1/5] bootstrap: convert Typ to a List* @ 2020-11-20 02:48 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 5+ 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 --------------614DDB87AFFED893713AC0E9 Content-Type: text/x-patch; charset=UTF-8; name="0002-Allow-composite-types-in-bootstrap-20210304.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0002-Allow-composite-types-in-bootstrap-20210304.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible @ 2024-09-03 21:47 Jacob Champion <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Jacob Champion @ 2024-09-03 21:47 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Noah Misch <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]> On Sun, Sep 1, 2024 at 5:10 PM Michael Paquier <[email protected]> wrote: > On Fri, Aug 30, 2024 at 04:10:32PM -0400, Andrew Dunstan wrote: > > Patch 0001 looks sane to me. > So does 0002 to me. Thanks both! > I'm not much a fan of the addition of > pgstat_bestart_pre_auth(), which is just a shortcut to set a different > state in the backend entry to tell that it is authenticating. Is > authenticating the term for this state of the process startups, > actually? Could it be more transparent to use a "startup" or > "starting"" state instead Yeah, I think I should rename that. Especially if we adopt new wait states to make it obvious where we're stuck. "startup", "starting", "initializing", "connecting"...? > that gets also used by pgstat_bestart() in > the case of the patch where !pre_auth? To clarify, do you want me to just add the new boolean directly to pgstat_bestart()'s parameter list? > The addition of the new wait event states in 0004 is a good idea, > indeed, Thanks! Any thoughts on the two open questions for it?: 1) Should we add a new wait event class rather than reusing IPC? 2) Is the level at which I've inserted calls to pgstat_report_wait_start()/_end() sane and maintainable? > and these can be seen in pg_stat_activity once we get out of > PGSTAT_END_WRITE_ACTIVITY() (err.. Right?). It doesn't look like pgstat_report_wait_start() uses that machinery. --Jacob ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible @ 2024-09-10 05:29 Michael Paquier <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Michael Paquier @ 2024-09-10 05:29 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Noah Misch <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]> On Tue, Sep 03, 2024 at 02:47:57PM -0700, Jacob Champion wrote: > On Sun, Sep 1, 2024 at 5:10 PM Michael Paquier <[email protected]> wrote: >> that gets also used by pgstat_bestart() in >> the case of the patch where !pre_auth? > > To clarify, do you want me to just add the new boolean directly to > pgstat_bestart()'s parameter list? No. My question was about splitting pgstat_bestart() and pgstat_bestart_pre_auth() in a cleaner way, because authenticated connections finish by calling both, meaning that we do twice the same setup for backend entries depending on the authentication path taken. That seems like a waste. >> The addition of the new wait event states in 0004 is a good idea, >> indeed, > > Thanks! Any thoughts on the two open questions for it?: > 1) Should we add a new wait event class rather than reusing IPC? A new category would be more adapted. IPC is not adapted because are not waiting for another server process. Perhaps just use a new "Authentication" class, as in "The server is waiting for an authentication operation to complete"? > 2) Is the level at which I've inserted calls to > pgstat_report_wait_start()/_end() sane and maintainable? These don't worry me. You are adding twelve event points with only 5 new wait names. Couldn't it be better to have a one-one mapping instead, adding twelve entries in wait_event_names.txt? I am not really on board with the test based on injection points proposed, though. It checks that the "authenticating" flag is set in pg_stat_activity, but it does nothing else. That seems limited. Or are you planning for more? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible @ 2024-09-11 21:29 Jacob Champion <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Jacob Champion @ 2024-09-11 21:29 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Noah Misch <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Robert Haas <[email protected]> On Mon, Sep 9, 2024 at 10:30 PM Michael Paquier <[email protected]> wrote: > No. My question was about splitting pgstat_bestart() and > pgstat_bestart_pre_auth() in a cleaner way, because authenticated > connections finish by calling both, meaning that we do twice the same > setup for backend entries depending on the authentication path taken. > That seems like a waste. I can try to separate them out. I'm a little wary of messing with the CRITICAL_SECTION guarantees, though. I thought the idea was that you filled in the entire struct to prevent tearing. (If I've misunderstood that, please let me know :D) > Perhaps just use a new > "Authentication" class, as in "The server is waiting for an > authentication operation to complete"? Sounds good. > Couldn't it be better to have a one-one mapping > instead, adding twelve entries in wait_event_names.txt? (I have no strong opinions on this myself, but while the debate is ongoing, I'll work on a version of the patch with more detailed wait events. It's easy to collapse them again if that gets the most votes.) > I am not really on board with the test based on injection points > proposed, though. It checks that the "authenticating" flag is set in > pg_stat_activity, but it does nothing else. That seems limited. Or > are you planning for more? I can test for specific contents of the entry, if you'd like. My primary goal was to test that an entry shows up if that part of the code hangs. I think a regression would otherwise go completely unnoticed. Thanks! --Jacob ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible @ 2024-09-11 23:42 Michael Paquier <[email protected]> parent: Jacob Champion <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Michael Paquier @ 2024-09-11 23:42 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Noah Misch <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Robert Haas <[email protected]> On Wed, Sep 11, 2024 at 02:29:49PM -0700, Jacob Champion wrote: > On Mon, Sep 9, 2024 at 10:30 PM Michael Paquier <[email protected]> wrote: >> No. My question was about splitting pgstat_bestart() and >> pgstat_bestart_pre_auth() in a cleaner way, because authenticated >> connections finish by calling both, meaning that we do twice the same >> setup for backend entries depending on the authentication path taken. >> That seems like a waste. > > I can try to separate them out. I'm a little wary of messing with the > CRITICAL_SECTION guarantees, though. I thought the idea was that you > filled in the entire struct to prevent tearing. (If I've misunderstood > that, please let me know :D) Hm, yeah. We surely should be careful about the consequences of that. Setting up twice the structure as the patch proposes is kind of a weird concept, but it feels to me that we should split that and set the fields in the pre-auth step and ignore the irrelevant ones, then complete the rest in a second step. We are going to do that anyway if we want to be able to have backend entries earlier in the authentication phase. >> Couldn't it be better to have a one-one mapping >> instead, adding twelve entries in wait_event_names.txt? > > (I have no strong opinions on this myself, but while the debate is > ongoing, I'll work on a version of the patch with more detailed wait > events. It's easy to collapse them again if that gets the most votes.) Thanks. Robert is arguing upthread about more granularity, which is also what I understand is the original intention of the wait events. Noah has a different view. Let's see where it goes but I've given my opinion. > I can test for specific contents of the entry, if you'd like. My > primary goal was to test that an entry shows up if that part of the > code hangs. I think a regression would otherwise go completely > unnoticed. Perhaps that would be useful, not sure. Based on my first impressions, I'd tend to say no to these extra test cycles, but I'm okay to be proved wrong, as well. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-09-11 23:42 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-20 02:48 [PATCH 1/5] bootstrap: convert Typ to a List* Justin Pryzby <[email protected]> 2024-09-03 21:47 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]> 2024-09-10 05:29 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]> 2024-09-11 21:29 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]> 2024-09-11 23:42 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[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