Postgres Pro
Downloads
Home   >   mailing lists

Thread: Remove post-increment in function quote_identifier of pg_upgrade

Remove post-increment in function quote_identifier of pg_upgrade

From
Vaibhav Dalvi
Date:
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.

Find an attached patch which does the same. This can be backported till v96.

Thanks & Regards,
Vaibhav Dalvi
image.png

Attachment

Re: Remove post-increment in function quote_identifier of pg_upgrade

From
Justin Pryzby
Date:
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;
>  }

Re: Remove post-increment in function quote_identifier of pg_upgrade

From
Tom Lane
Date:
Justin Pryzby <pryzby@telsasoft.com> writes:
> On Thu, Apr 29, 2021 at 06:35:28PM +0530, Vaibhav Dalvi wrote:
>> 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 ++.

Yeah: if the increment actually costs something, I'd expect the compiler
to optimize it away.  But on a lot of machine architectures, a pointer
post-increment is basically free anyhow.

> 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.

I agree --- cosmetically, this change isn't an improvement.

(On the other hand, if it were written the other way already, I'd also
argue to leave it like that.  Basically, this sort of change is just not
worth troubling over.  It doesn't improve things meaningfully and it
creates back-patching hazards.)

            regards, tom lane