Re: IF (NOT) EXISTS in psql-completion

Hello,

Thank you for looking this long-and-bothersome patch.

At Wed, 23 Nov 2016 07:12:00 +0100, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote in <CAFj8pRBxgUrg-6CKbVOy4VqwSFkrf--uCzj3q-vd9FgpSGV+qQ(at)mail(dot)gmail(dot)com>
> Hi
>
> 2016-11-15 12:26 GMT+01:00 Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)
> jp>:
>
> > Hello, I rebased this patch on the current master.
> >
> > At Mon, 31 Oct 2016 10:15:48 +0900 (Tokyo Standard Time), Kyotaro
> > HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <
> > 20161031(dot)101548(dot)162143279(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > > Anyway, I fixed space issues and addressed the
> > > COMPLETE_WITH_QUERY()'s almost unused parameter problem. And
> > > tried to write a README files.
> >
> > tab-complete.c have gotten some improvements after this time.
> >
> > 577f0bdd2b8904cbdfde6c98f4bda6fd93a05ffc psql: Tab completion for
> > renaming enum values.
> > 927d7bb6b120a2ca09a164898f887eb850b7a329 Improve tab completion for
> > CREATE TRIGGER.
> > 1d15d0db50a5f39ab69c1fe60f2d5dcc7e2ddb9c psql: Tab-complete LOCK [TABLE]
> > ... IN {ACCESS|ROW|SHARE}.
> >
> > The attached patchset is rebsaed on the master including these
> > patches.
> >
>
> I checked patches 0001, 0002, 0003 patches
>
> There are no any problems with patching, compiling, it is working as
> expected
>
> These patches can be committed separately - they are long, but the code is
> almost mechanical

Thanks.

You're right. I haven't consider about relations among them.

0001 (if-else refactoring) does not anyting functionally. It is
required by 0004(word-shift-and-removal) and 0005(if-not-exists).

0002 (keywords case improvement) is almost independent from all
other patches in this patch set. And it brings an obvious
improvement.

0003 (addition to 0002) is move embedded keywords out of defined
queries. Functionally can be united to 0002 but separated for
understandability

0004 (word-shift-and-removal) is quite arguable one. This
introduces an ability to modify (or destroy) previous_words
array. This reduces almost redundant matching predicates such as,

> if (TailMatches3("CREATE|UNIQUE", "INDEX", MatchAny) ||
> TailMatches4("CREATE|UNIQUE", "INDEX", "CONCURRENTLY", MatchAny))

into

> if (Matches3("CREATE", "INDEX", MatchAnyExcept("ON")))

by removing "CONCURRENTLY". This obviously simplifies the
predicates literally but it the code implies history of
modification. The implied history might be worse than the
previous shape, especially for the simple cases like this. For a
complex case of CREATE TRIGGER, it seems worse than the original
shape... I'll consider this a bit more. Maybe match-and-collapse
template should be written a predicate.

0005 (if-not-exists). I have admit that this is arguable
feature...

0006 is the terder point:( but.

> The README is perfect

Thank you, I'm relieved by hearing that.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center