Re: Split-up ECPG patches

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(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!