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

Hi,

On Thu, Jun 25, 2026 at 5:26 AM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)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