The fmtId() function used in pg_dump for optional quoting identifiers
has bothered me for a while now. The API is confusing: the returned
value needs to be used before fmtId() is called again, because the
buffer the return value points to is re-used for each call of fmtId().
That leads to bugs for those unaware of this requirement, and clumsy
code for those that are -- for example (pg_dump.c:2911)

    appendPQExpBuffer(delq, "DROP TYPE %s.",
                      fmtId(tinfo->typnamespace->nspname,
                            force_quotes));
    appendPQExpBuffer(delq, "%s;\n",
                      fmtId(tinfo->typname, force_quotes));

Should really only be 1 line of code. Similar ugliness occurs in many
places (e.g. several lines down, there is a section of 4 calls to
appendPQExpBuffer() that could be condensed down to 1, if not for
fmtId() ).

Lastly, it has a tendancy to produce memory leaks -- for example,
convertRegProcReference() in pg_dump.c will leak memory when used
with PostgreSQL >= 7.3. When I mentioned this to Tom Lane, he
said that he didn't see a way to fix it without convoluting the
code, and suggested I bring it up on -hackers.

My suggestion is: since fmtId() is almost always used with
appendPQExpBuffer(), we should add a wrapper function to pg_dump
that accepts an extra escape sequence (%S, or %i, perhaps), which
would properly quote the input string before passing it to
appendPQExpBuffer(). That should ensure that there won't be any
leaks from adding quotes to identifiers, but also allow us to avoid
playing games with static buffers, like fmtId() does now.

Any comments?

Cheers,

Neil

-- 
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

Mon, 15 Jul 2002 12:16:55 -0400
nconway@klamath.dyndns.org (Neil Conway)
fmtId() and pg_dump

Interesting idea.  Not sure how you are going to do that since
appendPQExpBuffer uses vsnprintf.  Would you spin through the format
string and modify the pointers sent to vsnprintf?  Seems like a lot of
work.

FYI, the 7.2 code had fmtId called pretty messed up in certain places
but I think I fixed them all.

---------------------------------------------------------------------------

Neil Conway wrote:
> The fmtId() function used in pg_dump for optional quoting identifiers
> has bothered me for a while now. The API is confusing: the returned
> value needs to be used before fmtId() is called again, because the
> buffer the return value points to is re-used for each call of fmtId().
> That leads to bugs for those unaware of this requirement, and clumsy
> code for those that are -- for example (pg_dump.c:2911)
> 
>     appendPQExpBuffer(delq, "DROP TYPE %s.",
>                       fmtId(tinfo->typnamespace->nspname,
>                             force_quotes));
>     appendPQExpBuffer(delq, "%s;\n",
>                       fmtId(tinfo->typname, force_quotes));
> 
> Should really only be 1 line of code. Similar ugliness occurs in many
> places (e.g. several lines down, there is a section of 4 calls to
> appendPQExpBuffer() that could be condensed down to 1, if not for
> fmtId() ).
> 
> Lastly, it has a tendancy to produce memory leaks -- for example,
> convertRegProcReference() in pg_dump.c will leak memory when used
> with PostgreSQL >= 7.3. When I mentioned this to Tom Lane, he
> said that he didn't see a way to fix it without convoluting the
> code, and suggested I bring it up on -hackers.
> 
> My suggestion is: since fmtId() is almost always used with
> appendPQExpBuffer(), we should add a wrapper function to pg_dump
> that accepts an extra escape sequence (%S, or %i, perhaps), which
> would properly quote the input string before passing it to
> appendPQExpBuffer(). That should ensure that there won't be any
> leaks from adding quotes to identifiers, but also allow us to avoid
> playing games with static buffers, like fmtId() does now.
> 
> Any comments?
> 
> Cheers,
> 
> Neil
> 
> -- 
> Neil Conway <neilconway@rogers.com>
> PGP Key ID: DB3C29FC
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
>     (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Tue, 16 Jul 2002 02:11:23 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: fmtId() and pg_dump
Neil Conway writes:

> My suggestion is: since fmtId() is almost always used with
> appendPQExpBuffer(), we should add a wrapper function to pg_dump
> that accepts an extra escape sequence (%S, or %i, perhaps), which
> would properly quote the input string before passing it to
> appendPQExpBuffer().

I think that would be nice.

-- 
Peter Eisentraut   peter_e@gmx.net


Wed, 17 Jul 2002 20:45:08 +0200 (CEST)
Peter Eisentraut <peter_e@gmx.net>
Re: fmtId() and pg_dump