Re: latest plperl - Mailing list 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