Re: IF (NOT) EXISTS in psql-completion

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: tgl(at)sss(dot)pgh(dot)pa(dot)us
Cc: pavel(dot)stehule(at)gmail(dot)com, david(at)pgmasters(dot)net, peter_e(at)gmx(dot)net, pgsql-hackers(at)postgresql(dot)org
Subject: Re: IF (NOT) EXISTS in psql-completion
Date: 2016-04-07 12:19:17
Message-ID: 20160407.211917.147996130.horiguchi.kyotaro@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Thank you for looking this and for the comment.

Since the end of this CF is quite soon and this seems in
uncommittable state, feel free to move this to the next CF if any
other patch with more priority.

The attached patch is rather WIPs.

1. The same as the first one in the previous version. Just adding
suggestion and making further matching ignoring them.

2. The same as the second one in the previous version. Fix so
that the case of almost all keywords follow an input, except
attributes.

3. The same as the third one in the previous version. Changes
COMPLETE_WITH_ATTR and COMPLETE_WITH_QUERY so that all
keywords can follow an input.

== The followings are the new patches

4. Applicable on top of 3. Addressing the comments. Refactors
tab-completion.c so that it no longer needs the long-long
else-if sequences. For the readability. Getting rid of the
if(.. && false) messes.

5. A sample implement of completion code for CREATE/GRANT/REVOKE.

At Wed, 06 Apr 2016 18:40:57 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in <4317(dot)1459982457(at)sss(dot)pgh(dot)pa(dot)us>
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.

The objective of this patch is not only suggesting "IF (NOT)
EXISTS", but also, enabling to continue completion even such
"noise" words are in command line, without providing multiple
match descriptions for both of with and without the
noise. Currently such words just prohibits continueing
completion.

So if many of us consider that completing such words is just a
crap, it is possible to remove only suggestion, but preserve
ignoring of such words.

> It increases the size of tab-complete.c by nearly 10%, and would

The increase is natural since it treats additional syntax but the
amount of 10% is arguable. However there's some more "noise"
words to be addressed.

CREATE ROLE name [[WITH] option [...]]
ALTER TABLE name ... col [SET DATA] TYPE
CREATE [LOCAL|GLOBAL] .. TABLE
CREATE [OR REPLACE] [TRUSTED] [PROCEDUAL] LANGUAGE
CREATE [OR REPLACE] FUNCTION
...

> 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.)

Added simple comments for them and others.

> 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.

Currently some of the matching conditions (even remote each
other) have order dependencies. Altering them caused some
unexpected change of completion behavior. They are already not
order independent at all, I think. (I'm sorry not to give an
instance for now.. It might be resolved by recent changes.)

But I must admit that the usage of MidMatchAndRemoveN is actually
a bit complicated, but it is finally removed in this patch.

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

This is also removed in the attached patch set.

> 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)
> return false;

The previous patch designed so as not to make drastic changes to
psql_completion, but since it makes the function ugly I
refactored the function in this patch set.

And as an example, the last (5th) patch does this and introduces
CREATE SCHEMA completion in more matured way (of behavior, not
coding), and make the completion for GRANT/REVOKE more simplly.

> 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 patch is doing the former since the completion for "CREATE
SCHEMA" cannot does other than returning so far.

> 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.

How COLLAPSE can be avoided is shown by the CREATE/GRANT/REOKE
code after applying the last patch. If this way is not bad, I'll
do this on the whole psql_completion.

> 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,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachment Content-Type Size
0001-Suggest-IF-NOT-EXISTS-for-tab-completion-of-psql.patch text/x-patch 45.4 KB
0002-Make-added-keywords-for-completion-queries-follow-to.patch text/x-patch 24.3 KB
0003-Make-COMPLETE_WITH_ATTR-to-accept-additional-keyword.patch text/x-patch 64.4 KB
0004-Refactoring-tab-complete-to-make-psql_completion-cod.patch text/x-patch 146.3 KB
0005-Restructure-completion-code-for-CREATE-GRANT-REVOKE-.patch text/x-patch 18.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2016-04-07 12:35:39 Re: PATCH: use foreign keys to improve join estimates v1
Previous Message Fujii Masao 2016-04-07 12:19:12 Re: Support for N synchronous standby servers - take 2