Dmitry Koval writes:
> Hi!
> Commit 9a374b77fb53 (Improve frontend error logging style.) replaced a
> line of file src/include/common/logging.h
> ```
> extern PGDLLIMPORT enum pg_log_level __pg_log_level;
> ```
> to
> ```
> extern enum pg_log_level __pg_log_level;
>
Hi!
Commit 9a374b77fb53 (Improve frontend error logging style.) replaced a
line of file src/include/common/logging.h
```
extern PGDLLIMPORT enum pg_log_level __pg_log_level;
```
to
```
extern enum pg_log_level __pg_log_level;
```
i.e. it is rollback of commit 8ec569479fc28 (Apply PGDLLIMPORT
On 23.05.22 11:53, Peter Eisentraut wrote:
On 29.03.22 16:24, Peter Eisentraut wrote:
I think I would want the program name/location also in front of the
detail and hint lines. I need to think about this a bit more. This
shouldn't hold up this patch; it would be a quick localized change.
Af
On 29.03.22 16:24, Peter Eisentraut wrote:
I think I would want the program name/location also in front of the
detail and hint lines. I need to think about this a bit more. This
shouldn't hold up this patch; it would be a quick localized change.
After experiencing this for a bit now, I propo
Peter Eisentraut writes:
> On 11.04.22 17:22, Tom Lane wrote:
>> The patch I presented keeps the unlikely() checks in the debug-level
>> macros. Do you think we should drop those too? I figured that avoiding
>> evaluating the arguments would be worth something.
> Oh, that's right, the whole thi
On 11.04.22 17:22, Tom Lane wrote:
Peter Eisentraut writes:
On 08.04.22 22:26, Tom Lane wrote:
I think we should put a centralized level check
into logging.c, and get rid of at least the "if (likely())"
checks, because those are going to succeed approximately 100.0%
of the time. Maybe there
Peter Eisentraut writes:
> On 08.04.22 22:26, Tom Lane wrote:
>>> I think we should put a centralized level check
>>> into logging.c, and get rid of at least the "if (likely())"
>>> checks, because those are going to succeed approximately 100.0%
>>> of the time. Maybe there's an argument for keep
On 08.04.22 22:26, Tom Lane wrote:
I wrote:
One other loose end is bothering me: I stuck with logging.h's
original choice to put "if (likely())" or "if (unlikely())"
conditionals into the macros, but I rather suspect that that's
just a waste. I think we should put a centralized level check
into
I wrote:
> One other loose end is bothering me: I stuck with logging.h's
> original choice to put "if (likely())" or "if (unlikely())"
> conditionals into the macros, but I rather suspect that that's
> just a waste. I think we should put a centralized level check
> into logging.c, and get rid of a
Daniel Gustafsson writes:
> On 30 Mar 2022, at 00:38, Tom Lane wrote:
>> Feel free to work on a followup editing patch though.
> Thats my plan, once this lands I'll rebase the comments on top of your work
> and
> we can have a separate discussion around them then.
The main patch is pushed now.
> On 30 Mar 2022, at 00:38, Tom Lane wrote:
>
> Daniel Gustafsson writes:
>> On 29 Mar 2022, at 16:38, Peter Eisentraut
>> wrote:
>>> Most of these should probably be addressed separately from Tom's patch.
>
>> Yeah, I think so too.
>
> Agreed. I tried to confine my patch to mechanical chan
Greg Stark writes:
> FYI this patch no longer applies.
No surprise :-(
> Given it's a Tom patch and due to its nature it'll bitrot rapidly
> anyways I'm don't see a point in updating the status though.
We now seem to have buy-in on the concept, so my plan is to let
this sit till the end of the
FYI this patch no longer applies.
Given it's a Tom patch and due to its nature it'll bitrot rapidly
anyways I'm don't see a point in updating the status though.
Daniel Gustafsson writes:
> On 29 Mar 2022, at 16:38, Peter Eisentraut
> wrote:
>> Most of these should probably be addressed separately from Tom's patch.
> Yeah, I think so too.
Agreed. I tried to confine my patch to mechanical changes, except
for changing places where the detail/hint featur
> On 29 Mar 2022, at 16:38, Peter Eisentraut
> wrote:
>
> On 29.03.22 12:13, Daniel Gustafsson wrote:
>
> Most of these should probably be addressed separately from Tom's patch.
Yeah, I think so too. I'll await input from Tom on how he wants to do, but I'm
happy to take these to a new thread.
On 29.03.22 12:13, Daniel Gustafsson wrote:
Most of these should probably be addressed separately from Tom's patch.
@@ -508,24 +502,15 @@ writefile(char *path, char **lines)
if (fclose(out_file))
- {
- pg_log_error("could not write file \"%s\": %m", path);
-
On 27.03.22 22:19, Tom Lane wrote:
Here's a rebase up to today's HEAD. I've fixed the merge problems,
but there may be some stray new error calls that could be converted
to use pg_fatal() and haven't been. I don't want to do a full
fresh scan of the code until we're about ready to commit this.
> On 27 Mar 2022, at 22:19, Tom Lane wrote:
>
> Here's a rebase up to today's HEAD. I've fixed the merge problems,
> but there may be some stray new error calls that could be converted
> to use pg_fatal() and haven't been. I don't want to do a full
> fresh scan of the code until we're about rea
Hi,
On 2022-03-21 22:00:37 -0400, Tom Lane wrote:
> Andres Freund writes:
> > This unfortunately has had some bitrot:
> > http://cfbot.cputube.org/patch_37_3574.log
>
> Yeah, I know. That patch touches enough places that it's sure to
> break every few days during a commitfest. I'm not real ex
Andres Freund writes:
> On 2022-02-25 12:15:25 -0500, Tom Lane wrote:
>> The attached revision does that, standardizes on pg_fatal() as the
>> abbreviation for pg_log_error() + exit(1), and invents detail/hint
>> features as per previous discussion.
> This unfortunately has had some bitrot:
> ht
Hi,
On 2022-02-25 12:15:25 -0500, Tom Lane wrote:
> The attached revision does that, standardizes on pg_fatal() as the
> abbreviation for pg_log_error() + exit(1), and invents detail/hint
> features as per previous discussion.
This unfortunately has had some bitrot:
http://cfbot.cputube.org/patc
Michael Paquier writes:
> Hm. I am not sure to like much this abstraction by having so many
> macros that understand the log level through their name. Wouldn't it
> be cleaner to have only one pg_log_hint() and one pg_log_detail() with
> the log level passed as argument of the macro?
Mmm ... th
On Fri, Feb 25, 2022 at 12:15:25PM -0500, Tom Lane wrote:
> I feel that the reasonable alternatives are either to drop the FATAL
> log level, or try to make it actually mean something by consistently
> using it for errors that are indeed fatal. I had a go at doing the
> latter, but eventually conc
Peter Eisentraut writes:
> This patch already illustrates a couple of things that are wrong with
> this approach:
All of these objections are a bit moot with my followup proposal, no?
> - It doesn't allow any other way of exiting. For example, in pg_dump,
> you have removed a few exit_nicely(
Hi,
On 2022-02-24 14:06:18 +0100, Peter Eisentraut wrote:
> My suggestion is to just get rid of pg_log_fatal() and replace them all with
> pg_log_error().
-1. This ignores that already several places came up with their slightly
different versions of fatal exit handlers. We don't gain anything by
On 23.02.22 00:23, Tom Lane wrote:
This conversation seems to have tailed off without full resolution,
but I observe that pretty much everyone except Peter is on board
with defining pg_log_fatal as pg_log_error + exit(1). So I think
we should just do that, unless Peter wants to produce a finishe
On Wed, Feb 23, 2022, at 12:30 PM, Tom Lane wrote:
> Updating the patch is going to be a bit tedious, so I'm not
> going to do it without buy-in that this solution would be
> okay to commit. Any objections?
No. I was confused by the multiple log functions spread in the client programs
while I was
Hi,
On 2022-02-23 10:30:02 -0500, Tom Lane wrote:
> So I now propose modifying yesterday's patch thus:
>
> * Reinstantiate the PG_LOG_FATAL enum value, add support macros
> pg_log_fatal, pg_log_fatal_hint, pg_log_fatal_detail.
>
> * Define pg_fatal as pg_log_fatal + exit(1). (This would essenti
I wrote:
> Andres Freund writes:
>> What about adding a pg_fatal() that's pg_log_fatal() + exit()? That keeps
>> pg_log_* stuff "log only", but adds something adjacent enough to hopefully
>> reduce future misunderstandings?
> I'd be okay with that, except that pg_upgrade already has a pg_fatal
>
On 2022-02-22 22:44:25 -0500, Tom Lane wrote:
> Andres Freund writes:
> > What about adding a pg_fatal() that's pg_log_fatal() + exit()? That keeps
> > pg_log_* stuff "log only", but adds something adjacent enough to hopefully
> > reduce future misunderstandings?
>
> I'd be okay with that, except
Andres Freund writes:
> What about adding a pg_fatal() that's pg_log_fatal() + exit()? That keeps
> pg_log_* stuff "log only", but adds something adjacent enough to hopefully
> reduce future misunderstandings?
I'd be okay with that, except that pg_upgrade already has a pg_fatal
(because it has it
Hi,
On 2022-02-22 18:23:19 -0500, Tom Lane wrote:
> Robert Haas writes:
> > On Fri, Nov 19, 2021 at 5:17 AM Peter Eisentraut
> > wrote:
> >> If people want to do that kind of thing (I'm undecided whether the
> >> complexity is worth it), then make it a different API. The pg_log_*
> >> calls are
On Fri, Nov 19, 2021 at 5:17 AM Peter Eisentraut
wrote:
> If people want to do that kind of thing (I'm undecided whether the
> complexity is worth it), then make it a different API. The pg_log_*
> calls are for writing formatted output. They normalized existing
> hand-coded patterns at the time.
On 16.11.21 16:18, Robert Haas wrote:
I think we need frontend facilities that look like the backend
facilities, so try/catch blocks, on-exit callbacks, and whatever else
there is. Otherwise code reuse is going to continue to be annoying.
If people want to do that kind of thing (I'm undecided w
On Mon, Nov 15, 2021 at 10:02 PM Michael Paquier wrote:
> On Mon, Nov 15, 2021 at 02:40:10PM -0500, Robert Haas wrote:
> > Having different frontend utilities each invent their own
> > slightly-different way of doing this makes it hard to reuse code, and
> > hard to understand code. We need to fin
On Mon, Nov 15, 2021 at 02:40:10PM -0500, Robert Haas wrote:
> Having different frontend utilities each invent their own
> slightly-different way of doing this makes it hard to reuse code, and
> hard to understand code. We need to find ways to make it more uniform,
> not just observe that it isn't
Peter Eisentraut writes:
> Several programs wrap, say, pg_log_fatal() into a pg_fatal(), that does
> logging, cleanup, and exit, as the case may be. I think that's a good
> solution.
I agree, and my draft patch formalized that by turning pg_log_fatal into
exactly that.
The question that I thi
On Mon, Nov 15, 2021 at 2:15 PM Peter Eisentraut
wrote:
> This is specifically designed not to do any flow control. In the
> backend, we have many instances, where log messages are issued with the
> wrong log level because the stronger log level would have flow control
> impact that is not approp
On 10.11.21 16:28, Robert Haas wrote:
What I think we ought
to be driving towards is having pg_log_fatal() forcibly exit, and
pg_log_error() do the same unless the error is somehow caught.
This is specifically designed not to do any flow control. In the
backend, we have many instances, where
On 09.11.21 23:20, Tom Lane wrote:
2. What is the preferred style for adding extra lines to log messages?
I see a lot of direct prints to stderr:
pg_log_error("missing required argument: database name");
fprintf(stderr, _("Try \"%s --help\" for more information.\n
On 09.11.21 23:20, Tom Lane wrote:
1. The distinction between "error" and "fatal" levels seems squishy
to the point of uselessness. I think we should either get rid of it
entirely, or make an effort to use "fatal" exactly for the cases that
are going to give up and exit right away. Of the ap
Kyotaro Horiguchi writes:
> Aren't DETAIL and HINT expected to be hidden at the targetted cutoff
> level? In other words, I suspect that people want to hide non-primary
> messages for a lower verbosity level. On the other hand I'm not sure
> it is a proper behavior that log_level = WARNING cause
At Wed, 10 Nov 2021 14:25:08 -0500, Tom Lane wrote in
> I wrote:
> > Hmm, interesting. Taking up my point #2, I'd been thinking about
> > proposing that we convert
> > pg_log_error("query failed: %s", PQerrorMessage(conn));
> > pg_log_error("query was: %s", todo);
> > to
I wrote:
> Hmm, interesting. Taking up my point #2, I'd been thinking about
> proposing that we convert
> pg_log_error("query failed: %s", PQerrorMessage(conn));
> pg_log_error("query was: %s", todo);
> to
> pg_log_error("query failed: %s", PQerrorMessage(
Robert Haas writes:
> I agree with this list of problems. I think that the end game here is
> getting to be able to use ereport() and friends in the frontend, which
> would require confronting both of these issues at a deep level. We
> don't necessarily have to do that now, though, but I think it'
On Tue, Nov 9, 2021 at 5:20 PM Tom Lane wrote:
> 1. The distinction between "error" and "fatal" levels seems squishy
> to the point of uselessness.
>
> 2. What is the preferred style for adding extra lines to log messages?
I agree with this list of problems. I think that the end game here is
get
At Tue, 09 Nov 2021 17:20:41 -0500, Tom Lane wrote in
> ISTM that the recently-introduced new frontend logging support
> (common/logging.h et al) could use a second pass now that we have
> some experience with it. There are two points that are bugging me:
>
> 1. The distinction between "error"
ISTM that the recently-introduced new frontend logging support
(common/logging.h et al) could use a second pass now that we have
some experience with it. There are two points that are bugging me:
1. The distinction between "error" and "fatal" levels seems squishy
to the point of uselessness. I
48 matches
Mail list logo
