Thread: Re: [PATCHES] Bundle of patches

Teodor Sigaev <teodor@sigaev.ru> writes:
> 2) ORDER BY .. [ NULLS ( FIRST | LAST ) ]
>    http://www.sigaev.ru/misc/NULLS_82-0.5.gz

i haven't looked at the other patches yet, but this one is just horrid :-(
Instead of creating a proper parse-tree representation of the NULLS
FIRST/LAST option, you've kluged it to convert the syntax into a
double-column ordering using "foo IS [NOT] NULL" as the first column.
This has obvious semantic disdvantages (what if foo is an expensive
function?); but the real problem is that there's no way for the planner
to reason about ordering in this representation.  This patch would
guarantee that an ORDER BY with the NULLS option couldn't use an
indexscan, even if the index sorts nulls at the correct end.

I think a reasonable implementation requires introducing an explicit
concept of nulls-first-or-last into the planner's model of sort order,
and probably into btree index opclasses as well.  There is already some
discussion about this in the archives with respect to the problem of
handling descending-sort opclasses nicely.  (If you just switch the
operators to make a descending-sort opclass, then nulls end up at the
"other end" from where they would otherwise, meaning that a backwards
index scan does something unexpected.  We have to fix that or else
descending-sort opclasses can break mergejoins...)

            regards, tom lane

> This has obvious semantic disdvantages (what if foo is an expensive
> function?);
Agree.

> but the real problem is that there's no way for the planner
> to reason about ordering in this representation.  This patch would
> guarantee that an ORDER BY with the NULLS option couldn't use an
> indexscan, even if the index sorts nulls at the correct end.

create table foo ( i int);
insert into foo values (1), (5), (NULL);
create index fooidx on foo (i);
set enable_seqscan=off;
set enable_bitmapscan=off;
explain select i from foo order by i asc nulls last;
                             QUERY PLAN
-------------------------------------------------------------------
  Index Scan using fooidx on foo  (cost=0.00..12.05 rows=3 width=4)
explain select i from foo order by i desc nulls first;
                                  QUERY PLAN
----------------------------------------------------------------------------
  Index Scan Backward using fooidx on foo  (cost=0.00..12.05 rows=3 width=4)

Patch is smart enough about "native" NULL's ordering, so it adds quals only if
it needed.

Index support of non-"native" NULL's ordering, IMHO, has some correlation with
suggested OR-patch. Sorting by ASC NULLS FIRST may done by two index scan with
append node:
Append
    Index Scan
        Cond: foo IS NULL
    Index Scan
        Cond: foo IS NOT NULL

> I think a reasonable implementation requires introducing an explicit
> concept of nulls-first-or-last into the planner's model of sort order,
Agree, but I tried to keep patches independent as possible...


If we will have agreement about ways to resolve, I'll will time to work
further in foreseeable future.
--
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

Teodor Sigaev <teodor@sigaev.ru> writes:
>> but the real problem is that there's no way for the planner
>> to reason about ordering in this representation.  This patch would
>> guarantee that an ORDER BY with the NULLS option couldn't use an
>> indexscan, even if the index sorts nulls at the correct end.

> Patch is smart enough about "native" NULL's ordering, so it adds quals
> only if it needed.

[ looks again... ]  Oh, so you've hard-wired the assumption that ASC/DESC
correlate to NULLS FIRST/LAST right into the grammar :-(

This is not a workable base for future extension.  To be able to do
anything interesting with descending-order opclasses, we really have to
separate ASC/DESC from NULLS FIRST/LAST, not bind them permanently
together.

BTW, another sufficient reason to reject this approach is that a query
entered as "ORDER BY foo NULLS FIRST" would come out as something quite
different, if reverse-listed by ruleutils.c.  We've paid the price of
that sort of shortsightedness too many times in the past: implementation
details should not get exposed in the reverse-listing.  As a rule of
thumb, if you are putting a transformation involving semantic knowledge
into gram.y, you are probably putting it in the wrong place.  (One of
the reasons I like the typmod patch is that it helps pull a lot of
inappropriate knowledge out of gram.y ...)

            regards, tom lane