Jan Wieck <JanWieck@Yahoo.com> writes:
> Here it comes.
Hm. I really don't like the way you've uglified the heap_fetch
interface; can't that be done better? It's just plain bizarre that
heap_fetch is now responsible for counting stats for index scans.
And surely this bit of code is wrong:
*userbuf = buffer;
+
+ if (iscan != NULL)
+ pgstat_count_heap_fetch(&iscan->xs_pgstat_info);
+ else
+ pgstat_count_heap_fetch(&relation->pgstat_info);
}
You can't count heapscan info via the relcache entry --- what if there's
more than one heapscan in progress on the same rel? What if the
relcache entry gets rebuilt by SI invalidation? I don't think the stats
stuff should touch the relcache at all.
There's been talk in the past of trying to unify the heapscan and
indexscan APIs, so that we wouldn't need ugliness like the two sets of
coding in AttrDefaultFetch and similar places. Maybe it's time to bite
that bullet, if it'd provide a cleaner place to put the stats calls.
Also, I do not like the pgStatPmPipe mechanism for detecting postmaster
shutdown. That eats two file descriptors in every backend, which is a
pretty substantial waste of resources. Why not just have the postmaster
send a signal to the stats collector when it's time to shut down?
BTW, PG_FUNCTION_INFO_V1() is not needed for built-in functions.
regards, tom lane