On Sun, Jan 23, 2022 at 11:31 AM Tom Lane <tgl@sss.pgh.pa.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.