| 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
| 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 |