Thread: pgstat_drop_relation bugfix

I wrote:
> > pgstat_drop_relation() is expecting relid (pg_class.oid) as the argument,
> > but we pass it relfilenode.
> I'm trying to fix the bug, because there is a possibility that some useless
> statistics data continue to occupy some parts of the statistics table.

Here is a patch to fix undropped entries in the runtime statistics table.
Now smgr records the relation oids and uses them to drop entries
corresponding the relations.

We could make stray entries easily using the following queries.
  CREATE TABLE test (i integer);
  INSERT INTO test VALUES(1);
  TRUNCATE test;
  DROP TABLE test;

Since we don't discard any of stat entries except pg_stat_reset(),
those useless entries would cause memory leaks, though it is very trivial.

I fell my fix is uncool; Let me know if there are any other better ways.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> I wrote:
>>> pgstat_drop_relation() is expecting relid (pg_class.oid) as the argument,
>>> but we pass it relfilenode.

> Here is a patch to fix undropped entries in the runtime statistics table.
> Now smgr records the relation oids and uses them to drop entries
> corresponding the relations.
> ...
> I fell my fix is uncool; Let me know if there are any other better ways.

I don't like this patch either.  It's ugly because it introduces
relation OIDs into a level of the system that should only be dealing in
relfilenodes --- which I think is not just cosmetic, it adds hazards of
using the wrong one (which indeed is exactly the type of bug we're
trying to fix...)  It's not even correct: the patch for
setNewRelfilenode specifies the rel OID to both the smgrcreate and
smgrscheduleunlink calls, which means that the stats collector will be
told to drop its stats for that OID on either commit or rollback.
Surely not what we'd like.  Now that could be fixed, but it illustrates
that this is actually a very fragile way of thinking about and dealing
with the problem.

I thought for awhile about changing the pgstats stuff to identify
relations by RelFileNode instead of OID.  That has a number of
attractions --- you could argue that it's more correct when dealing with
reassigned relfilenodes, for instance.  But it'd be a lot of work and
it would mean changing the signatures of all the pg_stat_get_foo()
functions, which would break people's custom stats views.

What I'm inclined to propose is that we just remove the
pgstat_drop_relation() call from smgr_internal_unlink, and rely entirely
on VACUUM to clean out dead entries in the pgstats data.

            regards, tom lane

I wrote:
> What I'm inclined to propose is that we just remove the
> pgstat_drop_relation() call from smgr_internal_unlink, and rely entirely
> on VACUUM to clean out dead entries in the pgstats data.

On checking the CVS history, that in fact is how the code worked before
8.1.3, when I introduced the bogus call in a fit of over-optimization :-(.
I vaguely recall thinking that the chance of losing data would be small
and pgstat is subject to losing data anyway.  However, we are definitely
moving (slowly) in the direction of making pgstat more trustworthy, so
it's probably best not to take the risk of dropping useful stats.

Call removed from 8.1.x and up.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:

> > What I'm inclined to propose is that we just remove the
> > pgstat_drop_relation() call from smgr_internal_unlink
> On checking the CVS history, that in fact is how the code worked before
> 8.1.3, when I introduced the bogus call in a fit of over-optimization :-(.

I'm worried that we leave garbage entries in the stats. As you suggested,
don't we need to remove unreferenced entries from stats periodically?

> > and rely entirely
> > on VACUUM to clean out dead entries in the pgstats data.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> I'm worried that we leave garbage entries in the stats. As you suggested,
> don't we need to remove unreferenced entries from stats periodically?

VACUUM does that already.

            regards, tom lane