Re: quoting psql varible as identifier

On Fri, Jan 29, 2010 at 2:08 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> 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.
>
> we can discus about programming style, but in this case I am sure. The
> problem is \set command. We cannot ignore error in this case. In other
> cases invalid escaping raises error, not in this case. So there is two
> ways again:
>
> a) remove escaped expansion from \command
> b) implement \set command differently

I don't view either of those solutions as appropriate or acceptable.

> 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 think so it was a back door for scripting support in psql. It can
> break backward compatibility!

I don't understand your point here.

>> I would like to go ahead and commit my version of this patch and will
>> do so later today if no one else objects.
>
> yes, I have.
>
> * your patch remove some feature without any warning and documentation
> * your patch doesn't solve issue with \set command

It does not remove any feature at all that I can see. The fact that
backslash commands don't properly report errors is a problem, but I
don't see why I have to solve that problem as part of this patch, and
even less why I should fix \set and leave all the others alone.

Another problem I've found looking at this code is that \copy does not
support variable substitutions at all, which seems rather unfortunate.
Still another is that the regular expression used for variable
substitutions in SQL commands is subtly different than the one used
for backslash commands: the latter matches a single colon with no
following characters, while the former does not.

All of this stuff may be worth fixing but I'm sticking with my
position that it's not material for this patch. If you want to submit
a separate patch to change the error handling, go ahead. But I don't
think fixing it for \set only is going to fly, and I don't think that
it should be specific to handling only errors resulting from encoding
violations or out of memory conditions in variable substitutions. It
needs to be a general mechanism that can be applied to existing types
of error conditions where it's appropriate and to new error conditions
that may arise in the future.

...Robert