Re: quoting psql varible as identifier

On Thu, Jan 28, 2010 at 11:59 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2010/1/28 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Thu, Jan 28, 2010 at 4:53 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> 2010/1/27 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>>> On Mon, Jan 25, 2010 at 7:36 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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