Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

On Mon, Jul 13, 2020 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jul 13, 2020 at 10:40 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Mon, Jul 13, 2020 at 10:14 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, Jul 10, 2020 at 3:37 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > On Sat, Jul 4, 2020 at 11:35 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > >
> > > > > 8. We can't stream the transaction before we reach the
> > > > > SNAPBUILD_CONSISTENT state because some other output plugin can apply
> > > > > those changes unlike what we do with pgoutput plugin (which writes to
> > > > > file). And, I think applying the transactions without reaching a
> > > > > consistent state would be anyway wrong.  So, we should avoid that and
> > > > > if do that then we should have an Assert for streamed txns rather than
> > > > > sending abort for them in ReorderBufferForget.
> > > >
> > > > I was analyzing this point so currently, we only enable streaming in
> > > > StartReplicationSlot so basically in CreateReplicationSlot the
> > > > streaming will be always off because by that time plugins are not yet
> > > > startup that will happen only on StartReplicationSlot.
> > > >
> > >
> > > What do you mean by 'startup' in the above sentence?  AFAICS, we do
> > > call startup_cb_wrapper in CreateInitDecodingContext which is called
> > > from both CreateReplicationSlot and create_logical_replication_slot
> > > before the start of decoding.  In CreateInitDecodingContext, we call
> > > StartupDecodingContext which should load the plugin.
> >
> > Yeah, you are right that we do call startup_cb_wrapper from
> > CreateInitDecodingContext as well.  I think I got confused by below
> > comment in patch 0007
> >
> > @@ -1016,6 +1016,12 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
> > WalSndPrepareWrite, WalSndWriteData,
> > WalSndUpdateProgress);
> > + /*
> > + * Make sure streaming is disabled here - we may have the methods,
> > + * but we don't have anywhere to send the data yet.
> > + */
> > + ctx->streaming = false;
> > +
> >
> > Basically, during CreateReplicationSlot we forcefully disable the
> > streaming with the comment "we don't have anywhere to send the data
> > yet".  So my point is during CreateReplicationSlot time the streaming
> > will always be off and once we are done with creating the slot we will
> > be having consistent snapshot.  So my point is can we just check that
> > while decoding unless the current LSN reaches the start_decoding_at
> > point we should not start streaming and after that we can start.  At
> > that time we can have an assert that the snapshot should be
> > CONSISTENT.  However, before doing that I need to check on this point
> > that why after creating slot we are setting ctx->streaming to false.
> >
>
> I think you can refer to commit message as well for that "We however
> must explicitly disable streaming replication during replication slot
> creation, even if the plugin supports it. We don't need to replicate
> the changes accumulated during this phase, and moreover, we don't have
> a replication connection open so we don't have where to send the data
> anyway.".  I don't think this is a good way to hack the streaming flag
> because for SQL API's, we don't have a good reason to disable the
> streaming in this way.  I guess if we had a condition related to
> reaching CONSISTENT snapshot during streaming then we won't need to
> hack the streaming flag in this way.  Once we reach the CONSISTENT
> snapshot state, we come out of the creation of a replication slot (see
> how we use DecodingContextReady to achieve that) phase.  So, I feel we
> should remove the ctx->streaming setting to false and add a CONSISTENT
> snapshot check during streaming unless you have a reason for not doing
> so.

I was worried about the point that streaming on/off is sent by the
subscriber on START REPLICATION, not on CREATE REPLICATION SLOT, so if
we keep streaming on during create then it may not be right.  But, I
agree with your point that it's better we can avoid streaming during
slot creation by CONSISTENT snapshot check instead of disabling this
way.  And, anyways as soon as we reach the consistent snapshot we will
stop processing further records so we will not attempt to stream
during slot creation.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com