fmtId() and pg_dump

Started by Nonameover 23 years ago3 messages
#1Noname
nconway@klamath.dyndns.org

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

#2Bruce Momjian
pgman@candle.pha.pa.us
In reply to: Noname (#1)
Re: 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
#3Peter Eisentraut
peter_e@gmx.net
In reply to: Noname (#1)
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