Re: Allow escape in application_name - Mailing list pgsql-hackers

At Mon, 27 Sep 2021 04:10:50 +0000, "kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com> wrote in 
> I'm sorry for sending a bad patch...

Thank you for the new version, and sorry for making the discussion go
back and forth:p

> > + * Note: StringInfo datatype cannot be accepted
> > + * because elog.h should not include postgres-original header file.
> > 
> > How about moving the function to guc.c from elog.c because it's for
> > the parameters, i.e., log_line_prefix and postgres_fdw.application_name?
> > This allows us to use StringInfo in the function?
> 
> Yeah, StringInfo can be used in guc.c. Hence moved it.
> Some variables and functions for timestamp became non-static function,
> because they were used both normal logging and log_line_prefix.
> I think their name is not too generic so I did not fix them.
> 
> > +                parse_pgfdw_appname(buf, values[i]);
> > +                /*
> > +                 * Note that appname may becomes an empty
> > string
> > +                 * if an input string has wrong format.
> > +                 */
> > +                values[i] = *buf;
> > 
> > If postgres_fdw.application_name contains only invalid escape characters like
> > "%b", parse_pgfdw_appname() returns an empty string. We discussed
> > there are four options to handle this case and we concluded (4) is better.
> > Right? But ISTM that the patch uses (2).
> > 
> > > (1) Use the original postgres_fdw.application_name like "%b"
> > > (2) Use the application_name of the server object (if set)
> > > (3) Use fallback_application_name
> > > (4) Use empty string as application_name
> 
> Yeah, currently my patch uses case (2). I tried to implement (4),
> but I found that libpq function(may be conninfo_array_parse()) must be modified in order to that.
> We moved the functionality to libpq layer because we want to avoid some side effects,
> so we started to think case (4) might be wrong.
> 
> Now we propose the following senario:
> 1. Use postgres_fdw.application_name when it is set and the parsing result is not empty
> 2. If not, use the foreign-server option when it is set and the parsing result is not empty
> 3. If not, use fallback_application_name
> 
> How do you think?

I think we don't have a predecessor of the case like this where a
behavior is decided from object option and GUC.

I'm a bit uncomfortable with .conf configuration overrides server
options, but I also think in-session-set GUC should override server
options.  So, it's slightly against POLA but from the standpoint of
usability +0.5 to that prioritization since I cannot come up with a
better idea.


I thought it is nice to share process_format_string but the function
is too tightly coupled with ErrorData detail as you pointed off-list.
So I came to think we cannot use the function from outside.  It could
be a possibility to make the function be just a skeleton that takes a
list of pairs of an escaped character and the associated callback
function but that is apparently too-much complex.  (It would be worth
doing if we share the same backbone processing with archive_command,
restore_command, recover_end_command and so on, but that is
necessarily accompanied by the need to change the behavior either
log_line_prefix or others.)

I personally don't care even if this feature implements
padding-reading logic differently from process_format_string, but if
we don't like that, I would return to suggest using strtol in both
functions.

As Fujii-san pointed upthread, strtol behaves a bit different way than
we expect here, but that can be handled by checking the starting
characters.

>        if (*p == '-' ? isdigit(p[1]) : isdigit(p[0]))
>        {
>            char *endptr;
>            padding = strtol(p, &endptr, 10);
>            if (p == endptr)
>                break;
>            p = endptr;
>        }
>        else
>            padding = 0;

The code gets a bit more complex but simplification by removing the
helper function wins. strtol is slower than the original function but
it can be thought in noise-level? isdigit on some platforms seems
following locale, but it is already widely used for example while
numeric parsing so I don't think that matters. (Of course we can get
rid of isdigit by using bare comparisons.)

I think it can be a separate preparatory patch of this patch.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center