Re: Bug fix for psql's meta-command \ev
On 2023-09-20 09:32, Michael Paquier wrote: Actually there was a bit more to it in the presence of \e, that could also get some unpredictible behaviors if some errors happen while editing a query, which is something unlikely, still leads to strange behaviors on failure injections. I was considering first to move the reset in do_edit(), but also we have the case of \e[v|f] where the buffer has no edits so it felt a bit more natural to do that in the upper layer like in this patch. Indeed, similar behaviours can happen with the \e. The patch you committed looks good to me. Thank you. Ryoga Yoshida
Re: Bug fix for psql's meta-command \ev
On Tue, Sep 19, 2023 at 01:23:54PM +0300, Aleksander Alekseev wrote: > You are right, I missed it. Your patch is correct while the original > one is not quite so. Actually there was a bit more to it in the presence of \e, that could also get some unpredictible behaviors if some errors happen while editing a query, which is something unlikely, still leads to strange behaviors on failure injections. I was considering first to move the reset in do_edit(), but also we have the case of \e[v|f] where the buffer has no edits so it felt a bit more natural to do that in the upper layer like in this patch. Another aspect of all these code paths is the line number that can be optionally number after an object name for \e[v|f] or a file name for \e (in the latter case it is possible to have a line number without a file name, as well). Anyway, we only fill the query buffer after validating all the options at hand. So, while the status is set to PSQL_CMD_ERROR, we'd still do a reset of the query buffer but nothing got added to it yet. I've also considered a backpatch for this change, but at the end discarded this option, at least for now. I don't think that someone is relying on the existing behavior of having the query buffer still around on failure if \ev or \ef fail their object lookup as the contents are undefined, because that's not unintuitive, but this change is not critical enough to make it backpatchable if somebody's been actually relying on the previous behavior. I'm OK to revisit this choice later on depending on the feedback, though. -- Michael signature.asc Description: PGP signature
Re: Bug fix for psql's meta-command \ev
Hi Michael, > The patch looks incorrect to me. In case you've not noticed, we'd > still have the same problem if do_edit() fails [...] You are right, I missed it. Your patch is correct while the original one is not quite so. -- Best regards, Aleksander Alekseev
Re: Bug fix for psql's meta-command \ev
On 2023-09-19 15:29, Ryoga Yoshida wrote: You can see attached file. I didn't notice that Michael attached the patch file. Just ignore my file. I apologize for the inconvenience. Ryoga Yoshida
Re: Bug fix for psql's meta-command \ev
On 2023-09-19 12:53, Michael Paquier wrote: 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. Thank you everyone for the reviews. I fixed the patch for the error and also added a comment . You can see attached file. Ryoga Yoshidadiff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index bcd8eb3538..eb306d8d77 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -1239,6 +1239,15 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch, status = PSQL_CMD_NEWEDIT; } + if (status == PSQL_CMD_ERROR) + { + /* + * clear the query buffer because it could be overwritten with + * an incomplete query string due to certain failures. + */ + resetPQExpBuffer(query_buf); + } + free(obj_desc); } else
Re: Bug fix for psql's meta-command \ev
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
Re: Bug fix for psql's meta-command \ev
Hi, I came across the patch since it was marked as "Needs review" (and then I realized that I mistakenly opened the upcoming commit fest, not the current one...). > Good catch! I agree to this. > > > This problem can be resolved by resetting the query buffer on > > error. You can see the attached source code. After that, it will > > result in output like the following: > > While exec_command_ef_ev() currently preserves the existing content of > the query buffer in case of certain failures, This behavior doesn't > seem to be particularly significant, especially given that both \ef > and \ev are intended to overwrite the query buffer on success. > > We have the option to fix get_create_object_cmd() and ensure > exec_command_ef_ev() retains the existing content of the query buffer > on failure. However, this approach seems like overly cumbersome. So > I'm +1 to this approach. > > A comment might be necessary to clarify that we need to wipe out the > query buffer because it could be overwritten with an incomplete query > string due to certain failures. 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. -- Best regards, Aleksander Alekseev
Re: Bug fix for psql's meta-command \ev
At Fri, 15 Sep 2023 11:37:46 +0900, Ryoga Yoshida wrote in > I think this is a bug in psql's \ev meta-command. Even when \ev fails, > it should not leave the garbage string in psql's query buffer and the > following query should be completed successfully. Good catch! I agree to this. > This problem can be resolved by resetting the query buffer on > error. You can see the attached source code. After that, it will > result in output like the following: While exec_command_ef_ev() currently preserves the existing content of the query buffer in case of certain failures, This behavior doesn't seem to be particularly significant, especially given that both \ef and \ev are intended to overwrite the query buffer on success. We have the option to fix get_create_object_cmd() and ensure exec_command_ef_ev() retains the existing content of the query buffer on failure. However, this approach seems like overly cumbersome. So I'm +1 to this approach. A comment might be necessary to clarify that we need to wipe out the query buffer because it could be overwritten with an incomplete query string due to certain failures. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Bug fix for psql's meta-command \ev
On Fri, Sep 15, 2023 at 11:37:46AM +0900, Ryoga Yoshida wrote: > I think this is a bug in psql's \ev meta-command. Even when \ev fails, it > should not leave the garbage string in psql's query buffer and the following > query should be completed successfully. Right. Good catch. Will look at that a bit more to see if the resets are correctly placed, particularly in light of \ef. -- Michael signature.asc Description: PGP signature
Bug fix for psql's meta-command \ev
Hi, When a table name is specified as the first argument of \ev meta-command, it reports the error message, the prompt string becomes "-#" and then the following valid query fails because the psql's query buffer contains the garbage string generated by failure of \ev. Please see the following example. =# \ev t "public.t" is not a view -# SELECT * FROM t; ERROR: syntax error at or near "public" at character 1 STATEMENT: public.t AS SELECT * FROM t; I think this is a bug in psql's \ev meta-command. Even when \ev fails, it should not leave the garbage string in psql's query buffer and the following query should be completed successfully. This problem can be resolved by resetting the query buffer on error. You can see the attached source code. After that, it will result in output like the following: =# \ev t "public.t" is not a view =# SELECT * FROM t; i --- 1 2 (2 rows) Ryoga Yoshidadiff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index bcd8eb3538..a853bce096 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -1238,6 +1238,8 @@ exec_command_ef_ev(PsqlScanState scan_state, bool active_branch, else status = PSQL_CMD_NEWEDIT; } + else + resetPQExpBuffer(query_buf); free(obj_desc); }