Re: Bug in AbstracJdbc2Statement.replaceProcessing when using dollar quoting?

> -----Original Message-----
> From: pgsql-jdbc-owner@postgresql.org [mailto:pgsql-jdbc-
> owner@postgresql.org] On Behalf Of Marc Geisinger
> Sent: Wednesday, October 10, 2012 3:55 AM
> To: pgsql-jdbc@postgresql.org
> Subject: Re: [JDBC] Bug in AbstracJdbc2Statement.replaceProcessing when
> using dollar quoting?
>
> Am Dienstag, 9. Oktober 2012 19:49:17 UTC+2 schrieb "David Johnston":
> > Hi,
> >
> >
> >
> > See below for my thoughts on how this is limited and inefficient.
> >
> ...
> >
> > This patch appears to throw the exception when an unquoted identifier
> >
> > includes a dollar-sign.  I cannot speak to the standard but it would
> > seem
> >
> > that presence of the dollar-sign should only be evaluated if we are
> >
> > "IN_SQLCODE" and the preceding character is NOT (alphanumeric or
> > underscore)
> >
> > {specifically whatever characters are allowed to begin an unquoted
> >
> > identifier}.
> >
> >
> >
> > Also, with the logic provided the addition of an exception and a
> > try/catch
> >
> > block appears to add unnecessary overhead.  At the point the exception
> > is
> >
> > thrown I would suggest that we instead:
> >
> >
> >
> > "return p_sql;"
> >
> >
> >
> > and let the replaceProcessing method remain ignorant of whether the
> > SQL it
> >
> > is getting back is the original SQL or a modified version.
> >
> >
> >
> > The fact is the presence of dollar-quoting with enabled escaping is
> > going to
> >
> > be the common situation.  Combine that with the fact we are not
> > prompting
> >
> > the user to "fix" anything means that using the exception mechanism a
> > poor
> >
> > choice - it is not Exceptional but rather ordinary.  Everything that
> > needs
> >
> > to be checked and confirmed can be done in the parseSQL method and
> > since it
> >
> > has access to the original statement it can return the original when
> >
> > necessary.
> >
> >
> >
> > David J.
>
> Hi,
> I know that throwing that exception is a bit... uncommon. But I did that
> because of the while loop in replaceProcessing. The code there does call
> parseSql several times,  at least it can. And therefor I don't know if it
is the
> first call when i found a dollar quote. If i just return the sql unchanged
it
> might have had changes before so the result would be replaced sql code
> together with original sql code.
> What I wanted to achieve was that replaceProcessing get's to know that
> there is a dollar quote and stop the replaceProcessing and return the
original
> sql string (just like replaceProcessingEnabled would have been set to
false). I
> wanted to have a kind of stateful exit of the parseSql method. But since
it
> only returns an int value, and might have been called before, throwing an
> exception was the only thing I found to be able to do that.
>
>

This is probably a situation where returning "-1" would have merit.  I
dislike the fact this was implemented as a "protected static" method though
- but changing the method signature is almost just as bad from that
perspective.

I'm also concerned that re-entry presumes that one is always in the
"IN_SQLCODE" state; since the invalid character that is being protected
against could theoretically appear anywhere.  But I missed the implication
of the re-entry on my first scan and need more time than I have now to
ponder this further.

I do at least like the fact that adding the checked exception will cause
existing code to fail to compile since we are changing the mechanics of how
the processing works.  But as I said before the overhead is something to be
concerned about given the fact this particular code path is one of the most
heavily used ones in the driver.

David J.