Re: Doing better at HINTing an appropriate column within errorMissingColumn()

From: Peter Geoghegan <pg(at)heroku(dot)com>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Josh Berkus <josh(at)agliodbs(dot)com>, Ian Barwick <ian(at)2ndquadrant(dot)com>, Andres Freund <andres(at)2ndquadrant(dot)com>, Greg Stark <stark(at)mit(dot)edu>, Jim Nasby <jim(at)nasby(dot)net>, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>
Subject: Re: Doing better at HINTing an appropriate column within errorMissingColumn()
Date: 2014-07-09 06:56:59
Message-ID: CAM3SWZQRghLhpOcr3RsTtwRgsKpXspwjY2rgcCd8qOZhHWV6HA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Jul 8, 2014 at 11:25 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>> So there'd be one variant within core and one within
>> contrib/fuzzystrmatch? I don't think that's an improvement.
>
> No. The main difference between varstr_leven_less_equal and varstr_leven is
> the use of the extra argument max_d in the former. My argument here is
> instead of blindly cut-pasting into core the code you are interested in to
> evaluate the string distances, is to refactor it to have a unique function,
> and to let the business with LEVENSHTEIN_LESS_EQUAL within
> contrib/fuzzystrmatch. This will require some reshuffling of the distance
> function, but by looking at this patch I am getting the feeling that this is
> necessary, and should even be split into a first patch for fuzzystrmatch
> that would facilitate its integration into core.
> Also why is rest_of_char_same within varlena.c?

Just as before, rest_of_char_same() exists for the express purpose of
being called by the two variants varstr_leven_less_equal() and
varstr_leven(). Why wouldn't I copy it over too along with those two?
Where do you propose to put it?

Obviously the existing macro hacks (that I haven't changed) that build
the two variants are not terribly pretty, but they're not arbitrary
either. They reflect the fact that there is no natural way to add
callbacks or something like that. If you pretended that the core code
didn't have to care about one case or the other, and that contrib was
somehow obligated to hook in its own handler for the
!LEVENSHTEIN_LESS_EQUAL case that it now only cares about, then you'd
end up with an even bigger mess. Besides, with the patch the core code
is calling varstr_leven_less_equal(), which is the bigger of the two
variants - it's the LEVENSHTEIN_LESS_EQUAL case, not the
!LEVENSHTEIN_LESS_EQUAL case that core cares about for the purposes of
building HINTs. In short, I don't know what you mean. What would that
reshuffling actually look like?

>> > 5) Do we want hints on system columns as well?
>> I think it's obvious that the answer must be no. That's going to
>> frequently result in suggestions of columns that users will complain
>> aren't even there. If you know about the system columns, you can just
>> get it right. They're supposed to be hidden for most purposes.
>
> This may sound ridiculous, but I have already found myself mistyping ctid by
> tid and cid while working on patches and modules that played with page
> format, and needing a couple of minutes to understand what was going on (bad
> morning).

I think that it's clearly not worth it, even if it is true that a
minority sometimes make this mistake. Most users don't know that there
are system columns. It's not even close to being worth it to bring
that into this.

--
Peter Geoghegan

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message M Tarkeshwar Rao 2014-07-09 09:18:40 Insert query hangs
Previous Message Peter Geoghegan 2014-07-09 06:29:01 Re: Doing better at HINTing an appropriate column within errorMissingColumn()