You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-dev@db.apache.org by Deepa Remesh <dr...@gmail.com> on 2006/02/23 23:11:42 UTC
[DRDA] Question about DRDAStatement.initialize() method
Hi,
For my DERBY-1002 patch, I have added a method DRDAStatement.reuse()
which resets all fields in this class.
/**
* Clean up statements and result set for reuse.
* This method should reset all members of this class except the
* following which will be set at initial creation or set
explicitly in the code:
* database, pkgnamcsn, pkgcnstkn, pkgid, pkgsn
*
* @throws SQLException
*/
protected void reuse() throws SQLException {
...
}
I was just going through all methods in DRDAStatement and found that
there is a method called initialize() which does this:
/**
* Initialize for reuse
*/
protected void initialize()
{
setTypDefValues();
}
where setTypDefValues() does this:
/**
* set TypDef values
*
*/
protected void setTypDefValues()
{
// initialize statement values to current database values
this.typDefNam = database.typDefNam;
this.byteOrder = database.byteOrder;
this.ccsidSBC = database.ccsidSBC;
this.ccsidDBC = database.ccsidDBC;
this.ccsidMBC = database.ccsidMBC;
this.ccsidSBCEncoding = database.ccsidSBCEncoding;
this.ccsidDBCEncoding = database.ccsidDBCEncoding;
this.ccsidMBCEncoding = database.ccsidMBCEncoding;
}
I have couple of questions:
1. I find it confusing to have a initialize and reuse method with
similar comments. Can initialize be removed since we can call
setTypDefValues directly at places where it is currently called?
2. initialize() is called at called at 5 places in the server code.
Two of these places (DRDAConnThread methods parseEXCSQLIMM and
parseEXCSQLSETobjects) have the following comment:
// initialize statement for reuse
drdaStmt.initialize();
I am not sure about the actual intention of calling initialize() at
these places - whether it is to just reset the typedef values or does
it expect all fields to be reset for reuse. Can someone familiar with
code look at these places to see it is enough to use initialize() at
these places? If this does not look right, I can open another issue
for this.
3. Is my comment for the new reuse method() okay? specifically, the
comment about fields which should not be reset?
"This method should reset all members of this class except the
following which will be set at initial creation or set explicitly in
the code: database, pkgnamcsn, pkgcnstkn, pkgid, pkgsn"
I would appreciate if someone can answer these.
Thanks,
Deepa
Re: [DRDA] Question about DRDAStatement.initialize() method
Posted by Kathey Marsden <km...@sbcglobal.net>.
Deepa Remesh wrote:
>* close() will close and dereference all the instance objects.
>* reset() will reinitialize all the variables (with few exceptions as
>noted in previous mails)
>
>Is this what you suggest?
>
>
>
Yes.
Re: [DRDA] Question about DRDAStatement.initialize() method
Posted by Deepa Remesh <dr...@gmail.com>.
On 2/24/06, Kathey Marsden <km...@sbcglobal.net> wrote:
> Deepa Remesh wrote:
>
> >On 2/24/06, Kathey Marsden <km...@sbcglobal.net> wrote:
> >
> >
> >I will add this comment, slightly modified, because we are now calling
> >only reset() instead of close in Database.newDrdaStatement. I am
> >rerunning tests after a minor change. Meantime, I will upload a draft
> >patch with comments from you and Knut.
> >
> >
> >
> My point though was that we should still keep close() and call both
> close() of the old statement and reset() to reset the statement. Just
> like reset was hidden when it was bundled in close. The new code with
> only reset() hides the fact that the old statement is getting closed.
> So that is why I suggest something like
>
> if (stmt != null) {
> // close the old statement
> stmt.close();
> // reset the statement so we can reuse it for this new statement.
> stmt.reset();
>
> ...
>
Sorry, I did'nt get it when I first read your mail. I just want to
make sure I got it right this time:
* close() will close and dereference all the instance objects.
* reset() will reinitialize all the variables (with few exceptions as
noted in previous mails)
Is this what you suggest?
Thanks,
Deepa
Re: [DRDA] Question about DRDAStatement.initialize() method
Posted by Kathey Marsden <km...@sbcglobal.net>.
Deepa Remesh wrote:
>On 2/24/06, Kathey Marsden <km...@sbcglobal.net> wrote:
>
>
>I will add this comment, slightly modified, because we are now calling
>only reset() instead of close in Database.newDrdaStatement. I am
>rerunning tests after a minor change. Meantime, I will upload a draft
>patch with comments from you and Knut.
>
>
>
My point though was that we should still keep close() and call both
close() of the old statement and reset() to reset the statement. Just
like reset was hidden when it was bundled in close. The new code with
only reset() hides the fact that the old statement is getting closed.
So that is why I suggest something like
if (stmt != null) {
// close the old statement
stmt.close();
// reset the statement so we can reuse it for this new statement.
stmt.reset();
...
Kathey
Re: [DRDA] Question about DRDAStatement.initialize() method
Posted by Deepa Remesh <dr...@gmail.com>.
On 2/24/06, Kathey Marsden <km...@sbcglobal.net> wrote:
>
> >Deepa Remesh <dr...@gmail.com> writes:
> >
> >
> >
> >> DRDAStatement stmt = getDRDAStatement(pkgnamcsn);
> >> if (stmt != null) {
> >> stmt.reset();
> >>
> >>
> [snip]
>
> This is not related to your question but I think there are two things
> going on here with both the old and new code.
> 1) We are closing the old statement
> 2) We are resetting the statement for reuse.
>
> I tend to think that with the new code although we gain more clarity
> that we are resetting this statement for reuse, we lose the fact that
> the old statement is being closed. I think maybe something like this
>
> // Because there is no close statement command in DRDA, the statement
> previously associated
> // with this pkgnamcsn may not have been closed on the server, even
> though it was closed on the client and
> // the section reused. Getting a request for a new statement with the
> same pkgnamcsn is how the
> // server finds out that the statement previously associated with this
> pkgnamcsn has been closed.
> // So when creating a new DRDAStatement for this pkgnamcsn, we
> //1) Retrieve the old statement associated with this pkgnamcsn and
> close it
> // 2) Reset the statement state for use with the new DRDAStatement
I will add this comment, slightly modified, because we are now calling
only reset() instead of close in Database.newDrdaStatement. I am
rerunning tests after a minor change. Meantime, I will upload a draft
patch with comments from you and Knut.
Thanks,
Deepa
Re: [DRDA] Question about DRDAStatement.initialize() method
Posted by Kathey Marsden <km...@sbcglobal.net>.
>Deepa Remesh <dr...@gmail.com> writes:
>
>
>
>> DRDAStatement stmt = getDRDAStatement(pkgnamcsn);
>> if (stmt != null) {
>> stmt.reset();
>>
>>
[snip]
This is not related to your question but I think there are two things
going on here with both the old and new code.
1) We are closing the old statement
2) We are resetting the statement for reuse.
I tend to think that with the new code although we gain more clarity
that we are resetting this statement for reuse, we lose the fact that
the old statement is being closed. I think maybe something like this
// Because there is no close statement command in DRDA, the statement
previously associated
// with this pkgnamcsn may not have been closed on the server, even
though it was closed on the client and
// the section reused. Getting a request for a new statement with the
same pkgnamcsn is how the
// server finds out that the statement previously associated with this
pkgnamcsn has been closed.
// So when creating a new DRDAStatement for this pkgnamcsn, we
//1) Retrieve the old statement associated with this pkgnamcsn and
close it
// 2) Reset the statement state for use with the new DRDAStatement
DRDAStatement stmt = getDRDAStatement(pkgnamcsn);
if (stmt != null) {
stmt.close();
stmt.reset();
....
Re: [DRDA] Question about DRDAStatement.initialize() method
Posted by Knut Anders Hatlen <Kn...@Sun.COM>.
Deepa Remesh <dr...@gmail.com> writes:
> On 2/24/06, Knut Anders Hatlen <Kn...@sun.com> wrote:
>> You might also consider getting rid of pkgcnstkn, pkgid and pkgsn
>> since they are stored in pkgnamcsn anyway. If you remove them, we only
>> have two variables that aren't reset like the others.
>
> It will be good to get rid of pkgcnstkn, pkgid and pkgsn (in next
> patch). There are 2 more variables which get assigned only in
> setPkgnamcsn() - cursorName and isolationLevel. So we have this
> comment - " ALL variables (except 'database' and variables which are
> set in 'setPkgnamcsn' based on the 'pkgnamcsn' value)" ?
The comments you have added to DRDAResultSet and DRDAStatement are
very good. Thanks for doing the cleanup!
--
Knut Anders
Re: [DRDA] Question about DRDAStatement.initialize() method
Posted by Deepa Remesh <dr...@gmail.com>.
On 2/24/06, Knut Anders Hatlen <Kn...@sun.com> wrote:
> You might also consider getting rid of pkgcnstkn, pkgid and pkgsn
> since they are stored in pkgnamcsn anyway. If you remove them, we only
> have two variables that aren't reset like the others.
It will be good to get rid of pkgcnstkn, pkgid and pkgsn (in next
patch). There are 2 more variables which get assigned only in
setPkgnamcsn() - cursorName and isolationLevel. So we have this
comment - " ALL variables (except 'database' and variables which are
set in 'setPkgnamcsn' based on the 'pkgnamcsn' value)" ?
> I also think it's a good idea to add a comment before the declarations
> of the instance variables saying something like
>
> // NOTE!
> //
> // Since DRDAStatements are reused, ALL variables (except 'database'
> // and 'pkgnamcsn') should be set to their default values in
> // reset().
>
Thanks,
Deepa
Re: [DRDA] Question about DRDAStatement.initialize() method
Posted by Knut Anders Hatlen <Kn...@Sun.COM>.
Deepa Remesh <dr...@gmail.com> writes:
> Thanks Knut and Kathey. I will open a jira issue for changing use of
> initialize or do it in a separate patch. For your answers to
> question3, I have a small concern. Please see below.
>
>
>> >>3. Is my comment for the new reuse method() okay? specifically, the
>> >>comment about fields which should not be reset?
>> >>"This method should reset all members of this class except the
>> >>following which will be set at initial creation or set explicitly in
>> >>the code: database, pkgnamcsn, pkgcnstkn, pkgid, pkgsn"
>> >>
>> >>
>> >>
>> > If these can be reset too that would be good and then get set whereever
>> > the get set just for completeness.
>>
>> If you reset these variables, you'll need to call
>> DRDAStatement.setPkgnamcsn() and DRDAStatement.setDatabase() later. In
>> DRDAConnThread, setPkgnamcsn() and setDatabase() are not called after
>> DRDAStatement.initialize(), so you'll have to add those calls there.
>
> pkgnamcsn is the key used for statements added to stmtTable. It seems
> to the basis for reusing statements. Resetting it seems a bit odd to
> me. Code in Database.newDRDAStatement will look like this:
>
> DRDAStatement stmt = getDRDAStatement(pkgnamcsn);
> if (stmt != null) {
> stmt.reset();
> stmt.setPkgnamcsn(pkgnamcsn);
> stmt.setDatabase(this);
> }
I agree, it's odd. The way DRDAStatements are reused, pkgnamcsn and
database rarely (if ever) have to be touched. I think it's OK not to
reset them.
You might also consider getting rid of pkgcnstkn, pkgid and pkgsn
since they are stored in pkgnamcsn anyway. If you remove them, we only
have two variables that aren't reset like the others.
I also think it's a good idea to add a comment before the declarations
of the instance variables saying something like
// NOTE!
//
// Since DRDAStatements are reused, ALL variables (except 'database'
// and 'pkgnamcsn') should be set to their default values in
// reset().
--
Knut Anders
Re: [DRDA] Question about DRDAStatement.initialize() method
Posted by Deepa Remesh <dr...@gmail.com>.
Thanks Knut and Kathey. I will open a jira issue for changing use of
initialize or do it in a separate patch. For your answers to
question3, I have a small concern. Please see below.
> >>3. Is my comment for the new reuse method() okay? specifically, the
> >>comment about fields which should not be reset?
> >>"This method should reset all members of this class except the
> >>following which will be set at initial creation or set explicitly in
> >>the code: database, pkgnamcsn, pkgcnstkn, pkgid, pkgsn"
> >>
> >>
> >>
> > If these can be reset too that would be good and then get set whereever
> > the get set just for completeness.
>
> If you reset these variables, you'll need to call
> DRDAStatement.setPkgnamcsn() and DRDAStatement.setDatabase() later. In
> DRDAConnThread, setPkgnamcsn() and setDatabase() are not called after
> DRDAStatement.initialize(), so you'll have to add those calls there.
pkgnamcsn is the key used for statements added to stmtTable. It seems
to the basis for reusing statements. Resetting it seems a bit odd to
me. Code in Database.newDRDAStatement will look like this:
DRDAStatement stmt = getDRDAStatement(pkgnamcsn);
if (stmt != null) {
stmt.reset();
stmt.setPkgnamcsn(pkgnamcsn);
stmt.setDatabase(this);
}
Do you think it is okay?
Thanks,
Deepa
Re: [DRDA] Question about DRDAStatement.initialize() method
Posted by Knut Anders Hatlen <Kn...@Sun.COM>.
Kathey Marsden <km...@sbcglobal.net> writes:
> Deepa Remesh wrote:
>>I have couple of questions:
>>1. I find it confusing to have a initialize and reuse method with
>>similar comments. Can initialize be removed since we can call
>>setTypDefValues directly at places where it is currently called?
>>
>>
>>
> I must say I find every use of initialize() absolutely perplexing except
> for the one in parseEXCSQLIMM,
> but regardless they all seem to be resetting the defaultDRDAStatement
> for reuse, so I think initialize can go away and you can use your new
> reuse() method. I think you can have a reuse, with or without a database
> parameter.
I think you are right, Kathey, but I'm absolutely not sure. I too find
the calls to initialize() odd (except parseEXCSQLIMM()).
Would it make sense to add a call to setTypDefValues() in reuse()? At
least when database != null? In that case you could replace all calls
to initialize() with reuse().
>>2. initialize() is called at called at 5 places in the server code.
>>Two of these places (DRDAConnThread methods parseEXCSQLIMM and
>>parseEXCSQLSETobjects) have the following comment:
>>
>>// initialize statement for reuse
>>drdaStmt.initialize();
>>
>>I am not sure about the actual intention of calling initialize() at
>>these places - whether it is to just reset the typedef values or does
>>it expect all fields to be reset for reuse. Can someone familiar with
>>code look at these places to see it is enough to use initialize() at
>>these places? If this does not look right, I can open another issue
>>for this.
If the intention was to reset the typedef values, I guess
setTypDefValues() would have been used instead of initialize(). The
comments indicate all fields should be reset.
>>3. Is my comment for the new reuse method() okay? specifically, the
>>comment about fields which should not be reset?
>>"This method should reset all members of this class except the
>>following which will be set at initial creation or set explicitly in
>>the code: database, pkgnamcsn, pkgcnstkn, pkgid, pkgsn"
>>
>>
>>
> If these can be reset too that would be good and then get set whereever
> the get set just for completeness.
If you reset these variables, you'll need to call
DRDAStatement.setPkgnamcsn() and DRDAStatement.setDatabase() later. In
DRDAConnThread, setPkgnamcsn() and setDatabase() are not called after
DRDAStatement.initialize(), so you'll have to add those calls there.
By the way, setDatabase() calls setTypDefValues(), so you could try to
replace initialize() with
reuse(); // including database, pkgnamcsn etc.
setPkgnamcsn(pkgnamcsn);
setDatabase(database);
> I wonder if reset() is beetter than reuse, but I am the worlds worst
> namer.
+1 to reset()
> Will there be a reuse() in DRDAResultSet as well?
+1 to that too.
> Will close no longer reset these values and the reuse only called when
> the statement is reused?
> Even with the rename, I worry a little about the maintainability of
> this. Part of that is just that DRDAStatement is way to busy and I
> think the ParameterMetadata should be broken out some day..
--
Knut Anders
Re: [DRDA] Question about DRDAStatement.initialize() method
Posted by Kathey Marsden <km...@sbcglobal.net>.
Deepa Remesh wrote:
>For my DERBY-1002 patch,
>
Thanks Deepa for working to make this code clearer. It had fooled many
developers and burnt you personally a couple of times I know.
> I have added a method DRDAStatement.reuse()
>which resets all fields in this class.
>
> /**
> * Clean up statements and result set for reuse.
> * This method should reset all members of this class except the
> * following which will be set at initial creation or set
>explicitly in the code:
> * database, pkgnamcsn, pkgcnstkn, pkgid, pkgsn
> *
> * @throws SQLException
> */
> protected void reuse() throws SQLException {
> ...
> }
>
>
>
>I was just going through all methods in DRDAStatement and found that
>there is a method called initialize() which does this:
> /**
> * Initialize for reuse
> */
> protected void initialize()
> {
> setTypDefValues();
> }
>where setTypDefValues() does this:
> /**
> * set TypDef values
> *
> */
> protected void setTypDefValues()
> {
> // initialize statement values to current database values
> this.typDefNam = database.typDefNam;
> this.byteOrder = database.byteOrder;
> this.ccsidSBC = database.ccsidSBC;
> this.ccsidDBC = database.ccsidDBC;
> this.ccsidMBC = database.ccsidMBC;
> this.ccsidSBCEncoding = database.ccsidSBCEncoding;
> this.ccsidDBCEncoding = database.ccsidDBCEncoding;
> this.ccsidMBCEncoding = database.ccsidMBCEncoding;
> }
>
>
>I have couple of questions:
>1. I find it confusing to have a initialize and reuse method with
>similar comments. Can initialize be removed since we can call
>setTypDefValues directly at places where it is currently called?
>
>
>
I must say I find every use of initialize() absolutely perplexing except
for the one in parseEXCSQLIMM,
but regardless they all seem to be resetting the defaultDRDAStatement
for reuse, so I think initialize can go away and you can use your new
reuse() method. I think you can have a reuse, with or without a database
parameter.
>2. initialize() is called at called at 5 places in the server code.
>Two of these places (DRDAConnThread methods parseEXCSQLIMM and
>parseEXCSQLSETobjects) have the following comment:
>
>// initialize statement for reuse
>drdaStmt.initialize();
>
>I am not sure about the actual intention of calling initialize() at
>these places - whether it is to just reset the typedef values or does
>it expect all fields to be reset for reuse. Can someone familiar with
>code look at these places to see it is enough to use initialize() at
>these places? If this does not look right, I can open another issue
>for this.
>
>
>
yes. 1
>3. Is my comment for the new reuse method() okay? specifically, the
>comment about fields which should not be reset?
>"This method should reset all members of this class except the
>following which will be set at initial creation or set explicitly in
>the code: database, pkgnamcsn, pkgcnstkn, pkgid, pkgsn"
>
>
>
If these can be reset too that would be good and then get set whereever
the get set just for completeness.
I wonder if reset() is beetter than reuse, but I am the worlds worst namer.
Will there be a reuse() in DRDAResultSet as well?
Will close no longer reset these values and the reuse only called when
the statement is reused?
Even with the rename, I worry a little about the maintainability of
this. Part of that is just that DRDAStatement is way to busy and I
think the ParameterMetadata should be broken out some day..
Thanks again Deepa