Re: Redacting information from logs

Hi,

On 2019-08-05 14:26:44 -0700, Jeff Davis wrote:
> On Sun, 2019-08-04 at 11:17 -0700, Andres Freund wrote:
> > I'm imagining something like
> > 
> > #define pg_printf(fmt, ...) \
> >     do { \
> >         if ( __builtin_constant_p(fmt)) \
> >         { \
> >             static processed_fmt processed_fmt_ = {.format = fmt}; \
> >             if (unlikely(!processed_fmt_.parsed)) \
> >                preprocess_format_string(&processed_fmt_) \
> >             pg_printf_processed(&processed_fmt_, __VA_ARGS__); \
> >         } \
> >         else \
> >             pg_printf_unprocessed(fmt, __VA_ARGS__); \
> >     } while (0) \
> 
> What would you do in the preprocessing exactly? Create a list of
> indexes into the string where the format codes are?

Yea, basically. If you look at snprint.c's dopr(), there's a fair bit of
parsing going on that is going to stay the same from call to call in
nearly all cases (and the cases where not we possibly ought to fix). And
things like the $ processing for argument order, or having named
arguments as I suggest, make that more pronounced.


> > If we have enough information to pass to the logging hook, we don't
> > even
> > need to define how all of this is going to look like exactly
> > (although
> > I'd probably argue that a logfmt or json target ought to be in core).
> 
> I think I see where you are going with this now: it is almost
> orthogonal to your new-style format strings ( %{foo}s ), but not quite.
> 
> You're suggesting that we save all of the arguments, along with some
> annotation, in the ErrorData struct, and then let emit_log_hook sort it
> out (perhaps by constructing some JSON message, perhaps translating the
> message_id, etc.).

Right.


> I like the idea, but still playing with the ergonomics a bit, and how
> it interacts with various message parts (msg, hint, detail, etc.). If
> we had the name-based format strings, then the message parts could all
> share a common set of parameters; but with the current positional
> format strings, I think each message part would need its own set of
> arguments.

Right, I think that's a good part of where I was coming from.


> Maybe we can make the parameters common to different message parts by
> using an integer index to reference the parameter, like:
> 
>    ereport(ERROR,
>            (errcode(ERRCODE_UNIQUE_VIOLATION),
>             errmsg_rich("duplicate key value violates unique constraint
> \"%s\"", 1 /* second errparam */),
>             errdetail_rich("Key %s already exists.", 0 /* first
> errparam */),
>             errparam("key", MSGUSERDATA, key_desc))
>             errparam("constraintname", MSGDEFAULT, 
>                      RelationGetRelationName(rel)))
>            );
> 
> Not quite ideal, but might get us closer.

If we insist that errmsg_rich/errdetail_rich may not have parameters,
then they can just use the same set of arguments, without any of this,
at the cost of sometimes more complicated % syntax (i.e. %1$d to refer
to the first argument).

I think the probable loss of gcc format warnings would be the biggest
issue with this whole proposal, and potential translator trouble the
biggest impediment for named parameters.

Greetings,

Andres Freund