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

On Mon, Jul 13, 2020 at 3:04 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Jul 13, 2020 at 2:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Jul 13, 2020 at 2:32 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Mon, Jul 13, 2020 at 11:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > >
> > > > 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.
> > >
> >
> > Then, how is that used on the publisher-side?  AFAICS, the streaming
> > is enabled based on whether streaming callbacks are provided and we do
> > that in 0003-Extend-the-logical-decoding-output-plugin-API-wi patch.
>
> Basically, first, we enable based on whether we have the callbacks or
> not but later once we get the START REPLICATION command from the
> subscriber then we set it to false if the streaming is not enabled
> from the subscriber side.  You can refer below code in patch 0007.
>
> pgoutput_startup
> {
> parse_output_parameters(ctx->output_plugin_options,
> &data->protocol_version,
> - &data->publication_names);
> + &data->publication_names,
> + &enable_streaming);
> /* Check if we support requested protocol */
> if (data->protocol_version > LOGICALREP_PROTO_VERSION_NUM)
> @@ -222,6 +284,27 @@ pgoutput_startup(LogicalDecodingContext *ctx,
> OutputPluginOptions *opt,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("publication_names parameter missing")));
> + /*
> + * Decide whether to enable streaming. It is disabled by default, in
> + * which case we just update the flag in decoding context. Otherwise
> + * we only allow it with sufficient version of the protocol, and when
> + * the output plugin supports it.
> + */
> + if (!enable_streaming)
> + ctx->streaming = false;
> }
>

Okay, in that case, we can do both enable and disable streaming in
this function itself rather than allow the caller to later modify it.
I suggest similarly we can enable/disable it for SQL API in
pg_decode_startup via output_plugin_options.  This way it will look
consistent for both SQL APIs and for command-based replication.  If we
can do so, then probably adding an Assert for Consistent Snapshot
while performing streaming should be okay.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com