"David G. Johnston" <david.g.johnston@gmail.com> writes:
> 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".
Yeah, I was not terribly impressed with the naming choices in that
code either, but I didn't feel like getting into cosmetic changes.
If I were renaming things in that area, I'd start with the function
name --- binary_upgrade_set_type_oids_by_type_oid is long enough
to aggravate carpal-tunnel problems, and yet it's as clear as mud;
what does "by" mean in this context? Don't expect the function's
comment to tell you, because there is none, another way in which
this code is subpar.
The bigger issue to me is that the behavior of PQexec masks what
seems like a pretty easy mistake to make. I don't like that,
but I'm not seeing any non-invasive way to improve things.
And, as you say, an invasive change seems like overreaction.
regards, tom lane