Re: Bug in AbstracJdbc2Statement.replaceProcessing when using dollar quoting? - Mailing list pgsql-jdbc
From | David Johnston |
---|---|
Subject | Re: Bug in AbstracJdbc2Statement.replaceProcessing when using dollar quoting? |
Date | |
Msg-id | 014101cda708$36382ac0$a2a88040$@yahoo.com Whole thread Raw |
In response to | Re: Bug in AbstracJdbc2Statement.replaceProcessing when using dollar quoting? (Marc Geisinger <marc.mg75@googlemail.com>) |
List | pgsql-jdbc |
> -----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.
pgsql-jdbc by date: