On Wed, May 21, 2025 at 5:20 PM Jacob Champion
wrote:
>
> I took a quick look at the authentication and oauth_validator changes.
> Maybe 007_pre_auth.pl should include `receipt`, to make it easier to
> debug pre-auth hangs using the logs? Other than that, the changes
> looked reasonable to me.
Co
On 20.05.25 17:16, Melanie Plageman wrote:
In src/backend/tcop/postgres.c, there is a call
SetConfigOption("log_connections", "true", context, source);
that could be adjusted.
Do you think the debug option should be 'all' or a list of the options
covered by "true" (whic
On 21.05.25 21:53, Melanie Plageman wrote:
On Tue, May 20, 2025 at 11:16 AM Melanie Plageman
wrote:
In earlier versions of my patch, I played around with replacing these references in the docs. I ended up not doing it because I wasn't sure we had
consensus on deprecating the "on", "true", "ye
On Wed, May 21, 2025 at 12:54 PM Melanie Plageman
wrote:
> Attached is a patch that updates these as well as changes all usages
> of log_connections in the tests. I made some judgment calls about
> where we might want expanded or reduced log_connections aspects. As
> such, the patch could use a on
On Tue, May 20, 2025 at 11:16 AM Melanie Plageman
wrote:
>
> In earlier versions of my patch, I played around with replacing these
> references in the docs. I ended up not doing it because I wasn't sure we had
> consensus on deprecating the "on", "true", "yes" options and that we would
> contin
On Tue, May 20, 2025 at 4:52 AM Peter Eisentraut
wrote:
> log_connections has been changed from a Boolean parameter to a string
> one, but a number of places in the documentation and in various pieces
> of test code still use the old values. I think it would be better if
> they were adjusted to
> On 20 May 2025, at 10:52, Peter Eisentraut wrote:
> src/test/ssl/t/SSL/Server.pm
While I don't have an immediate usecase or test in mind, having the SSL tests
use log_conections=all could be handy when hacking on the TLS support.
--
Daniel Gustafsson
On 12.03.25 16:43, Melanie Plageman wrote:
On Tue, Mar 11, 2025 at 6:27 PM Melanie Plageman
wrote:
I did more manual testing of my patches, and I think they are mostly
ready for commit except for the IsConnectionBackend() macro (if we
have something to change it to).
I've committed this and
On Tue, Mar 11, 2025 at 6:27 PM Melanie Plageman
wrote:
>
> I did more manual testing of my patches, and I think they are mostly
> ready for commit except for the IsConnectionBackend() macro (if we
> have something to change it to).
I've committed this and marked it as such in the CF app.
Thanks
On Mon, Mar 10, 2025 at 5:03 PM Daniel Gustafsson wrote:
>
> > On 7 Mar 2025, at 23:08, Melanie Plageman wrote:
>
> Sorry for being late to the party. I like this functionality and would
> definitely like to see it in v18.
Thanks so much for the review!
> + /* For backwards compatibility, we
Hi,
On Fri, Mar 07, 2025 at 03:29:14PM -0500, Melanie Plageman wrote:
> On Fri, Mar 7, 2025 at 6:16 AM Bertrand Drouvot
> wrote:
> > But at the end, what we're "really" interested in this thread, given its
> > $SUBJECT,
> > is to actually log the timings.
>
> I'm not sure that people would enab
> On 7 Mar 2025, at 23:08, Melanie Plageman wrote:
Sorry for being late to the party. I like this functionality and would
definitely like to see it in v18.
> An unrelated note about the attached v13:
> I changed the language from log_connections "stages" to "options" and
> "aspects" depending o
On Thu, Mar 6, 2025 at 3:16 PM Melanie Plageman
wrote:
>
> Jacob at some point had asked about this, and I didn't
> have a satisfactory answer. I'm not really sure what would be more
> useful to end users.
For the record, I'm not really sure either. I don't have a strong
opinion either way.
--Ja
On Fri, Mar 7, 2025 at 10:09 AM Bertrand Drouvot
wrote:
>
> Given that conn_timing.ready_for_use is only used here:
>
> + if (!reported_first_ready_for_query &&
> + (log_connections &
> LOG_CONNECTION_READY_FOR_USE) &&
> +
On Fri, Mar 7, 2025 at 2:17 PM Melanie Plageman
wrote:
> Yea, "auth_id" is too ambiguous for a GUC option. I went with
> "authorization" because it sounds a bit less like a stage while also
> being a recognizable word. It achieves symmetry -- though it doesn't
> match the prefix used in the logs.
On Fri, Mar 7, 2025 at 4:53 PM Jacob Champion
wrote:
>
> If I add a hypothetical auth method in the future that authenticates,
> and then farms the authorization decision out to some slow-moving
> network machinery, would "authenticated" retroactively become a stage
> then? (OAuth almost does this
Thanks for the review!
On Fri, Mar 7, 2025 at 2:08 AM Fujii Masao wrote:
>
> With the patch, any unambiguous prefix of a valid boolean value,
> like 'y', is no longer accepted even though it's currently valid
> for boolean GUCs. I don’t have a strong opinion on whether
> we should maintain compat
On Fri, Mar 7, 2025 at 12:29 PM Melanie Plageman
wrote:
> This got me thinking more about the existing log connections messages
> and whether or not they are actually all "stages". The authenticated
> and authorized messages occur at nearly the same time in the
> connection establishment timeline.
On Fri, Mar 7, 2025 at 6:16 AM Bertrand Drouvot
wrote:
>
> On Thu, Mar 06, 2025 at 11:41:03AM -0500, Melanie Plageman wrote:
>
> > Well, I actually didn't want to call it "timings" because it implies
> > that we only measure the timings when it is specified. But we actually
> > get the timings unc
Hi,
On Thu, Mar 06, 2025 at 06:16:07PM -0500, Melanie Plageman wrote:
> Attached v12 does this (uses timestamps instead of instr_time).
Thanks for the new version!
> I've done some assorted cleanup. Most notably:
>
> I removed LOG_CONNECTION_NONE because it could lead to wrong results
> to have
Hi,
On Thu, Mar 06, 2025 at 11:41:03AM -0500, Melanie Plageman wrote:
>
> I still need to do some more manual testing and validation.
>
> On Thu, Mar 6, 2025 at 9:56 AM Bertrand Drouvot
> wrote:
>
> > + /* If an empty string was passed, we're done. */
> > + if (list_length(elemlist
Hi,
On Thu, Mar 06, 2025 at 02:10:47PM -0500, Andres Freund wrote:
> On 2025-01-20 15:01:38 +, Bertrand Drouvot wrote:
> > /* If start_time is in the future or now, no time has elapsed */
> > if (start_time >= stop_time)
> > return 0;
> > "
> >
> > I think that it can happen d
On 2025/03/07 8:16, Melanie Plageman wrote:
On Thu, Mar 6, 2025 at 2:10 PM Andres Freund wrote:
I think it'd be better to use absolute times and store them as such in
ConnectionTimes or whatever. That way we have information about when a
connection was established for some future SQL functi
On Thu, Mar 6, 2025 at 2:10 PM Andres Freund wrote:
>
> I think it'd be better to use absolute times and store them as such in
> ConnectionTimes or whatever. That way we have information about when a
> connection was established for some future SQL functions and for debugging
> problems.
Attached
Hi,
On 2025-01-20 15:01:38 +, Bertrand Drouvot wrote:
> Regarding the TimestampTz vs instr_time choice, we have things like:
>
> + TimestampTz fork_time = ((BackendStartupData *) startup_data)->fork_time;
> + TimestampTz cur_time = GetCurrentTimestamp();
> +
> + conn_timing.fork_duration = Ti
Thanks for the continued review!
Attached v11 has a test added (passes locally but fails in CI, so I
have to fix that).
I still need to do some more manual testing and validation.
On Thu, Mar 6, 2025 at 9:56 AM Bertrand Drouvot
wrote:
>
<-- snip -->
> what do you think about also doing?
>
>
Hi,
On Wed, Mar 05, 2025 at 06:40:10PM -0500, Melanie Plageman wrote:
> On Wed, Mar 5, 2025 at 10:46 AM Melanie Plageman
> wrote:
> >
> > As such, I wonder if my PGC_SET idea is not worth the effort and I
> > should revise the earlier patch version which specified the stages but
> > allow for on,
On Wed, Mar 5, 2025 at 10:46 AM Melanie Plageman
wrote:
>
> As such, I wonder if my PGC_SET idea is not worth the effort and I
> should revise the earlier patch version which specified the stages but
> allow for on, off, true, yes, 1 for backwards compatibility and not
> include disconnections (so
On Wed, Mar 5, 2025 at 5:36 AM Bertrand Drouvot
wrote:
>
> Looking at the enum array:
>
> "
> static const struct config_enum_entry log_connections_options[] = {
> {"off", 0, false},
> {"on", LOG_CONNECTION_BASIC, false},
> {"true", LOG_CONNECTION_BASIC, true},
> {"false", 0, true}
Hi,
On Tue, Mar 04, 2025 at 06:25:42PM -0500, Melanie Plageman wrote:
> Attached v9 implements log_connections as an enum with a new third
> value "timings".
>
> On Mon, Mar 3, 2025 at 11:14 AM Bertrand Drouvot
> wrote:
> >
> >
> > On Fri, Feb 28, 2025 at 05:52:35PM -0500, Melanie Plageman wrote
Hi,
On Tue, Mar 04, 2025 at 05:47:26PM -0500, Melanie Plageman wrote:
> On Mon, Mar 3, 2025 at 9:07 AM Bertrand Drouvot
> wrote:
> >
> > I did not imagine that much ;-) I was just seeing this code being duplicated
> > and just thought about to avoid the duplication. But now that I read your
> >
On Tue, Mar 4, 2025 at 3:25 PM Melanie Plageman
wrote:
>
> I don't
> see any other TAP tests that are testing connections. There are a
> whole bunch of auth tests, I suppose.
I was going to suggest src/test/authentication, yeah. Not perfect, but
better than nothing?
--Jacob
On Mon, Mar 3, 2025 at 11:45 PM Fujii Masao wrote:
>
> When log_connection is empty string in postgresql.conf and I ran
> "psql -d "options=-clog_connections=all"", I got the following log message.
> You can see the total duration in this message is unexpected.
> It looks like this happens because
Thanks so much for your review!
On Mon, Mar 3, 2025 at 12:08 PM Fujii Masao wrote:
>
>
> > +bool
> > +check_log_connections(char **newval, void **extra, GucSource source)
> > +{
> >
> > This function is pretty close to check_debug_io_direct() for example and its
> > overall content, memory manage
Attached v9 implements log_connections as an enum with a new third
value "timings".
On Mon, Mar 3, 2025 at 11:14 AM Bertrand Drouvot
wrote:
>
>
> On Fri, Feb 28, 2025 at 05:52:35PM -0500, Melanie Plageman wrote:
>
> > We have to be really careful about when log_connections is actually
> > set bef
On Mon, Mar 3, 2025 at 9:07 AM Bertrand Drouvot
wrote:
>
> I did not imagine that much ;-) I was just seeing this code being duplicated
> and just thought about to avoid the duplication. But now that I read your
> comments
> above then I think we could just macro-ize the child_type check (as you
On Tue, Mar 4, 2025 at 4:40 AM Bertrand Drouvot
wrote:
>
> On Mon, Mar 03, 2025 at 06:24:59PM -0500, Melanie Plageman wrote:
> >
> > And I think it would be even simpler to do your idea of groups and to
> > have aliases for different combinations of options. And it would let
> > us keep 'on' and '
Hi,
On Mon, Mar 03, 2025 at 06:24:59PM -0500, Melanie Plageman wrote:
> On Mon, Mar 3, 2025 at 11:14 AM Bertrand Drouvot
> wrote:
> >
> >
> > On Fri, Feb 28, 2025 at 05:52:35PM -0500, Melanie Plageman wrote:
> >
> > +bool
> > +check_log_connections(char **newval, void **extra, GucSource source)
>
On 2025/03/01 7:52, Melanie Plageman wrote:
On Fri, Feb 28, 2025 at 12:54 AM Bertrand Drouvot
wrote:
On Thu, Feb 27, 2025 at 05:55:19PM -0500, Melanie Plageman wrote:
It still needs polishing (e.g. I need to figure out where to put the new guc
hook
functions and enums and such)
yeah, I
On Mon, Mar 3, 2025 at 11:14 AM Bertrand Drouvot
wrote:
>
>
> On Fri, Feb 28, 2025 at 05:52:35PM -0500, Melanie Plageman wrote:
>
> +bool
> +check_log_connections(char **newval, void **extra, GucSource source)
> +{
>
> This function is pretty close to check_debug_io_direct() for example and its
>
Hi,
On Fri, Feb 28, 2025 at 05:52:35PM -0500, Melanie Plageman wrote:
> On Fri, Feb 28, 2025 at 12:54 AM Bertrand Drouvot
> wrote:
> >
> > On Thu, Feb 27, 2025 at 05:55:19PM -0500, Melanie Plageman wrote:
> > > It still needs polishing (e.g. I need to figure out where to put the new
> > > guc ho
On 2025/03/04 1:14, Bertrand Drouvot wrote:
=== 2
+/*
+ * Validate the input for the log_connections GUC.
+ */
+bool
+check_log_connections(char **newval, void **extra, GucSource source)
+{
This function is pretty close to check_debug_io_direct() for example and its
overall content, memory m
Hi,
On Fri, Feb 28, 2025 at 05:42:37PM -0500, Melanie Plageman wrote:
> And all of these places we have to be super
> careful that the GUCs have already been read before using
> log_connections, so it seems a bit unsafe to check log_connections
> (the global variable) in a function.
Yeah, that co
On Fri, Feb 28, 2025 at 12:54 AM Bertrand Drouvot
wrote:
>
> On Thu, Feb 27, 2025 at 05:55:19PM -0500, Melanie Plageman wrote:
> > It still needs polishing (e.g. I need to figure out where to put the new
> > guc hook
> > functions and enums and such)
>
> yeah, I wonder if it would make sense to c
On Fri, Feb 28, 2025 at 12:16 AM Bertrand Drouvot
wrote:
>
> Yup but my idea was to put all those line:
>
> "
> if (Log_connections &&
> (child_type == B_BACKEND || child_type == B_WAL_SENDER))
> {
> instr_time fork_time = ((BackendStartupData *)
> startup_data)->fork_tim
Hi,
On Thu, Feb 27, 2025 at 05:55:19PM -0500, Melanie Plageman wrote:
> On Thu, Feb 27, 2025 at 11:30 AM Andres Freund wrote:
> >
> > On 2025-02-27 11:08:04 -0500, Melanie Plageman wrote:
> >
> > > However, since that is a bigger project (with more refactoring, etc),
> > > he suggested that we ch
Hi,
On Thu, Feb 27, 2025 at 11:08:04AM -0500, Melanie Plageman wrote:
> I was just talking to Andres off-list and he mentioned that the volume
> of log_connections messages added in recent releases can really be a
> problem for users. He said ideally we would emit one message which
> consolidated
Hi,
On Thu, Feb 27, 2025 at 11:14:56AM -0500, Andres Freund wrote:
> I don't think the timing overhead is a relevant factor here - compared to the
> fork of a new connection or performing authentication the cost of taking a few
> timestamps is neglegible. A timestamp costs 10s to 100s of cycles, a
On Thu, Feb 27, 2025 at 11:30 AM Andres Freund wrote:
>
> On 2025-02-27 11:08:04 -0500, Melanie Plageman wrote:
>
> > However, since that is a bigger project (with more refactoring, etc),
> > he suggested that we change log_connections to a GUC_LIST
> > (ConfigureNamesString) with options like "no
Hi,
On 2025-02-27 11:08:04 -0500, Melanie Plageman wrote:
> I was just talking to Andres off-list and he mentioned that the volume
> of log_connections messages added in recent releases can really be a
> problem for users.
For some added color: I've seen plenty systems where almost all the log vo
Hi,
On 2025-02-27 06:50:41 +, Bertrand Drouvot wrote:
> On Wed, Feb 26, 2025 at 01:45:39PM -0500, Melanie Plageman wrote:
> > Thanks for the continued review!
> >
> > On Wed, Feb 26, 2025 at 2:41 AM Bertrand Drouvot
> > wrote:
> > >
> > > On Wed, Feb 26, 2025 at 01:46:19PM +0900, Fujii Masao
On Thu, Feb 27, 2025 at 1:50 AM Bertrand Drouvot
wrote:
>
> Agree. IIUC, I think that Fujii-san's idea was to extend log_connections with
> a new option "timing" (i.e move it from ConfigureNamesBool to say
> ConfigureNamesEnum with say on, off and timing?). I think that's a good idea.
>
> I just d
Hi,
On Wed, Feb 26, 2025 at 01:45:39PM -0500, Melanie Plageman wrote:
> Thanks for the continued review!
>
> On Wed, Feb 26, 2025 at 2:41 AM Bertrand Drouvot
> wrote:
> >
> > On Wed, Feb 26, 2025 at 01:46:19PM +0900, Fujii Masao wrote:
> > >
> > > With the current patch, when log_connections is
On Wed, Feb 26, 2025 at 1:45 PM Melanie Plageman
wrote:
>
> Thanks for the continued review!
>
> On Wed, Feb 26, 2025 at 2:41 AM Bertrand Drouvot
> wrote:
>
> > Add a few words in the log_connections GUC doc? (anyway we will have to if
> > Fujii-san idea above about the timing is implemented)
>
>
Thanks for the continued review!
On Wed, Feb 26, 2025 at 2:41 AM Bertrand Drouvot
wrote:
>
> On Wed, Feb 26, 2025 at 01:46:19PM +0900, Fujii Masao wrote:
> >
> > With the current patch, when log_connections is enabled, the connection
> > time is always
> > captured, and which might introduce per
Thanks for the review!
On Tue, Feb 25, 2025 at 11:46 PM Fujii Masao
wrote:
>
> On 2025/02/26 6:36, Melanie Plageman wrote:
> > On Tue, Feb 25, 2025 at 3:23 PM Melanie Plageman
> > wrote:
>
> + /* Capture time Postmaster initiates fork for logging */
> + if (child_type == B_BACKEND)
>
On 26/02/2025 08:41, Bertrand Drouvot wrote:
Hi,
On Wed, Feb 26, 2025 at 01:46:19PM +0900, Fujii Masao wrote:
On 2025/02/26 6:36, Melanie Plageman wrote:
On Tue, Feb 25, 2025 at 3:23 PM Melanie Plageman
wrote:
Thanks for doing this! I have implemented your suggestion in attached v3.
Tha
Hi,
On Wed, Feb 26, 2025 at 01:46:19PM +0900, Fujii Masao wrote:
>
>
> On 2025/02/26 6:36, Melanie Plageman wrote:
> > On Tue, Feb 25, 2025 at 3:23 PM Melanie Plageman
> > wrote:
> > >
> > > Thanks for doing this! I have implemented your suggestion in attached v3.
Thanks for the new patch ver
On 2025/02/26 6:36, Melanie Plageman wrote:
On Tue, Feb 25, 2025 at 3:23 PM Melanie Plageman
wrote:
Thanks for doing this! I have implemented your suggestion in attached v3.
I missed an include in the EXEC_BACKEND not defined case. attached v4
is fixed up.
Thanks for updating the patch!
On Tue, Feb 25, 2025 at 3:23 PM Melanie Plageman
wrote:
>
> Thanks for doing this! I have implemented your suggestion in attached v3.
I missed an include in the EXEC_BACKEND not defined case. attached v4
is fixed up.
- Melanie
From c493f0be7243bfe965d3493f07982ea1072d89b7 Mon Sep 17 00:00:00 200
Thanks for taking a look!
On Mon, Jan 20, 2025 at 12:53 PM Jacob Champion
wrote:
>
> On Mon, Jan 20, 2025 at 7:01 AM Bertrand Drouvot
> wrote:
> > Though time changes are "rare", given the fact that those metrics could
> > provide
> > "inaccurate" measurements during that particular moment (tim
Thanks for taking a look!
On Mon, Jan 20, 2025 at 10:01 AM Bertrand Drouvot
wrote:
>
> The patch needed a rebase due to 34486b6092e. I did it in v2 attached (it's
> a minor rebase due to the AmRegularBackendProcess() introduction in
> miscadmin.h).
>
> v2 could rely on AmRegularBackendProcess()
On Mon, Dec 16, 2024 at 6:26 PM Jelte Fennema-Nio wrote:
>
> Two thoughts:
> 1. Would it make sense to also expose these timings in some pg_stat_xyz view?
So, right now there isn't an obvious place this would fit. There are
dynamic statistics views that are per connection -- like
pg_stat_ssl/gssa
On Mon, Jan 20, 2025 at 7:01 AM Bertrand Drouvot
wrote:
> Though time changes are "rare", given the fact that those metrics could
> provide
> "inaccurate" measurements during that particular moment (time change) then it
> might be worth considering instr_time instead for this particular metric.
Hi,
On Mon, Jan 06, 2025 at 10:08:36PM +0100, Guillaume Lelarge wrote:
> Hello,
>
> Le lun. 16 déc. 2024 à 22:00, Melanie Plageman
> a écrit :
>
> > Hi,
> >
> > Users wishing to debug slow connection establishment have little
> > visibility into which steps take the most time. We don't expose a
Hello,
Le lun. 16 déc. 2024 à 22:00, Melanie Plageman
a écrit :
> Hi,
>
> Users wishing to debug slow connection establishment have little
> visibility into which steps take the most time. We don't expose any
> stats and none of the logging includes durations.
>
> The attached patch logs duratio
On Mon, 16 Dec 2024 at 22:00, Melanie Plageman
wrote:
> Users wishing to debug slow connection establishment have little
> visibility into which steps take the most time. We don't expose any
> stats and none of the logging includes durations.
Two thoughts:
1. Would it make sense to also expose th
Hi,
Users wishing to debug slow connection establishment have little
visibility into which steps take the most time. We don't expose any
stats and none of the logging includes durations.
The attached patch logs durations for authentication, forking the
Postgres backend, and the end-to-end connect
68 matches
Mail list logo
