From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | David Rowley <dgrowleyml(at)gmail(dot)com> |
Cc: | Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, rushabh(dot)lathia(at)gmail(dot)com |
Subject: | Re: [PATCH] Keeps tracking the uniqueness with UniqueKey |
Date: | 2020-03-31 03:11:49 |
Message-ID: | 15836.1585624309@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
[ not a review, just some drive-by comments on David's comments ]
David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> 2. The following change does not seem like it should be part of this
> patch. I understand you perhaps have done as you think it will
> improve the performance of checking if an expression is in a list of
> expressions.
> - COMPARE_SCALAR_FIELD(varno);
> + /* Compare varattno first since it has higher selectivity than varno */
> COMPARE_SCALAR_FIELD(varattno);
> + COMPARE_SCALAR_FIELD(varno);
> If you think that is true, then please do it as a separate effort and
> provide benchmarks with your findings.
By and large, I'd reject such micro-optimizations on their face.
The rule in the nodes/ support files is to list fields in the same
order they're declared in. There is no chance that it's worth
deviating from that for this.
I can believe that there'd be value in, say, comparing all
scalar fields before all non-scalar ones. But piecemeal hacks
wouldn't be the way to handle that either. In any case, I'd
prefer to implement such a plan within the infrastructure to
auto-generate these files that Andres keeps muttering about.
> a. including a function call in the foreach macro is not a practise
> that we really follow. It's true that the macro now assigns the 2nd
> param to a variable. Previous to 1cff1b95ab6 this was not the case and
> it's likely best not to leave any bad examples around that code which
> might get backported might follow.
No, I think you're misremembering. foreach's second arg is
single-evaluation in all branches. There were some preliminary
versions of 1cff1b95ab6 in which it would not have been, but that
was sufficiently dangerous that I found a way to get rid of it.
> b. We generally subtract InvalidAttrNumber from varattno when
> including in a Bitmapset.
ITYM FirstLowInvalidHeapAttributeNumber, but yeah. Otherwise
the code fails on system columns, and there's seldom a good
reason to risk that.
regards, tom lane
From | Date | Subject | |
---|---|---|---|
Next Message | Fujii Masao | 2020-03-31 03:21:43 | Re: Planning counters in pg_stat_statements (using pgss_store) |
Previous Message | Amit Kapila | 2020-03-31 02:46:22 | Re: improve transparency of bitmap-only heap scans |