Re: Bogus duplicate command issued in pg_dump

On Sun, Jan 23, 2022 at 7:25 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Sun, Jan 23, 2022 at 01:31:03PM -0500, Tom Lane wrote:
> > 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.
>
> Could a backend-side, run-time configurable developper GUC,
> potentially help here? This could look after multiple queries in code
> paths where we don't want any, once you combine it with a specific
> compilation flag à-la-ENFORCE_REGRESSION_TEST.
>
>
I don't see how this helps unless you change the system (under the
compilation flag) into "Don't allow multiple commands under simple query
protocol unless the user has indicated that they will be doing that by
enabling the allow_multiple_commands_in_simple_query_protocol GUC at the
start of their, possibly multi-transaction, query." (I don't even want to
consider them toggling it). Forcing an author to specify when they don't
want multiple commands is just going to be ignored and since no errors will
be raised (even under the compiler flag) we will effectively remain status
quo.

I do not see that alternative mode being practical, let alone a net
positive.

Is there some trick in C where you can avoid the buffer? That is what
basically caused the issue. If one could write:
res = ExecuteSqlQueryForSingleRow(fout, "SELECT "
typname FROM " ...);
directly the decision to print or append the buffer would not be necessary
and I would expect one-shot queries to then be done using this, thus
avoiding the observed issue.

I could see having the executor operate in a mode where if a query result
is discarded it logs a warning. But that cannot be unconditional. "SELECT
perform_function(); SELECT * FROM table_the_function_just_populated;"
discards a result but because functions must be executed in SELECT this
situation is one that should be ignored. In short, the setup seems like it
should be easy enough (I'd hope we can figure out when we've discarded a
query result because a new one came after) but defining the exceptions to
the rule seems much trickier (we'd then probably want the GUC in order to
get rid of false positives that cannot be added to the exceptions). And in
order to catch existing bugs you still have to have confidence in the
check-world. But if you have that then a behavioral bug introduced by this
kind of error is sufficiently likely to be caught anyway that the need for
this decreases substantially.

So, it seems the time would probably be better spent doing organized code
exploring and improving test coverage if there is a real concern that there
are more bugs of this ilk out there causing behavioral or meaningful
performance issues. At least for pg_dump we ostensibly can test that the
most important outcome isn't violated - the what was dumped gets restored.
I presume we do that and it feels like if we missed capturing the outcome
of a select command that would be unlikely.

David J.