On Tue, Jan 23, 2018 at 4:17 AM, Edmund Horner <ejr...@gmail.com> wrote:
> Hi Vik, thanks for the feedback!
>
Don't forget to include the list :-)
I'm quoting the entirety of the message---which I would normally trim---for
the archives.
> On 23 January 2018 at 07:41, Vik Fearing <vik.fear...@2ndquadrant.com>
> wrote:
> > This looks better. One minor quibble I have is your use of != (which
> > PostgreSQL accepts) instead of <> (the SQL standard). I'll let the
> > committer worry about that.
>
> There are both forms in the existing queries, but if <> is more
> standard, we should use that.
>
> >> It also uses the server version to determine which query to run. I
> >> have not written a custom query for every version from 7.1! I've
> >> split up the server history into:
> >>
> >> pre-7.3 - does not even have pg_function_is_visible. Not supported.
> >> pre-9.0 - does not have several support functions in types, languages,
> >> etc. We don't bother filtering using columns in other tables.
> >> pre-9.6 - does not have various aggregate support functions.
> >> 9.6 or later - the full query
> >
> > I'm not sure how overboard we need to go with this going backwards so
> > what you did is probably fine.
>
> What I did might be considered overboard, too. :)
>
If this were my patch, I'd have one query for 8.0, and then queries for all
currently supported versions if needed. If 9.2 only gets 8.0-level
features, that's fine by me. That limits us to a maximum of six queries.
Just because we keep support for dead versions doesn't mean we have to
retroactively develop for them. Au contraire.
> >> I was able to test against 9.2, 9.6, and 10 servers, but compiling and
> >> testing the older releases was a bit much for a Friday evening. I'm
> >> not sure there's much value in support for old servers, as long as we
> >> can make completion queries fail a bit more gracefully.
> >
> > pg_dump stops at 8.0, we can surely do the same.
>
> Ok. I'll try to do a bit more testing against servers in that range.
>
> > Now for some other points:
> >
> > Your use of Matches1 is wrong, you should use TailMatches1 instead.
> > SELECT is a fully reserved keyword, so just checking if it's the
> > previous token is sufficient. By using Matches1, you miss cases like
> > this: SELECT (SELECT <tab>
> >
> > It also occurred to me that SELECT isn't actually a complete command (or
> > whatever you want to call it), it's an abbreviation for SELECT ALL. So
> > you should check for SELECT, SELECT ALL, and SELECT DISTINCT. (The FROM
> > tab completion is missing this trick but that's a different patch)
>
> Right. TailMatches it is.
>
> (I was thinking we could preprocess the input to parts extraneous to
> the current query for tab completion purposes, such as:
> - Input up to and including "(", to tab complete a sub-query.
> - Input inside "()", to ignore complete subqueries that might contain
> keywords
> - Everything inside quotes.
> etc.
> Which would be a step to support things like comma-separated SELECT,
> FROM, GROUP BY items. But that's all way too complicated for this
> patch.)
>
> I'll make another patch version, with these few differences.
>
Great!
--
Vik Fearing +33 6 46 75 15
36http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et
Support
