Re: Split-up ECPG patches - Mailing list pgsql-hackers

On Mon, Aug 03, 2009 at 06:12:43PM +0200, Boszormenyi Zoltan wrote:
> - sqlda support:
>   - sqlda.c now indicates license
>   - #defines inside #if 0 ... #endif are now omitted from sqltypes.h
>   (both per comments from Jaime Casanova)

I still owe you some first thoughts about this part of the patch, although I
didn't run it yet

> *************** ecpg_execute(struct statement * stmt)
> *** 1351,1356 ****
> --- 1409,1435 ----
>                   }
>                   var = var->next;
>               }
> +             else if (var != NULL && var->type == ECPGt_sqlda)
> +             {
> +                 pg_sqlda_t      **_sqlda = (pg_sqlda_t **)var->pointer;
> +                 pg_sqlda_t       *sqlda = *_sqlda;
> + 
> +                 if (!sqlda)
> +                 {
> +                     sqlda = ecpg_build_sqlda_for_PGresult(stmt->lineno, results);
> +                     if (!sqlda)
> +                         status = false;
> +                     else
> +                         *_sqlda = sqlda;
> +                 }
> +                 else if (!ecpg_compare_sqlda_with_PGresult(sqlda, results))
> +                     status = false;
> + 
> +                 if (status == true)
> +                     ecpg_set_sqlda_from_PGresult(stmt->lineno, _sqlda, results);

Please add some ecpg_log output here. The same is doen for a descriptor and for
variables, so it should be doen for sqlda too. Also please add some meaningful
comment as to what the code is supposed to do.

> + pg_sqlda_t *
> + ecpg_build_sqlda_for_PGresult(int line, PGresult *res)
> + {
> +     pg_sqlda_t *sqlda;
> +     pg_sqlvar_t*sqlvar;
> +     char       *fname;
> +     long        size;
> +     int        i;
> + 
> +     size = sizeof(pg_sqlda_t) + PQnfields(res) * sizeof(pg_sqlvar_t);
> +     for (i = 0; i < PQnfields(res); i++)
> +         size += strlen(PQfname(res, i)) + 1;
> +     /* round allocated size up to the next multiple of 8 */
> +     if (size % 8)
> +         size += 8 - (size % 8);

Same here, the question is not *what* does the code accomplish, but *why*.

> + 
> +     sqlda = (pg_sqlda_t *)ecpg_alloc(size, line);
> +     if (!sqlda)
> +     {
> +         ecpg_raise(line, ECPG_OUT_OF_MEMORY, ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
> +         return NULL;
> +     }

ecpg_alloc is being used because it already checks for ENOMEM, no need to do it again.

> + static long
> + ecpg_sqlda_size_round_align(long size, int alignment, int round)
> + {
> +     if (size % alignment)
> +         size += alignment - (size % alignment);
> +     size += round;
> +     return size;
> + }

Another implementation of the same code we saw a few lines ago?

> + static long
> + ecpg_sqlda_size_align(long size, int alignment)
> + {
> +     if (size % alignment)
> +         size += alignment - (size % alignment);
> +     return size;
> + }

And yet another one? Sure I see that the above function has an additional add
command, I just wonder why we need the alignment three times.

> +         sqlda = realloc(sqlda, size);

We have ecpg_realloc().

> *************** get_char_item(int lineno, void *var, enu
> *** 225,230 ****
> --- 226,237 ----
>       return (true);
>   }
>   
> + #define RETURN_IF_NO_DATA    if (ntuples < 1) \
> +                 { \
> +                     ecpg_raise(lineno, ECPG_NOT_FOUND, ECPG_SQLSTATE_NO_DATA, NULL); \
> +                     return (false); \
> +                 }
> + 

Could you please explain why you removed this test for some queries? Is there a
bug in there?

Michael
-- 
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo/Skype: michaelmeskes, Jabber: meskes@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!