From: | "kuroda(dot)hayato(at)fujitsu(dot)com" <kuroda(dot)hayato(at)fujitsu(dot)com> |
---|---|
To: | 'Kyotaro Horiguchi' <horikyota(dot)ntt(at)gmail(dot)com> |
Cc: | "masao(dot)fujii(at)oss(dot)nttdata(dot)com" <masao(dot)fujii(at)oss(dot)nttdata(dot)com>, "ikedamsh(at)oss(dot)nttdata(dot)com" <ikedamsh(at)oss(dot)nttdata(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "tgl(at)sss(dot)pgh(dot)pa(dot)us" <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | RE: Allow escape in application_name |
Date: | 2021-09-08 12:32:50 |
Message-ID: | TYAPR01MB5866EC4B7F0B21451D836A15F5D49@TYAPR01MB5866.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Dear Horiguchi-san,
Thank you for reviewing! I attached the fixed version.
> Is "the previous comment" "the comment above"?
Yeah, fixed.
> + for (i = n -1; i >= 0; i--)
>
> You might want a space between - and 1.
Fixed.
> +parse_application_name(StringInfo buf, const char *name)
>
> The name seems a bit too generic as it is a function only for
> postgres_fdw.
Indeed. I renamed to parse_pgfdw_appname().
> + /* must be a '%', so skip to the next char */
> + p++;
> + if (*p == '\0')
> + break; /* format error -
> ignore it */
>
> I'm surprised by finding that undefined %-escapes and stray % behave
> differently between archive_command and log_line_prefix. I understand
> this behaves like the latter.
Indeed. pgarch_archiveXlog() treats undefined escapes as nothing special,
but log_line_prefix() stop parsing immediately.
They have no description about it in docs.
I will not treat it in this thread and follow log_line_prefix(),
but I agree it is strange.
> + const char *username =
> MyProcPort->user_name;
>
> I'm not sure but even if user_name doesn't seem to be NULL, don't we
> want to do the same thing with %u of log_line_prefix for safety?
> Namely, setting [unknown] if user_name is NULL or "". The same can be
> said for %d.
I think they will be never NULL in current implementation,
but your suggestion is better. Checks were added in %a, %u and %d.
> + * process_padding --- helper function for processing the format
> + * string in log_line_prefix
>
> Since this is no longer a static helper function for a specific
> function, the name and the comment should be more descriptive.
>
> That being said, in the first place the function seems reducible
> almost to a call to atol. By a quick measurement the function is about
> 38% faster (0.024us/call(the function) vs 0.039us/call(strtol) so I'm
> not sure we want to replace process_log_prefix_padding(), but we don't
> need to reuse the function in parse_application_name since it is
> called only once per connection.
My first impression is that they use the function
because they want to know the end of sequence and do error-handling,
but I agree this can be replaced by strtol().
I changed the function to strtol() and restored process_log_prefix_padding().
How do you think? Does it follow your suggestion?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Attachment | Content-Type | Size |
---|---|---|
v11_0002_allow_escapes.patch | application/octet-stream | 11.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2021-09-08 12:41:27 | Re: Schema variables - new implementation for Postgres 15 |
Previous Message | Peter Smith | 2021-09-08 12:16:37 | Re: row filtering for logical replication |