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: mes...@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
