Thread: [PATCH] pg_upgrade: Split off pg_fatal() from pg_log()
The experiences with elog() and ereport() have shown that having one function that can return or not depending on some log level parameter isn't a good idea when you want to communicate well with the compiler. In pg_upgrade, there is a similar case with the pg_log() function. Since that isn't a public API, I'm proposing to change it and introduce a separate function pg_fatal() for those cases where the calls don't return.
Attachment
Hi Peter, On 2013-09-13 04:50, Peter Eisentraut wrote: > The experiences with elog() and ereport() have shown that having one > function that can return or not depending on some log level parameter > isn't a good idea when you want to communicate well with the compiler. > In pg_upgrade, there is a similar case with the pg_log() function. > Since that isn't a public API, I'm proposing to change it and introduce > a separate function pg_fatal() for those cases where the calls don't > return. I think the reasoning behind this patch is sound. However, I would like to raise a couple of small questions: 1) Is there a reason for the fmt string not being const char*? You changed it for pg_log_v(), but not for pg_log() and pg_fatal(). 2) Allowing PG_FATAL to be passed to pg_log() seems weird,but I don't feel strongly about that. Other than that, the patch looks good to me. Regards, Marko Tiikkaja
On Thu, Sep 12, 2013 at 10:50:42PM -0400, Peter Eisentraut wrote: > The experiences with elog() and ereport() have shown that having one > function that can return or not depending on some log level parameter > isn't a good idea when you want to communicate well with the compiler. > In pg_upgrade, there is a similar case with the pg_log() function. > Since that isn't a public API, I'm proposing to change it and introduce > a separate function pg_fatal() for those cases where the calls don't > return. Fine with me. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
On Sun, 2013-09-15 at 18:27 +0200, Marko Tiikkaja wrote: > I think the reasoning behind this patch is sound. However, I would like > to raise a couple of small questions: > > 1) Is there a reason for the fmt string not being const char*? You > changed it for pg_log_v(), but not for pg_log() and pg_fatal(). Good catch. I think I just left the existing code alone. I'll fix it. > 2) Allowing PG_FATAL to be passed to pg_log() seems weird, but I > don't feel strongly about that. Yeah, it's a bit weird. It's just because it all ends up in pg_log_v(). We could have pg_log() error about it, but that seems a bit excessive. It's not a public API or anything.
On Tue, Oct 1, 2013 at 9:33 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On Sun, 2013-09-15 at 18:27 +0200, Marko Tiikkaja wrote: >> I think the reasoning behind this patch is sound. However, I would like >> to raise a couple of small questions: >> >> 1) Is there a reason for the fmt string not being const char*? You >> changed it for pg_log_v(), but not for pg_log() and pg_fatal(). > > Good catch. I think I just left the existing code alone. I'll fix it. > >> 2) Allowing PG_FATAL to be passed to pg_log() seems weird, but I >> don't feel strongly about that. > > Yeah, it's a bit weird. It's just because it all ends up in pg_log_v(). > We could have pg_log() error about it, but that seems a bit excessive. > It's not a public API or anything. Since there are several votes in favor of this patch and none opposed, I'm marking it Ready for Committer. Peter, do you plan to commit it? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company