On 28 August 2015 at 22:20, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp>
wrote:
> On 2015/07/22 15:25, Etsuro Fujita wrote:
>
>> On 2015/07/10 21:59, David Rowley wrote:
>
> I just glanced at this and noticed that the method for determining if
>>> there's any system columns could be made a bit nicer.
>>>
>>
> /* Now, are any system columns requested from rel? */
>>> scan_plan->fsSystemCol = false;
>>> for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
>>> {
>>> if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
>>> {
>>> scan_plan->fsSystemCol = true;
>>> break;
>>> }
>>> }
>>>
>>
> I think could just be written as:
>>> /* Now, are any system columns requested from rel? */
>>> if (!bms_is_empty(attrs_used) &&
>>> bms_next_member(attrs_used, -1) < -FirstLowInvalidHeapAttributeNumber)
>>> scan_plan->fsSystemCol = true;
>>> else
>>> scan_plan->fsSystemCol = false;
>>>
>>
> I know you didn't change this, but just thought I'd mention it while
>>> there's an opportunity to fix it.
>>>
>>
> On second thought, I noticed that there is a case when that fix doesn't
> work well; bms_next_member wouldn't be efficient when only the rear
> user-columns are requested from a foreign table that has a large number of
> user-columns. So, I'm inclined to leave that as-is.
>
>
You might be right here. I'd failed to think about that.
It's likely not worth changing if there's cases when it'll be slower, but
curiosity got the better of me and I wondered how extreme a case it would
take to actually see a slowdown, and per my benchmark results the first
used column would have to be about attnum 500.
I used the attached to benchmark. has_system_columns is the current method,
has_system_columns2 has my changes. Lines are prefixed by the position
where the first (and only) attnum appears in the bitmap set.
1 has_system_columns complete in 1.196000
1 has_system_columns2 complete in 0.170000
2 has_system_columns complete in 1.198000
2 has_system_columns2 complete in 0.167000
4 has_system_columns complete in 1.197000
4 has_system_columns2 complete in 0.170000
8 has_system_columns complete in 1.206000
8 has_system_columns2 complete in 0.203000
16 has_system_columns complete in 1.202000
16 has_system_columns2 complete in 0.237000
32 has_system_columns complete in 1.206000
32 has_system_columns2 complete in 0.232000
64 has_system_columns complete in 1.207000
64 has_system_columns2 complete in 0.268000
128 has_system_columns complete in 1.205000
128 has_system_columns2 complete in 0.368000
256 has_system_columns complete in 1.203000
256 has_system_columns2 complete in 0.780000
512 has_system_columns complete in 1.202000
512 has_system_columns2 complete in 1.302000
1024 has_system_columns complete in 1.199000
1024 has_system_columns2 complete in 3.539000
So, for what it's worth, could be 6 times faster for an "average" sized
table, but hey, we're talking nanoseconds anyway...
Regards
David Rowley
--
David Rowley http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Training & Services
#include <stdio.h>
#include <time.h>
#include <stdlib.h>
#include <stddef.h>
#define FirstLowInvalidHeapAttributeNumber (-8)
#define FLEXIBLE_ARRAY_MEMBER 1
#define elog(elevel, s) \
do { \
printf("%s\n", s); \
exit(EXIT_FAILURE); \
} while(0)
#define BITS_PER_BITMAPWORD 32
#define WORDNUM(x) ((x) / BITS_PER_BITMAPWORD)
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
#define BITMAPSET_SIZE(nwords) \
(offsetof(Bitmapset, words) + (nwords) * sizeof(bitmapword))
typedef unsigned int bitmapword; /* must be an unsigned type */
typedef int signedbitmapword; /* must be the matching signed type */
typedef char bool;
#define true 1
#define false 0
typedef struct Bitmapset
{
int nwords; /* number of words in array */
bitmapword words[FLEXIBLE_ARRAY_MEMBER]; /* really [nwords] */
} Bitmapset;
void *
palloc0(size_t size)
{
void *p = malloc(size);
if (!p)
elog(ERROR, "out of memory");
memset(p, 0, size);
return p;
}
/*
* bms_is_member - is X a member of A?
*/
bool
bms_is_member(int x, const Bitmapset *a)
{
int wordnum,
bitnum;
/* XXX better to just return false for x<0 ? */
if (x < 0)
elog(ERROR, "negative bitmapset member not allowed");
if (a == NULL)
return false;
wordnum = WORDNUM(x);
bitnum = BITNUM(x);
if (wordnum >= a->nwords)
return false;
if ((a->words[wordnum] & ((bitmapword) 1 << bitnum)) != 0)
return true;
return false;
}
static const unsigned char rightmost_one_pos[256] = {
0, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
5, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
6, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
5, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
7, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
5, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
6, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
5, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0,
4, 0, 1, 0, 2, 0, 1, 0, 3, 0, 1, 0, 2, 0, 1, 0
};
int
bms_next_member(const Bitmapset *a, int prevbit)
{
int nwords;
int wordnum;
bitmapword mask;
if (a == NULL)
return -2;
nwords = a->nwords;
prevbit++;
mask = (~(bitmapword) 0) << BITNUM(prevbit);
for (wordnum = WORDNUM(prevbit); wordnum < nwords; wordnum++)
{
bitmapword w = a->words[wordnum];
/* ignore bits before prevbit */
w &= mask;
if (w != 0)
{
int result;
result = wordnum * BITS_PER_BITMAPWORD;
while ((w & 255) == 0)
{
w >>= 8;
result += 8;
}
result += rightmost_one_pos[w & 255];
return result;
}
/* in subsequent words, consider all bits */
mask = (~(bitmapword) 0);
}
return -2;
}
Bitmapset *
bms_make_singleton(int x)
{
Bitmapset *result;
int wordnum,
bitnum;
if (x < 0)
elog(ERROR, "negative bitmapset member not allowed");
wordnum = WORDNUM(x);
bitnum = BITNUM(x);
result = (Bitmapset *) palloc0(BITMAPSET_SIZE(wordnum + 1));
result->nwords = wordnum + 1;
result->words[wordnum] = ((bitmapword) 1 << bitnum);
return result;
}
int
has_system_columns(Bitmapset *attrs_used)
{
int i;
for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
{
if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
{
return 1;
}
}
return 0;
}
int has_system_columns2(Bitmapset *attrs_used)
{
int first = bms_next_member(attrs_used, -1);
if (first >= 0 && first < -FirstLowInvalidHeapAttributeNumber)
return 1;
return 0;
}
#define LOOPS 100000000
int main(void)
{
clock_t start, finish;
int firstcol;
for (firstcol = 1; firstcol <= 1024; firstcol <<= 1)
{
int loop;
int dummycounter;
Bitmapset *attrs_used = bms_make_singleton(firstcol - FirstLowInvalidHeapAttributeNumber);
start = clock();
for (loop = 0; loop <= LOOPS; loop++)
dummycounter += has_system_columns(attrs_used);
finish = clock();
printf("%d has_system_columns complete in %f\n", firstcol, (double) (finish - start) / CLOCKS_PER_SEC);
start = clock();
for (loop = 0; loop <= LOOPS; loop++)
dummycounter += has_system_columns2(attrs_used);
finish = clock();
printf("%d has_system_columns2 complete in %f\n", firstcol, (double) (finish - start) / CLOCKS_PER_SEC);
}
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
