Bogus duplicate command issued in pg_dump

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Bogus duplicate command issued in pg_dump
Date: 2022-01-23 18:31:03
Message-ID: 1714711.1642962663@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

While investigating for 353708e1f, I happened to notice that pg_dump's
binary_upgrade_set_type_oids_by_type_oid() contains

PQExpBuffer upgrade_query = createPQExpBuffer();
...
appendPQExpBuffer(upgrade_query,
"SELECT typarray "
...
res = ExecuteSqlQueryForSingleRow(fout, upgrade_query->data);
...
appendPQExpBuffer(upgrade_query,
"SELECT t.oid, t.typarray "
...
res = ExecuteSqlQueryForSingleRow(fout, upgrade_query->data);

How's that work? It turns out that the second ExecuteSqlQueryForSingleRow
is sending a string like "SELECT typarray ...;SELECT t.oid, t.typarray ..."
which the backend happily interprets as multiple commands. It sends
back multiple query results, and then PQexec discards all but the last
one. So, very accidentally, there's no observable bug, just some wasted
cycles.

I will go fix this, but I wondered if there are any other similar
errors, or what we might do to prevent the same type of mistake
in future. I experimented with replacing most of pg_dump's PQexec
calls with PQexecParams, as in the attached quick hack (NOT meant
for commit). That did not turn up any additional cases, but of
course I have limited faith in the code coverage of check-world.

We could consider a more global change to get rid of using
appendPQExpBuffer where it's not absolutely necessary, so that
there are fewer bad examples to copy. Another idea is to deem
it an anti-pattern to end a query with a semicolon. But I don't
have a lot of faith in people following those coding rules in
future, either. It'd also be a lot of code churn for what is
in the end a relatively harmless bug.

Thoughts?

regards, tom lane

Attachment Content-Type Size
hacky-check-for-bogus-pg_dump-queries.patch text/x-diff 4.3 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2022-01-23 18:35:22 Re: PSA: Autoconf has risen from the dead
Previous Message Joel Jacobson 2022-01-23 18:13:41 Re: PSA: Autoconf has risen from the dead