Horton
Menu

Thread

  1. Add tests for concurrent DML retry paths in logical replication apply

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2026-04-27T07:30:00Z

    Hi hackers,

While reading the logical replication apply code in execReplication.c, I
noticed that the retry paths in RelationFindReplTupleByIndex and
RelationFindReplTupleSeq for concurrent updates and deletes have no test
coverage [1]. Specifically, when the same tuple is being updated/deleted on
the publisher and subscriber at the same time, the dirty snapshot finds the
tuple being modified by another transaction, the apply worker waits and
retries the index/sequential scan.

The attached patch adds an injection point before table_tuple_lock and a
TAP test exercising these retry paths, hitting both TM_Updated and
TM_Deleted.

While working on this, I also noticed minor issues in the conflict handling
code:

1/ In RelationFindReplTupleByIndex, ExecMaterializeSlot was called before
checking should_refetch_tuple. If the tuple needs to be refetched due to a
concurrent modification, this materialization is wasted work. Moved it
after the retry check, so it only runs when we've successfully locked the
tuple.

2/ In RelationFindReplTupleSeq, ExecCopySlot and a separate TupleTableSlot
allocation were unnecessary. Made this function consistent with
RelationFindReplTupleByIndex by using outslot directly while scanning the
heap, avoiding the extra TTS allocation and copy overhead.

I'm aware that these are not major performance issues in practice, but it
keeps the two functions consistent and avoids unnecessary TTS materialize
and copy costs.

I also think that we could deduplicate these two functions since the code
looks mostly the same, but that would be an overkill IMHO.

Please find the attached patch. Appreciate any feedback. Thank you!

[1]
https://coverage.postgresql.org/src/backend/executor/execReplication.c.gcov.html

-- 
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

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

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-04-28T07:47:54Z

    Dear Bharath,


> While reading the logical replication apply code in execReplication.c, I noticed
> that the retry paths in RelationFindReplTupleByIndex and RelationFindReplTupleSeq
> for concurrent updates and deletes have no test coverage [1]. Specifically,
> when the same tuple is being updated/deleted on the publisher and subscriber at
> the same time, the dirty snapshot finds the tuple being modified by another
> transaction, the apply worker waits and retries the index/sequential scan.

Good catch.

> The attached patch adds an injection point before table_tuple_lock and a TAP test
> exercising these retry paths, hitting both TM_Updated and TM_Deleted.

I read  the code briefly. Here are questions:

1. 
The test looks like to add the test for retry acquiring the lock. But there is
another retry path, which waits till xwait finishes. Do you have a reason to
skip testing?

2.
Is it OK to use the same injection point name twice? I cannot find the existing
case.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  3. Re: Add tests for concurrent DML retry paths in logical replication apply

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2026-04-29T01:45:00Z

    Hi,

On Tue, Apr 28, 2026 at 12:47 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > While reading the logical replication apply code in execReplication.c, I noticed
> > that the retry paths in RelationFindReplTupleByIndex and RelationFindReplTupleSeq
> > for concurrent updates and deletes have no test coverage [1]. Specifically,
> > when the same tuple is being updated/deleted on the publisher and subscriber at
> > the same time, the dirty snapshot finds the tuple being modified by another
> > transaction, the apply worker waits and retries the index/sequential scan.
>
> Good catch.
>
> > The attached patch adds an injection point before table_tuple_lock and a TAP test
> > exercising these retry paths, hitting both TM_Updated and TM_Deleted.
>
> I read  the code briefly. Here are questions:

Thank you for reviewing!

> 1.
> The test looks like to add the test for retry acquiring the lock. But there is
> another retry path, which waits till xwait finishes. Do you have a reason to
> skip testing?

There are two retry paths, and I added TAP tests covering both. The
XactLockTableWait path is tested by holding an in-progress transaction
on the subscriber, which naturally blocks the apply worker until the
transaction finishes, then it retries the scan. The table_tuple_lock
retry path (TM_Updated / TM_Deleted) is tested using an injection
point that pauses the worker between finding and locking the tuple,
allowing concurrent DML to intervene.

> 2.
> Is it OK to use the same injection point name twice? I cannot find the existing
> case.

Good catch. I used separate injection points for each path.

Please find the attached v2 patch for further review. Thank you!

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

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

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-06-25T12:26:56Z

    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

  5. Re: Add tests for concurrent DML retry paths in logical replication apply

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2026-06-27T08:54:59Z

    Hi,

On Thu, Jun 25, 2026 at 5:26 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Thanks for updating and sorry for the late reply. I reviewed your patch.
> Basically it looks good, but here are my comments.

Thank you for reviewing!

> ```
> @@ -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?

The first part of the fix is this. RelationFindReplTupleSeq currently
allocates an additional tuple slot and copies the result slot into the
output slot. This is a redundant tuple copy, unlike its friend
RelationFindReplTupleByIndex, which directly stores the result tuple
into the output slot. I fixed RelationFindReplTupleSeq to use the same
approach that RelationFindReplTupleByIndex uses.

The second part of the fix is this. Before waiting for the in-progress
transaction to finish, we materialize the tuple slot. This, to me, is
unnecessary - the scan (sequential or index) already pins the buffer
referred to by the tuple slot (of TTSOpsBufferHeapTuple type), and a
pinned buffer is never evicted by the clock-sweep (StrategyGetBuffer
skips any buffer with a non-zero refcount), so materializing the slot
to guard against eviction is unnecessary. Once the wait for the
in-progress transaction is over, we don't do anything with the
materialized slot or the previous output slot, because we restart the
lookup anyway. Another important point is that we don't even need the
slot materialized before taking the tuple lock, because the tuple lock
relies only on the TID of the row found in the loop above (sequential
or index scan) - it fills in the slot with the tuple and transfers the
buffer pin anyway. So in the attached v3 patch I ended up
materializing the slot just once, before returning it to the caller -
neither the wait for the in-progress transaction nor the tuple-lock
retry path needs it.

> ```
> +# 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.

Fixed.

> ```
>         $node_subscriber->append_conf('postgresql.conf',
>                 "shared_preload_libraries = 'injection_points'");
>         $node_subscriber->restart;
> ```
>
> No need to set the shared_preload_libraries and restart.

Fixed.

Please find the attached v3 patch for further review. I would be happy
to split this patch into two if necessary.

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com