Fix tab completion in v18 for ALTER DATABASE/USER/ROLE ... RESET

On Thu, Jul 17, 2025 at 1:41 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:

Hi hackers,

These two patches are split out from my earlier thread about improving
tab completion for varous RESET forms
(/messages/by-id/87bjqwwtic.fsf@wibble.ilmari.org), so that the bug
fixes can be tracked as an open item for v18.

Commits 9df8727c5067 and c407d5426b87 added tab completion for ALTER
DATABASE ... RESET and ALTER ROLE/USER ... RESET, respectively, but
they're both differently buggy. The query for the DATABASE form
neglected to schema-qualify the unnest() call, and the query for the
ROLE/USER form shows all possible variables once you start typing a
variable name, not just the ones that are set on the role. The attached
patches fix both.

I played around with it, and overall it looks good.

jian he <jian.universality@gmail.com> writes:

On Thu, Jul 17, 2025 at 1:41 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:

Hi hackers,

These two patches are split out from my earlier thread about improving
tab completion for varous RESET forms
(/messages/by-id/87bjqwwtic.fsf@wibble.ilmari.org), so that the bug
fixes can be tracked as an open item for v18.

Commits 9df8727c5067 and c407d5426b87 added tab completion for ALTER
DATABASE ... RESET and ALTER ROLE/USER ... RESET, respectively, but
they're both differently buggy. The query for the DATABASE form
neglected to schema-qualify the unnest() call, and the query for the
ROLE/USER form shows all possible variables once you start typing a
variable name, not just the ones that are set on the role. The attached
patches fix both.

I played around with it, and overall it looks good.

Thanks for the review.

I realise I should have included Robins and Tomas in the original mail,
since they were the author and committer, respectively, of the original
patches. I've added them now, and reattached the patches for their
convenience.

- ilmari

On 7/22/25 13:18, Dagfinn Ilmari Mannsåker wrote:

jian he <jian.universality@gmail.com> writes:

On Thu, Jul 17, 2025 at 1:41 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:

Hi hackers,

These two patches are split out from my earlier thread about improving
tab completion for varous RESET forms
(/messages/by-id/87bjqwwtic.fsf@wibble.ilmari.org), so that the bug
fixes can be tracked as an open item for v18.

Commits 9df8727c5067 and c407d5426b87 added tab completion for ALTER
DATABASE ... RESET and ALTER ROLE/USER ... RESET, respectively, but
they're both differently buggy. The query for the DATABASE form
neglected to schema-qualify the unnest() call, and the query for the
ROLE/USER form shows all possible variables once you start typing a
variable name, not just the ones that are set on the role. The attached
patches fix both.

I played around with it, and overall it looks good.

Thanks for the review.

I realise I should have included Robins and Tomas in the original mail,
since they were the author and committer, respectively, of the original
patches. I've added them now, and reattached the patches for their
convenience.

Thanks for the fixes. Both seem correct to me. It took me a while to
reproduce some sort of issue with unnest(), but that was mostly because
I didn't realize the tab completion does not report errors to the user.

While testing the ALTER ROLE part, I realized there's a second related
issue with similar symptoms - after starting to type a variable that is
*not* set for the role, it was offering all matching variables anyway. I
believe that's because of the block at line ~5022. The "database" case
was already excluded, so I made 0002 to do that for ROLE too.

I plan to push the attached fixes soon ...

regards

--
Tomas Vondra

Tomas Vondra <tomas@vondra.me> writes:

On 7/22/25 13:18, Dagfinn Ilmari Mannsåker wrote:

jian he <jian.universality@gmail.com> writes:

On Thu, Jul 17, 2025 at 1:41 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:

Hi hackers,

These two patches are split out from my earlier thread about improving
tab completion for varous RESET forms
(/messages/by-id/87bjqwwtic.fsf@wibble.ilmari.org), so that the bug
fixes can be tracked as an open item for v18.

Commits 9df8727c5067 and c407d5426b87 added tab completion for ALTER
DATABASE ... RESET and ALTER ROLE/USER ... RESET, respectively, but
they're both differently buggy. The query for the DATABASE form
neglected to schema-qualify the unnest() call, and the query for the
ROLE/USER form shows all possible variables once you start typing a
variable name, not just the ones that are set on the role. The attached
patches fix both.

I played around with it, and overall it looks good.

Thanks for the review.

I realise I should have included Robins and Tomas in the original mail,
since they were the author and committer, respectively, of the original
patches. I've added them now, and reattached the patches for their
convenience.

Thanks for the fixes. Both seem correct to me. It took me a while to
reproduce some sort of issue with unnest(), but that was mostly because
I didn't realize the tab completion does not report errors to the user.

While testing the ALTER ROLE part, I realized there's a second related
issue with similar symptoms - after starting to type a variable that is
*not* set for the role, it was offering all matching variables anyway. I
believe that's because of the block at line ~5022. The "database" case
was already excluded, so I made 0002 to do that for ROLE too.

Ah, good catch, I missed that edge case.

@@ -5015,7 +5021,8 @@ match_previous_words(int pattern_id,
/* Complete with a variable name */
else if (TailMatches("SET|RESET") &&
!TailMatches("UPDATE", MatchAny, "SET") &&
-			 !TailMatches("ALTER", "DATABASE", MatchAny, "RESET"))
+			 !TailMatches("ALTER", "DATABASE", MatchAny, "RESET") &&
+			 !TailMatches("ALTER", "USER|ROLE", MatchAny, "RESET"))
COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_set_vars,
"CONSTRAINTS",
"TRANSACTION",

Instead of adding another !TailMatches() call, why not just change
"DATABASE" to "DATABASE|ROLE|USER"?

- ilmari

On 7/30/25 12:17, Dagfinn Ilmari Mannsåker wrote:

...

@@ -5015,7 +5021,8 @@ match_previous_words(int pattern_id,
/* Complete with a variable name */
else if (TailMatches("SET|RESET") &&
!TailMatches("UPDATE", MatchAny, "SET") &&
-			 !TailMatches("ALTER", "DATABASE", MatchAny, "RESET"))
+			 !TailMatches("ALTER", "DATABASE", MatchAny, "RESET") &&
+			 !TailMatches("ALTER", "USER|ROLE", MatchAny, "RESET"))
COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_set_vars,
"CONSTRAINTS",
"TRANSACTION",

Instead of adding another !TailMatches() call, why not just change
"DATABASE" to "DATABASE|ROLE|USER"?

It seemed to me separate calls would be easier to understand, but I see
combine it like this in many other places, so done that way ...

Pushed. Thanks for the fixes!

regards

--
Tomas Vondra

Tomas Vondra <tomas@vondra.me> writes:

On 7/30/25 12:17, Dagfinn Ilmari Mannsåker wrote:

...

@@ -5015,7 +5021,8 @@ match_previous_words(int pattern_id,
/* Complete with a variable name */
else if (TailMatches("SET|RESET") &&
!TailMatches("UPDATE", MatchAny, "SET") &&
-			 !TailMatches("ALTER", "DATABASE", MatchAny, "RESET"))
+			 !TailMatches("ALTER", "DATABASE", MatchAny, "RESET") &&
+			 !TailMatches("ALTER", "USER|ROLE", MatchAny, "RESET"))
COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_set_vars,
"CONSTRAINTS",
"TRANSACTION",

Instead of adding another !TailMatches() call, why not just change
"DATABASE" to "DATABASE|ROLE|USER"?

It seemed to me separate calls would be easier to understand, but I see
combine it like this in many other places, so done that way ...

Pushed.

Thanks!