Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)

At Thu, 25 Aug 2022 11:44:34 +0200, "Drouvot, Bertrand" <bdrouvot(at)amazon(dot)com> wrote in
> Looking closer at it, I think we are already good for the drop case on
> the tables (by making direct use of the pg_stat_get_* functions on the
> before dropped oid).
>
> So I think we can remove all the "table" new checks: new patch
> attached is doing so.
>
> On the other hand, for the index case, I think it's better to keep the
> "committed index creation one".

I agree.

> Indeed, to check that the drop behaves correctly I think it's better
> in "the same test" to ensure we've had the desired result before the
> drop (I mean having pg_stat_have_stats() returning false after a drop
> does not really help if we are not 100% sure it was returning true for
> the exact same index before the drop).

Sounds reasonable.

> > We have other variable-numbered stats kinds
> > FUNCTION/REPLSLOT/SUBSCRIPTION. Don't we need the same for these?
>
> I don't think we need more tests for the FUNCTION case (as it looks to
> me it is already covered in stat.sql by the
> pg_stat_get_function_calls() calls on the dropped functions oids).

Right.

> For SUBSCRIPTION, i think this is covered in 026_stats.pl:
>
> # Subscription stats for sub1 should be gone
> is( $node_subscriber->safe_psql(
>         $db, qq(SELECT pg_stat_have_stats('subscription', 0,
> $sub1_oid))),
>     qq(f),
>     qq(Subscription stats for subscription '$sub1_name' should be
> removed.));
>
> For REPLSLOT, I agree that we can add one test: I added it in
> contrib/test_decoding/sql/stats.sql. It relies on pg_stat_have_stats()
> (as relying on pg_stat_replication_slots and/or
> pg_stat_get_replication_slot() would not help that much for this test
> given that the slot has been removed from ReplicationSlotCtl)

Thanks for the searching.

+-- pg_stat_have_stats returns true for regression_slot_stats1
+-- Its index is 1 in ReplicationSlotCtl->replication_slots
+select pg_stat_have_stats('replslot', 0, 1);

This is wrong. The index is actually 0. We cannot know the id
reliably since we don't expose it at all. We could slightly increase
robustness by assuming the range of the id but that is just moving the
problem to another place. If the test is broken by a change of
replslot id assignment policy, it would be easily found and fixed.

So is it fine simply fixing the comment with the correct ID?

Or, contrarily we can be more sensitive to the change of ID assignment
policy by checking all the replication slots.

select count(n) from generate_series(0, 2) as n where pg_stat_have_stats('replslot', 0, n);

The number changes from 3 to 0 across the slots drop.. If any of the
slots has gone out of the range, the number before the drop decreases.

> Attaching v3-0001 (with the right "numbering" this time ;-) )

Yeah, Looks fine:p

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center