From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Allowing printf("%m") only where it actually works |
Date: | 2018-05-26 16:21:33 |
Message-ID: | 6025.1527351693@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
I wrote:
> Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> writes:
>> Here's an experimental way to do that, if you don't mind depending on
>> gory details of libc implementations (ie knowledge of what it expands
>> too). Not sure how to avoid that since it's a macro on all modern
>> systems, and we don't have a way to temporarily redefine a macro. If
>> you enable it for just ereport(), it compiles cleanly after 81256cd
>> (but fails on earlier commits). If you enable it for elog() too then
>> it finds problems with exec.c.
> Hmm ... that's pretty duff code in exec.c, isn't it. Aside from the
> question of errno unsafety, it's using elog where it really ought to be
> using ereport, it's not taking any thought for the reported SQLSTATE,
> etc. I'm hesitant to mess with it mere hours before the beta wrap,
> but we really oughta improve that.
I wrote up a patch that makes src/common/exec.c do error reporting more
like other frontend/backend-common files (attached). Now that I've done
so, though, I'm having second thoughts. The thing that I don't like
about this is that it doubles the number of translatable strings created
by this file. While there's not *that* many of them, translators have to
deal with each one several times because this file is included by several
different frontend programs. So that seems like a rather high price to
pay to deal with what, at present, is a purely hypothetical hazard.
(Basically what this would protect against is elog_start changing errno,
which it doesn't.) Improving the errcode situation is somewhat useful,
but still maybe it's not worth the cost.
Another approach we could consider is keeping exec.c's one-off approach
to error handling and letting it redefine pg_prevent_errno_in_scope() as
empty. But that's ugly.
Or we could make the affected call sites work like this:
int save_errno = errno;
log_error(_("could not identify current directory: %s"),
strerror(save_errno));
which on the whole might be the most expedient thing.
regards, tom lane
Attachment | Content-Type | Size |
---|---|---|
make-exec.c-do-error-reporting-like-everyplace-else.patch | text/x-diff | 13.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2018-05-26 16:54:45 | Re: Adding a new table to the system catalog |
Previous Message | Chapman Flack | 2018-05-26 15:44:19 | Re: SPI/backend equivalent of extended-query Describe(statement)? |