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

Michael Meskes írta:
> 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
>   

Okay, answers below...

>> *************** 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.
>   

Will add the ecpg_log(). What the code does is:
- sets up a minimal SQLDA on the first call (called with NULL ptr), so the field types and field names and some other
propertiesare in place. doesn't do further work if out of memory
 
- upon subsequent calls it checks whether a "compatible" sqlda was
passed in, i.e. same number of fields, same types, etc. Sanity check. Doesn't do
further work if the check fails.
- fills in the field values

>> + 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*.
>   

Arbitrary alignment, maybe not needed, Will check.

>> + 
>> +     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.
>   

Okay, thanks.

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

Padding to the position of the "current" variable.

>> +     size += round;
>>     

Position to the next variable.

>> +     return size;
>> + }
>>     
>
> Another implementation of the same code we saw a few lines ago?
>   

It's called with different alignments and padding at several places.
Used for computing the offset of the next variable.

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

Padding only.

>> +     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.
>   

The fixed size alignment can be done with a call
to ecpg_sqlda_size_round_align(), sure.

>> +         sqlda = realloc(sqlda, size);
>>     
>
> We have ecpg_realloc().
>   

Okay, thanks.

>> *************** 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?
>   

DESCRIBE can be used on queries not returning tuples.
This check at the beginning of the function prevented it.
I only added the check back to two or three places where
tuples were actually processed. Maybe I missed places.

Best regards,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/