Hi,
On Fri, Mar 07, 2025 at 10:26:23AM +0530, Amit Kapila wrote:
> On Fri, Mar 7, 2025 at 3:19 AM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> >
> > On Wed, Mar 5, 2025 at 4:05 AM Bertrand Drouvot
> > <bertranddrouvot...@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Mar 05, 2025 at 02:42:15PM +0530, Amit Kapila wrote:
> > > > On Wed, Mar 5, 2025 at 12:47 PM Bertrand Drouvot
> > > > <bertranddrouvot...@gmail.com> wrote:
> > > > >
> > > > > Agree, PFA a patch doing so.
> > > > >
> > > >
> > > > It would be better if you could add a few comments atop the
> > > > permutation line to explain the working of the test.
> > >
> > > yeah makes sense. Done in the attached, and bonus point I realized that
> > > the
> > > test could be simplified (so, removing useless steps in passing).
> > >
> >
> > Thank you for the patch.
> >
> > The new simplified test case can be pretty-formatted as:
> >
> > init
> > begin
> > savepoint
> > truncate
> > checkpoint-1
> > get_changes-1
> > commit
> > checkpoint-2
> > get_changes-2
> > info_catchange check
> > info_committed check
> > meta check
Yes.
> > IIUC if another checkpoint happens between get_change-2 and the
> > subsequent checks, the first snapshot would be removed during the
> > checkpoint, resulting in a test failure.
Good catch! Yeah you're right, thanks!
> I think we could check the
> > snapshot files while one transaction keeps open. The more simplified
> > test case would be:
> >
> > init
> > begin
> > savepoint
> > insert(cat-change)
> > begin
> > insert(cat-change)
> > commit
> > checkpoint
> > get_changes
> > info_catchange check
> > info_committed check
> > meta check
> > commit
> >
> > In this test case, we would have at least one serialized snapshot that
> > has both cat-changes and committed txns. What do you think?
Indeed, I think that would prevent snapshots to be removed.
The attached ends up doing:
init
begin
savepoint
truncate table1
create table table2
checkpoint
get_changes
info check
meta check
commit
As the 2 ongoing catalog changes and the committed catalog change are part of
the
same snapshot, then I grouped the catchanges and committed changes checks in the
same "info check".
> Your proposed change in the test sounds better than what we have now
> but I think we should also avoid autovacuum to perform analyze as that
> may add additional counts. For test_decoding, we keep
> autovacuum_naptime = 1d in logical.conf file, we can either use the
> same here or simply keep autovacuum off.
When writing the attached, I initially added extra paranoia in the tests by
using ">=", does that also address your autovacuum concern?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From 415d707187070df88e11e9bb1059309ab102953e Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Wed, 5 Mar 2025 07:06:58 +0000
Subject: [PATCH v3] Modify pg_logicalinspect isolation test
The previous version was relying on the fact that the test produces exactly
2 snapshots on disk, while in fact it can produce more. Changing the test knowing
that at least 2 snapshots are generated.
In passing, removing useless steps and adding some comments.
Per buildfarm member skink.
---
.../expected/logical_inspect.out | 44 +++++--------------
.../specs/logical_inspect.spec | 18 +++++---
2 files changed, 24 insertions(+), 38 deletions(-)
57.1% contrib/pg_logicalinspect/expected/
42.8% contrib/pg_logicalinspect/specs/
diff --git a/contrib/pg_logicalinspect/expected/logical_inspect.out b/contrib/pg_logicalinspect/expected/logical_inspect.out
index d95efa4d1e5..c86711e471d 100644
--- a/contrib/pg_logicalinspect/expected/logical_inspect.out
+++ b/contrib/pg_logicalinspect/expected/logical_inspect.out
@@ -1,6 +1,6 @@
Parsed test spec with 2 sessions
-starting permutation: s0_init s0_begin s0_savepoint s0_truncate s1_checkpoint s1_get_changes s0_commit s0_begin s0_insert s1_checkpoint s1_get_changes s0_commit s1_get_changes s1_get_logical_snapshot_info s1_get_logical_snapshot_meta
+starting permutation: s0_init s0_begin s0_savepoint s0_truncate s1_create_table s1_checkpoint s1_get_changes s1_get_logical_snapshot_info s1_get_logical_snapshot_meta s0_commit
step s0_init: SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding');
?column?
--------
@@ -10,43 +10,23 @@ init
step s0_begin: BEGIN;
step s0_savepoint: SAVEPOINT sp1;
step s0_truncate: TRUNCATE tbl1;
+step s1_create_table: CREATE TABLE tbl2 (val1 integer, val2 integer);
step s1_checkpoint: CHECKPOINT;
step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0');
data
----
(0 rows)
-step s0_commit: COMMIT;
-step s0_begin: BEGIN;
-step s0_insert: INSERT INTO tbl1 VALUES (1);
-step s1_checkpoint: CHECKPOINT;
-step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0');
-data
----------------------------------------
-BEGIN
-table public.tbl1: TRUNCATE: (no-flags)
-COMMIT
-(3 rows)
-
-step s0_commit: COMMIT;
-step s1_get_changes: SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0');
-data
--------------------------------------------------------------
-BEGIN
-table public.tbl1: INSERT: val1[integer]:1 val2[integer]:null
-COMMIT
-(3 rows)
-
-step s1_get_logical_snapshot_info: SELECT info.state, info.catchange_count, array_length(info.catchange_xip,1) AS catchange_array_length, info.committed_count, array_length(info.committed_xip,1) AS committed_array_length FROM pg_ls_logicalsnapdir(), pg_get_logical_snapshot_info(name) AS info ORDER BY 2;
-state |catchange_count|catchange_array_length|committed_count|committed_array_length
-----------+---------------+----------------------+---------------+----------------------
-consistent| 0| | 2| 2
-consistent| 2| 2| 0|
-(2 rows)
+step s1_get_logical_snapshot_info: SELECT count(*) > 0 as has_info FROM pg_ls_logicalsnapdir(), pg_get_logical_snapshot_info(name) AS info where info.catchange_count >= 2 and array_length(info.catchange_xip,1) >= 2 and info.committed_count >= 1 and array_length(info.committed_xip,1) >= 1;
+has_info
+--------
+t
+(1 row)
-step s1_get_logical_snapshot_meta: SELECT COUNT(meta.*) from pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta;
-count
------
- 2
+step s1_get_logical_snapshot_meta: SELECT COUNT(meta.*) > 0 AS has_meta from pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta;
+has_meta
+--------
+t
(1 row)
+step s0_commit: COMMIT;
diff --git a/contrib/pg_logicalinspect/specs/logical_inspect.spec b/contrib/pg_logicalinspect/specs/logical_inspect.spec
index 9851a6c18e4..a6bc58ae955 100644
--- a/contrib/pg_logicalinspect/specs/logical_inspect.spec
+++ b/contrib/pg_logicalinspect/specs/logical_inspect.spec
@@ -1,6 +1,6 @@
# Test the pg_logicalinspect functions: that needs some permutation to
-# ensure that we are creating multiple logical snapshots and that one of them
-# contains ongoing catalogs changes.
+# ensure that we are creating at least one snapshot that contains ongoing and
+# committed catalogs changes.
setup
{
DROP TABLE IF EXISTS tbl1;
@@ -11,6 +11,7 @@ setup
teardown
{
DROP TABLE tbl1;
+ DROP TABLE tbl2;
SELECT 'stop' FROM pg_drop_replication_slot('isolation_slot');
DROP EXTENSION pg_logicalinspect;
}
@@ -21,14 +22,19 @@ step "s0_init" { SELECT 'init' FROM pg_create_logical_replication_slot('isolatio
step "s0_begin" { BEGIN; }
step "s0_savepoint" { SAVEPOINT sp1; }
step "s0_truncate" { TRUNCATE tbl1; }
-step "s0_insert" { INSERT INTO tbl1 VALUES (1); }
step "s0_commit" { COMMIT; }
session "s1"
setup { SET synchronous_commit=on; }
step "s1_checkpoint" { CHECKPOINT; }
+step "s1_create_table" { CREATE TABLE tbl2 (val1 integer, val2 integer); }
step "s1_get_changes" { SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'skip-empty-xacts', '1', 'include-xids', '0'); }
-step "s1_get_logical_snapshot_meta" { SELECT COUNT(meta.*) from pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta;}
-step "s1_get_logical_snapshot_info" { SELECT info.state, info.catchange_count, array_length(info.catchange_xip,1) AS catchange_array_length, info.committed_count, array_length(info.committed_xip,1) AS committed_array_length FROM pg_ls_logicalsnapdir(), pg_get_logical_snapshot_info(name) AS info ORDER BY 2; }
+step "s1_get_logical_snapshot_meta" { SELECT COUNT(meta.*) > 0 AS has_meta from pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta; }
+step "s1_get_logical_snapshot_info" { SELECT count(*) > 0 as has_info FROM pg_ls_logicalsnapdir(), pg_get_logical_snapshot_info(name) AS info where info.catchange_count >= 2 and array_length(info.catchange_xip,1) >= 2 and info.committed_count >= 1 and array_length(info.committed_xip,1) >= 1; }
-permutation "s0_init" "s0_begin" "s0_savepoint" "s0_truncate" "s1_checkpoint" "s1_get_changes" "s0_commit" "s0_begin" "s0_insert" "s1_checkpoint" "s1_get_changes" "s0_commit" "s1_get_changes" "s1_get_logical_snapshot_info" "s1_get_logical_snapshot_meta"
+# s0 does not commit until the end of the test. This is needed to ensure that
+# a checkpoint will not remove any snapshots. s0 produces 2 ongoing catalog changes
+# (the truncate and its parent transaction). s1 produces a committed catalog change.
+# So that the get_changes produces (at least) one snapshot that contains 2
+# ongoing catalog changes and the committed catalog change.
+permutation "s0_init" "s0_begin" "s0_savepoint" "s0_truncate" "s1_create_table" "s1_checkpoint" "s1_get_changes" "s1_get_logical_snapshot_info" "s1_get_logical_snapshot_meta" "s0_commit"
--
2.34.1
