Hi Álvaro,

Thanks for the patch, the gap is real (SET LOCAL <TAB> currently falls
through to the generic Matches("SET", MatchAny) rule and unhelpfully
suggests TO).

I see a couple of issues with the current patch.
1. The new branch makes an existing rule dead code. Further down in the
same else if chain there is:
/* Complete SET SESSION with AUTHORIZATION or CHARACTERISTICS... */
  else if (Matches("SET", "SESSION"))
      COMPLETE_WITH("AUTHORIZATION", "CHARACTERISTICS AS TRANSACTION");

Because the new branch is inserted earlier, that rule never fires anymore,
and SET SESSION <TAB> loses its AUTHORIZATION / CHARACTERISTICS AS
TRANSACTION suggestions. That's a regression
Also LOCAL and SESSION aren't symmetric here. Looking at gram.y,
"AUTHORIZATION" and "CHARACTERISTICS AS TRANSACTION" are bound to a literal
SESSION token in set_rest_more / set_rest, not to the LOCAL/SESSION
modifier on SET.

I am thinking maybe we could split the cases as
    /* Complete "SET LOCAL" */
    else if (Matches("SET", "LOCAL"))
        COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_set_vars,
                                          "TIME ZONE",
                                          "SCHEMA",
                                          "NAMES");
    /* Complete "SET SESSION" */
    else if (Matches("SET", "SESSION"))
        COMPLETE_WITH_QUERY_VERBATIM_PLUS(Query_for_list_of_set_vars,
                                          "AUTHORIZATION",
                                          "CHARACTERISTICS AS TRANSACTION",
                                          "TIME ZONE",
                                          "SCHEMA",
                                          "NAMES");

and remove the now-redundant later Matches("SET", "SESSION") block

Worth considering whether ROLE should be in both lists too, since SET LOCAL
ROLE … and SET SESSION ROLE … are both valid.

Regards,
Surya Poondla

Reply via email to