Re: Allow escape in application_name

At Fri, 17 Dec 2021 14:50:37 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
> > FWIW, I don't understand why we care of the case where the function
> > itself changes its mind to set values[i] to null
>
> Whether we add this check or not, the behavior is the same, i.e., that
> setting value is not used. But by adding the check we can avoid
> unnecessary call of process_pgfdw_appname() when the value is
> NULL. OTOH, of course we can also remove the check. So I'm ok to
> remove that if you're thinking it's more consistent to remove that.

That depends. It seems to me defGetString is assumed to return a valid
pointer, since (AFAIS) all of the callers don't check against NULL. On
the other hand the length of the string may be zero. Actually
check_conn_params() called just after makes the same assumption (only
on "password", though). On the other hand PQconnectdbParams assumes
NULL value as not-set.

So assumption on the NULL value differs in some places and at least
postgres_fdw doesn't use NULL to represent "not exists".

Thus rewriting the code we're focusing on like the following would
make sense to me.

> if (strcmp(keywords[i], "application_name") == 0)
> {
> values[i] = process_pgfdw_appname(values[i]);
>
> /*
> * Break if we have a non-empty string. If we end up failing with
> * all candidates, fallback_application_name would work.
> */
> if (appanme[0] != '\0')
> break;
> }

> Probably now it's good chance to revisit this issue. As I commented at
> [1], at least for me it's intuitive to use empty string rather than
> "[unknown]" when appname or username, etc was NULL or '\0'. To
> implement this behavior, I argued to remove the check itself. But
> there are several options:

Thanks for revisiting.

> #1. use "[unknown]"
> #2. add the check but not use "[unknown]"
> #3. don't add the check (i.e., what the current patch does)
>
> For now, I'm ok to choose #2 or #3.

As I said before, given that we don't show "unkown" or somethig like
as the fallback, I'm fine with not having a NULL check since anyway it
bumps into SEGV immediately. In short I'm fine with #3 here.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center