From: | "Drouvot, Bertrand" <bdrouvot(at)amazon(dot)com> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back) |
Date: | 2022-09-01 06:40:54 |
Message-ID: | 3cd209a4-e2a1-0ae6-91a3-c945eb7db248@amazon.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 8/31/22 9:26 PM, Andres Freund wrote:
> Hiu,
>
> On 2022-08-25 11:44:34 +0200, Drouvot, Bertrand wrote:
>> 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)
> As Horiguchi-san noted, we can't rely on specific indexes being used.
Yeah.
> I feel
> ok with the current coverage in that area, but if we *really* feel we need to
> test it, we'd need to count the number of indexes with slots before dropping
> the slot and after the drop.
Thanks for the suggestion, I'm coming up with this proposal in v4 attached:
* count the number of slots
* ensure we have at least one for which pg_stat_have_stats() returns true
* get the list of ids (true_ids) for which pg_stat_have_stats()
returns true
* drop all the slots
* get the list of ids (false_ids) for which pg_stat_have_stats()
returns false
* ensure that both lists (true_ids and false_ids) are the same
I don't "really" feel the need we need to test it but i think that this
thread was a good opportunity to try to test it.
That said, that's also fine for me if this test is not part of the patch.
Maybe a better/simpler option could be to expose a function to get the
slot id based on its name and then write a "simple" test with it? (If so
I think that would better to start another patch/thread).
>> +-- pg_stat_have_stats returns true for committed index creation
> Maybe another test for an uncommitted index creation would be good too?
You mean in addition to the "-- pg_stat_have_stats returns false for
rolled back index creation" one?
> Could you try running this test with debug_discard_caches = 1 - it's pretty
> easy to write tests in this area that aren't reliable timing wise.
Thanks for the suggestion. I did and it passed without any issues.
>> +CREATE table stats_test_tab1 as select generate_series(1,10) a;
>> +CREATE index stats_test_idx1 on stats_test_tab1(a);
>> +SELECT oid AS dboid from pg_database where datname = current_database() \gset
> Since you introduced this, maybe convert the other instance of this query at
> the end of the file as well?
yeah good point. In v4, I moved the dboid recording at the top and use
it when appropriate.
Regards,
--
Bertrand Drouvot
Amazon Web Services:https://aws.amazon.com
Attachment | Content-Type | Size |
---|---|---|
v4-0001-pgstat_drop_relation-for-indexes.patch | text/plain | 11.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2022-09-01 07:12:19 | broken table formatting in psql |
Previous Message | Junwang Zhao | 2022-09-01 06:38:01 | Re: [PATCH v1] [doc] polish the comments of reloptions |