On Tue, Nov 25, 2014 at 4:01 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> - This appears to needlessly reindent the comments for PG_CACHE_LINE_SIZE.
Actually, the word "only" is removed (because PG_CACHE_LINE_SIZE has a
new client now). So it isn't quite the same paragraph as before.
> - I really don't think we need a #define in pg_config_manual.h for
> this. Please omit that.
You'd prefer to not offer a way to disable abbreviation? Okay. I guess
that makes sense - it should work well as a general optimization.
> - I'm much happier with the way the changes to sortsupport.h look in
> this version. However, I think that auth_comparator is a confusing
> name, because "auth" is often used as an abbreviation for
> "authentication". We can spell it out (authoritative_comparator) or
> come up with a different name (backup_comparator?
> abbrev_full_comparator?). Whatever we do, ApplySortComparatorAuth()
> should be renamed to match.
Okay.
> - Also, I don't think making abbrev_state an enumerated value with two
> values is really doing anything for us; we could just use a Boolean.
> I'm wondering if we should actually go a bit further and remove this
> from the SortSupport object and instead add an additional Boolean flag
> to PrepareSortSupportFrom(OrderingOp|IndexRel) that gets passed all
> the way down to the opclass's sortsupport function. It seems like
> that might be more clear. Once the opclass function has done its
> thing, the other three new nembers are enough to know whether we're
> using the optimization or not (and can be fiddled if we want to make a
> later decision to call the whole thing off).
I'm not sure about that. I'd prefer to have tuplesort (and one or two
other sites) set the "abbreviation is possible in principle" flag.
Otherwise, sortsupport needs to assume that the leading attribute is
going to be the abbreviation-applicable one, which might not always be
true. Still, it's not as if I feel strongly about it.
--
Peter Geoghegan
