Обсуждение: regclass and format('%I')

Hi All,

The difference in how format handles `regclass` and `name` seems like an
inconsistency:

    WITH conversions(casts, format, result) AS (
    VALUES (ARRAY['name']::regtype[],             '%I', format('%I',
name('select'))),
           (ARRAY['name']::regtype[],             '%L', format('%L',
name('select'))),
           (ARRAY['name']::regtype[],             '%s', format('%s',
name('select'))),
           (ARRAY['regclass']::regtype[],         '%I', format('%I',
regclass('select'))),
           (ARRAY['regclass']::regtype[],         '%L', format('%L',
regclass('select'))),
           (ARRAY['regclass']::regtype[],         '%s', format('%s',
regclass('select'))),
           (ARRAY['regclass', 'name']::regtype[], '%I', format('%I',
name(regclass('select')))),
           (ARRAY['regclass', 'name']::regtype[], '%L', format('%L',
name(regclass('select')))),
           (ARRAY['regclass', 'name']::regtype[], '%s', format('%s',
name(regclass('select'))))
    ) SELECT * FROM conversions;
          casts      | format |    result
    -----------------+--------+--------------
     {name}          | %I     | "select"
     {name}          | %L     | 'select'
     {name}          | %s     | select
     {regclass}      | %I     | """select"""
     {regclass}      | %L     | '"select"'
     {regclass}      | %s     | "select"
     {regclass,name} | %I     | """select"""
     {regclass,name} | %L     | '"select"'
     {regclass,name} | %s     | "select"

My assumption is that they both represent valid SQL identifiers, so it stands
to reason that `%I` should result in a valid identifier for both of them (or
neither one).

Kind Regards,
  Jason Dusek

It honestly seems far more reasonable to me that %s and %I should do
the exact same thing with regclass. My reasoning is as follows:

‘%I’ formats a something such that it is a valid identifier,

regclass is already a valid identifier,

therefore, do nothing.

Another line of reasoning:

If you format with ‘%s’ you are saying: I don’t care whether it’s a
valid identifier or literal or whatever, just put the string there,

but when we sub a regclass into a string, we want it to be a valid identifier,

therefore we should write ‘%I’ when subbing it, so as not to confuse
our readers,

therefore ‘%I’ should do nothing.

On 13 March 2015 at 12:42, David G. Johnston <david.g.johnston@gmail.com> wrote:
> On Fri, Mar 13, 2015 at 12:18 PM, Jason Dusek <jason.dusek@gmail.com> wrote:
>>
>> Hi All,
>>
>> The difference in how format handles `regclass` and `name` seems like an
>> inconsistency:
>>
>>     WITH conversions(casts, format, result) AS (
>>     VALUES (ARRAY['name']::regtype[],             '%I', format('%I',
>> name('select'))),
>>            (ARRAY['name']::regtype[],             '%L', format('%L',
>> name('select'))),
>>            (ARRAY['name']::regtype[],             '%s', format('%s',
>> name('select'))),
>>            (ARRAY['regclass']::regtype[],         '%I', format('%I',
>> regclass('select'))),
>>            (ARRAY['regclass']::regtype[],         '%L', format('%L',
>> regclass('select'))),
>>            (ARRAY['regclass']::regtype[],         '%s', format('%s',
>> regclass('select'))),
>>            (ARRAY['regclass', 'name']::regtype[], '%I', format('%I',
>> name(regclass('select')))),
>>            (ARRAY['regclass', 'name']::regtype[], '%L', format('%L',
>> name(regclass('select')))),
>>            (ARRAY['regclass', 'name']::regtype[], '%s', format('%s',
>> name(regclass('select'))))
>>     ) SELECT * FROM conversions;
>>           casts      | format |    result
>>     -----------------+--------+--------------
>>      {name}          | %I     | "select"
>>      {name}          | %L     | 'select'
>>      {name}          | %s     | select
>>      {regclass}      | %I     | """select"""
>>      {regclass}      | %L     | '"select"'
>>      {regclass}      | %s     | "select"
>>      {regclass,name} | %I     | """select"""
>>      {regclass,name} | %L     | '"select"'
>>      {regclass,name} | %s     | "select"
>>
>> My assumption is that they both represent valid SQL identifiers, so it
>> stands
>> to reason that `%I` should result in a valid identifier for both of them
>> (or
>> neither one).
>
>
> All three of the %I results are valid identifiers.
>
> regclass performs the same conversion that %I performs.  But since the
> output of the regclass conversion is a valid identifier, with double-quotes,
> the %I adds another pair of double-quotes and doubles-up the existing pair
> thus leaving you with 6.
>
> <select> is a reserved word and thus can only be used as an identifier if it
> is surrounded in double-quotes.  name() doesn't care (not that it is
> user-documented that I can find) about making its value usable as an
> identifier so when its output goes through %I you get the expected value.
>
> If you are going to use regclass you want to use %s to insert the result
> into your string; not %I.
>
> David J.
>

Jason Dusek <jason.dusek@gmail.com> writes:
> It honestly seems far more reasonable to me that %s and %I should do
> the exact same thing with regclass.

You're mistaken.  The operation of format() is first to convert the
non-format arguments to text strings, using the output functions for their
data types, and then to further process those text strings according to
the format specifiers:

%s -- no additional processing, just insert the string as-is.
%I -- apply double-quoting transformation to create a valid SQL identifier.
%L -- apply single-quoting transformation to create a valid SQL literal.

In the case of regclass, the output string is already double-quoted
as necessary, so applying %I to it produces a doubly double-quoted
string which is almost certainly not what you want.  But it's not
format()'s job to be smarter than the user.  If it tried to avoid
an extra pass of double quoting, it would get some cases wrong,
potentially creating security holes.

            regards, tom lane

It honestly seems far more reasonable to me that %s and %I should do
the exact same thing with regclass. My reasoning is as follows:

‘%I’ formats a something such that it is a valid identifier,

regclass is already a valid identifier,

therefore, do nothing.

Another line of reasoning:

If you format with ‘%s’ you are saying: I don’t care whether it’s a
valid identifier or literal or whatever, just put the string there,

but when we sub a regclass into a string, we want it to be a valid identifier,

therefore we should write ‘%I’ when subbing it, so as not to confuse
our readers,

therefore ‘%I’ should do nothing.

On 14 March 2015 at 09:17, David G. Johnston <david.g.johnston@gmail.com> wrote:
> On Saturday, March 14, 2015, Jason Dusek <jason.dusek@gmail.com> wrote:
>> It honestly seems far more reasonable to me that %s and %I should do
>> the exact same thing with regclass. My reasoning is as follows:
>>
>> ‘%I’ formats a something such that it is a valid identifier,
>>
>> regclass is already a valid identifier,
>>
>> therefore, do nothing.
>>
>> Another line of reasoning:
>>
>> If you format with ‘%s’ you are saying: I don’t care whether it’s a
>> valid identifier or literal or whatever, just put the string there,
>>
>> but when we sub a regclass into a string, we want it to be a valid
>> identifier,
>>
>> therefore we should write ‘%I’ when subbing it, so as not to confuse
>> our readers,
>>
>> therefore ‘%I’ should do nothing.
>>
>
> I agree with the theory but adding type specific logic to format is going to
> be difficult.  The first thing the system does is convert all of the inputs
> to text.  Inside format() everything is text and so it has no way to know
> that the type was regclass and should not be quoted again.

Could it work to add type-specific logic for the cast from `regclass`
to `name`? It would be nice to have something formulaic: always format
identifiers with `%I`, always cast to `name` before formatting.

Kind Regards,
  Jason Dusek

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> ​IOW, as long as the output string matches: ^"(?:"{2})*"$ I do not see how
> it is possible ​for format to lay in a value at %I that is any more
> insecure than the current behavior.  If the input string already matches
> that pattern then it could be output as-is without any additional risk and
> with the positive benefit of making this case work as expected.  The broken
> case then exists when someone actually intends to name their identifier
> <"something"> which then correctly becomes <"""something"""> on output.

But that's exactly the problem: you just broke a case that used to work.
format('%I') is not supposed to guess at what the user intends; it is
supposed to produce a string that, after being passed through identifier
parsing (dequoting or downcasing), will match the input.  It is not
format's business to break that contract just because the input has
already got some double quotes in it.

An example of where this might be important is if you're trying to
construct a query with arbitrary column headers in the output.  You
can do
    format('... AS %I ...', ..., column_label, ...)
and be confident that the label will be exactly what you've got in
column_label.  This proposed change would break that for labels that
happen to already have double-quotes --- but who are we to say that
that can't have been what you wanted?

            regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> ​IOW, as long as the output string matches: ^"(?:"{2})*"$ I do not see how
> it is possible ​for format to lay in a value at %I that is any more
> insecure than the current behavior.  If the input string already matches
> that pattern then it could be output as-is without any additional risk and
> with the positive benefit of making this case work as expected.  The broken
> case then exists when someone actually intends to name their identifier
> <"something"> which then correctly becomes <"""something"""> on output.

But that's exactly the problem: you just broke a case that used to work.
format('%I') is not supposed to guess at what the user intends; it is
supposed to produce a string that, after being passed through identifier
parsing (dequoting or downcasing), will match the input.  It is not
format's business to break that contract just because the input has
already got some double quotes in it.

An example of where this might be important is if you're trying to
construct a query with arbitrary column headers in the output.  You
can do
        format('... AS %I ...', ..., column_label, ...)
and be confident that the label will be exactly what you've got in
column_label.  This proposed change would break that for labels that
happen to already have double-quotes --- but who are we to say that
that can't have been what you wanted?

                        regards, tom lane