From: | "Andrew Dunstan" <andrew(at)dunslane(dot)net> |
---|---|
To: | <pgsql-patches(at)postgresql(dot)org> |
Cc: | <plperlng-devel(at)pgfoundry(dot)org> |
Subject: | Re: latest plperl |
Date: | 2004-07-01 14:48:10 |
Message-ID: | 58088.199.90.235.43.1088693290.squirrel@www.dunslane.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-patches |
Joe Conway said:
> Andrew Dunstan wrote:
>> The attached patch (and 2 new files incorporating previous
>> eloglvl.[ch] as before) has the following changes over previously
>> sent patch
>> (fixes all by me):
>
> Some comments below:
>
> --------------------
> In plperl_trigger_build_args(), this looks bogus:
>
> + char *tmp;
> +
> + tmp = (char *) malloc(sizeof(int));
> ...
> + sprintf(tmp, "%d", tdata->tg_trigger->tgnargs);
> + sv_catpvf(rv, ", argc => %s", tmp);
> ...
> + free(tmp);
Doh! Very bogus! sizeof(int)and a malloc to boot ???
I didn't check the trigger code much because it has supposedly been working
for quite a while. I will examine more closely.
>
> I changed it to:
>
> + sv_catpvf(rv, ", argc => %d", tdata->tg_trigger->tgnargs);
>
works for me.
>
> --------------------
> In this section, it appears that empty strings in the tuple will be
> coerced into NULL values:
>
> + plval = plperl_get_elem(hvNew, platt);
>
> + if (plval)
> + {
> + src = plval;
> + if (strlen(plval))
> + {
> + modvalues[j] = FunctionCall3(&finfo,
> + CStringGetDatum(src),
> + ObjectIdGetDatum(typelem),
> +
> Int32GetDatum(tupdesc->attrs[atti]->atttypmod)); +
> modnulls[j] = ' ';
> + }
> + else
> + {
> + modvalues[i] = (Datum) 0;
> + modnulls[j] = 'n';
> + }
> + }
> + plval = NULL;
>
> Shouldn't that look more like this?
>
> + plval = plperl_get_elem(hvNew, platt);
>
> + if (plval)
> + {
> + modvalues[j] = FunctionCall3(&finfo,
> + CStringGetDatum(plval),
> + ObjectIdGetDatum(typelem),
> + Int32GetDatum(tupdesc->attrs[atti]->atttypmod)); +
> modnulls[j] = ' ';
> + }
> + else
> + {
> + modvalues[i] = (Datum) 0;
> + modnulls[j] = 'n';
> + }
>
Yes, except that that [i] looks wrong too. Surely it should be [j]. And with
this change decl of src appears redundant.
I will do some checking on these changes, but with those caveats they look
good to me.
Do you need a revised patch?
cheers
andrew
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2004-07-01 15:00:50 | Re: [PATCH] s_lock support for win32 |
Previous Message | Claudio Natoli | 2004-07-01 07:39:51 | Re: [PATCH] s_lock support for win32 |