Re: Missing errcode() in ereport - Mailing list pgsql-hackers

Hi,

On 2020-03-19 19:32:55 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-03-19 14:07:04 -0400, Tom Lane wrote:
> >> Now that we can rely on having varargs macros, I think we could
> >> stop requiring the extra level of parentheses,
>
> > I think that'd be an improvement, because:
> > ane of the ones I saw confuse people is just:
> > /home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: error: ‘ereport’ undeclared (first use in this
function);did you mean ‘rresvport’?
 
> >  3727 |    ereport(FATAL,
> >       |    ^~~~~~~
> >       |    rresvport
> > /home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: note: each undeclared identifier is reported only
oncefor each function it appears in
 
> > because the extra parens haven't been added.
>
> Ah, so the macro isn't expanded at all if it appears to have more than
> two parameters?  That seems odd, but I suppose some compilers might
> work that way.  Switching to varargs would improve that for sure.

Yea. Newer gcc versions do warn about a parameter mismatch count, which
helps.  I'm not sure what the preprocessor / compiler should do intead?

FWIW, clang also doesn't expand the macro.


> > Another one I've both seen and committed byself is converting an elog to
> > an ereport, and not adding an errcode() around the error code - which
> > silently looks like it works.
>
> You mean not adding errmsg() around the error string?  Yeah, I can
> believe that one easily.  It's sort of the same problem as forgetting
> to wrap errcode() around the ERRCODE_ constant.

Both of these, I think.


> Could we restructure things so that the errcode/errmsg/etc calls form
> a standalone comma expression rather than appearing to be arguments of
> a varargs function?  The compiler's got no hope of realizing there's
> anything wrong when it thinks it's passing an integer or string
> constant to a function it knows nothing about.  But if it could see
> that nothing at all is being done with the constant, maybe we'd get
> somewhere.

Worth a try. Not doing a pointless varargs call could also end up
reducing elog overhead a bit (right now we push a lot of 0 as vararg
arguments for no reason).

A quick hack suggests that it works:

/home/andres/src/postgresql/src/backend/tcop/postgres.c: In function ‘process_postgres_switches’:
/home/andres/src/postgresql/src/backend/tcop/postgres.c:3728:27: warning: left-hand operand of comma expression has no
effect[-Wunused-value]
 
 3728 |      (ERRCODE_SYNTAX_ERROR,
      |                           ^
/home/andres/src/postgresql/src/include/utils/elog.h:126:4: note: in definition of macro ‘ereport_domain’
  126 |    rest; \
      |    ^~~~
/home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: note: in expansion of macro ‘ereport’
 3727 |    ereport(FATAL,
      |    ^~~~~~~


and in an optimized build the resulting code indeed looks a bit better:

before:
   text       data        bss        dec        hex    filename
8462795     176128     204464    8843387     86f07b    src/backend/postgres

Looking at FloatExceptionHandler as an example:

2860        /* We're not returning, so no need to save errno */
2861        ereport(ERROR,
   0x0000000000458bc0 <+0>:    push   %rbp
   0x0000000000458bc1 <+1>:    mov    $0xb2d,%edx
   0x0000000000458bc6 <+6>:    lea    0x214c9b(%rip),%rsi        # 0x66d868
   0x0000000000458bcd <+13>:    mov    %rsp,%rbp
   0x0000000000458bd0 <+16>:    push   %r13
   0x0000000000458bd2 <+18>:    xor    %r8d,%r8d
   0x0000000000458bd5 <+21>:    lea    0x2d9dc4(%rip),%rcx        # 0x7329a0 <__func__.41598>
   0x0000000000458bdc <+28>:    push   %r12
   0x0000000000458bde <+30>:    mov    $0x14,%edi
   0x0000000000458be3 <+35>:    callq  0x5a8c00 <errstart>
   0x0000000000458be8 <+40>:    lea    0x214e59(%rip),%rdi        # 0x66da48
   0x0000000000458bef <+47>:    xor    %eax,%eax
   0x0000000000458bf1 <+49>:    callq  0x5acb00 <errdetail>
   0x0000000000458bf6 <+54>:    mov    %eax,%r13d
   0x0000000000458bf9 <+57>:    lea    0x1bf7fb(%rip),%rdi        # 0x6183fb
   0x0000000000458c00 <+64>:    xor    %eax,%eax
   0x0000000000458c02 <+66>:    callq  0x5ac710 <errmsg>
   0x0000000000458c07 <+71>:    mov    $0x1020082,%edi
   0x0000000000458c0c <+76>:    mov    %eax,%r12d
   0x0000000000458c0f <+79>:    callq  0x5ac5b0 <errcode>
   0x0000000000458c14 <+84>:    mov    %eax,%edi
   0x0000000000458c16 <+86>:    mov    %r13d,%edx
   0x0000000000458c19 <+89>:    mov    %r12d,%esi
   0x0000000000458c1c <+92>:    xor    %eax,%eax
   0x0000000000458c1e <+94>:    callq  0x5ac020 <errfinish>

vs after:
   text       data        bss        dec        hex    filename
8395731     176128     204464    8776323     85ea83    src/backend/postgres
2861        ereport(ERROR,
   0x0000000000449dd0 <+0>:    push   %rbp
   0x0000000000449dd1 <+1>:    xor    %r8d,%r8d
   0x0000000000449dd4 <+4>:    lea    0x2d8a65(%rip),%rcx        # 0x722840 <__func__.41598>
   0x0000000000449ddb <+11>:    mov    %rsp,%rbp
   0x0000000000449dde <+14>:    mov    $0xb2d,%edx
   0x0000000000449de3 <+19>:    lea    0x2137ee(%rip),%rsi        # 0x65d5d8
   0x0000000000449dea <+26>:    mov    $0x14,%edi
   0x0000000000449def <+31>:    callq  0x5992e0 <errstart>
   0x0000000000449df4 <+36>:    mov    $0x1020082,%edi
   0x0000000000449df9 <+41>:    callq  0x59cc80 <errcode>
   0x0000000000449dfe <+46>:    lea    0x1be496(%rip),%rdi        # 0x60829b
   0x0000000000449e05 <+53>:    xor    %eax,%eax
   0x0000000000449e07 <+55>:    callq  0x59cde0 <errmsg>
   0x0000000000449e0c <+60>:    lea    0x2139a5(%rip),%rdi        # 0x65d7b8
   0x0000000000449e13 <+67>:    xor    %eax,%eax
   0x0000000000449e15 <+69>:    callq  0x59d1d0 <errdetail>
   0x0000000000449e1a <+74>:    callq  0x59c700 <errfinish>


I wonder if it'd become a relevant backpatch pain if we started to have
some ereports() without the additional parens in 13+. Would it perhaps
make sense to backpatch just the part that removes the need for the
parents, but not the return type changes?

This is patch 0001.


We can also remove elog() support code now, because with __VA_ARGS__
support it's really just a wrapper for ereport(elevel,
errmsg(__VA_ARGS_)).  This is patch 0002.


I think it might also be a good idea to move __FILE__, __LINE__,
PG_FUNCNAME_MACRO, domain from being parameters to errstart to
errfinish. For elevel < ERROR its sad that we currently pass them even
if we don't emit the message.  This is patch 0003.


I wonder if its worth additionally ensuring that errcode, errmsg,
... are only called within errstart/errfinish? We could have errstart()
return the error being "prepared" and store it in a local variable, and
make errmsg() etc macros that internally pass that variable to the
"real" errmsg() etc. Like

#define errmsg(...) errmsg_impl(current_error, __VA_ARGS__)

and set up current_error in ereport_domain() roughly like
    do {
        ErrorData  *current_error_;
        if ((current_error_ = errstart(elevel, the, rest)) != NULL)
        {
            ...
            errfinish()
        }
        ...

probably not worth it?

Greetings,

Andres Freund