Hello,

I attach patch for more strict bind parameters count checking.
It could avoid some bad behaviour (driver doesn't fail in some
situations).

Please review and comment

Luf


---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Hi Ludek, thanks to your recent effort to improve the psqlodbc driver.
Please let me comment on your patch a little.

Ludek Finstrle wrote:
> Hello,
>
> I attach patch for more strict bind parameters count checking.
> It could avoid some bad behaviour (driver doesn't fail in some
> situations).
>
> Please review and comment
>
> Luf
>
>
> ------------------------------------------------------------------------
>
> diff -c psqlodbc.orig\convert.c psqlodbc\convert.c
> *** psqlodbc.orig\convert.c Mon Dec 05 20:20:53 2005
> --- psqlodbc\convert.c Wed Dec 14 00:58:24 2005
> ***************
> *** 1858,1866 ****
> QB_Destructor(qb);
> return SQL_ERROR;
> }
> ! if (marker_count > 0)
> {
> ! if (ipdopts && (ipdopts->allocated == marker_count))
> {
> CVT_APPEND_CHAR(qb, '(');
> for (i = 0; i < marker_count; i++)
> --- 1858,1866 ----
> QB_Destructor(qb);
> return SQL_ERROR;
> }
> ! if (ipdopts->allocated == marker_count)

Why are you using the operator '==' not '>=' ?
AFAIK there's no such limitation.

> {
> ! if (marker_count > 0)
> {
> CVT_APPEND_CHAR(qb, '(');
> for (i = 0; i < marker_count; i++)
> ***************
> *** 1871,1883 ****
> }
> CVT_APPEND_CHAR(qb, ')');
> }
> ! else
> ! {
> ! SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter markers");
> ! SC_set_sqlstate(stmt, "07002");
> ! QB_Destructor(qb);
> ! return SQL_ERROR;
> ! }
> }
> CVT_APPEND_STR(qb, " as ");
> for (qp->opos = 0; qp->opos < qp->stmt_len; qp->opos++)
> --- 1871,1884 ----
> }
> CVT_APPEND_CHAR(qb, ')');
> }
> ! }
> ! else
> ! {
> ! CC_clear_error(stmt->hdbc);
> ! SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter markers");
> ! SC_set_sqlstate(stmt, "07002");
> ! QB_Destructor(qb);
> ! return SQL_ERROR;
> }
> CVT_APPEND_STR(qb, " as ");
> for (qp->opos = 0; qp->opos < qp->stmt_len; qp->opos++)
> diff -c psqlodbc.orig\execute.c psqlodbc\execute.c
> *** psqlodbc.orig\execute.c Tue Dec 06 21:53:30 2005
> --- psqlodbc\execute.c Wed Dec 14 01:18:40 2005
> ***************
> *** 136,141 ****
> --- 136,143 ----
> StatementClass *stmt = (StatementClass *) hstmt;
> RETCODE result;
> CSTR func = "PGAPI_ExecDirect";
> + IPDFields *ipdopts;
> + SWORD marker_count;
>
> mylog("%s: entering...\n", func);
>
> ***************
> *** 156,161 ****
> --- 158,175 ----
>
> mylog("**** %s: hstmt=%u, statement='%s'\n", func, hstmt, stmt->statement);
>
> + /* Check bind parameters count */
> + if (SQL_SUCCESS != PGAPI_NumParams(stmt, &marker_count))
> + return SQL_ERROR;
> + ipdopts = SC_get_IPDF(stmt);
> + if (ipdopts->allocated != marker_count)

Why are you using '!=' not '<' ?

> + {
> + CC_clear_error(stmt->hdbc);
> + SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter markers");
> + SC_set_sqlstate(stmt, "07002");
> + return SQL_ERROR;
> + }
> +
> /*
> * If an SQLPrepare was performed prior to this, but was left in the
> * premature state because an error occurred prior to SQLExecute then
> ***************
> *** 196,206 ****
> --- 210,234 ----
> APDFields *apdopts;
> IPDFields *ipdopts;
> BOOL prepare_before_exec = FALSE;
> + SWORD marker_count;
> +
>
> *exec_end = FALSE;
> conn = SC_get_conn(stmt);
> mylog("%s: copying statement params: trans_status=%d, len=%d, stmt='%s'\n", func, conn->transact_status, strlen(stmt->statement), stmt->statement);
>
> + /* Check bind parameters count */
> + if (SQL_SUCCESS != PGAPI_NumParams(stmt, &marker_count))
> + return SQL_ERROR;
> + ipdopts = SC_get_IPDF(stmt);
> + if (ipdopts->allocated != marker_count)

The same comment as the previous one.
In addtion, the check is not appropriate because Exec_with_parameter_resolved
could be called with insufficient parameters.

> + {
> + CC_clear_error(stmt->hdbc);
> + SC_set_error(stmt, STMT_COUNT_FIELD_INCORRECT, "The # of binded parameters < the # of parameter markers");
> + SC_set_sqlstate(stmt, "07002");
> + return SQL_ERROR;
> + }
> +
> /* save the cursor's info before the execution */
> cursor_type = stmt->options.cursor_type;
> scroll_concurrency = stmt->options.scroll_concurrency;


---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

> Hi Ludek, thanks to your recent effort to improve the psqlodbc driver.
> Please let me comment on your patch a little.

Hello,

your comments are welcome.

> > ! if (ipdopts->allocated == marker_count)
>
> Why are you using the operator '==' not '>=' ?
> AFAIK there's no such limitation.

We have problems with Visual FoxPro. It calls SQLCancel without
FreeStmt(SQL_RESET_PARAMS). It leads to failure becouse next
ExecDirect with fewer parameters in some cases think that it
has data_at_exec (which was parameters from previous).

So I think this could be good prevent againist driver failure.

> > + if (ipdopts->allocated != marker_count)
>
> Why are you using '!=' not '<' ?

It's the same as previous (only negated).

> > + ipdopts = SC_get_IPDF(stmt);
> > + if (ipdopts->allocated != marker_count)
>
> The same comment as the previous one.
> In addtion, the check is not appropriate because Exec_with_parameter_resolved
> could be called with insufficient parameters.

I take a look at it again.

Thanks for comments

Luf

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Ludek Finstrle wrote:
>>Hi Ludek, thanks to your recent effort to improve the psqlodbc driver.
>>Please let me comment on your patch a little.
>
>
> Hello,
>
> your comments are welcome.
>
>
>>>! if (ipdopts->allocated == marker_count)
>>
>>Why are you using the operator '==' not '>=' ?
>>AFAIK there's no such limitation.
>
>
> We have problems with Visual FoxPro. It calls SQLCancel without
> FreeStmt(SQL_RESET_PARAMS). It leads to failure becouse next
> ExecDirect with fewer parameters in some cases think that it
> has data_at_exec (which was parameters from previous).

Isn't it the right solution for the problem to check data_at_exec appropriately ?

regards, Hiroshi Inoue

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

> >>>! if (ipdopts->allocated == marker_count)
> >>
> >>Why are you using the operator '==' not '>=' ?
> >>AFAIK there's no such limitation.
> >
> >We have problems with Visual FoxPro. It calls SQLCancel without
> >FreeStmt(SQL_RESET_PARAMS). It leads to failure becouse next
> >ExecDirect with fewer parameters in some cases think that it
> >has data_at_exec (which was parameters from previous).
>
> Isn't it the right solution for the problem to check data_at_exec
> appropriately ?

You're absolutely right. Thank you for showing me another view.
This is bad patch. I'll try another way.

Luf

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

> >We have problems with Visual FoxPro. It calls SQLCancel without
> >FreeStmt(SQL_RESET_PARAMS). It leads to failure becouse next
> >ExecDirect with fewer parameters in some cases think that it
> >has data_at_exec (which was parameters from previous).
>
> Isn't it the right solution for the problem to check data_at_exec
> appropriately ?

Please Hiroshi could you take a look on solution for data_at_exec check?
I see no problem but I didn't see it in previous patch also.

Thanks a lot

Luf


---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

Ludek Finstrle wrote:

>>>We have problems with Visual FoxPro. It calls SQLCancel without
>>>FreeStmt(SQL_RESET_PARAMS). It leads to failure becouse next
>>>ExecDirect with fewer parameters in some cases think that it
>>>has data_at_exec (which was parameters from previous).
>>>
>>>
>>Isn't it the right solution for the problem to check data_at_exec
>>appropriately ?
>>
>>
>
>Please Hiroshi could you take a look on solution for data_at_exec check?
>I see no problem but I didn't see it in previous patch also.
>
>Thanks a lot
>
>Luf
>
>
>------------------------------------------------------------------------
>
>diff -c psqlodbc.orig\execute.c psqlodbc\execute.c
>*** psqlodbc.orig\execute.c Tue Dec 06 21:53:30 2005
>--- psqlodbc\execute.c Fri Dec 16 18:11:16 2005
>***************
>*** 542,547 ****
>--- 542,548 ----
> UInt4 offset = apdopts->param_offset_ptr ? *apdopts->param_offset_ptr : 0;
> Int4 bind_size = apdopts->param_bind_type;
> Int4 current_row = stmt->exec_current_row < 0 ? 0 : stmt->exec_current_row;
>+ SWORD param_count;
>
> /*
> * Increment the number of currently processed rows
>***************
>*** 549,555 ****
> if (ipdopts->param_processed_ptr)
> (*ipdopts->param_processed_ptr)++;
> stmt->data_at_exec = -1;
>! for (i = 0; i < apdopts->allocated; i++)
> {
> Int4 *pcVal = apdopts->parameters[i].used;
>
>--- 550,559 ----
> if (ipdopts->param_processed_ptr)
> (*ipdopts->param_processed_ptr)++;
> stmt->data_at_exec = -1;
>! /* Check bind parameters count */
>! if (SQL_SUCCESS != PGAPI_NumParams(stmt, &param_count))
>! return SQL_ERROR;
>! for (i = 0; i < param_count; i++)
>
>

Looks pretty good to me.
It's more safe to adjust the param_count in case of apdopts->allocated <
param_count.
Also it's safe to change the following code in PGAPI_ParamData.

/* At least 1 data at execution parameter, so Fill in the token value */
for (; i < apdopts->allocated; i++)

regars, Hiroshi Inoue.


---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

> Looks pretty good to me.

Very good.

> It's more safe to adjust the param_count in case of apdopts->allocated <
> param_count.

Thanks fot pointing it.

> Also it's safe to change the following code in PGAPI_ParamData.
>
> /* At least 1 data at execution parameter, so Fill in the token value */
> for (; i < apdopts->allocated; i++)

I implement it both. New patch attached.
I also change older test for param number from == to >= and
comment out some not used code.

Thank you very much again Hiroshi

Luf

P.S. Dave can I commit it if there it no negative answer?


---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org