On Mon, Sep 18, 2023 at 06:54:50PM +0300, Aleksander Alekseev wrote: > I tested the patch and it LGTM too. I don't have a strong opinion on > whether we should bother with a comment or not. > > As a side note I wonder whether we shouldn't assume that query_buf is > always properly initialized elsewhere. But this is probably out of > scope of this particular discussion.
The patch looks incorrect to me. In case you've not noticed, we'd still have the same problem if do_edit() fails for a reason or another, and there are plenty of these in this code path, even if I agree that all of them are very unlikely. For example: - Emulate a failure in do_edit(), any way is fine, like forcing a return false at the beginning of the routine. - Attempt \ev on a valid view. This passes lookup_object_oid() and get_create_object_cmd(), fails at do_edit while switching the status to PSQL_CMD_ERROR. - The query buffer is incorrect, a follow-up query still fails. Adding a comment looks important to me once we consider the edit as a path that can fail and the edited query is only executed then reset when we have PSQL_CMD_NEWEDIT as status. I would suggest the patch attached instead, taking care of the error case of this thread and the ones I've spotted. -- Michael
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index bcd8eb3538..c5f7331ecb 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -1239,6 +1239,13 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch, status = PSQL_CMD_NEWEDIT; } + /* + * On error while doing object lookup or while editing, reset the + * query buffer. + */ + if (status == PSQL_CMD_ERROR) + resetPQExpBuffer(query_buf); + free(obj_desc); } else
signature.asc
Description: PGP signature
