Re: logical replication of truncate command with trigger causes Assert

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> On Wed, Jun 9, 2021 at 5:29 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 2. Decide that we ought to ensure that a snapshot exists throughout
>> most of this code. It's not entirely obvious to me that there is no
>> code path reachable from, say, apply_handle_truncate's collection of
>> relation OIDs that needs a snapshot. If we went for that, I'd think
>> the right solution is to do PushActiveSnapshot right after each
>> ensure_transaction call, and then PopActiveSnapshot on the way out of
>> the respective subroutine. We could then drop the snapshot management
>> calls that are currently associated with the executor state.

> +1 for the second option as with that, apart from what you said it
> will take off some load from future developers to decide which part of
> changes should be after acquiring snapshot.

Here's a draft patch for that. I decided the most sensible way to
organize this is to pair the existing ensure_transaction() subroutine
with a cleanup subroutine. Rather unimaginatively, perhaps, I renamed
it to begin_transaction_step and named the cleanup end_transaction_step.
(Better ideas welcome.)

As written, this'll result in creating and deleting a snapshot for some
stream-control messages that maybe don't need one; but the point here is
not to have to think too hard about whether they do, so that's OK with
me. There are more CommandCounterIncrement calls than before, too,
but (a) those are cheap if there's nothing to do and (b) it's not real
clear to me that the extra calls are not necessary.

Somewhat unrelated, but ... am I reading the code correctly that
apply_handle_stream_start and related routines are using Asserts
to check that the remote sent stream-control messages in the correct
order? That seems many degrees short of acceptable.

regards, tom lane