Re: Bogus duplicate command issued in pg_dump

On Sun, Jan 23, 2022 at 11:31 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

>
> res = ExecuteSqlQueryForSingleRow(fout, upgrade_query->data);
> ...
> appendPQExpBuffer(upgrade_query,
> "SELECT t.oid, t.typarray "
> ...
> res = ExecuteSqlQueryForSingleRow(fout, upgrade_query->data);
>
> How's that work?

I just spent 10 minutes thinking you were wrong because I confused the
upgrade_query and upgrade_buffer variables in that function.

You might just as well have fixed the first upgrade_query command to be
print instead of append. And, better yet, renamed its variable to
"array_oid_query" then added a new PQExpBuffer variable "range_oid_query".
Because, is double-purposing a variable here, with a badly chosen generic
name, really worth saving a create buffer call? If it is, naming is
something like "oid_query" would be better than leading with "upgrade".
Though I am looking at this function in isolation...

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?
>
>
I would avoid overreacting. The biggest issue would be when the previous
query used to execute in some cases but using append incorrectly prevents
that prior execution. I don't think that is likely to get past review and
committed in practice. Here it is all new code and while as I noted above
it has some quality concerns it did work correctly when committed and that
isn't surprising. I don't see enough benefit to warrant refactoring here.

I think a contributing factor here is the fact that the upgrade_buffer is
designed around using appendPQExpBuffer. The kind of typo seems obvious
given that in most cases it will actually provide valid results. But it
also seems to restrict our ability to do something globally.

David J.