RE: Introduce XID age and inactive timeout based replication slot invalidation

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: Raw Message | Whole Thread | 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

In response to

Browse pgsql-hackers by date

  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.