Subject: Re: Performance improvement for joins where outer side is
 unique
To: David Rowley <david.rowley@2ndquadrant.com>, Tom Lane <tgl@sss.pgh.pa.us>
References: <20150320.121140.148610378.horiguchi.kyotaro@lab.ntt.co.jp>
 <CAPpHfdviphQFjk-gaGZGGxLAyt=YuVdJLnntsxOKnU296s9u-Q@mail.gmail.com>
 <CAKJS1f-=1y0-f5FFV2_r7LtS+CL2Lw-nhJjXB3O_0FmBqnK=3A@mail.gmail.com>
 <27152324.eFKJb4Khb5@dinodell> <8608754a32d1325c2f83ba8b1df71585@xs4all.nl>
 <55DA1F9F.5040204@2ndquadrant.com>
 <CAKJS1f-9UTo6fA+h8rojd30CujSKscZamLn5YnKw5+ASbV0Wwg@mail.gmail.com>
 <5085.1440375543@sss.pgh.pa.us>
 <CAKJS1f_8hrzTDCu0pwERWj_dLZu-6DYFQJAtNT+vM6UeOn0kHw@mail.gmail.com>
 <8907.1440383377@sss.pgh.pa.us>
 <CAKJS1f8JCt15ngEP8sZO-JYYT8mS6HUP_CT0T6S_0ogk1kQt+Q@mail.gmail.com>
 <CAKJS1f_4hm3qwKeHYKftd+BQuYBgdNR+rk+XP_JW9EY-ALHZbQ@mail.gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <56718B15.6060507@2ndquadrant.com>
Date: Wed, 16 Dec 2015 17:02:29 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.1.0
MIME-Version: 1.0
In-Reply-To: 
 <CAKJS1f_4hm3qwKeHYKftd+BQuYBgdNR+rk+XP_JW9EY-ALHZbQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

Hi,

On 12/16/2015 01:27 AM, David Rowley wrote:
>
> I've attached a rebased patch against current master as there were
> some conflicts from the recent changes to LATERAL join.

Thanks. I've looked at the rebased patch and have a few minor comments.

0) I know the patch does not tweak costing - any plans in this
    direction? Would it be possible to simply use the costing used by
    semijoin?

1) nodeHashjoin.c (and other join nodes)

I've noticed we have this in the ExecHashJoin() method:

  /*
   * When the inner side is unique or we're performing a
   * semijoin, we'll consider returning the first match, but
   * after that we're done with this outer tuple.
   */
  if (node->js.unique_inner)
      node->hj_JoinState = HJ_NEED_NEW_OUTER;

That seems a bit awkward because the comment speaks about unique joins 
*OR* semijoins, but the check only references unique_inner. That of 
course works because we do this in ExecInitHashJoin():

	switch (node->join.jointype)
	{
		case JOIN_SEMI:
			hjstate->js.unique_inner = true;
			/* fall through */

Either some semijoins may not be unique - in that case the naming is 
misleading at best, and we either need to find a better name or simply 
check two conditions like this:

  if (node->js.unique_inner || node->join.type == JOIN_SEMI)
     node->hj_JoinState = HJ_NEED_NEW_OUTER;

Or all semijoins are unique joins, and in that case the comment may need 
rephrasing. But more importantly, it begs the question why we're 
detecting this in the executor and not in the planner? Because if we 
detect it in executor, we can't use this bit in costing, for example.

FWIW the misleading naming was actually mentioned in the thread by 
Horiguchi-san, but I don't see any response to that (might have missed 
it though).

2) analyzejoins.c

I see that this code in join_is_removable()

     innerrel = find_base_rel(root, innerrelid);

     if (innerrel->reloptkind != RELOPT_BASEREL)
         return false;

was rewritten like this:

     innerrel = find_base_rel(root, innerrelid);

     Assert(innerrel->reloptkind == RELOPT_BASEREL);

That suggests that previously the function expected cases where 
reloptkind may not be RELOPT_BASEREL, but now it'll error out int such 
cases. I haven't noticed any changes in the surrounding code that'd 
guarantee this won't happen, but I also haven't been able to come up 
with an example triggering the assert (haven't been trying too hard). 
How do we know the assert() makes sense?


3) joinpath.c

- either "were" or "been" seems redundant

/* left joins were already been analyzed for uniqueness in 
mark_unique_joins() */


4) analyzejoins.c

comment format broken

/*
  * mark_unique_joins
	Analyze joins in order to determine if their inner side is unique based
	on the join condition.
  */

5) analyzejoins.c

missing line before the comment

}
/*
  * rel_supports_distinctness
  *		Returns true if rel has some properties which can prove the relation
  *		to be unique over some set of columns.
  *

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers