Date: Wed, 5 Mar 2025 12:05:16 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Peter Smith <smithpb2250@gmail.com>,
	Peter Eisentraut <peter@eisentraut.org>,
	shveta malik <shveta.malik@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Add contrib/pg_logicalsnapinspect
Message-ID: <Z8g9/GeoXv/gIL60@ip-10-97-1-34.eu-west-3.compute.internal>
References: <Zwkk9Z6rzNxArr1p@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoB31apHCzZ-45rDtB56qQm+TBwJnB48JjBvuqR5xviAEQ@mail.gmail.com>
 <CAD21AoD+opO0SaDSFvz+n+wAxc-kUrpF5Wtxbw80Bqpay+rkgw@mail.gmail.com>
 <CAHut+PsYdTW4+U+fioh9_yO8iT6RnY=PquzfCZu7zRU6Qg6W7g@mail.gmail.com>
 <Zwy4ys76aPcoVE/I@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoDuO87MFrPJFwtmRi=cFhDGac0zq7sq5pdEhVi7zUvbEg@mail.gmail.com>
 <5qbxud4pvnvmtuoi7weiizm5hmumxaeohx4vztfhrwlfhyz6rj@buh4435mllwo>
 <CAD21AoBM6vAcPGR-ng0nqGG0yemR_ufdg3+v3gkPa6Nc2ntnrA@mail.gmail.com>
 <Z8f6dQPR67ahM2DN@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA4eK1L0bZbZOr8-VNFXDfqaTwggdf_-HTAsneui+_ozMTvZQg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="wIhah5S3tYpa8EFa"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAA4eK1L0bZbZOr8-VNFXDfqaTwggdf_-HTAsneui+_ozMTvZQg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z8g9/GeoXv/gIL60%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk


--wIhah5S3tYpa8EFa
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

--wIhah5S3tYpa8EFa
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Modify-pg_logicalinspect-isolation-test.patch"

From 6154e847e4e5c2c7eb816d4302a4704d5a690954 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Wed, 5 Mar 2025 07:06:58 +0000
Subject: [PATCH v2] 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              | 36 ++++++++-----------
 .../specs/logical_inspect.spec                | 11 +++---
 2 files changed, 22 insertions(+), 25 deletions(-)
  58.4% contrib/pg_logicalinspect/expected/
  41.5% contrib/pg_logicalinspect/specs/

diff --git a/contrib/pg_logicalinspect/expected/logical_inspect.out b/contrib/pg_logicalinspect/expected/logical_inspect.out
index d95efa4d1e5..b7d01dbb68b 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_checkpoint s1_get_changes s0_commit s1_checkpoint s1_get_changes s1_get_logical_snapshot_info_catchange s1_get_logical_snapshot_info_committed s1_get_logical_snapshot_meta
 step s0_init: SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding');
 ?column?
 --------
@@ -17,8 +17,6 @@ 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                                   
@@ -28,25 +26,21 @@ 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_catchange: SELECT count(*) > 0 as has_catchange 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 = 0;
+has_catchange
+-------------
+t            
+(1 row)
 
-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_committed: SELECT count(*) > 0 as has_committed FROM pg_ls_logicalsnapdir(), pg_get_logical_snapshot_info(name) AS info where info.committed_count = 2 and array_length(info.committed_xip,1) = 2 and info.catchange_count = 0;
+has_committed
+-------------
+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.*) > 1 AS has_meta from pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta;
+has_meta
+--------
+t       
 (1 row)
 
diff --git a/contrib/pg_logicalinspect/specs/logical_inspect.spec b/contrib/pg_logicalinspect/specs/logical_inspect.spec
index 9851a6c18e4..631daf5db6c 100644
--- a/contrib/pg_logicalinspect/specs/logical_inspect.spec
+++ b/contrib/pg_logicalinspect/specs/logical_inspect.spec
@@ -21,14 +21,17 @@ 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_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.*) > 1 AS has_meta from pg_ls_logicalsnapdir(), pg_get_logical_snapshot_meta(name) as meta; }
+step "s1_get_logical_snapshot_info_catchange" { SELECT count(*) > 0 as has_catchange 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 = 0; }
+step "s1_get_logical_snapshot_info_committed" { SELECT count(*) > 0 as has_committed FROM pg_ls_logicalsnapdir(), pg_get_logical_snapshot_info(name) AS info where info.committed_count = 2 and array_length(info.committed_xip,1) = 2 and info.catchange_count = 0; }
 
-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"
+# The first get_changes produces (at least) one snapshot that contains 2 catchanges
+# (the truncate and its parent transaction). The second get_changes produces one
+# snapshot that contains the 2 transactions above as committed.
+permutation "s0_init" "s0_begin" "s0_savepoint" "s0_truncate" "s1_checkpoint" "s1_get_changes" "s0_commit" "s1_checkpoint" "s1_get_changes" "s1_get_logical_snapshot_info_catchange" "s1_get_logical_snapshot_info_committed" "s1_get_logical_snapshot_meta"
-- 
2.34.1


--wIhah5S3tYpa8EFa--