RE: Add tests for concurrent DML retry paths in logical replication apply

From: "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>
To: 'Bharath Rupireddy' <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: RE: Add tests for concurrent DML retry paths in logical replication apply
Date: 2026-06-25 12:26:56
Message-ID: OS9PR01MB121491200B47154926F826084F5EC2@OS9PR01MB12149.jpnprd01.prod.outlook.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dear Bharath,

Thanks for updating and sorry for the late reply. I reviewed your patch.
Basically it looks good, but here are my comments.

```
@@ -439,10 +442,12 @@ retry:

if (should_refetch_tuple(res, &tmfd))
goto retry;
+
+ /* Materialize the slot so it preserves pass-by-ref values. */
+ ExecMaterializeSlot(outslot);
```

Can you tell me why ExecMaterializeSlot() needs to be added? Maybe you've added
to make the code consistent with RelationFindReplTupleByIndex(), but till now
any errors have not been reported due to the missing materialization. Is there a
possibility that even RelationFindReplTupleByIndex() have done the unnecessary
materialization?

```
+# Create subscriber.
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
```

Minor point; allows_streaming is not set for the logical.

```
$node_subscriber->append_conf('postgresql.conf',
"shared_preload_libraries = 'injection_points'");
$node_subscriber->restart;
```

No need to set the shared_preload_libraries and restart.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Dilip Kumar 2026-06-25 12:59:04 Re: Proposal: Conflict log history table for Logical Replication
Previous Message Jakub Wartak 2026-06-25 12:19:36 Re: Adding basic NUMA awareness