On Tue, May 24, 2016 at 3:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> At the same time I am not getting why pgstat_fetch_stat_tabentry needs
>> to be that complicated. Based on the relation OID we can know if it is
>> a shared relation or not, there is no point in doing two times the
>> same lookup in the pgstat hash table.
>
> True, although I'm not sure if adding a dependency on IsSharedRelation()
> here is a good thing. In any case, you took the dependency too far ...
OK. It seemed more simple to me to have the dependency on catalog.h to
avoid two lookups. But I won't fight for it either.
>> Attached is a patch that fixes the issue here:
>
> I have not tested it, but I would bet a lot that this patch is broken:
> what will happen if the first request in a transaction is for a shared
> catalog is that we'll read just the shared-catalog data, and then
> subsequent requests for stats for an unshared table will find nothing.
> Moreover, even if you undid the change to the pgstat_read_statsfiles()
> call so that we still did read the appropriate unshared stats, this
> would have the effect of reversing the problem: if the first request
> is for a shared catalog, then we'd check to ensure the shared stats
> are up-to-date, but fail to ensure that the unshared ones are.
Your bet is right. I forgot the transactional behavior of this code
path. The stats obtained at the first request would not have been
correct for the database of the backend.
> In short, I think the attached is enough to fix it. There's some cosmetic
> cleanup that could be done here: a lot of these functions could use better
> comments, and I'm not happy about the autovac launcher depending on
> MyDatabaseId being zero. But those are not functional problems.
I am going to look at the second patch you sent.
--
Michael