Re: logical replication of truncate command with trigger causes Assert

2021-06-12 Thread Alvaro Herrera
On 2021-Jun-12, Tom Lane wrote: > Amit Kapila writes: > > On Fri, Jun 11, 2021 at 8:56 PM Tom Lane 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

Re: logical replication of truncate command with trigger causes Assert

2021-06-12 Thread Tom Lane
Amit Kapila writes: > On Fri, Jun 11, 2021 at 8:56 PM Tom Lane 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 s

Re: logical replication of truncate command with trigger causes Assert

2021-06-12 Thread Amit Kapila
On Fri, Jun 11, 2021 at 8:56 PM Tom Lane wrote: > > Amit Kapila writes: > > On Fri, Jun 11, 2021 at 12:20 AM Tom Lane 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 > >> translato

Re: logical replication of truncate command with trigger causes Assert

2021-06-11 Thread Tom Lane
Amit Kapila writes: > On Fri, Jun 11, 2021 at 12:20 AM Tom Lane 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

Re: logical replication of truncate command with trigger causes Assert

2021-06-10 Thread Amit Kapila
On Fri, Jun 11, 2021 at 12:20 AM Tom Lane wrote: > > Amit Kapila writes: > > On Wed, Jun 9, 2021 at 8:44 PM Mark Dilger > > wrote: > >> On Jun 9, 2021, at 7:52 AM, Tom Lane wrote: > >>> Somewhat unrelated, but ... am I reading the code correctly that > >>> apply_handle_stream_start and related

Re: logical replication of truncate command with trigger causes Assert

2021-06-10 Thread Tom Lane
Amit Kapila writes: > On Wed, Jun 9, 2021 at 8:44 PM Mark Dilger > wrote: >> On Jun 9, 2021, at 7:52 AM, Tom Lane 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 st

Re: logical replication of truncate command with trigger causes Assert

2021-06-09 Thread Amit Kapila
On Wed, Jun 9, 2021 at 8:44 PM Mark Dilger wrote: > > > On Jun 9, 2021, at 7:52 AM, Tom Lane 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 unimagina

Re: logical replication of truncate command with trigger causes Assert

2021-06-09 Thread Tom Lane
Mark Dilger writes: >> On Jun 9, 2021, at 7:52 AM, Tom Lane 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? That seems

Re: logical replication of truncate command with trigger causes Assert

2021-06-09 Thread Mark Dilger
> On Jun 9, 2021, at 7:52 AM, Tom Lane 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 a

Re: logical replication of truncate command with trigger causes Assert

2021-06-09 Thread Tom Lane
Amit Kapila writes: > On Wed, Jun 9, 2021 at 5:29 AM Tom Lane 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 O

Re: logical replication of truncate command with trigger causes Assert

2021-06-09 Thread Amit Kapila
On Wed, Jun 9, 2021 at 5:29 AM Tom Lane wrote: > > Mark Dilger writes: > > On Jun 8, 2021, at 3:55 PM, Tom Lane 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 h

Re: logical replication of truncate command with trigger causes Assert

2021-06-08 Thread Tom Lane
Mark Dilger writes: > On Jun 8, 2021, at 3:55 PM, Tom Lane 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

Re: logical replication of truncate command with trigger causes Assert

2021-06-08 Thread Mark Dilger
> On Jun 8, 2021, at 3:55 PM, Tom Lane wrote: > > The right way to say that is "commit 84f5c2908da exposed the pre-existing > unsafe behavior of this code". It's not okay to run arbitrary user code > without holding a snapshot to protect TOAST dereference operations. Sure, I didn't expect th

Re: logical replication of truncate command with trigger causes Assert

2021-06-08 Thread Tom Lane
Mark Dilger writes: > On master, when a statement level trigger is fired for a replicated truncate > command, the following stack trace is generated: Hmm. > I believe the issue was introduced in commit 84f5c2908da which added > EnsurePortalSnapshotExists. That's not going to work in the case

logical replication of truncate command with trigger causes Assert

2021-06-08 Thread Mark Dilger
Hackers, On master, when a statement level trigger is fired for a replicated truncate command, the following stack trace is generated: TRAP: FailedAssertion("portal != NULL", File: "pquery.c", Line: 1760, PID: 93854) 0 postgres0x000108e269f2 ExceptionalConditio