quoting psql varible as identifier
in [PgSql]
|
Prev: Port for Microsoft Services for Unix (SFU) or SUA
Next: [HACKERS] solution to make static changes in pg_hba.conf file?
From: Robert Haas on 27 Jan 2010 15:08 On Mon, Jan 25, 2010 at 7:36 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: > I hope, so this version is more readable and more clean. I removed > some not necessary checks. This still seems overly complicated to me. I spent a few hours today working up the attached patch. Let me know your thoughts. ....Robert
From: Pavel Stehule on 28 Jan 2010 04:53 2010/1/27 Robert Haas <robertmhaas(a)gmail.com>: > On Mon, Jan 25, 2010 at 7:36 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >> I hope, so this version is more readable and more clean. I removed >> some not necessary checks. > > This still seems overly complicated to me. Â I spent a few hours today > working up the attached patch. Â Let me know your thoughts. > There is serious issue. The "psql_error" only shows some message on output, but do nothing more - you don't set a result status for commands and for statements. So some potential error from parsing is pseudo quietly ignored - without respect to your setting ON_ERROR_STOP. This could be a problem for commands. Execution of broken SQL statements will raise syntax error. But for \set some variable will be a broken and the content can be used. I don't thing so it is good. It is limited. Your version is acceptable only when we don't enable escape syntax for commands. Then we don't need check it. On your version - I am not sure if it is fully compatible, and using a global variables isn't nice. I little bit modify my original code - it is more verbose (- useless using pqexpbuffer) - and more consistent with previous behave. Pavel > ...Robert >
From: Robert Haas on 28 Jan 2010 09:38 On Thu, Jan 28, 2010 at 4:53 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: > 2010/1/27 Robert Haas <robertmhaas(a)gmail.com>: >> On Mon, Jan 25, 2010 at 7:36 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >>> I hope, so this version is more readable and more clean. I removed >>> some not necessary checks. >> >> This still seems overly complicated to me. I spent a few hours today >> working up the attached patch. Let me know your thoughts. > > There is serious issue. The "psql_error" only shows some message on > output, but do nothing more - you don't set a result status for > commands and for statements. So some potential error from parsing is > pseudo quietly ignored - without respect to your setting > ON_ERROR_STOP. This could be a problem for commands. Execution of > broken SQL statements will raise syntax error. But for \set some > variable will be a broken and the content can be used. I don't thing > so it is good. It is limited. Well, what you seem to be proposing to do is allow the command to execute (on the screwed-up data) and then afterwards pretend that it failed by overriding the return status. I think that's unacceptable. The root of the problem here is that the parsing and execution stages for backslash commands are not cleanly separated. There's no clean way for the lexer to return an error that allows the command to finish parsing normally but then doesn't execute it. Fixing that is going to require an extensive refactoring of commands.c which I don't think it makes sense to undertake at this point in the release cycle. Even if it did, it seems like material for a separate patch rather than something which has to be done before this goes in. > Your version is acceptable only when we don't enable escape syntax for > commands. Then we don't need check it. On your version - I am not sure > if it is fully compatible, and using a global variables isn't nice. I'm not adding any new global variables - I'm just using the ones that are already there to avoid duplicating the same code four times. Referencing them from within the bodies of the lexer rules doesn't make the variables not global. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Pavel Stehule on 28 Jan 2010 11:59 2010/1/28 Robert Haas <robertmhaas(a)gmail.com>: > On Thu, Jan 28, 2010 at 4:53 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >> 2010/1/27 Robert Haas <robertmhaas(a)gmail.com>: >>> On Mon, Jan 25, 2010 at 7:36 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >>>> I hope, so this version is more readable and more clean. I removed >>>> some not necessary checks. >>> >>> This still seems overly complicated to me. Â I spent a few hours today >>> working up the attached patch. Â Let me know your thoughts. >> >> There is serious issue. The "psql_error" only shows some message on >> output, but do nothing more - you don't set a result status for >> commands and for statements. So some potential error from parsing is >> pseudo quietly ignored - without respect to your setting >> ON_ERROR_STOP. This could be a problem for commands. Execution of >> broken SQL statements will raise syntax error. But for \set some >> variable will be a broken and the content can be used. I don't thing >> so it is good. It is limited. > > Well, what you seem to be proposing to do is allow the command to > execute (on the screwed-up data) and then afterwards pretend that it > failed by overriding the return status. Â I think that's unacceptable. > The root of the problem here is that the parsing and execution stages > for backslash commands are not cleanly separated. Â There's no clean > way for the lexer to return an error that allows the command to finish > parsing normally but then doesn't execute it. Â Fixing that is going to > require an extensive refactoring of commands.c which I don't think it > makes sense to undertake at this point in the release cycle. Â Even if > it did, it seems like material for a separate patch rather than > something which has to be done before this goes in. so I removed support for escaping from backslah commands and refactorised code. I hope so this code is more verbose and clean than previous versions. Regards Pavel > >> Your version is acceptable only when we don't enable escape syntax for >> commands. Then we don't need check it. On your version - I am not sure >> if it is fully compatible, and using a global variables isn't nice. > > I'm not adding any new global variables - I'm just using the ones that > are already there to avoid duplicating the same code four times. > Referencing them from within the bodies of the lexer rules doesn't > make the variables not global. > > ...Robert >
From: Robert Haas on 28 Jan 2010 13:06
On Thu, Jan 28, 2010 at 11:59 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: > 2010/1/28 Robert Haas <robertmhaas(a)gmail.com>: >> On Thu, Jan 28, 2010 at 4:53 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >>> 2010/1/27 Robert Haas <robertmhaas(a)gmail.com>: >>>> On Mon, Jan 25, 2010 at 7:36 AM, Pavel Stehule <pavel.stehule(a)gmail.com> wrote: >>>>> I hope, so this version is more readable and more clean. I removed >>>>> some not necessary checks. >>>> >>>> This still seems overly complicated to me. I spent a few hours today >>>> working up the attached patch. Let me know your thoughts. >>> >>> There is serious issue. The "psql_error" only shows some message on >>> output, but do nothing more - you don't set a result status for >>> commands and for statements. So some potential error from parsing is >>> pseudo quietly ignored - without respect to your setting >>> ON_ERROR_STOP. This could be a problem for commands. Execution of >>> broken SQL statements will raise syntax error. But for \set some >>> variable will be a broken and the content can be used. I don't thing >>> so it is good. It is limited. >> >> Well, what you seem to be proposing to do is allow the command to >> execute (on the screwed-up data) and then afterwards pretend that it >> failed by overriding the return status. I think that's unacceptable. >> The root of the problem here is that the parsing and execution stages >> for backslash commands are not cleanly separated. There's no clean >> way for the lexer to return an error that allows the command to finish >> parsing normally but then doesn't execute it. Fixing that is going to >> require an extensive refactoring of commands.c which I don't think it >> makes sense to undertake at this point in the release cycle. Even if >> it did, it seems like material for a separate patch rather than >> something which has to be done before this goes in. > > so I removed support for escaping from backslah commands and refactorised code. > > I hope so this code is more verbose and clean than previous versions. I don't see any advantage of this over the code that I wrote. First, you can't just remove support for the escape syntax from \d commands without some discussion of whether or not that's the right thing to do, and I don't think it is. The cases where this will potentially cause a problem are limited to those where the input is invalidly encoded, and I don't think that's important enough to justify the surprise factor of having backslash commands behave differently from everything else. Second, even if it were OK to remove support for the escape syntax from \d commands, you failed to update the documentation you cribbed from my patch to match the behavior you implemented. Third, you've reintroduced all of the code duplication that I eliminated in my version of this patch, as well as at least one bug - you've used free() where I believe you need PQfreemem(). I am also thinking that it doesn't make sense to push the result of PQescapeLiteral() or PQescapeIdentifier() as a new buffer, because we don't want to process variable expansions recursively. At first I thought this was a security hole, but on further reflection I don't think it is: it'll rescan as a quoted string anyway, so any colon-escapes will be ignored. But I believe it's unnecessary at any rate. I would like to go ahead and commit my version of this patch and will do so later today if no one else objects. ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |