logical replication of truncate command with trigger causes Assert

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

logical replication of truncate command with trigger causes Assert

Mark Dilger-5
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

Mark Dilger-5
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

akapila
On Wed, Jun 9, 2021 at 5:29 AM Tom Lane <[hidden email]> wrote:

>
> Mark Dilger <[hidden email]> writes:
> > On Jun 8, 2021, at 3:55 PM, Tom Lane <[hidden email]> wrote:
> >> I suppose that either apply_dispatch or LogicalRepApplyLoop needs to
> >> grow some more snapshot management logic, but I've not looked at that
> >> code much, so I don't have an opinion on just where to add it.
>
> > I was looking at those for other reasons prior to hitting this bug.
>
> After looking at it a bit, I see a couple of options:
>
> 1. Just wrap the call of ExecuteTruncateGuts with
> PushActiveSnapshot(GetTransactionSnapshot()) and PopActiveSnapshot().
>
> 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.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

Mark Dilger-5
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

akapila
In reply to this post by Mark Dilger-5
On Wed, Jun 9, 2021 at 8:44 PM Mark Dilger <[hidden email]> wrote:

>
> > On Jun 9, 2021, at 7:52 AM, Tom Lane <[hidden email]> wrote:
> >
> > 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.)
>
> Thanks!  The regression test I posted earlier passes with this patch applied.
>

I have also read the patch and it looks good to me.

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

Yes. I think you are talking about Assert(!in_streamed_transaction).
There is no particular reason that such Asserts are required, so we
can change to test-and-elog as you suggested later in your email.

>  That seems many degrees short of acceptable.
>
> Even if you weren't reading that correctly, this bit:
>
>     xid = pq_getmsgint(s, 4);
>
>     Assert(TransactionIdIsValid(xid));
>
> simply asserts that the sending server didn't send an invalid subtransaction id.
>

This also needs to be changed to test-and-elog.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

akapila
On Fri, Jun 11, 2021 at 12:20 AM Tom Lane <[hidden email]> wrote:

>
> Amit Kapila <[hidden email]> writes:
> > On Wed, Jun 9, 2021 at 8:44 PM Mark Dilger <[hidden email]> wrote:
> >> On Jun 9, 2021, at 7:52 AM, Tom Lane <[hidden email]> wrote:
> >>> 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?
>
> > This also needs to be changed to test-and-elog.
>
> Here's a proposed patch for this.  It looks like pretty much all of the
> bogosity is new with the streaming business.
>

Except for the change in change in apply_handle_commit which seems to
be from the time it is introduced in commit 7c4f5240

>  You might quibble with
> which things I thought deserved elog versus report.

I wonder why you used elog in handle_streamed_transaction and
apply_handle_commit? It seems all the other places use ereport for
anything wrong it got from the protocol message.

>  Another thing
> I'm wondering is how many of these messages really need to be
> translated.  We could use errmsg_internal and avoid burdening the
> translators, perhaps.
>

Not sure but I see all existing similar ereport calls don't use errmsg_internal.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

akapila
On Fri, Jun 11, 2021 at 8:56 PM Tom Lane <[hidden email]> wrote:

>
> Amit Kapila <[hidden email]> writes:
> > On Fri, Jun 11, 2021 at 12:20 AM Tom Lane <[hidden email]> wrote:
> >> Another thing
> >> I'm wondering is how many of these messages really need to be
> >> translated.  We could use errmsg_internal and avoid burdening the
> >> translators, perhaps.
>
> > Not sure but I see all existing similar ereport calls don't use errmsg_internal.
>
> I was thinking maybe we could mark all these replication protocol
> violation errors non-translatable.  While we don't want to crash on a
> protocol violation, it shouldn't really be a user-facing case either.
>

I don't see any problem with that as these are not directly related to
any user operation. So, +1 for making these non-translatable.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

Tom Lane-2
CONTENTS DELETED
The author has deleted this message.
Reply | Threaded
Open this post in threaded view
|

Re: logical replication of truncate command with trigger causes Assert

Álvaro Herrera
On 2021-Jun-12, Tom Lane wrote:

> Amit Kapila <[hidden email]> writes:
> > On Fri, Jun 11, 2021 at 8:56 PM Tom Lane <[hidden email]> wrote:
> >> I was thinking maybe we could mark all these replication protocol
> >> violation errors non-translatable.  While we don't want to crash on a
> >> protocol violation, it shouldn't really be a user-facing case either.
>
> > I don't see any problem with that as these are not directly related to
> > any user operation. So, +1 for making these non-translatable.
>
> Done that way.

Good call, thanks.  Not only it's not very useful to translate such
messages, but it's also quite a burden because some of them are
difficult to translate.

--
Álvaro Herrera       Valdivia, Chile
"Tiene valor aquel que admite que es un cobarde" (Fernandel)