Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors - Mailing list pgsql-hackers

From Fabien COELHO
Subject Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
Date
Msg-id alpine.DEB.2.21.1807081014260.17811@lancre
Whole thread Raw
In response to Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors  (Alvaro Herrera <alvherre@2ndquadrant.com>)
List pgsql-hackers
Hello Alvaro,

> For context: in the backend, elog() is only used for internal messages
> (i.e. "can't-happen" conditions), and ereport() is used for user-facing
> messages.  There are many things ereport() has that elog() doesn't, such
> as additional message fields (HINT, DETAIL, etc) that I think could have
> some use in pgbench as well.  If you use elog() then you can't have that.
> [...]

Ok. Then forget elog, but I'm pretty against having a kind of ereport 
which looks greatly overkill to me, because:

  (1) the syntax is pretty heavy, and does not look like a function.

  (2) the implementation allocates a string buffer for the message
      this is greatly overkill for pgbench which only needs to print
      to stderr once.

This makes sense server-side because the generated message may be output 
several times (eg stderr, file logging, to the client), and the 
implementation has to work with cpp implementations which do not handle 
varags (and maybe other reasons).

So I would be in favor of having just a simpler error function. 
Incidentally, one already exists "pgbench_error" and could be improved, 
extended, replaced. There is also "syntax_error".

> One thing that just came to mind is that pgbench uses some src/fe_utils
> stuff.  I hope having ereport() doesn't cause a conflict with that ...

Currently ereport does not exists client-side. I do not think that this 
patch is the right moment to decide to do that. Also, there are some 
"elog" in libpq, but they are out with a "#ifndef FRONTEND".

> BTW I think abort() is not the right thing, as it'll cause core dumps if
> enabled.  Why not just exit(1)?

Yes, I agree and already reported that.

Conclusion:

My current opinion is that I'm pretty against bringing "ereport" to the 
front-end on this specific pgbench patch. I agree with you that "elog" 
would be misleading there as well, for the arguments you developed above.

I'd suggest to have just one clean and simple pgbench internal function to 
handle errors and possibly exit, debug... Something like

   void pgb_error(FATAL, "error %d raised", 12);

Implemented as

   void pgb_error(int/enum XXX level, const char * format, ...)
   {
      test level and maybe return immediately (eg debug);
      print to stderr;
      exit/abort/return depending;
   }

Then if some advanced error handling is introduced for front-end programs, 
possibly through some macros, then it would be time to improve upon that.

-- 
Fabien.


pgsql-hackers by date:

Previous
From: Fabien COELHO
Date:
Subject: Re: Desirability of client-side expressions in psql?
Next
From: Michael Paquier
Date:
Subject: Re: [PATCH] Use access() to check file existence inGetNewRelFileNode().