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