Обсуждение: pg_recvlogical: Prevent flushed data from being re-sent after restarting replication

Hi,

When pg_recvlogical loses connection, it reconnects and restarts
replication unless
--no-loop option is used. I noticed that in this scenario, data that
has already been
flushed can be re-sent after restarting replication. This happens
because the replication
start position used when restarting replication is taken from the write position
in the last status update message, which may be older than the actual
position of
the last flushed data. As a result, some flushed data could exist newer than
the replication start position and be re-sent. Is this a bug?

To fix this issue, I'd like to propose the attached patch that fixes
this by ensuring
all written data is flushed to disk before restarting replication and by using
the last flushed position as the replication start point. This prevents already
flushed data from being re-sent.

Additionally, when the --no-loop option is used, I found that pg_recvlogical
could previously exit without flushing written data, risking data loss.
The attached patch fixes this issue by also ensuring that all data is flushed
to disk before exiting with --no-loop.

Thought?

Regards,


-- 
Fujii Masao

Hi Fujii,


I see what you mean. For reviewing I started with writing a test that 
just reproduces the bug and documents the current behaviour.

I expected that by applying your patch the test would fail, but then we 
would just update the test accordingly. Surprisingly the test continues 
to pass.

I attached the test for your consideration. I'll have a look again 
tomorrow at both.


Kind regards,

Mircea Cadariu

Hi,


Update: I added another test to the attached test-only patch. This new 
test uses pg_terminate_backend to trigger reconnection.

Assuming the tests are fully correct (your input appreciated on this) we 
can use them to validate the solution.


Kind regards,

Mircea Cadariu

On Wed, Nov 19, 2025 at 11:36 PM Mircea Cadariu
<cadariu.mircea@gmail.com> wrote:
>
> Hi,
>
>
> Update: I added another test to the attached test-only patch. This new
> test uses pg_terminate_backend to trigger reconnection.
>
> Assuming the tests are fully correct (your input appreciated on this) we
> can use them to validate the solution.

Thanks for testing!

BTW, I reproduced the issue as follows:

#1. Start the server

#2. Start pg_recvlogical:
$ pg_recvlogical -S myslot -d postgres --create-slot --start -f test.out

#3. Insert data every second:
$ psql
=# create table t(i serial);
=# insert into t values(default);
=# \watch 1

#4. In a separate session, terminate walsender to force pg_recvlogical
to restart replication:
$ psql
=# select pg_terminate_backend(pid) from pg_stat_replication;

#5. Wait for pg_recvlogical to restart replication

#6. You will see duplicate records written to the output file, for example:
$ cat test.out
BEGIN 798
table public.t: INSERT: i[integer]:42
COMMIT 798
BEGIN 799
table public.t: INSERT: i[integer]:43
COMMIT 799
BEGIN 792
table public.t: INSERT: i[integer]:36
COMMIT 792
BEGIN 793
table public.t: INSERT: i[integer]:37
COMMIT 793
BEGIN 794
table public.t: INSERT: i[integer]:38
COMMIT 794
BEGIN 795
table public.t: INSERT: i[integer]:39
COMMIT 795
BEGIN 796
table public.t: INSERT: i[integer]:40
COMMIT 796
BEGIN 797
table public.t: INSERT: i[integer]:41
COMMIT 797
BEGIN 798
table public.t: INSERT: i[integer]:42
COMMIT 798
BEGIN 799
table public.t: INSERT: i[integer]:43
COMMIT 799


With the patch applied, these duplicate records no longer appear in
the pg_recvlogical output.

Regards,

--
Fujii Masao

On 19/11/2025 14:54, Fujii Masao wrote:

> With the patch applied, these duplicate records no longer appear in
> the pg_recvlogical output.

Thanks! Works like a charm. I confirm duplicates no longer appear with 
the patch applied.

You can consider adding a test about this in "030_pg_recvlogical.pl", 
proposal below:


use IPC::Run qw(start);
my $outfile = $node->basedir . '/reconnect.out';

$node->command_ok(
     [
         'pg_recvlogical',
         '--slot' => 'reconnect_test',
         '--dbname' => $node->connstr('postgres'),
         '--create-slot',
     ],
     'slot created for reconnection test');

$node->safe_psql('postgres', 'CREATE TABLE t(x int);');
$node->safe_psql('postgres', 'INSERT INTO t VALUES (1);');

my $recv = start [
     'pg_recvlogical',
     '--slot', 'reconnect_test',
     '--dbname', $node->connstr('postgres'),
     '--start',
     '--file', $outfile,
     '--fsync-interval', '1',
     '--status-interval', '60',
     '--verbose'
], '>', \my $out, '2>', \my $err;

sleep(3);

my $backend_pid = $node->safe_psql('postgres',
     "SELECT active_pid FROM pg_replication_slots WHERE slot_name = 
'reconnect_test'");

if ($backend_pid ne '')
{
     $node->safe_psql('postgres', "SELECT 
pg_terminate_backend($backend_pid)");
}

sleep(6);

$node->safe_psql('postgres', 'INSERT INTO t VALUES (2);');

sleep(3);

$recv->signal('TERM');
$recv->finish();

open(my $file, '<', $outfile);
my $count = 0;
while (<$file>) {
     if (/INSERT/) {
         $count = $count + 1;
     }
}
close($file);

cmp_ok($count, '==', 2, 'two INSERTs');

$node->command_ok(
     [
         'pg_recvlogical',
         '--slot' => 'reconnect_test',
         '--dbname' => $node->connstr('postgres'),
         '--drop-slot'
     ],
     'reconnect_test slot dropped');


-- 
Regards,
Mircea Cadariu

Hi,


An update: I have two topics from the review perspective.

On the test I proposed to be added to 030_pg_recvlogical.pl, I found a 
way to write it without using sleeps (which risk flakyness in CI). I've 
attached it as a patch for your consideration. I checked the test in the 
following way: on master it fails, but with your patch it passes.

Secondly I noticed in function sendFeedback at line 166, the startpos is 
set to output_written_lsn. This seems to counter conceptually the change 
you made in the patch, however it seems to not affect correctness. Shall 
we remove this line as to avoid confusion?