On Thu, Apr 29, 2021 at 06:35:28PM +0530, Vaibhav Dalvi wrote:
> Hi,
>
> The function quote_identifier has extra post-increment operation as
> highlighted below,
>
> char *
> quote_identifier(const char *s)
> {
> char *result = pg_malloc(strlen(s) * 2 + 3);
> char *r = result;
>
> *r++ = '"';
> while (*s)
> {
> if (*s == '"')
> *r++ = *s;
> *r++ = *s;
> s++;
> }
> *r++ = '"';
> **r++ = '\0';*
>
> return result;
> }
>
> I think *r = '\0' is enough here. Per precedence table the precedence of
> postfix increment operator is higher. The above statement increments 'r'
> pointer address but returns the original un-incremented pointer address,
> which is then dereferenced. Correct me if I am wrong here.
>
> If my understanding is correct then '++' is not needed in the
> above highlighted statement which is leading to overhead.
I don't think the integer increment during pg_upgrade is a meaningful overhead.
You could check the compiler's assembly output it may be the same even without
the ++.
I'd suggest to leave it as it's currently written, since the idiom on every
other line is *r++ = ..., it'd be strange to write it differently here, and
could end up being confusing or copied+pasted somewhere else.
> Find an attached patch which does the same. This can be backported till v96.
In any case, think it would not be backpatched, since it's essentially
cosmetic.
> diff --git a/src/bin/pg_upgrade/util.c b/src/bin/pg_upgrade/util.c
> index fc20472..dc000d0 100644
> --- a/src/bin/pg_upgrade/util.c
> +++ b/src/bin/pg_upgrade/util.c
> @@ -198,7 +198,7 @@ quote_identifier(const char *s)
> s++;
> }
> *r++ = '"';
> - *r++ = '\0';
> + *r = '\0';
>
> return result;
> }
