Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

On 10-06-2018 10:38, Fabien COELHO wrote:
> Hello Marina,

Hello!

>> v9-0003-Pgbench-errors-use-the-ereport-macro-to-report-de.patch
>> - a patch for the ereport() macro (this is used to report client 
>> failures that do not cause an aborts and this depends on the level of 
>> debugging).
> 
> ISTM that abort() is called under FATAL.

If you mean abortion of the client, this is not an abortion of the main 
program.

>> - implementation: if possible, use the local ErrorData structure 
>> during the errstart()/errmsg()/errfinish() calls. Otherwise use a 
>> static variable protected by a mutex if necessary. To do all of this 
>> export the function appendPQExpBufferVA from libpq.
> 
> This patch applies cleanly on top of the other ones (there are minimal
> interactions), compiles cleanly, global & pgbench "make check" are ok.

:-)

> IMO this patch is more controversial than the other ones.
> 
> It is not really related to the aim of the patch series, which could
> do without, couldn't it?

> I'd suggest that it should be an independent submission, unrelated to
> the pgbench error management patch.

I suppose that this is related; because of my patch there may be a lot 
of such code (see v7 in [1]):

-            fprintf(stderr,
-                    "malformed variable \"%s\" value: \"%s\"\n",
-                    var->name, var->svalue);
+            if (debug_level >= DEBUG_FAILS)
+            {
+                fprintf(stderr,
+                        "malformed variable \"%s\" value: \"%s\"\n",
+                        var->name, var->svalue);
+            }

-        if (debug)
+        if (debug_level >= DEBUG_ALL)
              fprintf(stderr, "client %d sending %s\n", st->id, sql);

That's why it was suggested to make the error function which hides all 
these things (see [2]):

There is a lot of checks like "if (debug_level >= DEBUG_FAILS)" with 
corresponding fprintf(stderr..) I think it's time to do it like in the 
main code, wrap with some function like log(level, msg).

> Moreover, it changes pgbench current
> behavior, which might be admissible, but should be discussed clearly.

> The semantics of the existing code is changed, the FATAL levels calls
> abort() and replace existing exit(1) calls. Maybe you want an ERROR
> level as well.

Oh, thanks, I agree with you. And I do not want to change the program 
exit code without good reasons, but I'm sorry I may not know all pros 
and cons in this matter..

Or did you also mean other changes?

> The code adapts/duplicates existing server-side "ereport" stuff and
> brings it to the frontend, where the logging needs are somehow quite
> different.
> 
> I'd prefer to avoid duplication and/or have some code sharing.

I was recommended to use the same interface in [3]:

On elog/errstart: we already have a convention for what ereport() calls 
look like; I suggest to use that instead of inventing your own.

> If it
> really needs to be duplicated, I'd suggest to put all this stuff in
> separated files. If we want to do that, I think that it would belong
> to fe_utils, and where it could/should be used by all front-end
> programs.

I'll try to do it..

> I do not understand why names are changed, eg ELEVEL_FATAL instead of
> FATAL. ISTM that part of the point of the move would be to be
> homogeneous, which suggests that the same names should be reused.

Ok!

> For logging purposes, ISTM that the "elog" macro interface is nicer,
> closer to the existing "fprintf(stderr", as it would not introduce the
> additional parentheses hack for "rest".

I was also recommended to use ereport() instead of elog() in [3]:

With that, is there a need for elog()?  In the backend we have it 
because $HISTORY but there's no need for that here -- I propose to lose 
elog() and use only ereport everywhere.

> I see no actual value in creating on the fly a dynamic buffer through
> plenty macros and functions as the end result is just to print the
> message out to stderr in the end.
> 
>   errfinishImpl: fprintf(stderr, "%s", error->message.data);
> 
> This looks like overkill. From reading the code, this does not look
> like an improvement:
> 
>   fprintf(stderr, "invalid socket: %s", PQerrorMessage(st->con));
> 
> vs
> 
>   ereport(ELEVEL_LOG, (errmsg("invalid socket: %s", 
> PQerrorMessage(st->con))));
> 
> The whole complexity of the server-side interface only make sense
> because TRY/CATCH stuff and complex logging requirements (eg several
> outputs) in the backend. The patch adds quite some code and complexity
> without clear added value that I can see.

> My 0.02€: maybe you just want to turn
> 
>   fprintf(stderr, format, ...);
>   // then possibly exit or abort depending...
> 
> into
> 
>   elog(level, format, ...);
> 
> which maybe would exit or abort depending on level, and possibly not
> actually report under some levels and/or some conditions. For that, it
> could enough to just provide an nice "elog" function.

I agree that elog() can be coded in this way. To use ereport() I need a 
structure to store the error level as a condition to exit.

> In conclusion, which you can disagree with because maybe I have missed
> something... anyway I currently think that:
> 
>  - it should be an independent submission
> 
>  - possibly at "fe_utils" level
> 
>  - possibly just a nice "elog" function is enough, if so just do that.

I hope I answered all this above..

[1] 
https://www.postgresql.org/message-id/453fa52de88477df2c4a2d82e09e461c%40postgrespro.ru
[2] 
https://www.postgresql.org/message-id/20180405180807.0bc1114f%40wp.localdomain
[3] 
https://www.postgresql.org/message-id/20180508105832.6o3uf3npfpjgk5m7%40alvherre.pgsql

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company