Re: [PATCH] Add use of asprintf()

On Tue, 2013-09-17 at 15:13 +0500, Asif Naeem wrote:

> 1. It seems that you have used strdup() on multiple places in the
> patch, e.g. in the below code snippet is it going to lead crash if
> newp->ident is NULL because of strdup() failure ?
> 
>         static EPlan *
>         find_plan(char *ident, EPlan **eplan, int *nplans)
>         {
>         ...
>              newp->ident = strdup(ident);
>         ...
>         }
> 
The previous code used unchecked malloc, so this doesn't change
anything.  It's only example code anyway.  (The use of malloc instead of
palloc is dubious anyway.)

> 
> 2. Can we rely on return value of asprintf() instead of recompute
> length as strlen(cmdbuf) ?
> 
>                   if (asprintf(&cmdbuf, "COMMENT ON LARGE OBJECT %u IS
>         '", loid) < 0)
>                        return fail_lo_xact("\\lo_import",
>         own_transaction);
>                   bufptr = cmdbuf + strlen(cmdbuf);
> 
Yes, good point.

> 3. There seems naming convention for functions like pfree (that seems
> similar to free()), pstrdup etc; but psprintf seems different than
> sprintf. Can't we use pg_asprintf instead in the patch, as it seems
> that both (psprintf and pg_asprintf) provide almost same
> functionality ?

pg_asprintf uses malloc, psprintf uses palloc, so they have to be
different functions.

The naming convention where

psomething = something with memory context management
pg_something = something with error checking

isn't very helpful, but it's what we have.  Better ideas welcome.

> 
> 5. It seems that in the previously implementation, error handling was
> not there, is it appropriate here that if there is issue in
> duplicating environment, quit the application ? i.e.
> 
> 
>         /*
>          * Handy subroutine for setting an environment variable "var"
>         to "val"
>          */
>         static void
>         doputenv(const char *var, const char *val)
>         {
>              char        *s;
>              pg_asprintf(&s, "%s=%s", var, val);
>              putenv(s);
>         }
> 
I think so, yes.