Subject: Re: [PATCH] psql: Add tab-complete for optional view parameters

From: Thomas Munro <[email-masked]>
Date: Thu, 12 Jan 2023 11:29:11 +1300

On Thu, Jan 12, 2023 at 5:50 AM vignesh C <[email-masked]> wrote:
> For some reason CFBot is not able to apply the patch, please have a
> look and post an updated version if required, also check and handle
> Dean Rasheed and Jim Jones  comment if applicable:
> === Applying patches on top of PostgreSQL commit ID
> 5f6401f81cb24bd3930e0dc589fc4aa8b5424cdc ===
> === applying patch
> ./v2-0001-psql-Add-tab-complete-for-optional-view-parameter.patch
> gpatch: **** Only garbage was found in the patch input.

Melanie pointed me at this issue.  This particular entry is now fixed,
and I think I know what happened:  cfbot wasn't checking the HTTP
status when downloading patches from the web archives, because I had
incorrectly assumed Python's requests.get() would raise an exception
if the web server sent an error status, but it turns out you have to
ask for that.  I've now fixed that.  So I think it was probably trying
to apply one of those "guru meditation" error message the web archives
occasionally spit out.

---

From: Mikhail Gribkov <[email-masked]>
Date: Sun, 29 Jan 2023 10:19:12 +0000

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, failed
Documentation:            tested, passed

Hi Christoph,

The patch have a potential, although I have to agree with Jim Jones, it obviously have a problem with "alter view <name> set<tab>" handling.

First of all user can notice, that SET and RESET alternatives are proposed in an absolutely equivalent way:
postgres=# alter view VVV <tab>
ALTER COLUMN  OWNER TO      RENAME        RESET (       SET (         SET SCHEMA

But completion of a parentheses differs fore these alternatives:

postgres=# alter view VVV reset<tab> -> completes with "<space>(" -> <tab>
CHECK_OPTION      SECURITY_BARRIER  SECURITY_INVOKER

postgres=# alter view VVV set<tab> -> completes with a single spase -> <tab>
Display all 188 possibilities? (y or n)
(and these 188 possibilities do not contain "(")

The probmen is obviously in the newly added second line of the following clause:
		COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
					  "SET SCHEMA", "SET (", "RESET (");

"set schema" and "set (" alternatives are competing, while completion of the common part "set<space>" leads to a string composition which does not have the check branch (Matches("ALTER", "VIEW", MatchAny, "SET")).

I think it may worth looking at "alter materialized view"  completion tree and making "alter view" the same way.

The new status of this patch is: Waiting on Author

---

From: "Gregory Stark (as CFM)" <[email-masked]>
Date: Mon, 20 Mar 2023 14:40:43 -0400

On Sun, 29 Jan 2023 at 05:20, Mikhail Gribkov <[email-masked]> wrote:
>
> The problem is obviously in the newly added second line of the following clause:

I think this patch received real feedback and it looks like it's clear
where there's more work needed. I'll move this to the next commitfest.
If you plan to work on it this month we can always move it back.

-- 
Gregory Stark
As Commitfest Manager

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 6 Jul 2023 15:28:33 +0200

> On 29 Jan 2023, at 11:19, Mikhail Gribkov <[email-masked]> wrote:

> The new status of this patch is: Waiting on Author

This patch has been Waiting on Author since January with the thread being
stale, so I am marking this as Returned with Feedback for now.  Please feel
free to resubmit to a future CF when there is renewed interest in working on
this.

--
Daniel Gustafsson