Обсуждение: 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 Yoshida

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

At Fri, 15 Sep 2023 11:37:46 +0900, Ryoga Yoshida <bt23yoshidar@oss.nttdata.com> 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

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

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

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 Yoshida

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

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

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

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