Re: latest plperl

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