public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 3/8] Move XLOG stuff from heap_insert and heap_delete
19+ messages / 7 participants
[nested] [flat]
* [PATCH 3/8] Move XLOG stuff from heap_insert and heap_delete
@ 2019-03-25 04:29 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Kyotaro Horiguchi @ 2019-03-25 04:29 UTC (permalink / raw)
Succeeding commit makes heap_update emit insert and delete WAL
records. Move out XLOG stuff for insert and delete so that heap_update
can use the stuff.
---
src/backend/access/heap/heapam.c | 277 ++++++++++++++++++++++-----------------
1 file changed, 157 insertions(+), 120 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 137cc9257d..c6e71dba6b 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -71,6 +71,11 @@
static HeapTuple heap_prepare_insert(Relation relation, HeapTuple tup,
TransactionId xid, CommandId cid, int options);
+static XLogRecPtr log_heap_insert(Relation relation, Buffer buffer,
+ HeapTuple heaptup, int options, bool all_visible_cleared);
+static XLogRecPtr log_heap_delete(Relation relation, Buffer buffer,
+ HeapTuple tp, HeapTuple old_key_tuple, TransactionId new_xmax,
+ bool changingPart, bool all_visible_cleared);
static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf,
Buffer newbuf, HeapTuple oldtup,
HeapTuple newtup, HeapTuple old_key_tup,
@@ -1860,6 +1865,7 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
TransactionId xid = GetCurrentTransactionId();
HeapTuple heaptup;
Buffer buffer;
+ Page page;
Buffer vmbuffer = InvalidBuffer;
bool all_visible_cleared = false;
@@ -1896,16 +1902,18 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
*/
CheckForSerializableConflictIn(relation, NULL, InvalidBuffer);
+ page = BufferGetPage(buffer);
+
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();
RelationPutHeapTuple(relation, buffer, heaptup,
(options & HEAP_INSERT_SPECULATIVE) != 0);
- if (PageIsAllVisible(BufferGetPage(buffer)))
+ if (PageIsAllVisible(page))
{
all_visible_cleared = true;
- PageClearAllVisible(BufferGetPage(buffer));
+ PageClearAllVisible(page);
visibilitymap_clear(relation,
ItemPointerGetBlockNumber(&(heaptup->t_self)),
vmbuffer, VISIBILITYMAP_VALID_BITS);
@@ -1927,76 +1935,11 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
/* XLOG stuff */
if (!(options & HEAP_INSERT_SKIP_WAL) && RelationNeedsWAL(relation))
{
- xl_heap_insert xlrec;
- xl_heap_header xlhdr;
XLogRecPtr recptr;
- Page page = BufferGetPage(buffer);
- uint8 info = XLOG_HEAP_INSERT;
- int bufflags = 0;
-
- /*
- * If this is a catalog, we need to transmit combocids to properly
- * decode, so log that as well.
- */
- if (RelationIsAccessibleInLogicalDecoding(relation))
- log_heap_new_cid(relation, heaptup);
-
- /*
- * If this is the single and first tuple on page, we can reinit the
- * page instead of restoring the whole thing. Set flag, and hide
- * buffer references from XLogInsert.
- */
- if (ItemPointerGetOffsetNumber(&(heaptup->t_self)) == FirstOffsetNumber &&
- PageGetMaxOffsetNumber(page) == FirstOffsetNumber)
- {
- info |= XLOG_HEAP_INIT_PAGE;
- bufflags |= REGBUF_WILL_INIT;
- }
-
- xlrec.offnum = ItemPointerGetOffsetNumber(&heaptup->t_self);
- xlrec.flags = 0;
- if (all_visible_cleared)
- xlrec.flags |= XLH_INSERT_ALL_VISIBLE_CLEARED;
- if (options & HEAP_INSERT_SPECULATIVE)
- xlrec.flags |= XLH_INSERT_IS_SPECULATIVE;
- Assert(ItemPointerGetBlockNumber(&heaptup->t_self) == BufferGetBlockNumber(buffer));
-
- /*
- * For logical decoding, we need the tuple even if we're doing a full
- * page write, so make sure it's included even if we take a full-page
- * image. (XXX We could alternatively store a pointer into the FPW).
- */
- if (RelationIsLogicallyLogged(relation) &&
- !(options & HEAP_INSERT_NO_LOGICAL))
- {
- xlrec.flags |= XLH_INSERT_CONTAINS_NEW_TUPLE;
- bufflags |= REGBUF_KEEP_DATA;
- }
-
- XLogBeginInsert();
- XLogRegisterData((char *) &xlrec, SizeOfHeapInsert);
-
- xlhdr.t_infomask2 = heaptup->t_data->t_infomask2;
- xlhdr.t_infomask = heaptup->t_data->t_infomask;
- xlhdr.t_hoff = heaptup->t_data->t_hoff;
-
- /*
- * note we mark xlhdr as belonging to buffer; if XLogInsert decides to
- * write the whole page to the xlog, we don't need to store
- * xl_heap_header in the xlog.
- */
- XLogRegisterBuffer(0, buffer, REGBUF_STANDARD | bufflags);
- XLogRegisterBufData(0, (char *) &xlhdr, SizeOfHeapHeader);
- /* PG73FORMAT: write bitmap [+ padding] [+ oid] + data */
- XLogRegisterBufData(0,
- (char *) heaptup->t_data + SizeofHeapTupleHeader,
- heaptup->t_len - SizeofHeapTupleHeader);
-
- /* filtering by origin on a row level is much more efficient */
- XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN);
-
- recptr = XLogInsert(RM_HEAP_ID, info);
+ recptr = log_heap_insert(relation, buffer, heaptup,
+ options, all_visible_cleared);
+
PageSetLSN(page, recptr);
}
@@ -2715,58 +2658,10 @@ l1:
*/
if (RelationNeedsWAL(relation))
{
- xl_heap_delete xlrec;
- xl_heap_header xlhdr;
XLogRecPtr recptr;
- /* For logical decode we need combocids to properly decode the catalog */
- if (RelationIsAccessibleInLogicalDecoding(relation))
- log_heap_new_cid(relation, &tp);
-
- xlrec.flags = 0;
- if (all_visible_cleared)
- xlrec.flags |= XLH_DELETE_ALL_VISIBLE_CLEARED;
- if (changingPart)
- xlrec.flags |= XLH_DELETE_IS_PARTITION_MOVE;
- xlrec.infobits_set = compute_infobits(tp.t_data->t_infomask,
- tp.t_data->t_infomask2);
- xlrec.offnum = ItemPointerGetOffsetNumber(&tp.t_self);
- xlrec.xmax = new_xmax;
-
- if (old_key_tuple != NULL)
- {
- if (relation->rd_rel->relreplident == REPLICA_IDENTITY_FULL)
- xlrec.flags |= XLH_DELETE_CONTAINS_OLD_TUPLE;
- else
- xlrec.flags |= XLH_DELETE_CONTAINS_OLD_KEY;
- }
-
- XLogBeginInsert();
- XLogRegisterData((char *) &xlrec, SizeOfHeapDelete);
-
- XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
-
- /*
- * Log replica identity of the deleted tuple if there is one
- */
- if (old_key_tuple != NULL)
- {
- xlhdr.t_infomask2 = old_key_tuple->t_data->t_infomask2;
- xlhdr.t_infomask = old_key_tuple->t_data->t_infomask;
- xlhdr.t_hoff = old_key_tuple->t_data->t_hoff;
-
- XLogRegisterData((char *) &xlhdr, SizeOfHeapHeader);
- XLogRegisterData((char *) old_key_tuple->t_data
- + SizeofHeapTupleHeader,
- old_key_tuple->t_len
- - SizeofHeapTupleHeader);
- }
-
- /* filtering by origin on a row level is much more efficient */
- XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN);
-
- recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_DELETE);
-
+ recptr = log_heap_delete(relation, buffer, &tp, old_key_tuple, new_xmax,
+ changingPart, all_visible_cleared);
PageSetLSN(page, recptr);
}
@@ -7016,6 +6911,148 @@ log_heap_visible(RelFileNode rnode, Buffer heap_buffer, Buffer vm_buffer,
return recptr;
}
+/*
+ * Perform XLogInsert for a heap-insert operation. Caller must already
+ * have modified the buffer and marked it dirty.
+ */
+XLogRecPtr
+log_heap_insert(Relation relation, Buffer buffer,
+ HeapTuple heaptup, int options, bool all_visible_cleared)
+{
+ xl_heap_insert xlrec;
+ xl_heap_header xlhdr;
+ uint8 info = XLOG_HEAP_INSERT;
+ int bufflags = 0;
+ Page page = BufferGetPage(buffer);
+
+ /*
+ * If this is a catalog, we need to transmit combocids to properly
+ * decode, so log that as well.
+ */
+ if (RelationIsAccessibleInLogicalDecoding(relation))
+ log_heap_new_cid(relation, heaptup);
+
+ /*
+ * If this is the single and first tuple on page, we can reinit the
+ * page instead of restoring the whole thing. Set flag, and hide
+ * buffer references from XLogInsert.
+ */
+ if (ItemPointerGetOffsetNumber(&(heaptup->t_self)) == FirstOffsetNumber &&
+ PageGetMaxOffsetNumber(page) == FirstOffsetNumber)
+ {
+ info |= XLOG_HEAP_INIT_PAGE;
+ bufflags |= REGBUF_WILL_INIT;
+ }
+
+ xlrec.offnum = ItemPointerGetOffsetNumber(&heaptup->t_self);
+ xlrec.flags = 0;
+ if (all_visible_cleared)
+ xlrec.flags |= XLH_INSERT_ALL_VISIBLE_CLEARED;
+ if (options & HEAP_INSERT_SPECULATIVE)
+ xlrec.flags |= XLH_INSERT_IS_SPECULATIVE;
+ Assert(ItemPointerGetBlockNumber(&heaptup->t_self) == BufferGetBlockNumber(buffer));
+
+ /*
+ * For logical decoding, we need the tuple even if we're doing a full
+ * page write, so make sure it's included even if we take a full-page
+ * image. (XXX We could alternatively store a pointer into the FPW).
+ */
+ if (RelationIsLogicallyLogged(relation) &&
+ !(options & HEAP_INSERT_NO_LOGICAL))
+ {
+ xlrec.flags |= XLH_INSERT_CONTAINS_NEW_TUPLE;
+ bufflags |= REGBUF_KEEP_DATA;
+ }
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, SizeOfHeapInsert);
+
+ xlhdr.t_infomask2 = heaptup->t_data->t_infomask2;
+ xlhdr.t_infomask = heaptup->t_data->t_infomask;
+ xlhdr.t_hoff = heaptup->t_data->t_hoff;
+
+ /*
+ * note we mark xlhdr as belonging to buffer; if XLogInsert decides to
+ * write the whole page to the xlog, we don't need to store
+ * xl_heap_header in the xlog.
+ */
+ XLogRegisterBuffer(0, buffer, REGBUF_STANDARD | bufflags);
+ XLogRegisterBufData(0, (char *) &xlhdr, SizeOfHeapHeader);
+ /* PG73FORMAT: write bitmap [+ padding] [+ oid] + data */
+ XLogRegisterBufData(0,
+ (char *) heaptup->t_data + SizeofHeapTupleHeader,
+ heaptup->t_len - SizeofHeapTupleHeader);
+
+ /* filtering by origin on a row level is much more efficient */
+ XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN);
+
+ return XLogInsert(RM_HEAP_ID, info);
+}
+
+/*
+ * Perform XLogInsert for a heap-insert operation. Caller must already
+ * have modified the buffer and marked it dirty.
+ *
+ * NB: heap_abort_speculative() uses the same xlog record and replay
+ * routines.
+ */
+static XLogRecPtr
+log_heap_delete(Relation relation, Buffer buffer,
+ HeapTuple tp, HeapTuple old_key_tuple, TransactionId new_xmax,
+ bool changingPart, bool all_visible_cleared)
+{
+ xl_heap_delete xlrec;
+ xl_heap_header xlhdr;
+
+ /* For logical decode we need combocids to properly decode the catalog */
+ if (RelationIsAccessibleInLogicalDecoding(relation))
+ log_heap_new_cid(relation, tp);
+
+ xlrec.flags = 0;
+ if (all_visible_cleared)
+ xlrec.flags |= XLH_DELETE_ALL_VISIBLE_CLEARED;
+ if (changingPart)
+ xlrec.flags |= XLH_DELETE_IS_PARTITION_MOVE;
+ xlrec.infobits_set = compute_infobits(tp->t_data->t_infomask,
+ tp->t_data->t_infomask2);
+ xlrec.offnum = ItemPointerGetOffsetNumber(&tp->t_self);
+ xlrec.xmax = new_xmax;
+
+ if (old_key_tuple != NULL)
+ {
+ if (relation->rd_rel->relreplident == REPLICA_IDENTITY_FULL)
+ xlrec.flags |= XLH_DELETE_CONTAINS_OLD_TUPLE;
+ else
+ xlrec.flags |= XLH_DELETE_CONTAINS_OLD_KEY;
+ }
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, SizeOfHeapDelete);
+
+ XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+
+ /*
+ * Log replica identity of the deleted tuple if there is one
+ */
+ if (old_key_tuple != NULL)
+ {
+ xlhdr.t_infomask2 = old_key_tuple->t_data->t_infomask2;
+ xlhdr.t_infomask = old_key_tuple->t_data->t_infomask;
+ xlhdr.t_hoff = old_key_tuple->t_data->t_hoff;
+
+ XLogRegisterData((char *) &xlhdr, SizeOfHeapHeader);
+ XLogRegisterData((char *) old_key_tuple->t_data
+ + SizeofHeapTupleHeader,
+ old_key_tuple->t_len
+ - SizeofHeapTupleHeader);
+ }
+
+ /* filtering by origin on a row level is much more efficient */
+ XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN);
+
+ return XLogInsert(RM_HEAP_ID, XLOG_HEAP_DELETE);
+}
+
/*
* Perform XLogInsert for a heap-update operation. Caller must already
* have modified the buffer(s) and marked them dirty.
--
2.16.3
----Next_Part(Tue_Mar_26_16_35_07_2019_128)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v9-0004-Add-infrastructure-to-WAL-logging-skip-feature.patch"
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2024-09-10 17:27 Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Noah Misch @ 2024-09-10 17:27 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Jacob Champion <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Tue, Sep 10, 2024 at 02:29:57PM +0900, Michael Paquier wrote:
> 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?
No, I think the patch's level of detail is better. One shouldn't expect the
two ldap_simple_bind_s() calls to have different-enough performance
characteristics to justify exposing that level of detail to the DBA.
ldap_search_s() and InitializeLDAPConnection() differ more, but the DBA mostly
just needs to know the scale of their LDAP responsiveness problem.
(Someday, it might be good to expose the file:line and/or backtrace associated
with a wait, like we do for ereport(). As a way to satisfy rare needs for
more detail, I'd prefer that over giving every pgstat_report_wait_start() a
different name.)
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
@ 2024-09-10 18:51 ` Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Robert Haas @ 2024-09-10 18:51 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Michael Paquier <[email protected]>; Jacob Champion <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Tue, Sep 10, 2024 at 1:27 PM Noah Misch <[email protected]> wrote:
> On Tue, Sep 10, 2024 at 02:29:57PM +0900, Michael Paquier wrote:
> > 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?
>
> No, I think the patch's level of detail is better. One shouldn't expect the
> two ldap_simple_bind_s() calls to have different-enough performance
> characteristics to justify exposing that level of detail to the DBA.
> ldap_search_s() and InitializeLDAPConnection() differ more, but the DBA mostly
> just needs to know the scale of their LDAP responsiveness problem.
>
> (Someday, it might be good to expose the file:line and/or backtrace associated
> with a wait, like we do for ereport(). As a way to satisfy rare needs for
> more detail, I'd prefer that over giving every pgstat_report_wait_start() a
> different name.)
I think unique names are a good idea. If a user doesn't care about the
difference between sdgjsA and sdjgsB, they can easily ignore the
trailing suffix, and IME, people typically do that without really
stopping to think about it. If on the other hand the two are lumped
together as sdjgs and a user needs to distinguish them, they can't. So
I see unique names as having much more upside than downside.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
@ 2024-09-10 20:58 ` Noah Misch <[email protected]>
2024-09-10 22:33 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
0 siblings, 2 replies; 19+ messages in thread
From: Noah Misch @ 2024-09-10 20:58 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Michael Paquier <[email protected]>; Jacob Champion <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Tue, Sep 10, 2024 at 02:51:23PM -0400, Robert Haas wrote:
> On Tue, Sep 10, 2024 at 1:27 PM Noah Misch <[email protected]> wrote:
> > On Tue, Sep 10, 2024 at 02:29:57PM +0900, Michael Paquier wrote:
> > > 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?
> >
> > No, I think the patch's level of detail is better. One shouldn't expect the
> > two ldap_simple_bind_s() calls to have different-enough performance
> > characteristics to justify exposing that level of detail to the DBA.
> > ldap_search_s() and InitializeLDAPConnection() differ more, but the DBA mostly
> > just needs to know the scale of their LDAP responsiveness problem.
> >
> > (Someday, it might be good to expose the file:line and/or backtrace associated
> > with a wait, like we do for ereport(). As a way to satisfy rare needs for
> > more detail, I'd prefer that over giving every pgstat_report_wait_start() a
> > different name.)
>
> I think unique names are a good idea. If a user doesn't care about the
> difference between sdgjsA and sdjgsB, they can easily ignore the
> trailing suffix, and IME, people typically do that without really
> stopping to think about it. If on the other hand the two are lumped
> together as sdjgs and a user needs to distinguish them, they can't. So
> I see unique names as having much more upside than downside.
I agree a person can ignore the distinction, but that requires the person to
be consuming the raw event list. It's reasonable to tell your monitoring tool
to give you the top N wait events. Individual AuthnLdap* events may all miss
the cut even though their aggregate would have made the cut. Before you know
to teach that monitoring tool to group AuthnLdap* together, it won't show you
any of those names.
I felt commit c789f0f also chose sub-optimally in this respect, particularly
with the DblinkGetConnect/DblinkConnect pair. I didn't feel strongly enough
to complain at the time, but a rule of "each wait event appears in one
pgstat_report_wait_start()" would be a rule I don't want. One needs
familiarity with the dblink implementation internals to grasp the
DblinkGetConnect/DblinkConnect distinction, and a plausible refactor of dblink
would make those names cease to fit. I see this level of fine-grained naming
as making the event name a sort of stable proxy for FILE:LINE. I'd value
exposing such a proxy, all else being equal, but I don't think wait event
names like AuthLdapBindLdapbinddn/AuthLdapBindUser are the right way. Wait
event names should be more independent of today's code-level details.
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
@ 2024-09-10 22:33 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 19+ messages in thread
From: Michael Paquier @ 2024-09-10 22:33 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Robert Haas <[email protected]>; Jacob Champion <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Tue, Sep 10, 2024 at 01:58:50PM -0700, Noah Misch wrote:
> On Tue, Sep 10, 2024 at 02:51:23PM -0400, Robert Haas wrote:
>> I think unique names are a good idea. If a user doesn't care about the
>> difference between sdgjsA and sdjgsB, they can easily ignore the
>> trailing suffix, and IME, people typically do that without really
>> stopping to think about it. If on the other hand the two are lumped
>> together as sdjgs and a user needs to distinguish them, they can't. So
>> I see unique names as having much more upside than downside.
>
> I agree a person can ignore the distinction, but that requires the person to
> be consuming the raw event list. It's reasonable to tell your monitoring tool
> to give you the top N wait events. Individual AuthnLdap* events may all miss
> the cut even though their aggregate would have made the cut. Before you know
> to teach that monitoring tool to group AuthnLdap* together, it won't show you
> any of those names.
That's a fair point. I use a bunch of aggregates with group bys for
any monitoring queries looking for event point patterns. In my
experience, when dealing with enough connections, patterns show up
anyway even if there is noise because some of the events that I was
looking for are rather short-term, like a sync events interleaving
with locks storing an average of the events into a secondary table
with some INSERT SELECT.
> I felt commit c789f0f also chose sub-optimally in this respect, particularly
> with the DblinkGetConnect/DblinkConnect pair. I didn't feel strongly enough
> to complain at the time, but a rule of "each wait event appears in one
> pgstat_report_wait_start()" would be a rule I don't want. One needs
> familiarity with the dblink implementation internals to grasp the
> DblinkGetConnect/DblinkConnect distinction, and a plausible refactor of dblink
> would make those names cease to fit. I see this level of fine-grained naming
> as making the event name a sort of stable proxy for FILE:LINE. I'd value
> exposing such a proxy, all else being equal, but I don't think wait event
> names like AuthLdapBindLdapbinddn/AuthLdapBindUser are the right way. Wait
> event names should be more independent of today's code-level details.
Depends. I'd rather choose more granularity to know exactly which
part of the code I am dealing with, especially in the case of this
thread where these are embedded around external function calls. If,
for example, one notices that a stack of pg_stat_activity scans are
complaining about a specific step in the authentication process, it is
going to offer a much better hint than having to guess which part of
the authentication step is slow, like in LDAP.
Wait event additions are also kind of cheap in terms of maintenance in
core, creating a new translation cost. So I also think there are more
upsides to be wilder here with more points and more granularity.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
@ 2024-09-11 13:00 ` Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
1 sibling, 1 reply; 19+ messages in thread
From: Robert Haas @ 2024-09-11 13:00 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Michael Paquier <[email protected]>; Jacob Champion <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Tue, Sep 10, 2024 at 4:58 PM Noah Misch <[email protected]> wrote:
> ... a rule of "each wait event appears in one
> pgstat_report_wait_start()" would be a rule I don't want.
As the original committer of the wait event stuff, I intended for the
rule that you do not want to be the actual rule. However, I see that I
didn't spell that out anywhere in the commit message, or the commit
itself.
> I see this level of fine-grained naming
> as making the event name a sort of stable proxy for FILE:LINE. I'd value
> exposing such a proxy, all else being equal, but I don't think wait event
> names like AuthLdapBindLdapbinddn/AuthLdapBindUser are the right way. Wait
> event names should be more independent of today's code-level details.
I don't agree with that. One of the most difficult parts of supporting
PostgreSQL, in my experience, is that it's often very difficult to
find out what has gone wrong when a system starts behaving badly. It
is often necessary to ask customers to install a debugger and do stuff
with it, or give them an instrumented build, in order to determine the
root cause of a problem that in some cases is not even particularly
complicated. While needing to refer to specific source code details
may not be a common experience for the typical end user, it is
extremely common for me. This problem commonly arises with error
messages, because we have lots of error messages that are exactly the
same, although thankfully it has become less common due to "could not
find tuple for THINGY %u" no longer being a message that no longer
typically reaches users. But even when someone has a complaint about
an error message and there are multiple instances of that error
message, I know that:
(1) I can ask them to set the error verbosity to verbose. I don't have
that option for wait events.
(2) The primary function of the error message is to be understandable
to the user, which means that it needs to be written in plain English.
The primary function of a wait event is to make it possible to
understand the behavior of the system and troubleshoot problems, and
it becomes much less effective as soon as it starts saying that thing
A and thing B are so similar that nobody will ever care about the
distinction. It is very hard to be certain of that. When somebody
reports that they've got a whole bunch of wait events on some wait
event that nobody has ever complained about before, I want to go look
at the code in that specific place and try to figure out what's
happening. If I have to start imagining possible scenarios based on 2
or more call sites, or if I have to start by getting them to install a
modified build with those properly split apart and trying to reproduce
the problem, it's a lot harder.
In my experience, the number of distinct wait events that a particular
installation experiences is rarely very large. It is probably measured
in dozens. A user who wishes to disregard the distinction between
similarly-named wait events won't find it prohibitively difficult to
look over the list of all the wait events they ever see and decide
which ones they'd like to merge for reporting purposes. But a user who
really needs things separated out and finds that they aren't is simply
out of luck.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
@ 2024-09-13 14:56 ` Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Noah Misch @ 2024-09-13 14:56 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Michael Paquier <[email protected]>; Jacob Champion <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Wed, Sep 11, 2024 at 09:00:33AM -0400, Robert Haas wrote:
> On Tue, Sep 10, 2024 at 4:58 PM Noah Misch <[email protected]> wrote:
> > ... a rule of "each wait event appears in one
> > pgstat_report_wait_start()" would be a rule I don't want.
>
> As the original committer of the wait event stuff, I intended for the
> rule that you do not want to be the actual rule. However, I see that I
> didn't spell that out anywhere in the commit message, or the commit
> itself.
>
> > I see this level of fine-grained naming
> > as making the event name a sort of stable proxy for FILE:LINE. I'd value
> > exposing such a proxy, all else being equal, but I don't think wait event
> > names like AuthLdapBindLdapbinddn/AuthLdapBindUser are the right way. Wait
> > event names should be more independent of today's code-level details.
>
> I don't agree with that. One of the most difficult parts of supporting
> PostgreSQL, in my experience, is that it's often very difficult to
> find out what has gone wrong when a system starts behaving badly. It
> is often necessary to ask customers to install a debugger and do stuff
> with it, or give them an instrumented build, in order to determine the
> root cause of a problem that in some cases is not even particularly
> complicated. While needing to refer to specific source code details
> may not be a common experience for the typical end user, it is
> extremely common for me. This problem commonly arises with error
> messages
That is a problem. Half the time, error verbosity doesn't disambiguate enough
for me, and I need backtrace_functions. I now find it hard to believe how
long we coped without backtrace_functions.
I withdraw the objection to "each wait event appears in one
pgstat_report_wait_start()".
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
@ 2024-11-01 21:47 ` Jacob Champion <[email protected]>
2024-11-06 05:48 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Jacob Champion @ 2024-11-01 21:47 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
Hi all,
Here's a v4, with a separate wait event for each location. (I could
use some eyes on the specific phrasing I've chosen for each one.)
On Sun, Sep 1, 2024 at 5:10 PM Michael Paquier <[email protected]> wrote:
> Could it be more transparent to use a "startup" or
> "starting"" state instead that gets also used by pgstat_bestart() in
> the case of the patch where !pre_auth?
Done. (I've used "starting".)
On Mon, Sep 9, 2024 at 10:30 PM Michael Paquier <[email protected]> wrote:
> 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"?
Added a new "Auth" class (to cover both authn and authz during
startup), plus documentation.
On Wed, Sep 11, 2024 at 4:42 PM Michael Paquier <[email protected]> wrote:
> 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.
The more I look at this, the more uneasy I feel about the goal. Best I
can tell, the pre-auth step can't ignore irrelevant fields, because
they may contain junk from the previous owner of the shared memory. So
if we want to optimize, we can only change the second step to skip
fields that were already filled in by the pre-auth step.
That has its own problems: not every backend type uses the pre-auth
step in the current patch. Which means a bunch of backends that don't
benefit from the two-step initialization nevertheless have to either
do two PGSTAT_BEGIN_WRITE_ACTIVITY() dances in a row, or else we
duplicate a bunch of the logic to make sure they maintain the same
efficient code path as before.
Finally, if we're okay with all of that, future maintainers need to be
careful about which fields get copied in the first (preauth) step, the
second step, or both. GSS, for example, can be set up during transport
negotiation (first step) or authentication (second step), so we have
to duplicate the logic there. SSL is currently first-step-only, I
think -- but are we sure we want to hardcode the assumption that cert
auth can't change any of those parameters after the transport has been
established? (I've been brainstorming ways we might use TLS 1.3's
post-handshake CertificateRequest, for example.)
So before I commit to this path, I just want to double-check that all
of the above sounds good and non-controversial. :)
--
In the meantime, is anyone willing and able to commit 0001 and/or 0002?
Thanks!
--Jacob
1: 8f9315949e = 1: 64289b97e5 BackgroundPsql: handle empty query results
2: 7f404f5ee8 ! 2: 18a9531a25 Test::Cluster: let background_psql() work asynchronously
@@ src/test/perl/PostgreSQL/Test/Cluster.pm: connection.
=cut
@@ src/test/perl/PostgreSQL/Test/Cluster.pm: sub background_psql
- '-');
+ '-XAtq', '-d', $psql_connstr, '-f', '-');
$params{on_error_stop} = 1 unless defined $params{on_error_stop};
+ $params{wait} = 1 unless defined $params{wait};
3: a7a7551d09 ! 3: c8071f91d8 pgstat: report in earlier with STATE_AUTHENTICATING
@@ Metadata
Author: Jacob Champion <[email protected]>
## Commit message ##
- pgstat: report in earlier with STATE_AUTHENTICATING
+ pgstat: report in earlier with STATE_STARTING
- Add pgstat_bestart_pre_auth(), which reports an 'authenticating' state
- while waiting for backend initialization and client authentication to
+ Add pgstat_bestart_pre_auth(), which reports a 'starting' state while
+ waiting for backend initialization and client authentication to
complete. Since we hold a transaction open for a good amount of that,
and some authentication methods call out to external systems, having a
pg_stat_activity entry helps DBAs debug when things go badly wrong.
+ ## doc/src/sgml/monitoring.sgml ##
+@@ doc/src/sgml/monitoring.sgml: postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
+ Current overall state of this backend.
+ Possible values are:
+ <itemizedlist>
++ <listitem>
++ <para>
++ <literal>starting</literal>: The backend is in initial startup. Client
++ authentication is performed during this phase.
++ </para>
++ </listitem>
+ <listitem>
+ <para>
+ <literal>active</literal>: The backend is executing a query.
+
## src/backend/utils/activity/backend_status.c ##
@@ src/backend/utils/activity/backend_status.c: static int localNumBackends = 0;
static MemoryContext backendStatusSnapContext;
@@ src/backend/utils/activity/backend_status.c: pgstat_bestart(void)
#endif
- lbeentry.st_state = STATE_UNDEFINED;
-+ lbeentry.st_state = pre_auth ? STATE_AUTHENTICATING : STATE_UNDEFINED;
++ lbeentry.st_state = pre_auth ? STATE_STARTING : STATE_UNDEFINED;
lbeentry.st_progress_command = PROGRESS_COMMAND_INVALID;
lbeentry.st_progress_command_target = InvalidOid;
lbeentry.st_query_id = UINT64CONST(0);
@@ src/backend/utils/adt/pgstatfuncs.c: pg_stat_get_activity(PG_FUNCTION_ARGS)
switch (beentry->st_state)
{
-+ case STATE_AUTHENTICATING:
-+ values[4] = CStringGetTextDatum("authenticating");
++ case STATE_STARTING:
++ values[4] = CStringGetTextDatum("starting");
+ break;
case STATE_IDLE:
values[4] = CStringGetTextDatum("idle");
@@ src/backend/utils/init/postinit.c: InitPostgres(const char *in_dbname, Oid dboid
+ pgstat_beinit();
+
+ /*
-+ * This is a convenient time to sketch in a partial pgstat entry. That way,
-+ * if LWLocks or third-party authentication should happen to hang, the DBA
-+ * will still be able to see what's going on. (A later call to
++ * This is a convenient time to sketch in a partial pgstat entry. That
++ * way, if LWLocks or third-party authentication should happen to hang,
++ * the DBA will still be able to see what's going on. (A later call to
+ * pgstat_bestart() will fill in the rest of the status.)
+ */
+ if (!bootstrap)
@@ src/include/utils/backend_status.h
typedef enum BackendState
{
STATE_UNDEFINED,
-+ STATE_AUTHENTICATING,
++ STATE_STARTING,
STATE_IDLE,
STATE_RUNNING,
STATE_IDLEINTRANSACTION,
@@ src/test/authentication/t/007_injection_points.pl (new)
+my $psql = $node->background_psql('postgres');
+$psql->query_safe("SELECT injection_points_attach('init-pre-auth', 'wait')");
+
-+# From this point on, all new connections will hang in authentication. Use the
-+# $psql connection handle for server interaction.
++# From this point on, all new connections will hang during startup, just before
++# authentication. Use the $psql connection handle for server interaction.
+my $conn = $node->background_psql('postgres', wait => 0);
+
+# Wait for the connection to show up.
@@ src/test/authentication/t/007_injection_points.pl (new)
+while (1)
+{
+ $pid = $psql->query(
-+ "SELECT pid FROM pg_stat_activity WHERE state = 'authenticating';");
++ "SELECT pid FROM pg_stat_activity WHERE state = 'starting';");
+ last if $pid ne "";
+
+ usleep(500_000);
@@ src/test/authentication/t/007_injection_points.pl (new)
+# Make sure the pgstat entry is updated eventually.
+while (1)
+{
-+ my $state = $psql->query(
-+ "SELECT state FROM pg_stat_activity WHERE pid = $pid;");
++ my $state =
++ $psql->query("SELECT state FROM pg_stat_activity WHERE pid = $pid;");
+ last if $state eq "idle";
+
+ note "state for backend $pid is '$state'; waiting for 'idle'...";
4: 5c85c3e0e9 ! 4: d14b97cb77 WIP: report external auth calls as wait events
@@ Metadata
Author: Jacob Champion <[email protected]>
## Commit message ##
- WIP: report external auth calls as wait events
+ Report external auth calls as wait events
- Introduce new WAIT_EVENT_AUTHN_* types for various external
- authentication systems, to make it obvious what's going wrong if one of
- those systems hangs.
+ Introduce a new "Auth" wait class for various external authentication
+ systems, to make it obvious what's going wrong if one of those systems
+ hangs. Each new wait event is unique in order to more easily pinpoint
+ problematic locations in the code.
- TODO:
- - don't abuse the IPC wait event group like this
- - test
+ Discussion: https://postgr.es/m/CAOYmi%2B%3D60deN20WDyCoHCiecgivJxr%3D98s7s7-C8SkXwrCfHXg%40mail.gmail.com
+
+ ## doc/src/sgml/monitoring.sgml ##
+@@ doc/src/sgml/monitoring.sgml: postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
+ see <xref linkend="wait-event-activity-table"/>.
+ </entry>
+ </row>
++ <row>
++ <entry><literal>Auth</literal></entry>
++ <entry>The server process is waiting for an external system to
++ authenticate and/or authorize the client connection.
++ <literal>wait_event</literal> will identify the specific wait point;
++ see <xref linkend="wait-event-auth-table"/>.
++ </entry>
++ </row>
+ <row>
+ <entry><literal>BufferPin</literal></entry>
+ <entry>The server process is waiting for exclusive access to
## src/backend/libpq/auth.c ##
@@
@@ src/backend/libpq/auth.c: pg_GSS_recvauth(Port *port)
elog(DEBUG4, "processing received GSS token of length %u",
(unsigned int) gbuf.length);
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_GSSAPI);
++ pgstat_report_wait_start(WAIT_EVENT_GSSAPI_ACCEPT_SEC_CONTEXT);
maj_stat = gss_accept_sec_context(&min_stat,
&port->gss->ctx,
port->gss->cred,
@@ src/backend/libpq/auth.c: pg_SSPI_recvauth(Port *port)
/*
* Acquire a handle to the server credentials.
*/
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_SSPI);
++ pgstat_report_wait_start(WAIT_EVENT_SSPI_ACQUIRE_CREDENTIALS_HANDLE);
r = AcquireCredentialsHandle(NULL,
"negotiate",
SECPKG_CRED_INBOUND,
@@ src/backend/libpq/auth.c: pg_SSPI_recvauth(Port *port)
elog(DEBUG4, "processing received SSPI token of length %u",
(unsigned int) buf.len);
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_SSPI);
++ pgstat_report_wait_start(WAIT_EVENT_SSPI_ACCEPT_SECURITY_CONTEXT);
r = AcceptSecurityContext(&sspicred,
sspictx,
&inbuf,
@@ src/backend/libpq/auth.c: pg_SSPI_recvauth(Port *port)
CloseHandle(token);
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_SSPI);
++ pgstat_report_wait_start(WAIT_EVENT_SSPI_LOOKUP_ACCOUNT_SID);
if (!LookupAccountSid(NULL, tokenuser->User.Sid, accountname, &accountnamesize,
domainname, &domainnamesize, &accountnameuse))
ereport(ERROR,
@@ src/backend/libpq/auth.c: pg_SSPI_recvauth(Port *port)
- port->hba->upn_username);
+ int status;
+
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_SSPI);
++ pgstat_report_wait_start(WAIT_EVENT_SSPI_MAKE_UPN);
+ status = pg_SSPI_make_upn(accountname, sizeof(accountname),
+ domainname, sizeof(domainname),
+ port->hba->upn_username);
@@ src/backend/libpq/auth.c: CheckPAMAuth(Port *port, const char *user, const char
return STATUS_ERROR;
}
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_PAM);
++ pgstat_report_wait_start(WAIT_EVENT_PAM_AUTHENTICATE);
retval = pam_authenticate(pamh, 0);
+ pgstat_report_wait_end();
@@ src/backend/libpq/auth.c: CheckPAMAuth(Port *port, const char *user, const char
return pam_no_password ? STATUS_EOF : STATUS_ERROR;
}
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_PAM);
++ pgstat_report_wait_start(WAIT_EVENT_PAM_ACCT_MGMT);
retval = pam_acct_mgmt(pamh, 0);
+ pgstat_report_wait_end();
@@ src/backend/libpq/auth.c: CheckLDAPAuth(Port *port)
return STATUS_EOF; /* client wouldn't send password */
- if (InitializeLDAPConnection(port, &ldap) == STATUS_ERROR)
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_LDAP);
++ pgstat_report_wait_start(WAIT_EVENT_LDAP_INITIALIZE);
+ r = InitializeLDAPConnection(port, &ldap);
+ pgstat_report_wait_end();
+
@@ src/backend/libpq/auth.c: CheckLDAPAuth(Port *port)
* Bind with a pre-defined username/password (if available) for
* searching. If none is specified, this turns into an anonymous bind.
*/
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_LDAP);
++ pgstat_report_wait_start(WAIT_EVENT_LDAP_BIND_FOR_SEARCH);
r = ldap_simple_bind_s(ldap,
port->hba->ldapbinddn ? port->hba->ldapbinddn : "",
port->hba->ldapbindpasswd ? ldap_password_hook(port->hba->ldapbindpasswd) : "");
@@ src/backend/libpq/auth.c: CheckLDAPAuth(Port *port)
search_message = NULL;
+
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_LDAP);
++ pgstat_report_wait_start(WAIT_EVENT_LDAP_SEARCH);
r = ldap_search_s(ldap,
port->hba->ldapbasedn,
port->hba->ldapscope,
@@ src/backend/libpq/auth.c: CheckLDAPAuth(Port *port)
port->user_name,
port->hba->ldapsuffix ? port->hba->ldapsuffix : "");
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_LDAP);
++ pgstat_report_wait_start(WAIT_EVENT_LDAP_BIND);
r = ldap_simple_bind_s(ldap, fulluser, passwd);
+ pgstat_report_wait_end();
@@ src/backend/libpq/auth.c: CheckRADIUSAuth(Port *port)
- passwd);
+ int ret;
+
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_RADIUS);
++ pgstat_report_wait_start(WAIT_EVENT_RADIUS_TRANSACTION);
+ ret = PerformRadiusTransaction(lfirst(server),
+ lfirst(secrets),
+ radiusports ? lfirst(radiusports) : NULL,
@@ src/backend/libpq/auth.c: CheckRADIUSAuth(Port *port)
/*------
* STATUS_OK = Login OK
+ ## src/backend/utils/activity/wait_event.c ##
+@@ src/backend/utils/activity/wait_event.c: static const char *pgstat_get_wait_client(WaitEventClient w);
+ static const char *pgstat_get_wait_ipc(WaitEventIPC w);
+ static const char *pgstat_get_wait_timeout(WaitEventTimeout w);
+ static const char *pgstat_get_wait_io(WaitEventIO w);
++static const char *pgstat_get_wait_auth(WaitEventAuth w);
+
+
+ static uint32 local_my_wait_event_info;
+@@ src/backend/utils/activity/wait_event.c: pgstat_get_wait_event_type(uint32 wait_event_info)
+ case PG_WAIT_INJECTIONPOINT:
+ event_type = "InjectionPoint";
+ break;
++ case PG_WAIT_AUTH:
++ event_type = "Auth";
++ break;
+ default:
+ event_type = "???";
+ break;
+@@ src/backend/utils/activity/wait_event.c: pgstat_get_wait_event(uint32 wait_event_info)
+ event_name = pgstat_get_wait_io(w);
+ break;
+ }
++ case PG_WAIT_AUTH:
++ {
++ WaitEventAuth w = (WaitEventAuth) wait_event_info;
++
++ event_name = pgstat_get_wait_auth(w);
++ break;
++ }
+ default:
+ event_name = "unknown wait event";
+ break;
+
## src/backend/utils/activity/wait_event_names.txt ##
-@@ src/backend/utils/activity/wait_event_names.txt: Section: ClassName - WaitEventIPC
- APPEND_READY "Waiting for subplan nodes of an <literal>Append</literal> plan node to be ready."
- ARCHIVE_CLEANUP_COMMAND "Waiting for <xref linkend="guc-archive-cleanup-command"/> to complete."
- ARCHIVE_COMMAND "Waiting for <xref linkend="guc-archive-command"/> to complete."
-+AUTHN_GSSAPI "Waiting for a response from a Kerberos server via GSSAPI."
-+AUTHN_LDAP "Waiting for a response from an LDAP server."
-+AUTHN_PAM "Waiting for a response from the local PAM service."
-+AUTHN_RADIUS "Waiting for a response from a RADIUS server."
-+AUTHN_SSPI "Waiting for a response from a Windows security provider via SSPI."
- BACKEND_TERMINATION "Waiting for the termination of another backend."
- BACKUP_WAIT_WAL_ARCHIVE "Waiting for WAL files required for a backup to be successfully archived."
- BGWORKER_SHUTDOWN "Waiting for background worker to shut down."
+@@ src/backend/utils/activity/wait_event_names.txt: XACT_GROUP_UPDATE "Waiting for the group leader to update transaction status at
+
+ ABI_compatibility:
+
++#
++# Wait Events - Auth
++#
++# Use this category when a process is waiting for a third party to
++# authenticate/authorize the user.
++#
++
++Section: ClassName - WaitEventAuth
++
++GSSAPI_ACCEPT_SEC_CONTEXT "Waiting for a response from a Kerberos server via GSSAPI."
++LDAP_BIND "Waiting for an LDAP bind operation to authenticate the user."
++LDAP_BIND_FOR_SEARCH "Waiting for an LDAP bind operation to search the directory."
++LDAP_INITIALIZE "Waiting to initialize an LDAP connection."
++LDAP_SEARCH "Waiting for an LDAP search operation to complete."
++PAM_ACCT_MGMT "Waiting for the local PAM service to validate the user account."
++PAM_AUTHENTICATE "Waiting for the local PAM service to authenticate the user."
++RADIUS_TRANSACTION "Waiting for a RADIUS transaction to complete."
++SSPI_ACCEPT_SECURITY_CONTEXT "Waiting for a Windows security provider to accept the client's SSPI token."
++SSPI_ACQUIRE_CREDENTIALS_HANDLE "Waiting for a Windows security provider to acquire server credentials for SSPI."
++SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's account SID."
++SSPI_MAKE_UPN "Waiting for Windows to translate a Kerberos UPN."
++
++ABI_compatibility:
++
+ #
+ # Wait Events - Timeout
+ #
+
+ ## src/include/utils/wait_event.h ##
+@@
+ #define PG_WAIT_TIMEOUT 0x09000000U
+ #define PG_WAIT_IO 0x0A000000U
+ #define PG_WAIT_INJECTIONPOINT 0x0B000000U
++#define PG_WAIT_AUTH 0x0C000000U
+
+ /* enums for wait events */
+ #include "utils/wait_event_types.h"
+
+ ## src/test/regress/expected/sysviews.out ##
+@@ src/test/regress/expected/sysviews.out: select type, count(*) > 0 as ok FROM pg_wait_events
+ type | ok
+ -----------+----
+ Activity | t
++ Auth | t
+ BufferPin | t
+ Client | t
+ Extension | t
+@@ src/test/regress/expected/sysviews.out: select type, count(*) > 0 as ok FROM pg_wait_events
+ LWLock | t
+ Lock | t
+ Timeout | t
+-(9 rows)
++(10 rows)
+
+ -- Test that the pg_timezone_names and pg_timezone_abbrevs views are
+ -- more-or-less working. We can't test their contents in any great detail
Attachments:
[text/plain] since-v3.diff.txt (16.5K, ../../CAOYmi+kLzSWrDHZbJg8bWZ94oP_K98mkoEvetgupOBVoy5H_ag@mail.gmail.com/2-since-v3.diff.txt)
download | inline:
1: 8f9315949e = 1: 64289b97e5 BackgroundPsql: handle empty query results
2: 7f404f5ee8 ! 2: 18a9531a25 Test::Cluster: let background_psql() work asynchronously
@@ src/test/perl/PostgreSQL/Test/Cluster.pm: connection.
=cut
@@ src/test/perl/PostgreSQL/Test/Cluster.pm: sub background_psql
- '-');
+ '-XAtq', '-d', $psql_connstr, '-f', '-');
$params{on_error_stop} = 1 unless defined $params{on_error_stop};
+ $params{wait} = 1 unless defined $params{wait};
3: a7a7551d09 ! 3: c8071f91d8 pgstat: report in earlier with STATE_AUTHENTICATING
@@ Metadata
Author: Jacob Champion <[email protected]>
## Commit message ##
- pgstat: report in earlier with STATE_AUTHENTICATING
+ pgstat: report in earlier with STATE_STARTING
- Add pgstat_bestart_pre_auth(), which reports an 'authenticating' state
- while waiting for backend initialization and client authentication to
+ Add pgstat_bestart_pre_auth(), which reports a 'starting' state while
+ waiting for backend initialization and client authentication to
complete. Since we hold a transaction open for a good amount of that,
and some authentication methods call out to external systems, having a
pg_stat_activity entry helps DBAs debug when things go badly wrong.
+ ## doc/src/sgml/monitoring.sgml ##
+@@ doc/src/sgml/monitoring.sgml: postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
+ Current overall state of this backend.
+ Possible values are:
+ <itemizedlist>
++ <listitem>
++ <para>
++ <literal>starting</literal>: The backend is in initial startup. Client
++ authentication is performed during this phase.
++ </para>
++ </listitem>
+ <listitem>
+ <para>
+ <literal>active</literal>: The backend is executing a query.
+
## src/backend/utils/activity/backend_status.c ##
@@ src/backend/utils/activity/backend_status.c: static int localNumBackends = 0;
static MemoryContext backendStatusSnapContext;
@@ src/backend/utils/activity/backend_status.c: pgstat_bestart(void)
#endif
- lbeentry.st_state = STATE_UNDEFINED;
-+ lbeentry.st_state = pre_auth ? STATE_AUTHENTICATING : STATE_UNDEFINED;
++ lbeentry.st_state = pre_auth ? STATE_STARTING : STATE_UNDEFINED;
lbeentry.st_progress_command = PROGRESS_COMMAND_INVALID;
lbeentry.st_progress_command_target = InvalidOid;
lbeentry.st_query_id = UINT64CONST(0);
@@ src/backend/utils/adt/pgstatfuncs.c: pg_stat_get_activity(PG_FUNCTION_ARGS)
switch (beentry->st_state)
{
-+ case STATE_AUTHENTICATING:
-+ values[4] = CStringGetTextDatum("authenticating");
++ case STATE_STARTING:
++ values[4] = CStringGetTextDatum("starting");
+ break;
case STATE_IDLE:
values[4] = CStringGetTextDatum("idle");
@@ src/backend/utils/init/postinit.c: InitPostgres(const char *in_dbname, Oid dboid
+ pgstat_beinit();
+
+ /*
-+ * This is a convenient time to sketch in a partial pgstat entry. That way,
-+ * if LWLocks or third-party authentication should happen to hang, the DBA
-+ * will still be able to see what's going on. (A later call to
++ * This is a convenient time to sketch in a partial pgstat entry. That
++ * way, if LWLocks or third-party authentication should happen to hang,
++ * the DBA will still be able to see what's going on. (A later call to
+ * pgstat_bestart() will fill in the rest of the status.)
+ */
+ if (!bootstrap)
@@ src/include/utils/backend_status.h
typedef enum BackendState
{
STATE_UNDEFINED,
-+ STATE_AUTHENTICATING,
++ STATE_STARTING,
STATE_IDLE,
STATE_RUNNING,
STATE_IDLEINTRANSACTION,
@@ src/test/authentication/t/007_injection_points.pl (new)
+my $psql = $node->background_psql('postgres');
+$psql->query_safe("SELECT injection_points_attach('init-pre-auth', 'wait')");
+
-+# From this point on, all new connections will hang in authentication. Use the
-+# $psql connection handle for server interaction.
++# From this point on, all new connections will hang during startup, just before
++# authentication. Use the $psql connection handle for server interaction.
+my $conn = $node->background_psql('postgres', wait => 0);
+
+# Wait for the connection to show up.
@@ src/test/authentication/t/007_injection_points.pl (new)
+while (1)
+{
+ $pid = $psql->query(
-+ "SELECT pid FROM pg_stat_activity WHERE state = 'authenticating';");
++ "SELECT pid FROM pg_stat_activity WHERE state = 'starting';");
+ last if $pid ne "";
+
+ usleep(500_000);
@@ src/test/authentication/t/007_injection_points.pl (new)
+# Make sure the pgstat entry is updated eventually.
+while (1)
+{
-+ my $state = $psql->query(
-+ "SELECT state FROM pg_stat_activity WHERE pid = $pid;");
++ my $state =
++ $psql->query("SELECT state FROM pg_stat_activity WHERE pid = $pid;");
+ last if $state eq "idle";
+
+ note "state for backend $pid is '$state'; waiting for 'idle'...";
4: 5c85c3e0e9 ! 4: d14b97cb77 WIP: report external auth calls as wait events
@@ Metadata
Author: Jacob Champion <[email protected]>
## Commit message ##
- WIP: report external auth calls as wait events
+ Report external auth calls as wait events
- Introduce new WAIT_EVENT_AUTHN_* types for various external
- authentication systems, to make it obvious what's going wrong if one of
- those systems hangs.
+ Introduce a new "Auth" wait class for various external authentication
+ systems, to make it obvious what's going wrong if one of those systems
+ hangs. Each new wait event is unique in order to more easily pinpoint
+ problematic locations in the code.
- TODO:
- - don't abuse the IPC wait event group like this
- - test
+ Discussion: https://postgr.es/m/CAOYmi%2B%3D60deN20WDyCoHCiecgivJxr%3D98s7s7-C8SkXwrCfHXg%40mail.gmail.com
+
+ ## doc/src/sgml/monitoring.sgml ##
+@@ doc/src/sgml/monitoring.sgml: postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
+ see <xref linkend="wait-event-activity-table"/>.
+ </entry>
+ </row>
++ <row>
++ <entry><literal>Auth</literal></entry>
++ <entry>The server process is waiting for an external system to
++ authenticate and/or authorize the client connection.
++ <literal>wait_event</literal> will identify the specific wait point;
++ see <xref linkend="wait-event-auth-table"/>.
++ </entry>
++ </row>
+ <row>
+ <entry><literal>BufferPin</literal></entry>
+ <entry>The server process is waiting for exclusive access to
## src/backend/libpq/auth.c ##
@@
@@ src/backend/libpq/auth.c: pg_GSS_recvauth(Port *port)
elog(DEBUG4, "processing received GSS token of length %u",
(unsigned int) gbuf.length);
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_GSSAPI);
++ pgstat_report_wait_start(WAIT_EVENT_GSSAPI_ACCEPT_SEC_CONTEXT);
maj_stat = gss_accept_sec_context(&min_stat,
&port->gss->ctx,
port->gss->cred,
@@ src/backend/libpq/auth.c: pg_SSPI_recvauth(Port *port)
/*
* Acquire a handle to the server credentials.
*/
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_SSPI);
++ pgstat_report_wait_start(WAIT_EVENT_SSPI_ACQUIRE_CREDENTIALS_HANDLE);
r = AcquireCredentialsHandle(NULL,
"negotiate",
SECPKG_CRED_INBOUND,
@@ src/backend/libpq/auth.c: pg_SSPI_recvauth(Port *port)
elog(DEBUG4, "processing received SSPI token of length %u",
(unsigned int) buf.len);
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_SSPI);
++ pgstat_report_wait_start(WAIT_EVENT_SSPI_ACCEPT_SECURITY_CONTEXT);
r = AcceptSecurityContext(&sspicred,
sspictx,
&inbuf,
@@ src/backend/libpq/auth.c: pg_SSPI_recvauth(Port *port)
CloseHandle(token);
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_SSPI);
++ pgstat_report_wait_start(WAIT_EVENT_SSPI_LOOKUP_ACCOUNT_SID);
if (!LookupAccountSid(NULL, tokenuser->User.Sid, accountname, &accountnamesize,
domainname, &domainnamesize, &accountnameuse))
ereport(ERROR,
@@ src/backend/libpq/auth.c: pg_SSPI_recvauth(Port *port)
- port->hba->upn_username);
+ int status;
+
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_SSPI);
++ pgstat_report_wait_start(WAIT_EVENT_SSPI_MAKE_UPN);
+ status = pg_SSPI_make_upn(accountname, sizeof(accountname),
+ domainname, sizeof(domainname),
+ port->hba->upn_username);
@@ src/backend/libpq/auth.c: CheckPAMAuth(Port *port, const char *user, const char
return STATUS_ERROR;
}
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_PAM);
++ pgstat_report_wait_start(WAIT_EVENT_PAM_AUTHENTICATE);
retval = pam_authenticate(pamh, 0);
+ pgstat_report_wait_end();
@@ src/backend/libpq/auth.c: CheckPAMAuth(Port *port, const char *user, const char
return pam_no_password ? STATUS_EOF : STATUS_ERROR;
}
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_PAM);
++ pgstat_report_wait_start(WAIT_EVENT_PAM_ACCT_MGMT);
retval = pam_acct_mgmt(pamh, 0);
+ pgstat_report_wait_end();
@@ src/backend/libpq/auth.c: CheckLDAPAuth(Port *port)
return STATUS_EOF; /* client wouldn't send password */
- if (InitializeLDAPConnection(port, &ldap) == STATUS_ERROR)
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_LDAP);
++ pgstat_report_wait_start(WAIT_EVENT_LDAP_INITIALIZE);
+ r = InitializeLDAPConnection(port, &ldap);
+ pgstat_report_wait_end();
+
@@ src/backend/libpq/auth.c: CheckLDAPAuth(Port *port)
* Bind with a pre-defined username/password (if available) for
* searching. If none is specified, this turns into an anonymous bind.
*/
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_LDAP);
++ pgstat_report_wait_start(WAIT_EVENT_LDAP_BIND_FOR_SEARCH);
r = ldap_simple_bind_s(ldap,
port->hba->ldapbinddn ? port->hba->ldapbinddn : "",
port->hba->ldapbindpasswd ? ldap_password_hook(port->hba->ldapbindpasswd) : "");
@@ src/backend/libpq/auth.c: CheckLDAPAuth(Port *port)
search_message = NULL;
+
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_LDAP);
++ pgstat_report_wait_start(WAIT_EVENT_LDAP_SEARCH);
r = ldap_search_s(ldap,
port->hba->ldapbasedn,
port->hba->ldapscope,
@@ src/backend/libpq/auth.c: CheckLDAPAuth(Port *port)
port->user_name,
port->hba->ldapsuffix ? port->hba->ldapsuffix : "");
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_LDAP);
++ pgstat_report_wait_start(WAIT_EVENT_LDAP_BIND);
r = ldap_simple_bind_s(ldap, fulluser, passwd);
+ pgstat_report_wait_end();
@@ src/backend/libpq/auth.c: CheckRADIUSAuth(Port *port)
- passwd);
+ int ret;
+
-+ pgstat_report_wait_start(WAIT_EVENT_AUTHN_RADIUS);
++ pgstat_report_wait_start(WAIT_EVENT_RADIUS_TRANSACTION);
+ ret = PerformRadiusTransaction(lfirst(server),
+ lfirst(secrets),
+ radiusports ? lfirst(radiusports) : NULL,
@@ src/backend/libpq/auth.c: CheckRADIUSAuth(Port *port)
/*------
* STATUS_OK = Login OK
+ ## src/backend/utils/activity/wait_event.c ##
+@@ src/backend/utils/activity/wait_event.c: static const char *pgstat_get_wait_client(WaitEventClient w);
+ static const char *pgstat_get_wait_ipc(WaitEventIPC w);
+ static const char *pgstat_get_wait_timeout(WaitEventTimeout w);
+ static const char *pgstat_get_wait_io(WaitEventIO w);
++static const char *pgstat_get_wait_auth(WaitEventAuth w);
+
+
+ static uint32 local_my_wait_event_info;
+@@ src/backend/utils/activity/wait_event.c: pgstat_get_wait_event_type(uint32 wait_event_info)
+ case PG_WAIT_INJECTIONPOINT:
+ event_type = "InjectionPoint";
+ break;
++ case PG_WAIT_AUTH:
++ event_type = "Auth";
++ break;
+ default:
+ event_type = "???";
+ break;
+@@ src/backend/utils/activity/wait_event.c: pgstat_get_wait_event(uint32 wait_event_info)
+ event_name = pgstat_get_wait_io(w);
+ break;
+ }
++ case PG_WAIT_AUTH:
++ {
++ WaitEventAuth w = (WaitEventAuth) wait_event_info;
++
++ event_name = pgstat_get_wait_auth(w);
++ break;
++ }
+ default:
+ event_name = "unknown wait event";
+ break;
+
## src/backend/utils/activity/wait_event_names.txt ##
-@@ src/backend/utils/activity/wait_event_names.txt: Section: ClassName - WaitEventIPC
- APPEND_READY "Waiting for subplan nodes of an <literal>Append</literal> plan node to be ready."
- ARCHIVE_CLEANUP_COMMAND "Waiting for <xref linkend="guc-archive-cleanup-command"/> to complete."
- ARCHIVE_COMMAND "Waiting for <xref linkend="guc-archive-command"/> to complete."
-+AUTHN_GSSAPI "Waiting for a response from a Kerberos server via GSSAPI."
-+AUTHN_LDAP "Waiting for a response from an LDAP server."
-+AUTHN_PAM "Waiting for a response from the local PAM service."
-+AUTHN_RADIUS "Waiting for a response from a RADIUS server."
-+AUTHN_SSPI "Waiting for a response from a Windows security provider via SSPI."
- BACKEND_TERMINATION "Waiting for the termination of another backend."
- BACKUP_WAIT_WAL_ARCHIVE "Waiting for WAL files required for a backup to be successfully archived."
- BGWORKER_SHUTDOWN "Waiting for background worker to shut down."
+@@ src/backend/utils/activity/wait_event_names.txt: XACT_GROUP_UPDATE "Waiting for the group leader to update transaction status at
+
+ ABI_compatibility:
+
++#
++# Wait Events - Auth
++#
++# Use this category when a process is waiting for a third party to
++# authenticate/authorize the user.
++#
++
++Section: ClassName - WaitEventAuth
++
++GSSAPI_ACCEPT_SEC_CONTEXT "Waiting for a response from a Kerberos server via GSSAPI."
++LDAP_BIND "Waiting for an LDAP bind operation to authenticate the user."
++LDAP_BIND_FOR_SEARCH "Waiting for an LDAP bind operation to search the directory."
++LDAP_INITIALIZE "Waiting to initialize an LDAP connection."
++LDAP_SEARCH "Waiting for an LDAP search operation to complete."
++PAM_ACCT_MGMT "Waiting for the local PAM service to validate the user account."
++PAM_AUTHENTICATE "Waiting for the local PAM service to authenticate the user."
++RADIUS_TRANSACTION "Waiting for a RADIUS transaction to complete."
++SSPI_ACCEPT_SECURITY_CONTEXT "Waiting for a Windows security provider to accept the client's SSPI token."
++SSPI_ACQUIRE_CREDENTIALS_HANDLE "Waiting for a Windows security provider to acquire server credentials for SSPI."
++SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's account SID."
++SSPI_MAKE_UPN "Waiting for Windows to translate a Kerberos UPN."
++
++ABI_compatibility:
++
+ #
+ # Wait Events - Timeout
+ #
+
+ ## src/include/utils/wait_event.h ##
+@@
+ #define PG_WAIT_TIMEOUT 0x09000000U
+ #define PG_WAIT_IO 0x0A000000U
+ #define PG_WAIT_INJECTIONPOINT 0x0B000000U
++#define PG_WAIT_AUTH 0x0C000000U
+
+ /* enums for wait events */
+ #include "utils/wait_event_types.h"
+
+ ## src/test/regress/expected/sysviews.out ##
+@@ src/test/regress/expected/sysviews.out: select type, count(*) > 0 as ok FROM pg_wait_events
+ type | ok
+ -----------+----
+ Activity | t
++ Auth | t
+ BufferPin | t
+ Client | t
+ Extension | t
+@@ src/test/regress/expected/sysviews.out: select type, count(*) > 0 as ok FROM pg_wait_events
+ LWLock | t
+ Lock | t
+ Timeout | t
+-(9 rows)
++(10 rows)
+
+ -- Test that the pg_timezone_names and pg_timezone_abbrevs views are
+ -- more-or-less working. We can't test their contents in any great detail
[application/octet-stream] v4-0001-BackgroundPsql-handle-empty-query-results.patch (2.6K, ../../CAOYmi+kLzSWrDHZbJg8bWZ94oP_K98mkoEvetgupOBVoy5H_ag@mail.gmail.com/3-v4-0001-BackgroundPsql-handle-empty-query-results.patch)
download | inline diff:
From 64289b97e571fe660be57273efec360ba11d96ff Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Mon, 8 Jul 2024 10:11:56 -0700
Subject: [PATCH v4 1/4] BackgroundPsql: handle empty query results
There won't be a newline at the end of an empty query result. (Before
this fix, the $banner showed up in the result, leading to confusing
debugging sessions.)
recovery/t/037_invalid_database was relying on the non-empty query
results, so I have switched those cases to use "bare" calls to
query_safe() instead.
---
src/test/perl/PostgreSQL/Test/BackgroundPsql.pm | 2 +-
src/test/recovery/t/037_invalid_database.pl | 12 ++++--------
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
index 3c2aca1c5d..2760e4bc8d 100644
--- a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
+++ b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
@@ -223,7 +223,7 @@ sub query
$output = $self->{stdout};
# remove banner again, our caller doesn't care
- $output =~ s/\n$banner\n$//s;
+ $output =~ s/\n?$banner\n$//s;
# clear out output for the next query
$self->{stdout} = '';
diff --git a/src/test/recovery/t/037_invalid_database.pl b/src/test/recovery/t/037_invalid_database.pl
index 6d1c711796..e16a3616b2 100644
--- a/src/test/recovery/t/037_invalid_database.pl
+++ b/src/test/recovery/t/037_invalid_database.pl
@@ -96,13 +96,12 @@ my $bgpsql = $node->background_psql('postgres', on_error_stop => 0);
my $pid = $bgpsql->query('SELECT pg_backend_pid()');
# create the database, prevent drop database via lock held by a 2PC transaction
-ok( $bgpsql->query_safe(
+$bgpsql->query_safe(
qq(
CREATE DATABASE regression_invalid_interrupt;
BEGIN;
LOCK pg_tablespace;
- PREPARE TRANSACTION 'lock_tblspc';)),
- "blocked DROP DATABASE completion");
+ PREPARE TRANSACTION 'lock_tblspc';));
# Try to drop. This will wait due to the still held lock.
$bgpsql->query_until(qr//, "DROP DATABASE regression_invalid_interrupt;\n");
@@ -135,11 +134,8 @@ is($node->psql('regression_invalid_interrupt', ''),
# To properly drop the database, we need to release the lock previously preventing
# doing so.
-ok($bgpsql->query_safe(qq(ROLLBACK PREPARED 'lock_tblspc')),
- "unblock DROP DATABASE");
-
-ok($bgpsql->query(qq(DROP DATABASE regression_invalid_interrupt)),
- "DROP DATABASE invalid_interrupt");
+$bgpsql->query_safe(qq(ROLLBACK PREPARED 'lock_tblspc'));
+$bgpsql->query_safe(qq(DROP DATABASE regression_invalid_interrupt));
$bgpsql->quit();
--
2.34.1
[application/octet-stream] v4-0002-Test-Cluster-let-background_psql-work-asynchronou.patch (3.1K, ../../CAOYmi+kLzSWrDHZbJg8bWZ94oP_K98mkoEvetgupOBVoy5H_ag@mail.gmail.com/4-v4-0002-Test-Cluster-let-background_psql-work-asynchronou.patch)
download | inline diff:
From 18a9531a25ad27eebed2f800346f839f8c8d4e72 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Mon, 8 Jul 2024 10:46:55 -0700
Subject: [PATCH v4 2/4] Test::Cluster: let background_psql() work
asynchronously
Specifying `wait => 0` as a parameter to background_psql() causes it to
return immediately, which lets the client run code during connection.
(This is useful if, for example, connections are blocked on injected
waitpoints.) Clients later call ->wait_connect() manually to complete
the asynchronous connection.
---
.../perl/PostgreSQL/Test/BackgroundPsql.pm | 23 ++++++++++++++-----
src/test/perl/PostgreSQL/Test/Cluster.pm | 10 +++++++-
2 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
index 2760e4bc8d..13489ee95e 100644
--- a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
+++ b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
@@ -81,7 +81,7 @@ string. For C<interactive> sessions, IO::Pty is required.
sub new
{
my $class = shift;
- my ($interactive, $psql_params, $timeout) = @_;
+ my ($interactive, $psql_params, $timeout, $wait) = @_;
my $psql = {
'stdin' => '',
'stdout' => '',
@@ -119,14 +119,25 @@ sub new
my $self = bless $psql, $class;
- $self->_wait_connect();
+ $wait = 1 unless defined($wait);
+ if ($wait)
+ {
+ $self->wait_connect();
+ }
return $self;
}
-# Internal routine for awaiting psql starting up and being ready to consume
-# input.
-sub _wait_connect
+=pod
+
+=item $session->wait_connect
+
+Returns once psql has started up and is ready to consume input. This is called
+automatically for clients unless requested otherwise in the constructor.
+
+=cut
+
+sub wait_connect
{
my ($self) = @_;
@@ -187,7 +198,7 @@ sub reconnect_and_clear
$self->{stdin} = '';
$self->{stdout} = '';
- $self->_wait_connect();
+ $self->wait_connect();
}
=pod
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 007571e948..aad09cc53e 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -2286,6 +2286,12 @@ connection.
If given, it must be an array reference containing additional parameters to B<psql>.
+=item wait => 1
+
+By default, this method will not return until connection has completed (or
+failed). Set B<wait> to 0 to return immediately instead. (Clients can call the
+session's C<wait_connect> method manually when needed.)
+
=back
=cut
@@ -2316,13 +2322,15 @@ sub background_psql
'-XAtq', '-d', $psql_connstr, '-f', '-');
$params{on_error_stop} = 1 unless defined $params{on_error_stop};
+ $params{wait} = 1 unless defined $params{wait};
$timeout = $params{timeout} if defined $params{timeout};
push @psql_params, '-v', 'ON_ERROR_STOP=1' if $params{on_error_stop};
push @psql_params, @{ $params{extra_params} }
if defined $params{extra_params};
- return PostgreSQL::Test::BackgroundPsql->new(0, \@psql_params, $timeout);
+ return PostgreSQL::Test::BackgroundPsql->new(0, \@psql_params, $timeout,
+ $params{wait});
}
=pod
--
2.34.1
[application/octet-stream] v4-0003-pgstat-report-in-earlier-with-STATE_STARTING.patch (9.9K, ../../CAOYmi+kLzSWrDHZbJg8bWZ94oP_K98mkoEvetgupOBVoy5H_ag@mail.gmail.com/5-v4-0003-pgstat-report-in-earlier-with-STATE_STARTING.patch)
download | inline diff:
From c8071f91d81906f15eed739877ba75a99028713c Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Fri, 3 May 2024 15:54:58 -0700
Subject: [PATCH v4 3/4] pgstat: report in earlier with STATE_STARTING
Add pgstat_bestart_pre_auth(), which reports a 'starting' state while
waiting for backend initialization and client authentication to
complete. Since we hold a transaction open for a good amount of that,
and some authentication methods call out to external systems, having a
pg_stat_activity entry helps DBAs debug when things go badly wrong.
---
doc/src/sgml/monitoring.sgml | 6 ++
src/backend/utils/activity/backend_status.c | 37 +++++++++-
src/backend/utils/adt/pgstatfuncs.c | 3 +
src/backend/utils/init/postinit.c | 20 ++++-
src/include/utils/backend_status.h | 2 +
src/test/authentication/Makefile | 2 +
src/test/authentication/meson.build | 4 +
.../authentication/t/007_injection_points.pl | 73 +++++++++++++++++++
8 files changed, 140 insertions(+), 7 deletions(-)
create mode 100644 src/test/authentication/t/007_injection_points.pl
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 331315f8d3..81a4a95152 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -899,6 +899,12 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
Current overall state of this backend.
Possible values are:
<itemizedlist>
+ <listitem>
+ <para>
+ <literal>starting</literal>: The backend is in initial startup. Client
+ authentication is performed during this phase.
+ </para>
+ </listitem>
<listitem>
<para>
<literal>active</literal>: The backend is executing a query.
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index bdb3a296ca..d71d7c1b4f 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -69,6 +69,7 @@ static int localNumBackends = 0;
static MemoryContext backendStatusSnapContext;
+static void pgstat_bestart_internal(bool pre_auth);
static void pgstat_beshutdown_hook(int code, Datum arg);
static void pgstat_read_current_status(void);
static void pgstat_setup_backend_status_context(void);
@@ -269,6 +270,34 @@ pgstat_beinit(void)
*/
void
pgstat_bestart(void)
+{
+ pgstat_bestart_internal(false);
+}
+
+
+/* ----------
+ * pgstat_bestart_pre_auth() -
+ *
+ * Like pgstat_beinit(), above, but it's designed to be called before
+ * authentication has been performed (so we have no user or database IDs).
+ * Called from InitPostgres.
+ *----------
+ */
+void
+pgstat_bestart_pre_auth(void)
+{
+ pgstat_bestart_internal(true);
+}
+
+
+/* ----------
+ * pgstat_bestart_internal() -
+ *
+ * Implementation of both flavors of pgstat_bestart().
+ *----------
+ */
+static void
+pgstat_bestart_internal(bool pre_auth)
{
volatile PgBackendStatus *vbeentry = MyBEEntry;
PgBackendStatus lbeentry;
@@ -318,9 +347,9 @@ pgstat_bestart(void)
lbeentry.st_databaseid = MyDatabaseId;
/* We have userid for client-backends, wal-sender and bgworker processes */
- if (lbeentry.st_backendType == B_BACKEND
- || lbeentry.st_backendType == B_WAL_SENDER
- || lbeentry.st_backendType == B_BG_WORKER)
+ if (!pre_auth && (lbeentry.st_backendType == B_BACKEND
+ || lbeentry.st_backendType == B_WAL_SENDER
+ || lbeentry.st_backendType == B_BG_WORKER))
lbeentry.st_userid = GetSessionUserId();
else
lbeentry.st_userid = InvalidOid;
@@ -375,7 +404,7 @@ pgstat_bestart(void)
lbeentry.st_gss = false;
#endif
- lbeentry.st_state = STATE_UNDEFINED;
+ lbeentry.st_state = pre_auth ? STATE_STARTING : STATE_UNDEFINED;
lbeentry.st_progress_command = PROGRESS_COMMAND_INVALID;
lbeentry.st_progress_command_target = InvalidOid;
lbeentry.st_query_id = UINT64CONST(0);
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index f7b50e0b5a..c461bbd400 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -366,6 +366,9 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
switch (beentry->st_state)
{
+ case STATE_STARTING:
+ values[4] = CStringGetTextDatum("starting");
+ break;
case STATE_IDLE:
values[4] = CStringGetTextDatum("idle");
break;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index a024b1151d..adaa83e745 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -58,6 +58,7 @@
#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/pg_locale.h"
#include "utils/portal.h"
@@ -714,6 +715,21 @@ InitPostgres(const char *in_dbname, Oid dboid,
*/
InitProcessPhase2();
+ /* Initialize status reporting */
+ pgstat_beinit();
+
+ /*
+ * This is a convenient time to sketch in a partial pgstat entry. That
+ * way, if LWLocks or third-party authentication should happen to hang,
+ * the DBA will still be able to see what's going on. (A later call to
+ * pgstat_bestart() will fill in the rest of the status.)
+ */
+ if (!bootstrap)
+ {
+ pgstat_bestart_pre_auth();
+ INJECTION_POINT("init-pre-auth");
+ }
+
/*
* Initialize my entry in the shared-invalidation manager's array of
* per-backend data.
@@ -782,9 +798,6 @@ InitPostgres(const char *in_dbname, Oid dboid,
/* Initialize portal manager */
EnablePortalManager();
- /* Initialize status reporting */
- pgstat_beinit();
-
/*
* Load relcache entries for the shared system catalogs. This must create
* at least entries for pg_database and catalogs used for authentication.
@@ -882,6 +895,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
{
/* normal multiuser case */
Assert(MyProcPort != NULL);
+
PerformAuthentication(MyProcPort);
InitializeSessionUserId(username, useroid, false);
/* ensure that auth_method is actually valid, aka authn_id is not NULL */
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 97874300c3..8a6d573ce3 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -24,6 +24,7 @@
typedef enum BackendState
{
STATE_UNDEFINED,
+ STATE_STARTING,
STATE_IDLE,
STATE_RUNNING,
STATE_IDLEINTRANSACTION,
@@ -309,6 +310,7 @@ extern void BackendStatusShmemInit(void);
/* Initialization functions */
extern void pgstat_beinit(void);
+extern void pgstat_bestart_pre_auth(void);
extern void pgstat_bestart(void);
extern void pgstat_clear_backend_activity_snapshot(void);
diff --git a/src/test/authentication/Makefile b/src/test/authentication/Makefile
index da0b71873a..7bbc3b6e57 100644
--- a/src/test/authentication/Makefile
+++ b/src/test/authentication/Makefile
@@ -13,6 +13,8 @@ subdir = src/test/authentication
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+export enable_injection_points
+
check:
$(prove_check)
diff --git a/src/test/authentication/meson.build b/src/test/authentication/meson.build
index 8f5688dcc1..09bad8f2b8 100644
--- a/src/test/authentication/meson.build
+++ b/src/test/authentication/meson.build
@@ -5,6 +5,9 @@ tests += {
'sd': meson.current_source_dir(),
'bd': meson.current_build_dir(),
'tap': {
+ 'env': {
+ 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no',
+ },
'tests': [
't/001_password.pl',
't/002_saslprep.pl',
@@ -12,6 +15,7 @@ tests += {
't/004_file_inclusion.pl',
't/005_sspi.pl',
't/006_login_trigger.pl',
+ 't/007_injection_points.pl',
],
},
}
diff --git a/src/test/authentication/t/007_injection_points.pl b/src/test/authentication/t/007_injection_points.pl
new file mode 100644
index 0000000000..a6176290a6
--- /dev/null
+++ b/src/test/authentication/t/007_injection_points.pl
@@ -0,0 +1,73 @@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Tests requiring injection_points functionality, to check on behavior that
+# would otherwise race against authentication.
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Time::HiRes qw(usleep);
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init;
+$node->append_conf(
+ 'postgresql.conf', q[
+log_connections = on
+]);
+
+$node->start;
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points');
+
+# Connect to the server and inject a waitpoint.
+my $psql = $node->background_psql('postgres');
+$psql->query_safe("SELECT injection_points_attach('init-pre-auth', 'wait')");
+
+# From this point on, all new connections will hang during startup, just before
+# authentication. Use the $psql connection handle for server interaction.
+my $conn = $node->background_psql('postgres', wait => 0);
+
+# Wait for the connection to show up.
+my $pid;
+while (1)
+{
+ $pid = $psql->query(
+ "SELECT pid FROM pg_stat_activity WHERE state = 'starting';");
+ last if $pid ne "";
+
+ usleep(500_000);
+}
+
+note "backend $pid is authenticating";
+ok(1, 'authenticating connections are recorded in pg_stat_activity');
+
+# Detach the waitpoint and wait for the connection to complete.
+$psql->query_safe("SELECT injection_points_wakeup('init-pre-auth');");
+$conn->wait_connect();
+
+# Make sure the pgstat entry is updated eventually.
+while (1)
+{
+ my $state =
+ $psql->query("SELECT state FROM pg_stat_activity WHERE pid = $pid;");
+ last if $state eq "idle";
+
+ note "state for backend $pid is '$state'; waiting for 'idle'...";
+ usleep(500_000);
+}
+
+ok(1, 'authenticated connections reach idle state in pg_stat_activity');
+
+$psql->query_safe("SELECT injection_points_detach('init-pre-auth');");
+$psql->quit();
+$conn->quit();
+
+done_testing();
--
2.34.1
[application/octet-stream] v4-0004-Report-external-auth-calls-as-wait-events.patch (11.5K, ../../CAOYmi+kLzSWrDHZbJg8bWZ94oP_K98mkoEvetgupOBVoy5H_ag@mail.gmail.com/6-v4-0004-Report-external-auth-calls-as-wait-events.patch)
download | inline diff:
From d14b97cb7737e32d9d809794a6671b47788879c0 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Fri, 3 May 2024 15:58:23 -0700
Subject: [PATCH v4 4/4] Report external auth calls as wait events
Introduce a new "Auth" wait class for various external authentication
systems, to make it obvious what's going wrong if one of those systems
hangs. Each new wait event is unique in order to more easily pinpoint
problematic locations in the code.
Discussion: https://postgr.es/m/CAOYmi%2B%3D60deN20WDyCoHCiecgivJxr%3D98s7s7-C8SkXwrCfHXg%40mail.gmail.com
---
doc/src/sgml/monitoring.sgml | 8 +++
src/backend/libpq/auth.c | 54 +++++++++++++++----
src/backend/utils/activity/wait_event.c | 11 ++++
.../utils/activity/wait_event_names.txt | 24 +++++++++
src/include/utils/wait_event.h | 1 +
src/test/regress/expected/sysviews.out | 3 +-
6 files changed, 90 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 81a4a95152..a148e63711 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1045,6 +1045,14 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
see <xref linkend="wait-event-activity-table"/>.
</entry>
</row>
+ <row>
+ <entry><literal>Auth</literal></entry>
+ <entry>The server process is waiting for an external system to
+ authenticate and/or authorize the client connection.
+ <literal>wait_event</literal> will identify the specific wait point;
+ see <xref linkend="wait-event-auth-table"/>.
+ </entry>
+ </row>
<row>
<entry><literal>BufferPin</literal></entry>
<entry>The server process is waiting for exclusive access to
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 47e8c91606..bbcde591ae 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -38,6 +38,7 @@
#include "replication/walsender.h"
#include "storage/ipc.h"
#include "utils/memutils.h"
+#include "utils/wait_event.h"
/*----------------------------------------------------------------
* Global authentication functions
@@ -1000,6 +1001,7 @@ pg_GSS_recvauth(Port *port)
elog(DEBUG4, "processing received GSS token of length %u",
(unsigned int) gbuf.length);
+ pgstat_report_wait_start(WAIT_EVENT_GSSAPI_ACCEPT_SEC_CONTEXT);
maj_stat = gss_accept_sec_context(&min_stat,
&port->gss->ctx,
port->gss->cred,
@@ -1011,6 +1013,7 @@ pg_GSS_recvauth(Port *port)
&gflags,
NULL,
pg_gss_accept_delegation ? &delegated_creds : NULL);
+ pgstat_report_wait_end();
/* gbuf no longer used */
pfree(buf.data);
@@ -1222,6 +1225,7 @@ pg_SSPI_recvauth(Port *port)
/*
* Acquire a handle to the server credentials.
*/
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_ACQUIRE_CREDENTIALS_HANDLE);
r = AcquireCredentialsHandle(NULL,
"negotiate",
SECPKG_CRED_INBOUND,
@@ -1231,6 +1235,8 @@ pg_SSPI_recvauth(Port *port)
NULL,
&sspicred,
&expiry);
+ pgstat_report_wait_end();
+
if (r != SEC_E_OK)
pg_SSPI_error(ERROR, _("could not acquire SSPI credentials"), r);
@@ -1296,6 +1302,7 @@ pg_SSPI_recvauth(Port *port)
elog(DEBUG4, "processing received SSPI token of length %u",
(unsigned int) buf.len);
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_ACCEPT_SECURITY_CONTEXT);
r = AcceptSecurityContext(&sspicred,
sspictx,
&inbuf,
@@ -1305,6 +1312,7 @@ pg_SSPI_recvauth(Port *port)
&outbuf,
&contextattr,
NULL);
+ pgstat_report_wait_end();
/* input buffer no longer used */
pfree(buf.data);
@@ -1402,19 +1410,25 @@ pg_SSPI_recvauth(Port *port)
CloseHandle(token);
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_LOOKUP_ACCOUNT_SID);
if (!LookupAccountSid(NULL, tokenuser->User.Sid, accountname, &accountnamesize,
domainname, &domainnamesize, &accountnameuse))
ereport(ERROR,
(errmsg_internal("could not look up account SID: error code %lu",
GetLastError())));
+ pgstat_report_wait_end();
free(tokenuser);
if (!port->hba->compat_realm)
{
- int status = pg_SSPI_make_upn(accountname, sizeof(accountname),
- domainname, sizeof(domainname),
- port->hba->upn_username);
+ int status;
+
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_MAKE_UPN);
+ status = pg_SSPI_make_upn(accountname, sizeof(accountname),
+ domainname, sizeof(domainname),
+ port->hba->upn_username);
+ pgstat_report_wait_end();
if (status != STATUS_OK)
/* Error already reported from pg_SSPI_make_upn */
@@ -2119,7 +2133,9 @@ CheckPAMAuth(Port *port, const char *user, const char *password)
return STATUS_ERROR;
}
+ pgstat_report_wait_start(WAIT_EVENT_PAM_AUTHENTICATE);
retval = pam_authenticate(pamh, 0);
+ pgstat_report_wait_end();
if (retval != PAM_SUCCESS)
{
@@ -2132,7 +2148,9 @@ CheckPAMAuth(Port *port, const char *user, const char *password)
return pam_no_password ? STATUS_EOF : STATUS_ERROR;
}
+ pgstat_report_wait_start(WAIT_EVENT_PAM_ACCT_MGMT);
retval = pam_acct_mgmt(pamh, 0);
+ pgstat_report_wait_end();
if (retval != PAM_SUCCESS)
{
@@ -2483,7 +2501,11 @@ CheckLDAPAuth(Port *port)
if (passwd == NULL)
return STATUS_EOF; /* client wouldn't send password */
- if (InitializeLDAPConnection(port, &ldap) == STATUS_ERROR)
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_INITIALIZE);
+ r = InitializeLDAPConnection(port, &ldap);
+ pgstat_report_wait_end();
+
+ if (r == STATUS_ERROR)
{
/* Error message already sent */
pfree(passwd);
@@ -2530,9 +2552,12 @@ CheckLDAPAuth(Port *port)
* Bind with a pre-defined username/password (if available) for
* searching. If none is specified, this turns into an anonymous bind.
*/
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_BIND_FOR_SEARCH);
r = ldap_simple_bind_s(ldap,
port->hba->ldapbinddn ? port->hba->ldapbinddn : "",
port->hba->ldapbindpasswd ? ldap_password_hook(port->hba->ldapbindpasswd) : "");
+ pgstat_report_wait_end();
+
if (r != LDAP_SUCCESS)
{
ereport(LOG,
@@ -2555,6 +2580,8 @@ CheckLDAPAuth(Port *port)
filter = psprintf("(uid=%s)", port->user_name);
search_message = NULL;
+
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_SEARCH);
r = ldap_search_s(ldap,
port->hba->ldapbasedn,
port->hba->ldapscope,
@@ -2562,6 +2589,7 @@ CheckLDAPAuth(Port *port)
attributes,
0,
&search_message);
+ pgstat_report_wait_end();
if (r != LDAP_SUCCESS)
{
@@ -2630,7 +2658,9 @@ CheckLDAPAuth(Port *port)
port->user_name,
port->hba->ldapsuffix ? port->hba->ldapsuffix : "");
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_BIND);
r = ldap_simple_bind_s(ldap, fulluser, passwd);
+ pgstat_report_wait_end();
if (r != LDAP_SUCCESS)
{
@@ -2890,12 +2920,16 @@ CheckRADIUSAuth(Port *port)
identifiers = list_head(port->hba->radiusidentifiers);
foreach(server, port->hba->radiusservers)
{
- int ret = PerformRadiusTransaction(lfirst(server),
- lfirst(secrets),
- radiusports ? lfirst(radiusports) : NULL,
- identifiers ? lfirst(identifiers) : NULL,
- port->user_name,
- passwd);
+ int ret;
+
+ pgstat_report_wait_start(WAIT_EVENT_RADIUS_TRANSACTION);
+ ret = PerformRadiusTransaction(lfirst(server),
+ lfirst(secrets),
+ radiusports ? lfirst(radiusports) : NULL,
+ identifiers ? lfirst(identifiers) : NULL,
+ port->user_name,
+ passwd);
+ pgstat_report_wait_end();
/*------
* STATUS_OK = Login OK
diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
index d930277140..a388999b1a 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -34,6 +34,7 @@ static const char *pgstat_get_wait_client(WaitEventClient w);
static const char *pgstat_get_wait_ipc(WaitEventIPC w);
static const char *pgstat_get_wait_timeout(WaitEventTimeout w);
static const char *pgstat_get_wait_io(WaitEventIO w);
+static const char *pgstat_get_wait_auth(WaitEventAuth w);
static uint32 local_my_wait_event_info;
@@ -413,6 +414,9 @@ pgstat_get_wait_event_type(uint32 wait_event_info)
case PG_WAIT_INJECTIONPOINT:
event_type = "InjectionPoint";
break;
+ case PG_WAIT_AUTH:
+ event_type = "Auth";
+ break;
default:
event_type = "???";
break;
@@ -495,6 +499,13 @@ pgstat_get_wait_event(uint32 wait_event_info)
event_name = pgstat_get_wait_io(w);
break;
}
+ case PG_WAIT_AUTH:
+ {
+ WaitEventAuth w = (WaitEventAuth) wait_event_info;
+
+ event_name = pgstat_get_wait_auth(w);
+ break;
+ }
default:
event_name = "unknown wait event";
break;
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 8efb4044d6..e5c27a2d2c 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -162,6 +162,30 @@ XACT_GROUP_UPDATE "Waiting for the group leader to update transaction status at
ABI_compatibility:
+#
+# Wait Events - Auth
+#
+# Use this category when a process is waiting for a third party to
+# authenticate/authorize the user.
+#
+
+Section: ClassName - WaitEventAuth
+
+GSSAPI_ACCEPT_SEC_CONTEXT "Waiting for a response from a Kerberos server via GSSAPI."
+LDAP_BIND "Waiting for an LDAP bind operation to authenticate the user."
+LDAP_BIND_FOR_SEARCH "Waiting for an LDAP bind operation to search the directory."
+LDAP_INITIALIZE "Waiting to initialize an LDAP connection."
+LDAP_SEARCH "Waiting for an LDAP search operation to complete."
+PAM_ACCT_MGMT "Waiting for the local PAM service to validate the user account."
+PAM_AUTHENTICATE "Waiting for the local PAM service to authenticate the user."
+RADIUS_TRANSACTION "Waiting for a RADIUS transaction to complete."
+SSPI_ACCEPT_SECURITY_CONTEXT "Waiting for a Windows security provider to accept the client's SSPI token."
+SSPI_ACQUIRE_CREDENTIALS_HANDLE "Waiting for a Windows security provider to acquire server credentials for SSPI."
+SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's account SID."
+SSPI_MAKE_UPN "Waiting for Windows to translate a Kerberos UPN."
+
+ABI_compatibility:
+
#
# Wait Events - Timeout
#
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index 9f18a753d4..014d536441 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -25,6 +25,7 @@
#define PG_WAIT_TIMEOUT 0x09000000U
#define PG_WAIT_IO 0x0A000000U
#define PG_WAIT_INJECTIONPOINT 0x0B000000U
+#define PG_WAIT_AUTH 0x0C000000U
/* enums for wait events */
#include "utils/wait_event_types.h"
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index fad7fc3a7e..030d1a8321 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -179,6 +179,7 @@ select type, count(*) > 0 as ok FROM pg_wait_events
type | ok
-----------+----
Activity | t
+ Auth | t
BufferPin | t
Client | t
Extension | t
@@ -187,7 +188,7 @@ select type, count(*) > 0 as ok FROM pg_wait_events
LWLock | t
Lock | t
Timeout | t
-(9 rows)
+(10 rows)
-- Test that the pg_timezone_names and pg_timezone_abbrevs views are
-- more-or-less working. We can't test their contents in any great detail
--
2.34.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2024-11-06 05:48 ` Michael Paquier <[email protected]>
2024-11-07 03:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Michael Paquier @ 2024-11-06 05:48 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Fri, Nov 01, 2024 at 02:47:38PM -0700, Jacob Champion wrote:
> On Sun, Sep 1, 2024 at 5:10 PM Michael Paquier <[email protected]> wrote:
>> Could it be more transparent to use a "startup" or
>> "starting"" state instead that gets also used by pgstat_bestart() in
>> the case of the patch where !pre_auth?
>
> Done. (I've used "starting".)
0003 looks much cleaner this way.
> Added a new "Auth" class (to cover both authn and authz during
> startup), plus documentation.
+PAM_ACCT_MGMT "Waiting for the local PAM service to validate the user account."
+PAM_AUTHENTICATE "Waiting for the local PAM service to authenticate the user."
Is "local" required for both? Perhaps just use "the PAM service".
+SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's account SID."
We don't document SID in doc/. So perhaps this should add be "SID
(system identifier)".
+SSPI_MAKE_UPN "Waiting for Windows to translate a Kerberos UPN."
UPN is mentioned once in doc/ already. Perhaps this one is OK left
alone..
Except for these tweaks 0004 looks OK.
> The more I look at this, the more uneasy I feel about the goal. Best I
> can tell, the pre-auth step can't ignore irrelevant fields, because
> they may contain junk from the previous owner of the shared memory. So
> if we want to optimize, we can only change the second step to skip
> fields that were already filled in by the pre-auth step.
>
> That has its own problems: not every backend type uses the pre-auth
> step in the current patch. Which means a bunch of backends that don't
> benefit from the two-step initialization nevertheless have to either
> do two PGSTAT_BEGIN_WRITE_ACTIVITY() dances in a row, or else we
> duplicate a bunch of the logic to make sure they maintain the same
> efficient code path as before.
>
> Finally, if we're okay with all of that, future maintainers need to be
> careful about which fields get copied in the first (preauth) step, the
> second step, or both. GSS, for example, can be set up during transport
> negotiation (first step) or authentication (second step), so we have
> to duplicate the logic there. SSL is currently first-step-only, I
> think -- but are we sure we want to hardcode the assumption that cert
> auth can't change any of those parameters after the transport has been
> established? (I've been brainstorming ways we might use TLS 1.3's
> post-handshake CertificateRequest, for example.)
The future field maintenance and what one would need to think more
about in the future is a good point. I still feel slightly uneasy
about the way 0003 is shaped with its new pgstat_bestart_pre_auth(),
but I think that I'm just going to put my hands down on 0003 and see
if I can finish with something I'm a bit more comfortable with. Let's
see..
> So before I commit to this path, I just want to double-check that all
> of the above sounds good and non-controversial. :)
The goal of the thread is sound.
I'm OK with 0002 to add the wait parameter to BackgroundPsql and be
able to take some actions until a manual wait_connect(). I'll go do
this one. Also perhaps 0001 while on it but I am a bit puzzled by the
removal of the three ok() calls in 037_invalid_database.pl.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-06 05:48 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
@ 2024-11-07 03:15 ` Michael Paquier <[email protected]>
2024-11-07 17:20 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Michael Paquier @ 2024-11-07 03:15 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Wed, Nov 06, 2024 at 02:48:31PM +0900, Michael Paquier wrote:
> I'm OK with 0002 to add the wait parameter to BackgroundPsql and be
> able to take some actions until a manual wait_connect(). I'll go do
> this one. Also perhaps 0001 while on it but I am a bit puzzled by the
> removal of the three ok() calls in 037_invalid_database.pl.
0002 has been done as ba08edb06545 after adding a bit more
documentation that was missing. 0001 as well with 70291a3c66ec. The
original expectation of 037_invalid_database.pl with the banner data
expected in the output was interesting..
Note that 0003 is lacking an EXTRA_INSTALL in the Makefile of
src/test/authentication/, or the test would fail if doing for example
a `make check` in this path.
The following nit is also required in the script for installcheck, to
skip the test if the module is not installed:
if (!$node->check_extension('injection_points'))
{
plan skip_all => 'Extension injection_points not installed';
}
See src/test/modules/test_misc/t/005_timeouts.pl as one example. (I
know, these are tricky to know about..)
007_injection_points.pl is a name too generic as it could apply in a
lot more places, without being linked to injection points. How about
something like 007_pre_auth.pl?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-06 05:48 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 03:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
@ 2024-11-07 17:20 ` Jacob Champion <[email protected]>
2024-11-07 18:12 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Jacob Champion @ 2024-11-07 17:20 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Tue, Nov 5, 2024 at 9:48 PM Michael Paquier <[email protected]> wrote:
> +PAM_ACCT_MGMT "Waiting for the local PAM service to validate the user account."
> +PAM_AUTHENTICATE "Waiting for the local PAM service to authenticate the user."
>
> Is "local" required for both? Perhaps just use "the PAM service".
Done in v5.
> +SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's account SID."
>
> We don't document SID in doc/. So perhaps this should add be "SID
> (system identifier)".
I switched to "user's security identifier", which seems to be
search-engine-friendly.
On Wed, Nov 6, 2024 at 7:15 PM Michael Paquier <[email protected]> wrote:
> 0002 has been done as ba08edb06545 after adding a bit more
> documentation that was missing. 0001 as well with 70291a3c66ec.
Thanks!
> Note that 0003 is lacking an EXTRA_INSTALL in the Makefile of
> src/test/authentication/, or the test would fail if doing for example
> a `make check` in this path.
>
> The following nit is also required in the script for installcheck, to
> skip the test if the module is not installed:
> if (!$node->check_extension('injection_points'))
> {
> plan skip_all => 'Extension injection_points not installed';
> }
Fixed.
> 007_injection_points.pl is a name too generic as it could apply in a
> lot more places, without being linked to injection points. How about
> something like 007_pre_auth.pl?
Renamed.
Thanks!
--Jacob
1: 64289b97e5 < -: ---------- BackgroundPsql: handle empty query results
2: 18a9531a25 < -: ---------- Test::Cluster: let background_psql() work asynchronously
3: c8071f91d8 ! 1: e755fdccf1 pgstat: report in earlier with STATE_STARTING
@@ src/test/authentication/Makefile: subdir = src/test/authentication
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
++EXTRA_INSTALL = src/test/modules/injection_points
++
+export enable_injection_points
+
check:
@@ src/test/authentication/meson.build: tests += {
't/004_file_inclusion.pl',
't/005_sspi.pl',
't/006_login_trigger.pl',
-+ 't/007_injection_points.pl',
++ 't/007_pre_auth.pl',
],
},
}
- ## src/test/authentication/t/007_injection_points.pl (new) ##
+ ## src/test/authentication/t/007_pre_auth.pl (new) ##
@@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
-+# Tests requiring injection_points functionality, to check on behavior that
-+# would otherwise race against authentication.
++# Tests for connection behavior prior to authentication.
+
+use strict;
+use warnings FATAL => 'all';
@@ src/test/authentication/t/007_injection_points.pl (new)
+]);
+
+$node->start;
++
++# Check if the extension injection_points is available, as it may be
++# possible that this script is run with installcheck, where the module
++# would not be installed by default.
++if (!$node->check_extension('injection_points'))
++{
++ plan skip_all => 'Extension injection_points not installed';
++}
++
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points');
+
+# Connect to the server and inject a waitpoint.
4: d14b97cb77 ! 2: 858e95f996 Report external auth calls as wait events
@@ src/backend/utils/activity/wait_event_names.txt: XACT_GROUP_UPDATE "Waiting for
+LDAP_BIND_FOR_SEARCH "Waiting for an LDAP bind operation to search the directory."
+LDAP_INITIALIZE "Waiting to initialize an LDAP connection."
+LDAP_SEARCH "Waiting for an LDAP search operation to complete."
-+PAM_ACCT_MGMT "Waiting for the local PAM service to validate the user account."
-+PAM_AUTHENTICATE "Waiting for the local PAM service to authenticate the user."
++PAM_ACCT_MGMT "Waiting for the PAM service to validate the user account."
++PAM_AUTHENTICATE "Waiting for the PAM service to authenticate the user."
+RADIUS_TRANSACTION "Waiting for a RADIUS transaction to complete."
+SSPI_ACCEPT_SECURITY_CONTEXT "Waiting for a Windows security provider to accept the client's SSPI token."
+SSPI_ACQUIRE_CREDENTIALS_HANDLE "Waiting for a Windows security provider to acquire server credentials for SSPI."
-+SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's account SID."
++SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's security identifier."
+SSPI_MAKE_UPN "Waiting for Windows to translate a Kerberos UPN."
+
+ABI_compatibility:
Attachments:
[text/plain] since-v4.diff.txt (3.1K, ../../CAOYmi+k7Mz5RmaeHKTEcOPSXBLzATD=ofOXS+JpHB8=-cNHR0Q@mail.gmail.com/2-since-v4.diff.txt)
download | inline:
1: 64289b97e5 < -: ---------- BackgroundPsql: handle empty query results
2: 18a9531a25 < -: ---------- Test::Cluster: let background_psql() work asynchronously
3: c8071f91d8 ! 1: e755fdccf1 pgstat: report in earlier with STATE_STARTING
@@ src/test/authentication/Makefile: subdir = src/test/authentication
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
++EXTRA_INSTALL = src/test/modules/injection_points
++
+export enable_injection_points
+
check:
@@ src/test/authentication/meson.build: tests += {
't/004_file_inclusion.pl',
't/005_sspi.pl',
't/006_login_trigger.pl',
-+ 't/007_injection_points.pl',
++ 't/007_pre_auth.pl',
],
},
}
- ## src/test/authentication/t/007_injection_points.pl (new) ##
+ ## src/test/authentication/t/007_pre_auth.pl (new) ##
@@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
-+# Tests requiring injection_points functionality, to check on behavior that
-+# would otherwise race against authentication.
++# Tests for connection behavior prior to authentication.
+
+use strict;
+use warnings FATAL => 'all';
@@ src/test/authentication/t/007_injection_points.pl (new)
+]);
+
+$node->start;
++
++# Check if the extension injection_points is available, as it may be
++# possible that this script is run with installcheck, where the module
++# would not be installed by default.
++if (!$node->check_extension('injection_points'))
++{
++ plan skip_all => 'Extension injection_points not installed';
++}
++
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points');
+
+# Connect to the server and inject a waitpoint.
4: d14b97cb77 ! 2: 858e95f996 Report external auth calls as wait events
@@ src/backend/utils/activity/wait_event_names.txt: XACT_GROUP_UPDATE "Waiting for
+LDAP_BIND_FOR_SEARCH "Waiting for an LDAP bind operation to search the directory."
+LDAP_INITIALIZE "Waiting to initialize an LDAP connection."
+LDAP_SEARCH "Waiting for an LDAP search operation to complete."
-+PAM_ACCT_MGMT "Waiting for the local PAM service to validate the user account."
-+PAM_AUTHENTICATE "Waiting for the local PAM service to authenticate the user."
++PAM_ACCT_MGMT "Waiting for the PAM service to validate the user account."
++PAM_AUTHENTICATE "Waiting for the PAM service to authenticate the user."
+RADIUS_TRANSACTION "Waiting for a RADIUS transaction to complete."
+SSPI_ACCEPT_SECURITY_CONTEXT "Waiting for a Windows security provider to accept the client's SSPI token."
+SSPI_ACQUIRE_CREDENTIALS_HANDLE "Waiting for a Windows security provider to acquire server credentials for SSPI."
-+SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's account SID."
++SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's security identifier."
+SSPI_MAKE_UPN "Waiting for Windows to translate a Kerberos UPN."
+
+ABI_compatibility:
[application/octet-stream] v5-0001-pgstat-report-in-earlier-with-STATE_STARTING.patch (10.2K, ../../CAOYmi+k7Mz5RmaeHKTEcOPSXBLzATD=ofOXS+JpHB8=-cNHR0Q@mail.gmail.com/3-v5-0001-pgstat-report-in-earlier-with-STATE_STARTING.patch)
download | inline diff:
From e755fdccf16cb4fcd3036e1209291750ffecd261 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Fri, 3 May 2024 15:54:58 -0700
Subject: [PATCH v5 1/2] pgstat: report in earlier with STATE_STARTING
Add pgstat_bestart_pre_auth(), which reports a 'starting' state while
waiting for backend initialization and client authentication to
complete. Since we hold a transaction open for a good amount of that,
and some authentication methods call out to external systems, having a
pg_stat_activity entry helps DBAs debug when things go badly wrong.
---
doc/src/sgml/monitoring.sgml | 6 ++
src/backend/utils/activity/backend_status.c | 37 +++++++++-
src/backend/utils/adt/pgstatfuncs.c | 3 +
src/backend/utils/init/postinit.c | 20 ++++-
src/include/utils/backend_status.h | 2 +
src/test/authentication/Makefile | 4 +
src/test/authentication/meson.build | 4 +
src/test/authentication/t/007_pre_auth.pl | 81 +++++++++++++++++++++
8 files changed, 150 insertions(+), 7 deletions(-)
create mode 100644 src/test/authentication/t/007_pre_auth.pl
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 331315f8d3..81a4a95152 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -899,6 +899,12 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
Current overall state of this backend.
Possible values are:
<itemizedlist>
+ <listitem>
+ <para>
+ <literal>starting</literal>: The backend is in initial startup. Client
+ authentication is performed during this phase.
+ </para>
+ </listitem>
<listitem>
<para>
<literal>active</literal>: The backend is executing a query.
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index bdb3a296ca..d71d7c1b4f 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -69,6 +69,7 @@ static int localNumBackends = 0;
static MemoryContext backendStatusSnapContext;
+static void pgstat_bestart_internal(bool pre_auth);
static void pgstat_beshutdown_hook(int code, Datum arg);
static void pgstat_read_current_status(void);
static void pgstat_setup_backend_status_context(void);
@@ -269,6 +270,34 @@ pgstat_beinit(void)
*/
void
pgstat_bestart(void)
+{
+ pgstat_bestart_internal(false);
+}
+
+
+/* ----------
+ * pgstat_bestart_pre_auth() -
+ *
+ * Like pgstat_beinit(), above, but it's designed to be called before
+ * authentication has been performed (so we have no user or database IDs).
+ * Called from InitPostgres.
+ *----------
+ */
+void
+pgstat_bestart_pre_auth(void)
+{
+ pgstat_bestart_internal(true);
+}
+
+
+/* ----------
+ * pgstat_bestart_internal() -
+ *
+ * Implementation of both flavors of pgstat_bestart().
+ *----------
+ */
+static void
+pgstat_bestart_internal(bool pre_auth)
{
volatile PgBackendStatus *vbeentry = MyBEEntry;
PgBackendStatus lbeentry;
@@ -318,9 +347,9 @@ pgstat_bestart(void)
lbeentry.st_databaseid = MyDatabaseId;
/* We have userid for client-backends, wal-sender and bgworker processes */
- if (lbeentry.st_backendType == B_BACKEND
- || lbeentry.st_backendType == B_WAL_SENDER
- || lbeentry.st_backendType == B_BG_WORKER)
+ if (!pre_auth && (lbeentry.st_backendType == B_BACKEND
+ || lbeentry.st_backendType == B_WAL_SENDER
+ || lbeentry.st_backendType == B_BG_WORKER))
lbeentry.st_userid = GetSessionUserId();
else
lbeentry.st_userid = InvalidOid;
@@ -375,7 +404,7 @@ pgstat_bestart(void)
lbeentry.st_gss = false;
#endif
- lbeentry.st_state = STATE_UNDEFINED;
+ lbeentry.st_state = pre_auth ? STATE_STARTING : STATE_UNDEFINED;
lbeentry.st_progress_command = PROGRESS_COMMAND_INVALID;
lbeentry.st_progress_command_target = InvalidOid;
lbeentry.st_query_id = UINT64CONST(0);
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index f7b50e0b5a..c461bbd400 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -366,6 +366,9 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
switch (beentry->st_state)
{
+ case STATE_STARTING:
+ values[4] = CStringGetTextDatum("starting");
+ break;
case STATE_IDLE:
values[4] = CStringGetTextDatum("idle");
break;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index a024b1151d..adaa83e745 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -58,6 +58,7 @@
#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/pg_locale.h"
#include "utils/portal.h"
@@ -714,6 +715,21 @@ InitPostgres(const char *in_dbname, Oid dboid,
*/
InitProcessPhase2();
+ /* Initialize status reporting */
+ pgstat_beinit();
+
+ /*
+ * This is a convenient time to sketch in a partial pgstat entry. That
+ * way, if LWLocks or third-party authentication should happen to hang,
+ * the DBA will still be able to see what's going on. (A later call to
+ * pgstat_bestart() will fill in the rest of the status.)
+ */
+ if (!bootstrap)
+ {
+ pgstat_bestart_pre_auth();
+ INJECTION_POINT("init-pre-auth");
+ }
+
/*
* Initialize my entry in the shared-invalidation manager's array of
* per-backend data.
@@ -782,9 +798,6 @@ InitPostgres(const char *in_dbname, Oid dboid,
/* Initialize portal manager */
EnablePortalManager();
- /* Initialize status reporting */
- pgstat_beinit();
-
/*
* Load relcache entries for the shared system catalogs. This must create
* at least entries for pg_database and catalogs used for authentication.
@@ -882,6 +895,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
{
/* normal multiuser case */
Assert(MyProcPort != NULL);
+
PerformAuthentication(MyProcPort);
InitializeSessionUserId(username, useroid, false);
/* ensure that auth_method is actually valid, aka authn_id is not NULL */
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 97874300c3..8a6d573ce3 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -24,6 +24,7 @@
typedef enum BackendState
{
STATE_UNDEFINED,
+ STATE_STARTING,
STATE_IDLE,
STATE_RUNNING,
STATE_IDLEINTRANSACTION,
@@ -309,6 +310,7 @@ extern void BackendStatusShmemInit(void);
/* Initialization functions */
extern void pgstat_beinit(void);
+extern void pgstat_bestart_pre_auth(void);
extern void pgstat_bestart(void);
extern void pgstat_clear_backend_activity_snapshot(void);
diff --git a/src/test/authentication/Makefile b/src/test/authentication/Makefile
index da0b71873a..d05b15de7c 100644
--- a/src/test/authentication/Makefile
+++ b/src/test/authentication/Makefile
@@ -13,6 +13,10 @@ subdir = src/test/authentication
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+EXTRA_INSTALL = src/test/modules/injection_points
+
+export enable_injection_points
+
check:
$(prove_check)
diff --git a/src/test/authentication/meson.build b/src/test/authentication/meson.build
index 8f5688dcc1..fe4ca51873 100644
--- a/src/test/authentication/meson.build
+++ b/src/test/authentication/meson.build
@@ -5,6 +5,9 @@ tests += {
'sd': meson.current_source_dir(),
'bd': meson.current_build_dir(),
'tap': {
+ 'env': {
+ 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no',
+ },
'tests': [
't/001_password.pl',
't/002_saslprep.pl',
@@ -12,6 +15,7 @@ tests += {
't/004_file_inclusion.pl',
't/005_sspi.pl',
't/006_login_trigger.pl',
+ 't/007_pre_auth.pl',
],
},
}
diff --git a/src/test/authentication/t/007_pre_auth.pl b/src/test/authentication/t/007_pre_auth.pl
new file mode 100644
index 0000000000..dd554462d3
--- /dev/null
+++ b/src/test/authentication/t/007_pre_auth.pl
@@ -0,0 +1,81 @@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Tests for connection behavior prior to authentication.
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Time::HiRes qw(usleep);
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+ plan skip_all => 'Injection points not supported by this build';
+}
+
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init;
+$node->append_conf(
+ 'postgresql.conf', q[
+log_connections = on
+]);
+
+$node->start;
+
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+if (!$node->check_extension('injection_points'))
+{
+ plan skip_all => 'Extension injection_points not installed';
+}
+
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points');
+
+# Connect to the server and inject a waitpoint.
+my $psql = $node->background_psql('postgres');
+$psql->query_safe("SELECT injection_points_attach('init-pre-auth', 'wait')");
+
+# From this point on, all new connections will hang during startup, just before
+# authentication. Use the $psql connection handle for server interaction.
+my $conn = $node->background_psql('postgres', wait => 0);
+
+# Wait for the connection to show up.
+my $pid;
+while (1)
+{
+ $pid = $psql->query(
+ "SELECT pid FROM pg_stat_activity WHERE state = 'starting';");
+ last if $pid ne "";
+
+ usleep(500_000);
+}
+
+note "backend $pid is authenticating";
+ok(1, 'authenticating connections are recorded in pg_stat_activity');
+
+# Detach the waitpoint and wait for the connection to complete.
+$psql->query_safe("SELECT injection_points_wakeup('init-pre-auth');");
+$conn->wait_connect();
+
+# Make sure the pgstat entry is updated eventually.
+while (1)
+{
+ my $state =
+ $psql->query("SELECT state FROM pg_stat_activity WHERE pid = $pid;");
+ last if $state eq "idle";
+
+ note "state for backend $pid is '$state'; waiting for 'idle'...";
+ usleep(500_000);
+}
+
+ok(1, 'authenticated connections reach idle state in pg_stat_activity');
+
+$psql->query_safe("SELECT injection_points_detach('init-pre-auth');");
+$psql->quit();
+$conn->quit();
+
+done_testing();
--
2.34.1
[application/octet-stream] v5-0002-Report-external-auth-calls-as-wait-events.patch (11.5K, ../../CAOYmi+k7Mz5RmaeHKTEcOPSXBLzATD=ofOXS+JpHB8=-cNHR0Q@mail.gmail.com/4-v5-0002-Report-external-auth-calls-as-wait-events.patch)
download | inline diff:
From 858e95f996589461e2840047fa35675b6f96e46d Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Fri, 3 May 2024 15:58:23 -0700
Subject: [PATCH v5 2/2] Report external auth calls as wait events
Introduce a new "Auth" wait class for various external authentication
systems, to make it obvious what's going wrong if one of those systems
hangs. Each new wait event is unique in order to more easily pinpoint
problematic locations in the code.
Discussion: https://postgr.es/m/CAOYmi%2B%3D60deN20WDyCoHCiecgivJxr%3D98s7s7-C8SkXwrCfHXg%40mail.gmail.com
---
doc/src/sgml/monitoring.sgml | 8 +++
src/backend/libpq/auth.c | 54 +++++++++++++++----
src/backend/utils/activity/wait_event.c | 11 ++++
.../utils/activity/wait_event_names.txt | 24 +++++++++
src/include/utils/wait_event.h | 1 +
src/test/regress/expected/sysviews.out | 3 +-
6 files changed, 90 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 81a4a95152..a148e63711 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1045,6 +1045,14 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
see <xref linkend="wait-event-activity-table"/>.
</entry>
</row>
+ <row>
+ <entry><literal>Auth</literal></entry>
+ <entry>The server process is waiting for an external system to
+ authenticate and/or authorize the client connection.
+ <literal>wait_event</literal> will identify the specific wait point;
+ see <xref linkend="wait-event-auth-table"/>.
+ </entry>
+ </row>
<row>
<entry><literal>BufferPin</literal></entry>
<entry>The server process is waiting for exclusive access to
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 47e8c91606..bbcde591ae 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -38,6 +38,7 @@
#include "replication/walsender.h"
#include "storage/ipc.h"
#include "utils/memutils.h"
+#include "utils/wait_event.h"
/*----------------------------------------------------------------
* Global authentication functions
@@ -1000,6 +1001,7 @@ pg_GSS_recvauth(Port *port)
elog(DEBUG4, "processing received GSS token of length %u",
(unsigned int) gbuf.length);
+ pgstat_report_wait_start(WAIT_EVENT_GSSAPI_ACCEPT_SEC_CONTEXT);
maj_stat = gss_accept_sec_context(&min_stat,
&port->gss->ctx,
port->gss->cred,
@@ -1011,6 +1013,7 @@ pg_GSS_recvauth(Port *port)
&gflags,
NULL,
pg_gss_accept_delegation ? &delegated_creds : NULL);
+ pgstat_report_wait_end();
/* gbuf no longer used */
pfree(buf.data);
@@ -1222,6 +1225,7 @@ pg_SSPI_recvauth(Port *port)
/*
* Acquire a handle to the server credentials.
*/
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_ACQUIRE_CREDENTIALS_HANDLE);
r = AcquireCredentialsHandle(NULL,
"negotiate",
SECPKG_CRED_INBOUND,
@@ -1231,6 +1235,8 @@ pg_SSPI_recvauth(Port *port)
NULL,
&sspicred,
&expiry);
+ pgstat_report_wait_end();
+
if (r != SEC_E_OK)
pg_SSPI_error(ERROR, _("could not acquire SSPI credentials"), r);
@@ -1296,6 +1302,7 @@ pg_SSPI_recvauth(Port *port)
elog(DEBUG4, "processing received SSPI token of length %u",
(unsigned int) buf.len);
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_ACCEPT_SECURITY_CONTEXT);
r = AcceptSecurityContext(&sspicred,
sspictx,
&inbuf,
@@ -1305,6 +1312,7 @@ pg_SSPI_recvauth(Port *port)
&outbuf,
&contextattr,
NULL);
+ pgstat_report_wait_end();
/* input buffer no longer used */
pfree(buf.data);
@@ -1402,19 +1410,25 @@ pg_SSPI_recvauth(Port *port)
CloseHandle(token);
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_LOOKUP_ACCOUNT_SID);
if (!LookupAccountSid(NULL, tokenuser->User.Sid, accountname, &accountnamesize,
domainname, &domainnamesize, &accountnameuse))
ereport(ERROR,
(errmsg_internal("could not look up account SID: error code %lu",
GetLastError())));
+ pgstat_report_wait_end();
free(tokenuser);
if (!port->hba->compat_realm)
{
- int status = pg_SSPI_make_upn(accountname, sizeof(accountname),
- domainname, sizeof(domainname),
- port->hba->upn_username);
+ int status;
+
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_MAKE_UPN);
+ status = pg_SSPI_make_upn(accountname, sizeof(accountname),
+ domainname, sizeof(domainname),
+ port->hba->upn_username);
+ pgstat_report_wait_end();
if (status != STATUS_OK)
/* Error already reported from pg_SSPI_make_upn */
@@ -2119,7 +2133,9 @@ CheckPAMAuth(Port *port, const char *user, const char *password)
return STATUS_ERROR;
}
+ pgstat_report_wait_start(WAIT_EVENT_PAM_AUTHENTICATE);
retval = pam_authenticate(pamh, 0);
+ pgstat_report_wait_end();
if (retval != PAM_SUCCESS)
{
@@ -2132,7 +2148,9 @@ CheckPAMAuth(Port *port, const char *user, const char *password)
return pam_no_password ? STATUS_EOF : STATUS_ERROR;
}
+ pgstat_report_wait_start(WAIT_EVENT_PAM_ACCT_MGMT);
retval = pam_acct_mgmt(pamh, 0);
+ pgstat_report_wait_end();
if (retval != PAM_SUCCESS)
{
@@ -2483,7 +2501,11 @@ CheckLDAPAuth(Port *port)
if (passwd == NULL)
return STATUS_EOF; /* client wouldn't send password */
- if (InitializeLDAPConnection(port, &ldap) == STATUS_ERROR)
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_INITIALIZE);
+ r = InitializeLDAPConnection(port, &ldap);
+ pgstat_report_wait_end();
+
+ if (r == STATUS_ERROR)
{
/* Error message already sent */
pfree(passwd);
@@ -2530,9 +2552,12 @@ CheckLDAPAuth(Port *port)
* Bind with a pre-defined username/password (if available) for
* searching. If none is specified, this turns into an anonymous bind.
*/
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_BIND_FOR_SEARCH);
r = ldap_simple_bind_s(ldap,
port->hba->ldapbinddn ? port->hba->ldapbinddn : "",
port->hba->ldapbindpasswd ? ldap_password_hook(port->hba->ldapbindpasswd) : "");
+ pgstat_report_wait_end();
+
if (r != LDAP_SUCCESS)
{
ereport(LOG,
@@ -2555,6 +2580,8 @@ CheckLDAPAuth(Port *port)
filter = psprintf("(uid=%s)", port->user_name);
search_message = NULL;
+
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_SEARCH);
r = ldap_search_s(ldap,
port->hba->ldapbasedn,
port->hba->ldapscope,
@@ -2562,6 +2589,7 @@ CheckLDAPAuth(Port *port)
attributes,
0,
&search_message);
+ pgstat_report_wait_end();
if (r != LDAP_SUCCESS)
{
@@ -2630,7 +2658,9 @@ CheckLDAPAuth(Port *port)
port->user_name,
port->hba->ldapsuffix ? port->hba->ldapsuffix : "");
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_BIND);
r = ldap_simple_bind_s(ldap, fulluser, passwd);
+ pgstat_report_wait_end();
if (r != LDAP_SUCCESS)
{
@@ -2890,12 +2920,16 @@ CheckRADIUSAuth(Port *port)
identifiers = list_head(port->hba->radiusidentifiers);
foreach(server, port->hba->radiusservers)
{
- int ret = PerformRadiusTransaction(lfirst(server),
- lfirst(secrets),
- radiusports ? lfirst(radiusports) : NULL,
- identifiers ? lfirst(identifiers) : NULL,
- port->user_name,
- passwd);
+ int ret;
+
+ pgstat_report_wait_start(WAIT_EVENT_RADIUS_TRANSACTION);
+ ret = PerformRadiusTransaction(lfirst(server),
+ lfirst(secrets),
+ radiusports ? lfirst(radiusports) : NULL,
+ identifiers ? lfirst(identifiers) : NULL,
+ port->user_name,
+ passwd);
+ pgstat_report_wait_end();
/*------
* STATUS_OK = Login OK
diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
index d930277140..a388999b1a 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -34,6 +34,7 @@ static const char *pgstat_get_wait_client(WaitEventClient w);
static const char *pgstat_get_wait_ipc(WaitEventIPC w);
static const char *pgstat_get_wait_timeout(WaitEventTimeout w);
static const char *pgstat_get_wait_io(WaitEventIO w);
+static const char *pgstat_get_wait_auth(WaitEventAuth w);
static uint32 local_my_wait_event_info;
@@ -413,6 +414,9 @@ pgstat_get_wait_event_type(uint32 wait_event_info)
case PG_WAIT_INJECTIONPOINT:
event_type = "InjectionPoint";
break;
+ case PG_WAIT_AUTH:
+ event_type = "Auth";
+ break;
default:
event_type = "???";
break;
@@ -495,6 +499,13 @@ pgstat_get_wait_event(uint32 wait_event_info)
event_name = pgstat_get_wait_io(w);
break;
}
+ case PG_WAIT_AUTH:
+ {
+ WaitEventAuth w = (WaitEventAuth) wait_event_info;
+
+ event_name = pgstat_get_wait_auth(w);
+ break;
+ }
default:
event_name = "unknown wait event";
break;
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 16144c2b72..a341c6e7c5 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -161,6 +161,30 @@ XACT_GROUP_UPDATE "Waiting for the group leader to update transaction status at
ABI_compatibility:
+#
+# Wait Events - Auth
+#
+# Use this category when a process is waiting for a third party to
+# authenticate/authorize the user.
+#
+
+Section: ClassName - WaitEventAuth
+
+GSSAPI_ACCEPT_SEC_CONTEXT "Waiting for a response from a Kerberos server via GSSAPI."
+LDAP_BIND "Waiting for an LDAP bind operation to authenticate the user."
+LDAP_BIND_FOR_SEARCH "Waiting for an LDAP bind operation to search the directory."
+LDAP_INITIALIZE "Waiting to initialize an LDAP connection."
+LDAP_SEARCH "Waiting for an LDAP search operation to complete."
+PAM_ACCT_MGMT "Waiting for the PAM service to validate the user account."
+PAM_AUTHENTICATE "Waiting for the PAM service to authenticate the user."
+RADIUS_TRANSACTION "Waiting for a RADIUS transaction to complete."
+SSPI_ACCEPT_SECURITY_CONTEXT "Waiting for a Windows security provider to accept the client's SSPI token."
+SSPI_ACQUIRE_CREDENTIALS_HANDLE "Waiting for a Windows security provider to acquire server credentials for SSPI."
+SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's security identifier."
+SSPI_MAKE_UPN "Waiting for Windows to translate a Kerberos UPN."
+
+ABI_compatibility:
+
#
# Wait Events - Timeout
#
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index 9f18a753d4..014d536441 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -25,6 +25,7 @@
#define PG_WAIT_TIMEOUT 0x09000000U
#define PG_WAIT_IO 0x0A000000U
#define PG_WAIT_INJECTIONPOINT 0x0B000000U
+#define PG_WAIT_AUTH 0x0C000000U
/* enums for wait events */
#include "utils/wait_event_types.h"
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index fad7fc3a7e..030d1a8321 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -179,6 +179,7 @@ select type, count(*) > 0 as ok FROM pg_wait_events
type | ok
-----------+----
Activity | t
+ Auth | t
BufferPin | t
Client | t
Extension | t
@@ -187,7 +188,7 @@ select type, count(*) > 0 as ok FROM pg_wait_events
LWLock | t
Lock | t
Timeout | t
-(9 rows)
+(10 rows)
-- Test that the pg_timezone_names and pg_timezone_abbrevs views are
-- more-or-less working. We can't test their contents in any great detail
--
2.34.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-06 05:48 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 03:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 17:20 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2024-11-07 18:12 ` Andres Freund <[email protected]>
2024-11-07 18:44 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Andres Freund @ 2024-11-07 18:12 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
Hi,
On 2024-11-07 09:20:24 -0800, Jacob Champion wrote:
> From e755fdccf16cb4fcd3036e1209291750ffecd261 Mon Sep 17 00:00:00 2001
> From: Jacob Champion <[email protected]>
> Date: Fri, 3 May 2024 15:54:58 -0700
> Subject: [PATCH v5 1/2] pgstat: report in earlier with STATE_STARTING
>
> Add pgstat_bestart_pre_auth(), which reports a 'starting' state while
> waiting for backend initialization and client authentication to
> complete. Since we hold a transaction open for a good amount of that,
> and some authentication methods call out to external systems, having a
> pg_stat_activity entry helps DBAs debug when things go badly wrong.
I don't understand why the pgstat_bestart()/pgstat_bestart_pre_auth() split
makes sense. The latter is going to redo most of the work that the former
did. What's the point of that?
Why not have a new function that initializes just the missing additional
information? Or for that matter, why not move most of what pgstat_bestart()
does into pgstat_beinit()?
As-is I'm -1 on this patch, it makes something complicated and fragile even
more so.
> From 858e95f996589461e2840047fa35675b6f96e46d Mon Sep 17 00:00:00 2001
> From: Jacob Champion <[email protected]>
> Date: Fri, 3 May 2024 15:58:23 -0700
> Subject: [PATCH v5 2/2] Report external auth calls as wait events
>
> Introduce a new "Auth" wait class for various external authentication
> systems, to make it obvious what's going wrong if one of those systems
> hangs. Each new wait event is unique in order to more easily pinpoint
> problematic locations in the code.
This doesn't really seem like it's actually using wait events to describe
waits. The new wait events cover stuff like memory allocations etc, see
e.g. pg_SSPI_make_upn().
I have some sympathy for that, it'd be nice if we had some generic way to
describe what code is doing - but it doesn't really seem good to use wait
events for that. Right now a backend reporting a wait allows to conclude that
a backend isn't running postgres code and busy or blocked outside of postgres
- but that's not true anymore if you have wait event cover generic things like
memory allocations (or even various library functions).
This isn't just pedantry - all the relevant code really needs to be rewritten
to allow the blocking to happen in an interruptible way, otherwise
authentication timeout etc can't realiably work. Once that's done you can
actually define useful wait events too.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-06 05:48 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 03:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 17:20 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 18:12 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
@ 2024-11-07 18:44 ` Jacob Champion <[email protected]>
2024-11-07 19:41 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Jacob Champion @ 2024-11-07 18:44 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Thu, Nov 7, 2024 at 10:12 AM Andres Freund <[email protected]> wrote:
> I don't understand why the pgstat_bestart()/pgstat_bestart_pre_auth() split
> makes sense. The latter is going to redo most of the work that the former
> did. What's the point of that?
>
> Why not have a new function that initializes just the missing additional
> information? Or for that matter, why not move most of what pgstat_bestart()
> does into pgstat_beinit()?
I talk about that up above [1]. I agree that this is all complicated
and fragile, but at the moment, I think splitting things apart is not
going to reduce the complexity in any way. I'm all ears for a
different approach, though (and it sounds like Michael is taking a
stab at it too).
> This doesn't really seem like it's actually using wait events to describe
> waits. The new wait events cover stuff like memory allocations etc, see
> e.g. pg_SSPI_make_upn().
I've also asked about the "scope" of the waits in the OP [2]. I can
move them downwards in the stack, if you'd prefer.
All of these are intended to cover parts of the code that can actually
hang, but for things like SSPI I'm just working off of inspection and
Win32 documentation. So if it's not actually true that some of these
call points can hang, let me know and I can remove them. (For the
particular example you called out, I'm just trying to cover both calls
to TranslateName() in a maintainable place. The documentation says
"TranslateName fails if it cannot bind to Active Directory on a domain
controller." which seemed pretty wait-worthy to me.)
> This isn't just pedantry - all the relevant code really needs to be rewritten
> to allow the blocking to happen in an interruptible way, otherwise
> authentication timeout etc can't realiably work. Once that's done you can
> actually define useful wait events too.
I agree that would be amazing! I'm not about to tackle reliable
interrupts for all of the current blocking auth code for v18, though.
I'm just trying to make it observable when we do something that
blocks.
--Jacob
[1] https://www.postgresql.org/message-id/CAOYmi%2BkLzSWrDHZbJg8bWZ94oP_K98mkoEvetgupOBVoy5H_ag%40mail.g...
[2] https://www.postgresql.org/message-id/CAOYmi%2B%3D60deN20WDyCoHCiecgivJxr%3D98s7s7-C8SkXwrCfHXg%40ma...
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-06 05:48 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 03:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 17:20 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 18:12 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 18:44 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2024-11-07 19:41 ` Andres Freund <[email protected]>
2024-11-07 20:11 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Andres Freund @ 2024-11-07 19:41 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
Hi,
On 2024-11-07 10:44:25 -0800, Jacob Champion wrote:
> On Thu, Nov 7, 2024 at 10:12 AM Andres Freund <[email protected]> wrote:
> > I don't understand why the pgstat_bestart()/pgstat_bestart_pre_auth() split
> > makes sense. The latter is going to redo most of the work that the former
> > did. What's the point of that?
> >
> > Why not have a new function that initializes just the missing additional
> > information? Or for that matter, why not move most of what pgstat_bestart()
> > does into pgstat_beinit()?
>
> I talk about that up above [1]. I agree that this is all complicated
> and fragile, but at the moment, I think splitting things apart is not
> going to reduce the complexity in any way. I'm all ears for a
> different approach, though (and it sounds like Michael is taking a
> stab at it too).
I think the patch should not be merged as-is. It's just too ugly and fragile.
> > This doesn't really seem like it's actually using wait events to describe
> > waits. The new wait events cover stuff like memory allocations etc, see
> > e.g. pg_SSPI_make_upn().
>
> I've also asked about the "scope" of the waits in the OP [2]. I can
> move them downwards in the stack, if you'd prefer.
Well, right now they're just not actually wait events, so yes, they'd need to
be moved down.
I think it might make more sense to use pgstat_report_activity() or such here,
rather than using wait events to describe something that's not a wait.
> > This isn't just pedantry - all the relevant code really needs to be rewritten
> > to allow the blocking to happen in an interruptible way, otherwise
> > authentication timeout etc can't realiably work. Once that's done you can
> > actually define useful wait events too.
>
> I agree that would be amazing! I'm not about to tackle reliable
> interrupts for all of the current blocking auth code for v18, though.
> I'm just trying to make it observable when we do something that
> blocks.
Well, with that justification we could end up adding wait events for large
swaths of code that might not actually ever wait.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-06 05:48 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 03:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 17:20 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 18:12 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 18:44 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 19:41 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
@ 2024-11-07 20:11 ` Jacob Champion <[email protected]>
2024-11-07 21:36 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Jacob Champion @ 2024-11-07 20:11 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Thu, Nov 7, 2024 at 11:41 AM Andres Freund <[email protected]> wrote:
> I think the patch should not be merged as-is. It's just too ugly and fragile.
Understood; I'm trying to find a way forward, and I'm pointing out
that the alternatives I've tried seem to me to be _more_ fragile.
Are there any items in this list that you disagree with/are less
concerned about?
- the pre-auth step must always initialize the entire pgstat struct
- two-step initialization requires two PGSTAT_BEGIN_WRITE_ACTIVITY()
calls for _every_ backend
- two-step initialization requires us to couple against the order that
authentication information is being filled in (pre-auth, post-auth, or
both)
> I think it might make more sense to use pgstat_report_activity() or such here,
> rather than using wait events to describe something that's not a wait.
I'm not sure why you're saying these aren't waits. If pam_authenticate
is capable of hanging for hours and bringing down a production system,
is that not a "wait"?
> > I agree that would be amazing! I'm not about to tackle reliable
> > interrupts for all of the current blocking auth code for v18, though.
> > I'm just trying to make it observable when we do something that
> > blocks.
>
> Well, with that justification we could end up adding wait events for large
> swaths of code that might not actually ever wait.
If it were hypothetically useful to do so, would that be a problem?
I'm trying not to propose things that aren't actually useful.
--Jacob
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-06 05:48 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 03:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 17:20 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 18:12 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 18:44 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 19:41 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 20:11 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2024-11-07 21:36 ` Andres Freund <[email protected]>
2024-11-07 22:29 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Andres Freund @ 2024-11-07 21:36 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
Hi,
On 2024-11-07 12:11:46 -0800, Jacob Champion wrote:
> On Thu, Nov 7, 2024 at 11:41 AM Andres Freund <[email protected]> wrote:
> > I think the patch should not be merged as-is. It's just too ugly and fragile.
>
> Understood; I'm trying to find a way forward, and I'm pointing out
> that the alternatives I've tried seem to me to be _more_ fragile.
>
> Are there any items in this list that you disagree with/are less
> concerned about?
>
> - the pre-auth step must always initialize the entire pgstat struct
Correct. And that has to happen exactly once, not twice.
> - two-step initialization requires two PGSTAT_BEGIN_WRITE_ACTIVITY()
> calls for _every_ backend
That's fine - PGSTAT_BEGIN_WRITE_ACTIVITY() is *extremely* cheap on the write
side. That's the whole point of of the sequence-lock like mechanism.
> - two-step initialization requires us to couple against the order that
> authentication information is being filled in (pre-auth, post-auth, or
> both)
Not sure what you mean with this?
> > I think it might make more sense to use pgstat_report_activity() or such here,
> > rather than using wait events to describe something that's not a wait.
>
> I'm not sure why you're saying these aren't waits. If pam_authenticate
> is capable of hanging for hours and bringing down a production system,
> is that not a "wait"?
It may or may not be. If you increase the iteration count for whatever secret
"hashing" method to be very high, it's not a wait, it's just CPU
use. Similarly, if you have a cpu expensive WHERE clause, that's not a
wait. But if you wait for network IO due to pam using ldap underneath or you
need to read toast values from disk, those are waits.
> > > I agree that would be amazing! I'm not about to tackle reliable
> > > interrupts for all of the current blocking auth code for v18, though.
> > > I'm just trying to make it observable when we do something that
> > > blocks.
> >
> > Well, with that justification we could end up adding wait events for large
> > swaths of code that might not actually ever wait.
>
> If it were hypothetically useful to do so, would that be a problem?
> I'm trying not to propose things that aren't actually useful.
My point is that you're redefining wait events to be "in some region of code"
and that once you start doing that, there's a lot of other places you could
suddenly use wait events.
But wait events aren't actually suitable for that - they're a *single-depth*
mechanism, which means that if you start waiting, the prior wait event is
lost, and
a) the nested region isn't attributed to the parent while active
b) once the nested wait event is over, the parent isn't reset
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-06 05:48 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 03:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 17:20 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 18:12 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 18:44 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 19:41 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 20:11 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 21:36 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
@ 2024-11-07 22:29 ` Jacob Champion <[email protected]>
2024-11-07 22:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
0 siblings, 1 reply; 19+ messages in thread
From: Jacob Champion @ 2024-11-07 22:29 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
On Thu, Nov 7, 2024 at 1:37 PM Andres Freund <[email protected]> wrote:
> > - the pre-auth step must always initialize the entire pgstat struct
>
> Correct. And that has to happen exactly once, not twice.
What goes wrong if it happens twice?
> > - two-step initialization requires two PGSTAT_BEGIN_WRITE_ACTIVITY()
> > calls for _every_ backend
>
> That's fine - PGSTAT_BEGIN_WRITE_ACTIVITY() is *extremely* cheap on the write
> side. That's the whole point of of the sequence-lock like mechanism.
Okay, cool. I'll retract that concern.
> > - two-step initialization requires us to couple against the order that
> > authentication information is being filled in (pre-auth, post-auth, or
> > both)
>
> Not sure what you mean with this?
In other words: if we split it, people who make changes to the order
that authentication information is determined during startup must know
to keep an eye on this code as well. Whereas with the current
patchset, the layers are decoupled and the order doesn't matter.
Quoting from above:
Finally, if we're okay with all of that, future maintainers need to be
careful about which fields get copied in the first (preauth) step, the
second step, or both. GSS, for example, can be set up during transport
negotiation (first step) or authentication (second step), so we have
to duplicate the logic there. SSL is currently first-step-only, I
think -- but are we sure we want to hardcode the assumption that cert
auth can't change any of those parameters after the transport has been
established? (I've been brainstorming ways we might use TLS 1.3's
post-handshake CertificateRequest, for example.)
> If you increase the iteration count for whatever secret
> "hashing" method to be very high, it's not a wait, it's just CPU
> use.
I don't yet understand why this is a useful distinction to make. I
understand that they are different, but what are the bad consequences
if pg_stat_activity records a CPU busy wait in the same way it records
an I/O wait -- as long as they're not nested?
> My point is that you're redefining wait events to be "in some region of code"
> and that once you start doing that, there's a lot of other places you could
> suddenly use wait events.
>
> But wait events aren't actually suitable for that - they're a *single-depth*
> mechanism, which means that if you start waiting, the prior wait event is
> lost, and
> a) the nested region isn't attributed to the parent while active
> b) once the nested wait event is over, the parent isn't reset
I understand that they shouldn't be nested. But as long as they're
not, isn't that fine? And if the concern is that they'll accidentally
get nested, whether in this patch or in the future, can't we just
programmatically assert that we haven't?
Thanks,
--Jacob
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-06 05:48 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 03:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 17:20 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 18:12 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 18:44 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 19:41 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 20:11 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 21:36 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 22:29 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2024-11-07 22:56 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Andres Freund @ 2024-11-07 22:56 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>
Hi,
On 2024-11-07 14:29:18 -0800, Jacob Champion wrote:
> On Thu, Nov 7, 2024 at 1:37 PM Andres Freund <[email protected]> wrote:
> > > - the pre-auth step must always initialize the entire pgstat struct
> >
> > Correct. And that has to happen exactly once, not twice.
>
> What goes wrong if it happens twice?
Primarily it's architecturally wrong. For no reason that I can see.
It does actually make things harder - what if somebody added a
pgstat_report_activity() somewhere between the call? It would suddenly get
lost after the second "initialization". Actually, the proposed patch already
has weird, externally visible, consequences - the application name is set,
then suddenly becomes unset, then is set again.
> > > - two-step initialization requires two PGSTAT_BEGIN_WRITE_ACTIVITY()
> > > calls for _every_ backend
> >
> > That's fine - PGSTAT_BEGIN_WRITE_ACTIVITY() is *extremely* cheap on the write
> > side. That's the whole point of of the sequence-lock like mechanism.
>
> Okay, cool. I'll retract that concern.
Additionally PGSTAT_BEGIN_WRITE_ACTIVITY() would already happen twice if you
initialize twice...
> > > - two-step initialization requires us to couple against the order that
> > > authentication information is being filled in (pre-auth, post-auth, or
> > > both)
> >
> > Not sure what you mean with this?
>
> In other words: if we split it, people who make changes to the order
> that authentication information is determined during startup must know
> to keep an eye on this code as well. Whereas with the current
> patchset, the layers are decoupled and the order doesn't matter.
> Quoting from above:
>
> Finally, if we're okay with all of that, future maintainers need to be
> careful about which fields get copied in the first (preauth) step, the
> second step, or both. GSS, for example, can be set up during transport
> negotiation (first step) or authentication (second step), so we have
> to duplicate the logic there. SSL is currently first-step-only, I
> think -- but are we sure we want to hardcode the assumption that cert
> auth can't change any of those parameters after the transport has been
> established? (I've been brainstorming ways we might use TLS 1.3's
> post-handshake CertificateRequest, for example.)
That doesn't seem like a reason to just initialize twice to me. If you have
one initialization step that properly initializes everything to a minimal
default state, you then can have granular functions that set up the user,
database, SSL, GSS information separately.
> > If you increase the iteration count for whatever secret
> > "hashing" method to be very high, it's not a wait, it's just CPU
> > use.
>
> I don't yet understand why this is a useful distinction to make. I
> understand that they are different, but what are the bad consequences
> if pg_stat_activity records a CPU busy wait in the same way it records
> an I/O wait -- as long as they're not nested?
Well, first, because you suddenly can't use wait events anymore to find waits.
But more importantly, because of not having nesting, adding one "coarse" "wait
event" means that anyone adding a wait event at a finer grade suddenly needs
to be aware of all the paths that could lead to the execution of the new
code and change all of them to not use the wait event anymore. It imposes a
tax on measuring actual "out of postgres" wait events.
> > My point is that you're redefining wait events to be "in some region of code"
> > and that once you start doing that, there's a lot of other places you could
> > suddenly use wait events.
> >
> > But wait events aren't actually suitable for that - they're a *single-depth*
> > mechanism, which means that if you start waiting, the prior wait event is
> > lost, and
> > a) the nested region isn't attributed to the parent while active
> > b) once the nested wait event is over, the parent isn't reset
>
> I understand that they shouldn't be nested. But as long as they're
> not, isn't that fine? And if the concern is that they'll accidentally
> get nested, whether in this patch or in the future, can't we just
> programmatically assert that we haven't?
One very useful wait event would be for memory allocations that hit the
kernel. Those can take a fairly long time, because they might need to write
dirty buffers to disk before there is enough free memory. Now imagine that we
redefine the system memory allocator (or just postgres') so that all memory
allocations from the kernel use a wait event. Now suddenly all that code that
uses "coarse" wait events suddenly has a *rare* path - because most of the time
we can carve memory out of a larger OS level memory allocation - where wait
events would be nested.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 19+ messages in thread
* [PATCH v26 2/9] Row pattern recognition patch (parse/analysis).
@ 2024-12-30 12:44 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 19+ messages in thread
From: Tatsuo Ishii @ 2024-12-30 12:44 UTC (permalink / raw)
---
src/backend/parser/parse_agg.c | 7 +
src/backend/parser/parse_clause.c | 297 +++++++++++++++++++++++++++++-
src/backend/parser/parse_expr.c | 6 +
src/backend/parser/parse_func.c | 3 +
4 files changed, 312 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 04b4596a65..6af3bcb375 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -580,6 +580,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
+
/*
* There is intentionally no default: case here, so that the
* compiler will warn if we add a new ParseExprKind without
@@ -970,6 +974,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 979926b605..5a44c68b08 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -96,7 +96,14 @@ static WindowClause *findWindowClause(List *wclist, const char *name);
static Node *transformFrameOffset(ParseState *pstate, int frameOptions,
Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc,
Node *clause);
-
+static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist);
+static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist);
+static void transformPatternClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef);
+static List *transformMeasureClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef);
/*
* transformFromClause -
@@ -2954,6 +2961,10 @@ transformWindowDefinitions(ParseState *pstate,
rangeopfamily, rangeopcintype,
&wc->endInRangeFunc,
windef->endOffset);
+
+ /* Process Row Pattern Recognition related clauses */
+ transformRPR(pstate, wc, windef, targetlist);
+
wc->winref = winref;
result = lappend(result, wc);
@@ -3821,3 +3832,287 @@ transformFrameOffset(ParseState *pstate, int frameOptions,
return node;
}
+
+/*
+ * transformRPR
+ * Process Row Pattern Recognition related clauses
+ */
+static void
+transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist)
+{
+ /*
+ * Window definition exists?
+ */
+ if (windef == NULL)
+ return;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return;
+
+ /* Check Frame option. Frame must start at current row */
+ if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("FRAME must start at current row when row patttern recognition is used")));
+
+ /* Transform AFTER MACH SKIP TO clause */
+ wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo;
+
+ /* Transform AFTER MACH SKIP TO variable */
+ wc->rpSkipVariable = windef->rpCommonSyntax->rpSkipVariable;
+
+ /* Transform SEEK or INITIAL clause */
+ wc->initial = windef->rpCommonSyntax->initial;
+
+ /* Transform DEFINE clause into list of TargetEntry's */
+ wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist);
+
+ /* Check PATTERN clause and copy to patternClause */
+ transformPatternClause(pstate, wc, windef);
+
+ /* Transform MEASURE clause */
+ transformMeasureClause(pstate, wc, windef);
+}
+
+/*
+ * transformDefineClause
+ * Process DEFINE clause and transform ResTarget into list of
+ * TargetEntry.
+ *
+ * XXX we only support column reference in row pattern definition search
+ * condition, e.g. "price". <row pattern definition variable name>.<column
+ * reference> is not supported, e.g. "A.price".
+ */
+static List *
+transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef,
+ List **targetlist)
+{
+ /* DEFINE variable name initials */
+ static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz";
+
+ ListCell *lc,
+ *l;
+ ResTarget *restarget,
+ *r;
+ List *restargets;
+ List *defineClause;
+ char *name;
+ int initialLen;
+ int i;
+
+ /*
+ * If Row Definition Common Syntax exists, DEFINE clause must exist. (the
+ * raw parser should have already checked it.)
+ */
+ Assert(windef->rpCommonSyntax->rpDefs != NULL);
+
+ /*
+ * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE
+ * per the SQL standard.
+ */
+ restargets = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpPatterns)
+ {
+ A_Expr *a;
+ bool found = false;
+
+ if (!IsA(lfirst(lc), A_Expr))
+ ereport(ERROR,
+ errmsg("node type is not A_Expr"));
+
+ a = (A_Expr *) lfirst(lc);
+ name = strVal(a->lexpr);
+
+ foreach(l, windef->rpCommonSyntax->rpDefs)
+ {
+ restarget = (ResTarget *) lfirst(l);
+
+ if (!strcmp(restarget->name, name))
+ {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ {
+ /*
+ * "name" is missing. So create "name AS name IS TRUE" ResTarget
+ * node and add it to the temporary list.
+ */
+ A_Const *n;
+
+ restarget = makeNode(ResTarget);
+ n = makeNode(A_Const);
+ n->val.boolval.type = T_Boolean;
+ n->val.boolval.boolval = true;
+ n->location = -1;
+ restarget->name = pstrdup(name);
+ restarget->indirection = NIL;
+ restarget->val = (Node *) n;
+ restarget->location = -1;
+ restargets = lappend((List *) restargets, restarget);
+ }
+ }
+
+ if (list_length(restargets) >= 1)
+ {
+ /* add missing DEFINEs */
+ windef->rpCommonSyntax->rpDefs =
+ list_concat(windef->rpCommonSyntax->rpDefs, restargets);
+ list_free(restargets);
+ }
+
+ /*
+ * Check for duplicate row pattern definition variables. The standard
+ * requires that no two row pattern definition variable names shall be
+ * equivalent.
+ */
+ restargets = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpDefs)
+ {
+ restarget = (ResTarget *) lfirst(lc);
+ name = restarget->name;
+
+ /*
+ * Add DEFINE expression (Restarget->val) to the targetlist as a
+ * TargetEntry if it does not exist yet. Planner will add the column
+ * ref var node to the outer plan's target list later on. This makes
+ * DEFINE expression could access the outer tuple while evaluating
+ * PATTERN.
+ *
+ * XXX: adding whole expressions of DEFINE to the plan.targetlist is
+ * not so good, because it's not necessary to evalute the expression
+ * in the target list while running the plan. We should extract the
+ * var nodes only then add them to the plan.targetlist.
+ */
+ findTargetlistEntrySQL99(pstate, (Node *) restarget->val,
+ targetlist, EXPR_KIND_RPR_DEFINE);
+
+ /*
+ * Make sure that the row pattern definition search condition is a
+ * boolean expression.
+ */
+ transformWhereClause(pstate, restarget->val,
+ EXPR_KIND_RPR_DEFINE, "DEFINE");
+
+ foreach(l, restargets)
+ {
+ char *n;
+
+ r = (ResTarget *) lfirst(l);
+ n = r->name;
+
+ if (!strcmp(n, name))
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause",
+ name),
+ parser_errposition(pstate, exprLocation((Node *) r))));
+ }
+ restargets = lappend(restargets, restarget);
+ }
+ list_free(restargets);
+
+ /*
+ * Create list of row pattern DEFINE variable name's initial. We assign
+ * [a-z] to them (up to 26 variable names are allowed).
+ */
+ restargets = NIL;
+ i = 0;
+ initialLen = strlen(defineVariableInitials);
+
+ foreach(lc, windef->rpCommonSyntax->rpDefs)
+ {
+ char initial[2];
+
+ restarget = (ResTarget *) lfirst(lc);
+ name = restarget->name;
+
+ if (i >= initialLen)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("number of row pattern definition variable names exceeds %d",
+ initialLen),
+ parser_errposition(pstate,
+ exprLocation((Node *) restarget))));
+ }
+ initial[0] = defineVariableInitials[i++];
+ initial[1] = '\0';
+ wc->defineInitial = lappend(wc->defineInitial,
+ makeString(pstrdup(initial)));
+ }
+
+ defineClause = transformTargetList(pstate, windef->rpCommonSyntax->rpDefs,
+ EXPR_KIND_RPR_DEFINE);
+
+ /* mark column origins */
+ markTargetListOrigins(pstate, defineClause);
+
+ /* mark all nodes in the DEFINE clause tree with collation information */
+ assign_expr_collations(pstate, (Node *) defineClause);
+
+ return defineClause;
+}
+
+/*
+ * transformPatternClause
+ * Process PATTERN clause and return PATTERN clause in the raw parse tree
+ */
+static void
+transformPatternClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef)
+{
+ ListCell *lc;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return;
+
+ wc->patternVariable = NIL;
+ wc->patternRegexp = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpPatterns)
+ {
+ A_Expr *a;
+ char *name;
+ char *regexp;
+
+ if (!IsA(lfirst(lc), A_Expr))
+ ereport(ERROR,
+ errmsg("node type is not A_Expr"));
+
+ a = (A_Expr *) lfirst(lc);
+ name = strVal(a->lexpr);
+
+ wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name)));
+ regexp = strVal(lfirst(list_head(a->name)));
+
+ wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp)));
+ }
+}
+
+/*
+ * transformMeasureClause
+ * Process MEASURE clause
+ * XXX MEASURE clause is not supported yet
+ */
+static List *
+transformMeasureClause(ParseState *pstate, WindowClause *wc,
+ WindowDef *windef)
+{
+ if (windef->rowPatternMeasures == NIL)
+ return NIL;
+
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s", "MEASURE clause is not supported yet"),
+ parser_errposition(pstate, exprLocation((Node *) windef->rowPatternMeasures))));
+ return NIL;
+}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index c2806297aa..e827c59fd4 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -575,6 +575,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
case EXPR_KIND_COPY_WHERE:
case EXPR_KIND_GENERATED_COLUMN:
case EXPR_KIND_CYCLE_MARK:
+ case EXPR_KIND_RPR_DEFINE:
/* okay */
break;
@@ -1858,6 +1859,9 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_GENERATED_COLUMN:
err = _("cannot use subquery in column generation expression");
break;
+ case EXPR_KIND_RPR_DEFINE:
+ err = _("cannot use subquery in DEFINE expression");
+ break;
/*
* There is intentionally no default: case here, so that the
@@ -3197,6 +3201,8 @@ ParseExprKindName(ParseExprKind exprKind)
return "GENERATED AS";
case EXPR_KIND_CYCLE_MARK:
return "CYCLE";
+ case EXPR_KIND_RPR_DEFINE:
+ return "DEFINE";
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 9b23344a3b..4c482abb30 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2658,6 +2658,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
--
2.25.1
----Next_Part(Mon_Dec_30_22_37_18_2024_171)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v26-0003-Row-pattern-recognition-patch-rewriter.patch"
^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2024-12-30 12:44 UTC | newest]
Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-03-25 04:29 [PATCH 3/8] Move XLOG stuff from heap_insert and heap_delete Kyotaro Horiguchi <[email protected]>
2024-09-10 17:27 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 18:51 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-10 20:58 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-09-10 22:33 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-09-11 13:00 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Robert Haas <[email protected]>
2024-09-13 14:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Noah Misch <[email protected]>
2024-11-01 21:47 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-06 05:48 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 03:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2024-11-07 17:20 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 18:12 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 18:44 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 19:41 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 20:11 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 21:36 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-11-07 22:29 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2024-11-07 22:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2024-12-30 12:44 [PATCH v26 2/9] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[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