Re: IF (NOT) EXISTS in psql-completion - Mailing list pgsql-hackers

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 1. We want this patch - it increase a functionality of autocomplete

TBH, I do not think that is an agreed-to statement.  I concur with
Peter's comments upthread questioning how much use-case there is for
interactive completion of IF (NOT) EXISTS.  If it were a small and
uncomplicated patch, I wouldn't object ... but this is neither.

It increases the size of tab-complete.c by nearly 10%, and would
increase it more than that if it were adequately documented as to
what all these new macros and variables do.  (To take just the first
example, APPEND_COMP_CHARP and SET_COMP_CHARP not only lack any
documentation, but have been inserted between a comment documenting
some macros and those macros.  Another complaint in the same vein is
that MatchesN() no longer means even approximately what it did before,
but the comment for those macros hasn't been changed.)  On top of that,
it seems like there's an awful lot of klugery going on here.  An example
is the use of the COLLAPSE macro (hidden inside MidMatchAndRemoveN),
which seems like a seriously bad idea because it destroys data even if
the match the macro is embedded in ultimately fails.  That will create
order dependencies between match rules, which is not something we want
IMO, most especially when it's not clearly marked in the match rules
what's dependent on what.

Seeing things like "if (something-with-side-effects && false)" doesn't
fill me with any great admiration for the cleanliness of the code, either.

In short, I'm not sold that we need autocomplete for IF EXISTS,
and if the price we have to pay for it is klugery on this scale,
it's no sale.  I think this needs to be sent back for a considerable
amount of rethinking.

One thing that might be worth considering to get rid of this
side-effects-in-IF-conditions mess is to move the match rules into
a separate function so that after doing a successful match, we just
"return".  This could be implemented in most places by adding
return statements into the COMPLETE_WITH_FOO macros.  Then we would
not need the enormous else-if chain, but just simple independent
if statements, where we know a successful match will end with a
"return" instead of falling through to the next statement.  The
big advantage of that is that then you can do operations with
side-effects explicitly as separate statements, instead of having
to make them look like phony else-if cases.  So for example the
CREATE SCHEMA case might be handled like
   if (Matches2("CREATE", "SCHEMA"))   {       ... handle possible autocompletions of CREATE SCHEMA itself here ...
       /* Else, move head match point past CREATE SCHEMA */       if ((n = find_last_index_of("CREATE")) > 0)
HEAD_SHIFT(n);  }
 
   /*    * Statements that can appear in CREATE SCHEMA should be considered here!    */
   if (Matches2("CREATE", "TABLE"))       ... handle CREATE TABLE here ...
   ... handle other statements that can appear in CREATE SCHEMA here ...

After exhausting the possibilities for sub-statements of CREATE SCHEMA,
we could either return failure if we'd seen CREATE SCHEMA:
   /*    * Fail if we saw CREATE SCHEMA; no rules below here should be considered.    */   if (head_shift > 0)
returnfalse;
 

or reset the head match point before continuing with unrelated rules:
   /*    * Done considering CREATE SCHEMA sub-rules, so forget about    * whether we saw CREATE SCHEMA.    */
HEAD_SHIFT(0);

Immediately failing seems like the right thing for CREATE SCHEMA, but
maybe in other cases we just undo the head_shift change and keep trying.
This is still order dependent, but at least the places where we change
the match basis can be made to be fairly obvious actions instead of
being disguised as just-another-candidate-match.

I don't immediately have a better idea to replace COLLAPSE, but I really
don't like that as it stands.  I wonder whether we could dodge the need
for it by just increasing head_shift when deciding to skip over an
IF (NOT) EXISTS clause.  Otherwise, I think what I'd want to see is
some way to explicitly undo the effects of COLLAPSE when done with
rules that could benefit from it.  Or at least a coding convention
that makes it clear that you return failure once you've considered
all subsidiary rules, rather than continuing on with unrelated rules
that would have a risk of false match thanks to the data removal.
        regards, tom lane