| From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> | 
|---|---|
| To: | Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> | 
| Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com>, 'vignesh C' <vignesh21(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Ajin Cherian <itsajin(at)gmail(dot)com>, Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> | 
| Subject: | RE: Introduce XID age and inactive timeout based replication slot invalidation | 
| Date: | 2025-02-04 12:16:28 | 
| Message-ID: | OSCPR01MB14966A918EBB0674E5423EDE0F5F42@OSCPR01MB14966.jpnprd01.prod.outlook.com | 
| Views: | Whole Thread | Raw Message | Download mbox | Resend email | 
| Thread: | |
| Lists: | pgsql-hackers | 
Dear Nisha,
Thanks for updating the patch! Here are my comments.
01.
```
+# Test for replication slots invalidation
```
Since the file tests only timeout invalidations, the comment seems too general.
02.
```
+       # Check that an invalidated slot cannot be acquired
+       my ($result, $stdout, $stderr);
+       ($result, $stdout, $stderr) = $node->psql(
+               'postgres', qq[
+                       SELECT pg_replication_slot_advance('$slot', '0/1');
+       ]);
+       ok( $stderr =~ /can no longer access replication slot "$slot"/,
+               "detected error upon trying to acquire invalidated slot $slot on node $node_name"
+         )
+         or die
+         "could not detect error upon trying to acquire invalidated slot $slot on node $node_name";
```
This part can be removal because this is not directly related with timeout invalidation.
If needed this can be outside the function and we can confirm only once.
03.
```
+# Initialize primary
+my $node = PostgreSQL::Test::Cluster->new('primary');
+$node->init(allows_streaming => 'logical');
```
I think this node is not "primary" because there are no standby nodes. We can use new('node').
Also some comments which used "primary" can be removed.
04.
```
+$node->psql('postgres',
+       q{SELECT pg_create_logical_replication_slot('logical_slot', 'test_decoding');}
+);
```
Please use safe_psql() instead of psql().
05.
```
my $logstart = -s $node->logfile;
```
According to other tests, the variable name can be $log_offset.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
| From | Date | Subject | |
|---|---|---|---|
| Next Message | vignesh C | 2025-02-04 12:46:24 | Re: Restrict publishing of partitioned table with a foreign table as partition | 
| Previous Message | Shlok Kyal | 2025-02-04 12:09:05 | Re: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility. |