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.