Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions

On Thu, Dec 12, 2019 at 9:45 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Dec 11, 2019 at 5:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Dec 9, 2019 at 1:27 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > I have review the patch set and here are few comments/questions
> > >
> > > 1.
> > > +static void
> > > +pg_decode_stream_change(LogicalDecodingContext *ctx,
> > > + ReorderBufferTXN *txn,
> > > + Relation relation,
> > > + ReorderBufferChange *change)
> > > +{
> > > + OutputPluginPrepareWrite(ctx, true);
> > > + appendStringInfo(ctx->out, "streaming change for TXN %u", txn->xid);
> > > + OutputPluginWrite(ctx, true);
> > > +}
> > >
> > > Should we show the tuple in the streamed change like we do for the
> > > pg_decode_change?
> > >
> >
> > I think so.  The patch shows the message in
> > pg_decode_stream_message(), so why to prohibit showing tuple here?
> >
> > > 2. pg_logical_slot_get_changes_guts
> > > It recreate the decoding slot [ctx =
> > > CreateDecodingContext(InvalidXLogRecPtr] but doesn't set the streaming
> > > to false, should we pass a parameter to
> > > pg_logical_slot_get_changes_guts saying whether we want streamed results or not
> > >
> >
> > CreateDecodingContext internally calls StartupDecodingContext which
> > sets the value of streaming based on if the plugin has provided
> > callbacks for streaming functions. Isn't that sufficient?  Why do we
> > need additional parameters here?
>
> I don't think that if plugin provides streaming function then we
> should stream.  Like pgoutput plugin provides streaming function but
> we only stream if streaming is on in create subscription command.  So
> I feel that should be true with any plugin.
>

How about adding a new boolean parameter (streaming) in
pg_create_logical_replication_slot()?

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