Missing errcode() in ereport

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
31 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Missing errcode() in ereport

Masahiko Sawada-2
Hi,

In PageIsVerified() we report a WARNING as follows:

        ereport(WARNING,
                (ERRCODE_DATA_CORRUPTED,
                 errmsg("page verification failed, calculated checksum
%u but expected %u",
                        checksum, p->pd_checksum)));

However the error message won't have sql error code due to missing
errcode(). As far as I can see there are four places:

$ git grep "(ERRCODE" | grep -v errcode
contrib/adminpack/adminpack.c:
(ERRCODE_DUPLICATE_FILE,
contrib/adminpack/adminpack.c:                          (ERRCODE_DUPLICATE_FILE,
contrib/adminpack/adminpack.c:
 (ERRCODE_UNDEFINED_FILE,
src/backend/storage/page/bufpage.c:
(ERRCODE_DATA_CORRUPTED,
src/pl/plpgsql/src/pl_exec.c:           else if
(ERRCODE_IS_CATEGORY(sqlerrstate) &&

Attached patch add errcode() to these places.

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

add_errcode.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

akapila
On Tue, Mar 17, 2020 at 2:08 PM Masahiko Sawada
<[hidden email]> wrote:

>
> Hi,
>
> In PageIsVerified() we report a WARNING as follows:
>
>         ereport(WARNING,
>                 (ERRCODE_DATA_CORRUPTED,
>                  errmsg("page verification failed, calculated checksum
> %u but expected %u",
>                         checksum, p->pd_checksum)));
>
> However the error message won't have sql error code due to missing
> errcode(). As far as I can see there are four places:
>
> $ git grep "(ERRCODE" | grep -v errcode
> contrib/adminpack/adminpack.c:
> (ERRCODE_DUPLICATE_FILE,
> contrib/adminpack/adminpack.c:                          (ERRCODE_DUPLICATE_FILE,
> contrib/adminpack/adminpack.c:
>  (ERRCODE_UNDEFINED_FILE,
> src/backend/storage/page/bufpage.c:
> (ERRCODE_DATA_CORRUPTED,
> src/pl/plpgsql/src/pl_exec.c:           else if
> (ERRCODE_IS_CATEGORY(sqlerrstate) &&
>
> Attached patch add errcode() to these places.
>

+1.  This looks like an oversight to me.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Julien Rouhaud
On Tue, Mar 17, 2020 at 10:00 AM Amit Kapila <[hidden email]> wrote:

>
> On Tue, Mar 17, 2020 at 2:08 PM Masahiko Sawada
> <[hidden email]> wrote:
> >
> > Hi,
> >
> > In PageIsVerified() we report a WARNING as follows:
> >
> >         ereport(WARNING,
> >                 (ERRCODE_DATA_CORRUPTED,
> >                  errmsg("page verification failed, calculated checksum
> > %u but expected %u",
> >                         checksum, p->pd_checksum)));
> >
> > However the error message won't have sql error code due to missing
> > errcode(). As far as I can see there are four places:
> >
> > $ git grep "(ERRCODE" | grep -v errcode
> > contrib/adminpack/adminpack.c:
> > (ERRCODE_DUPLICATE_FILE,
> > contrib/adminpack/adminpack.c:                          (ERRCODE_DUPLICATE_FILE,
> > contrib/adminpack/adminpack.c:
> >  (ERRCODE_UNDEFINED_FILE,
> > src/backend/storage/page/bufpage.c:
> > (ERRCODE_DATA_CORRUPTED,
> > src/pl/plpgsql/src/pl_exec.c:           else if
> > (ERRCODE_IS_CATEGORY(sqlerrstate) &&
> >
> > Attached patch add errcode() to these places.
> >
>
> +1.  This looks like an oversight to me.

good catch!  And patch LGTM.


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Michael Paquier-2
On Tue, Mar 17, 2020 at 10:26:57AM +0100, Julien Rouhaud wrote:
> On Tue, Mar 17, 2020 at 10:00 AM Amit Kapila <[hidden email]> wrote:
>> +1.  This looks like an oversight to me.
>
> good catch!  And patch LGTM.

Definitely an oversight.  All stable branches down to 9.5 have
mistakes in the same area, with nothing extra by grepping around.
Amit, I guess that you will take care of it?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

akapila
On Tue, Mar 17, 2020 at 3:28 PM Michael Paquier <[hidden email]> wrote:

>
> On Tue, Mar 17, 2020 at 10:26:57AM +0100, Julien Rouhaud wrote:
> > On Tue, Mar 17, 2020 at 10:00 AM Amit Kapila <[hidden email]> wrote:
> >> +1.  This looks like an oversight to me.
> >
> > good catch!  And patch LGTM.
>
> Definitely an oversight.  All stable branches down to 9.5 have
> mistakes in the same area, with nothing extra by grepping around.
> Amit, I guess that you will take care of it?
>

Yes, I will unless I see any objections in a day or so.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Tom Lane-2
Amit Kapila <[hidden email]> writes:
> On Tue, Mar 17, 2020 at 3:28 PM Michael Paquier <[hidden email]> wrote:
>> Definitely an oversight.  All stable branches down to 9.5 have
>> mistakes in the same area, with nothing extra by grepping around.
>> Amit, I guess that you will take care of it?

> Yes, I will unless I see any objections in a day or so.

No need to wait, it's a pretty obvious thinko.

We might want to spend some effort thinking how to find or prevent
additional bugs of the same ilk ... but correcting the ones already
found needn't wait on that.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

akapila
On Tue, Mar 17, 2020 at 7:39 PM Tom Lane <[hidden email]> wrote:

>
> Amit Kapila <[hidden email]> writes:
> > On Tue, Mar 17, 2020 at 3:28 PM Michael Paquier <[hidden email]> wrote:
> >> Definitely an oversight.  All stable branches down to 9.5 have
> >> mistakes in the same area, with nothing extra by grepping around.
> >> Amit, I guess that you will take care of it?
>
> > Yes, I will unless I see any objections in a day or so.
>
> No need to wait, it's a pretty obvious thinko.
>

Okay, I will push in some time.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

akapila
On Wed, Mar 18, 2020 at 9:01 AM Amit Kapila <[hidden email]> wrote:

>
> On Tue, Mar 17, 2020 at 7:39 PM Tom Lane <[hidden email]> wrote:
> >
> > Amit Kapila <[hidden email]> writes:
> > > On Tue, Mar 17, 2020 at 3:28 PM Michael Paquier <[hidden email]> wrote:
> > >> Definitely an oversight.  All stable branches down to 9.5 have
> > >> mistakes in the same area, with nothing extra by grepping around.
> > >> Amit, I guess that you will take care of it?
> >
> > > Yes, I will unless I see any objections in a day or so.
> >
> > No need to wait, it's a pretty obvious thinko.
> >
>
> Okay, I will push in some time.
>

Pushed.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Masahiko Sawada-2
On Wed, 18 Mar 2020 at 13:57, Amit Kapila <[hidden email]> wrote:

>
> On Wed, Mar 18, 2020 at 9:01 AM Amit Kapila <[hidden email]> wrote:
> >
> > On Tue, Mar 17, 2020 at 7:39 PM Tom Lane <[hidden email]> wrote:
> > >
> > > Amit Kapila <[hidden email]> writes:
> > > > On Tue, Mar 17, 2020 at 3:28 PM Michael Paquier <[hidden email]> wrote:
> > > >> Definitely an oversight.  All stable branches down to 9.5 have
> > > >> mistakes in the same area, with nothing extra by grepping around.
> > > >> Amit, I guess that you will take care of it?
> > >
> > > > Yes, I will unless I see any objections in a day or so.
> > >
> > > No need to wait, it's a pretty obvious thinko.
> > >
> >
> > Okay, I will push in some time.
> >
>
> Pushed.

Thank you!


Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Andres Freund
In reply to this post by Tom Lane-2
Hi,

On 2020-03-17 10:09:18 -0400, Tom Lane wrote:
> We might want to spend some effort thinking how to find or prevent
> additional bugs of the same ilk ...

Yea, that'd be good.  Trying to help people new to postgres write their
first patches I found that ereport is very confusing to them - largely
because the syntax doesn't make much sense. Made worse by the compiler
error messages being terrible in many cases.

Not sure there's much we can do without changing ereport's "signature"
though :(

Regards,

Andres


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2020-03-17 10:09:18 -0400, Tom Lane wrote:
>> We might want to spend some effort thinking how to find or prevent
>> additional bugs of the same ilk ...

> Yea, that'd be good.  Trying to help people new to postgres write their
> first patches I found that ereport is very confusing to them - largely
> because the syntax doesn't make much sense. Made worse by the compiler
> error messages being terrible in many cases.

> Not sure there's much we can do without changing ereport's "signature"
> though :(

Now that we can rely on having varargs macros, I think we could
stop requiring the extra level of parentheses, ie instead of

        ereport(ERROR,
                (errcode(ERRCODE_DIVISION_BY_ZERO),
                 errmsg("division by zero")));

it could be just

        ereport(ERROR,
                errcode(ERRCODE_DIVISION_BY_ZERO),
                errmsg("division by zero"));

(The old syntax had better still work, of course.  I'm not advocating
running around and changing existing calls.)

I'm not sure that this'd really move the goalposts much in terms of making
it any less confusing, but possibly it would improve the compiler errors?
Do you have any concrete examples of crummy error messages?

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Andres Freund
Hi,

On 2020-03-19 14:07:04 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > On 2020-03-17 10:09:18 -0400, Tom Lane wrote:
> >> We might want to spend some effort thinking how to find or prevent
> >> additional bugs of the same ilk ...
>
> > Yea, that'd be good.  Trying to help people new to postgres write their
> > first patches I found that ereport is very confusing to them - largely
> > because the syntax doesn't make much sense. Made worse by the compiler
> > error messages being terrible in many cases.
>
> > Not sure there's much we can do without changing ereport's "signature"
> > though :(
>
> Now that we can rely on having varargs macros, I think we could
> stop requiring the extra level of parentheses, ie instead of
>
>         ereport(ERROR,
>                 (errcode(ERRCODE_DIVISION_BY_ZERO),
>                  errmsg("division by zero")));
>
> it could be just
>
>         ereport(ERROR,
>                 errcode(ERRCODE_DIVISION_BY_ZERO),
>                 errmsg("division by zero"));
>
> (The old syntax had better still work, of course.  I'm not advocating
> running around and changing existing calls.)

I think that'd be an improvement, because:

> I'm not sure that this'd really move the goalposts much in terms of making
> it any less confusing, but possibly it would improve the compiler errors?
> Do you have any concrete examples of crummy error messages?

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 once for each function it appears in

because the extra parens haven't been added.


I personally actually hit a variant of that on a semi-regular basis:
Closing the parens for "rest" early, as there's just so many parens in
ereports, especially if an errmsg argument is a function call
itself. Which leads to a variation of the above error message.  I know
how to address it, obviously, but I do find it somewhat annoying to deal
with.

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.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Tom Lane-2
Andres Freund <[hidden email]> 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 once for 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.

> 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.

I think that at least some compilers will complain about side-effect-free
subexpressions of a comma expression.  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.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Tom Lane-2
I wrote:
> I think that at least some compilers will complain about side-effect-free
> subexpressions of a comma expression.  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?

Yeah, the attached quick-hack patch seems to work really well for this.
I find that this now works:

    ereport(ERROR,
            errcode(ERRCODE_DIVISION_BY_ZERO),
            errmsg("foo"));

where before it gave the weird error you quoted.  Also, these both
draw warnings about side-effect-free expressions:

    ereport(ERROR,
            ERRCODE_DIVISION_BY_ZERO,
            errmsg("foo"));

    ereport(ERROR,
            "%d", 42);

With gcc you just need -Wall to get such warnings, and clang
seems to give them by default.

As a nice side benefit, the backend gets a couple KB smaller from
removal of errfinish's useless dummy argument.

I think that we could now also change all the helper functions
(errmsg et al) to return void instead of a dummy value, but that
would make the patch a lot longer and probably not move the
goalposts much in terms of error detection.

It also looks like we could use the same trick to get plain elog()
to have the behavior of not evaluating its arguments when it's
not going to print anything.  I've not poked at that yet either.

                        regards, tom lane


diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 62eef7b..a29ccd9 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -438,7 +438,7 @@ matches_backtrace_functions(const char *funcname)
  * See elog.h for the error level definitions.
  */
 void
-errfinish(int dummy,...)
+errfinish(void)
 {
  ErrorData  *edata = &errordata[errordata_stack_depth];
  int elevel;
@@ -1463,7 +1463,7 @@ elog_finish(int elevel, const char *fmt,...)
  /*
  * And let errfinish() finish up.
  */
- errfinish(0);
+ errfinish();
 }
 
 
@@ -1749,7 +1749,7 @@ ThrowErrorData(ErrorData *edata)
  recursion_depth--;
 
  /* Process the error. */
- errfinish(0);
+ errfinish();
 }
 
 /*
@@ -1863,7 +1863,7 @@ pg_re_throw(void)
  */
  error_context_stack = NULL;
 
- errfinish(0);
+ errfinish();
  }
 
  /* Doesn't return ... */
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 0a4ef02..e6fa85f 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -118,34 +118,34 @@
  *----------
  */
 #ifdef HAVE__BUILTIN_CONSTANT_P
-#define ereport_domain(elevel, domain, rest) \
+#define ereport_domain(elevel, domain, ...) \
  do { \
  pg_prevent_errno_in_scope(); \
  if (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
- errfinish rest; \
+ __VA_ARGS__, errfinish(); \
  if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
  pg_unreachable(); \
  } while(0)
 #else /* !HAVE__BUILTIN_CONSTANT_P */
-#define ereport_domain(elevel, domain, rest) \
+#define ereport_domain(elevel, domain, ...) \
  do { \
  const int elevel_ = (elevel); \
  pg_prevent_errno_in_scope(); \
  if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
- errfinish rest; \
+ __VA_ARGS__, errfinish(); \
  if (elevel_ >= ERROR) \
  pg_unreachable(); \
  } while(0)
 #endif /* HAVE__BUILTIN_CONSTANT_P */
 
-#define ereport(elevel, rest) \
- ereport_domain(elevel, TEXTDOMAIN, rest)
+#define ereport(elevel, ...) \
+ ereport_domain(elevel, TEXTDOMAIN, __VA_ARGS__)
 
 #define TEXTDOMAIN NULL
 
 extern bool errstart(int elevel, const char *filename, int lineno,
  const char *funcname, const char *domain);
-extern void errfinish(int dummy,...);
+extern void errfinish(void);
 
 extern int errcode(int sqlerrcode);
 
Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Andres Freund
In reply to this post by Tom Lane-2
Hi,

On 2020-03-19 19:32:55 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> 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 once for 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

v2-0001-WIP-elog-Make-ereport-a-bit-easier-to-use-and-a-b.patch (27K) Download Attachment
v2-0002-WIP-reimplement-elog-using-ereport.patch (5K) Download Attachment
v2-0003-WIP-elog-Move-file-line-function-domain-setup-to-.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Andres Freund
In reply to this post by Tom Lane-2
Hi,

On 2020-03-19 21:03:17 -0400, Tom Lane wrote:
> I wrote:
> > I think that at least some compilers will complain about side-effect-free
> > subexpressions of a comma expression.  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?
>
> Yeah, the attached quick-hack patch seems to work really well for
> this.

Heh, you're too fast. I just sent something similar, and a few followup
patches.

What is your thinking about pain around backpatching it might introduce?
We can't easily backpatch support for ereport-without-extra-parens, I
think, because it needs __VA_ARGS__?


> As a nice side benefit, the backend gets a couple KB smaller from
> removal of errfinish's useless dummy argument.

I don't think it's the removal of the dummy parameter itself that
constitutes most of the savings, but instead it's not having to push the
return value of errmsg(), errcode(), et al as vararg arguments.


> I think that we could now also change all the helper functions
> (errmsg et al) to return void instead of a dummy value, but that
> would make the patch a lot longer and probably not move the
> goalposts much in terms of error detection.

I did include that in my prototype patch. Agree that it's not necessary
for the error detection capability, but it seems misleading to leave the
return values around if they're not actually needed.


> It also looks like we could use the same trick to get plain elog()
> to have the behavior of not evaluating its arguments when it's
> not going to print anything.  I've not poked at that yet either.

I've included a patch for that. I think it's now sufficient to
#define elog(elevel, ...)  \
        ereport(elevel, errmsg(__VA_ARGS__))

which imo is quite nice, as it allows us to get rid of a lot of
duplication.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Tom Lane-2
In reply to this post by Andres Freund
Andres Freund <[hidden email]> writes:
> I wonder if it'd become a relevant backpatch pain if we started to have
> some ereports() without the additional parens in 13+.

Yeah, it would be a nasty backpatch hazard.

> Would it perhaps
> make sense to backpatch just the part that removes the need for the
> parents, but not the return type changes?

I was just looking into that.  It would be pretty painless to do it
in v12, but before that we weren't requiring C99.  Having said that,
trolling the buildfarm database shows exactly zero active members
that don't report having __VA_ARGS__, in the branches where we test
that.  (And pg_config.h.win32 was assuming that MSVC had that, too.)

Could we get away with moving the compiler goalposts for the back
branches?  I dunno, but it's a fact that we aren't testing anymore
with any compilers that would complain about unconditional use of
__VA_ARGS__.  So it might be broken already and we wouldn't know it.
(I suspect the last buildfarm animal that would've complained about
this was pademelon, which I retired more than a year ago IIRC.)

> 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.

Yeah, something similar had occurred to me but I didn't write the patch
yet.  Note it should be errmsg_internal(); also, does the default
for errcode come out the same?

> 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.

Oh, that's a good idea that hadn't occurred to me.

> I wonder if its worth additionally ensuring that errcode, errmsg,
> ... are only called within errstart/errfinish?

Meh.  That's wrong at least for errcontext(), and I'm not sure it's
really worth anything to enforce it for the others.

I think the key decision we'd have to make to move forward on this
is to decide whether it's still project style to prefer the extra
parens, or whether we want new code to do without them going
forward.  If we don't want to risk requiring __VA_ARGS__ for the
old branches then I'd vote in favor of keeping the parens as
preferred style, at least till v11 is out of support.  If we do
change that in the back branches then there'd be reason to prefer
to go without parens.  New coders might still be confused about
why there are all these calls with the useless parens, though.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Andres Freund
Hi,

On 2020-03-19 22:32:30 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> writes:
> > I wonder if it'd become a relevant backpatch pain if we started to have
> > some ereports() without the additional parens in 13+.
>
> Yeah, it would be a nasty backpatch hazard.
>
> > Would it perhaps
> > make sense to backpatch just the part that removes the need for the
> > parents, but not the return type changes?
>
> I was just looking into that.  It would be pretty painless to do it
> in v12, but before that we weren't requiring C99.  Having said that,
> trolling the buildfarm database shows exactly zero active members
> that don't report having __VA_ARGS__, in the branches where we test
> that.  (And pg_config.h.win32 was assuming that MSVC had that, too.)
>
> Could we get away with moving the compiler goalposts for the back
> branches?  I dunno, but it's a fact that we aren't testing anymore
> with any compilers that would complain about unconditional use of
> __VA_ARGS__.  So it might be broken already and we wouldn't know it.

FWIW, I did grep for unprotected uses, and  didn't find anything.


> (I suspect the last buildfarm animal that would've complained about
> this was pademelon, which I retired more than a year ago IIRC.)

I guess a query that searches the logs backwards for animals without
__VA_ARGS__ would be a good idea?


> > 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.
>
> Yeah, something similar had occurred to me but I didn't write the patch
> yet.  Note it should be errmsg_internal();

Oh, right.


> also, does the default for errcode come out the same?

I think so - there's no distinct code setting sqlerrcode in
elog_start/finish. That already relied on errstart().


> > I wonder if its worth additionally ensuring that errcode, errmsg,
> > ... are only called within errstart/errfinish?
>
> Meh.  That's wrong at least for errcontext(), and I'm not sure it's
> really worth anything to enforce it for the others.

Yea, I'm not convinced either. Especially after changing the err* return
types to void, as that presumably will cause errors with a lot of
incorrect parens, e.g. about a function with void return type as an
argument to errmsg().


> I think the key decision we'd have to make to move forward on this
> is to decide whether it's still project style to prefer the extra
> parens, or whether we want new code to do without them going
> forward.  If we don't want to risk requiring __VA_ARGS__ for the
> old branches then I'd vote in favor of keeping the parens as
> preferred style, at least till v11 is out of support.

Agreed.


> If we do change that in the back branches then there'd be reason to
> prefer to go without parens.  New coders might still be confused about
> why there are all these calls with the useless parens, though.

That seems to be an acceptable pain, from my POV.

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Tom Lane-2
Andres Freund <[hidden email]> writes:
> On 2020-03-19 22:32:30 -0400, Tom Lane wrote:
>> Could we get away with moving the compiler goalposts for the back
>> branches?  I dunno, but it's a fact that we aren't testing anymore
>> with any compilers that would complain about unconditional use of
>> __VA_ARGS__.  So it might be broken already and we wouldn't know it.

> FWIW, I did grep for unprotected uses, and  didn't find anything.

Yeah, I also grepped the v11 branch for that.

>> (I suspect the last buildfarm animal that would've complained about
>> this was pademelon, which I retired more than a year ago IIRC.)

> I guess a query that searches the logs backwards for animals without
> __VA_ARGS__ would be a good idea?

I did a more wide-ranging scan, looking at the 9.4 branch as far back
as 2015.  Indeed, pademelon is the only animal that ever reported
not having __VA_ARGS__ in that timeframe.

So I've got mixed emotions about this.  On the one hand, it seems
quite unlikely that anyone would ever object if we started requiring
__VA_ARGS__ in the back branches.  On the other hand, it's definitely
not project policy to change requirements like that in minor releases.
Also the actual benefit might not be much.  If anyone did mistakenly
back-patch a fix that included a paren-free ereport, the buildfarm
would point out the error soon enough.

I thought for a little bit about making the back branches define ereport
with "..." if HAVE__VA_ARGS and otherwise not, but if we did that then
the buildfarm would *not* complain about paren-free ereports in the back
branches.  I think that would inevitably lead to there soon being some,
so that we'd effectively be requiring __VA_ARGS__ anyway.  (I suppose
I could resurrect pademelon ... but who knows whether that old war
horse will keep working for another four years till v11 is retired.)

On balance I'm leaning towards keeping the parens as preferred style
for now, adjusting v12 so that the macro will allow paren omission
but we don't break ABI, and not touching the older branches.  But
if there's a consensus to require __VA_ARGS__ in the back branches,
I won't stand in the way.

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: Missing errcode() in ereport

Alvaro Herrera-9
In reply to this post by Tom Lane-2
On 2020-Mar-19, Tom Lane wrote:

> I think the key decision we'd have to make to move forward on this
> is to decide whether it's still project style to prefer the extra
> parens, or whether we want new code to do without them going
> forward.  If we don't want to risk requiring __VA_ARGS__ for the
> old branches then I'd vote in favor of keeping the parens as
> preferred style, at least till v11 is out of support.  If we do
> change that in the back branches then there'd be reason to prefer
> to go without parens.  New coders might still be confused about
> why there are all these calls with the useless parens, though.

It seems fine to accept new code in pg14 without the extra parens.  All
existing committers are (or should be) used to the style with the
parens, so it's unlikely that we'll mess up when backpatching bugfixes;
and even if we do, the buildfarm would alert us to that soon enough.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


12