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

Hi,

On 8/25/22 4:47 AM, Kyotaro Horiguchi wrote:
> Good catch, and thanks for the patch!
>
> (The file name would correctly be v2-0001-...:)
>
> At Tue, 23 Aug 2022 09:58:03 +0200, "Drouvot, Bertrand" <bdrouvot@amazon.com> wrote in
>> Agree it's better to move it to heap_create(): it's done in the new
>> version attached.
> +1 (not considering stats splitting)
>
>> We'll see later on if it needs to be duplicated for the table/index
>> split work.
> The code changes looks good to me.

Thanks for looking at it!

> +-- pg_stat_have_stats returns true for table creation inserting data
> +-- pg_stat_have_stats returns true for committed index creation
> +
>
> Not sure we need this, as we check that already in the same file. (In
> other words, if we failed this, the should have failed earlier.)

That's right.

> Maybe
> we only need check for drop operations

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".

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).

> 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).

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)

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

Regards,

-- 
Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com