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

On Fri, May 15, 2020 at 4:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, May 15, 2020 at 2:47 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > > 6. I think it will be good if we can provide an example of streaming
> > > changes via test_decoding at
> > > https://www.postgresql.org/docs/devel/test-decoding.html. I think we
> > > can also explain there why the user is not expected to see the actual
> > > data in the stream.
> >
> > I have a few problems to solve here.
> > -  With streaming transaction also shall we show the actual values or
> > we shall do like it is currently in the patch
> > (appendStringInfo(ctx->out, "streaming change for TXN %u",
> > txn->xid);).  I think we should show the actual values instead of what
> > we are doing now.
> >
>
> I think why we don't want to display the tuple at this stage is
> because it is not clear by this time if the transaction will commit or
> abort.  I am not sure if displaying the contents of aborted
> transactions is a good idea but if there is a reason for doing so, we
> can do it later as well.

Ok.

>
> > - In the example we can not show a real example, because of the
> > in-progress transaction to show the changes, we might have to
> > implement a lot of tuple.  I think we can show the partial output?
> >
>
> I think we can display what API will actually display, what is the
> confusion here.

What, I meant is that even with the logical_decoding_work_mem=64kb, we
need to have quite a few changes in a transaction to stream it so the
example output will be quite big in size.  So I told we might not show
the real example instead we will just show a few lines and cut the
remaining.  But, I got your point we can just show how it will look
like.

>
> I have a few more comments on the previous version of patch
> v20-0005-Implement-streaming-mode-in-ReorderBuffer.  If you have fixed
> any, then leave those and fix others.
>
> Review comments:
> ------------------------------
> 1.
> @@ -1762,10 +1952,16 @@ ReorderBufferCommit(ReorderBuffer *rb,
> TransactionId xid,
>   }
>
>   case REORDER_BUFFER_CHANGE_MESSAGE:
> - rb->message(rb, txn, change->lsn, true,
> - change->data.msg.prefix,
> - change->data.msg.message_size,
> - change->data.msg.message);
> + if (streaming)
> + rb->stream_message(rb, txn, change->lsn, true,
> +    change->data.msg.prefix,
> +    change->data.msg.message_size,
> +    change->data.msg.message);
> + else
> + rb->message(rb, txn, change->lsn, true,
> +    change->data.msg.prefix,
> +    change->data.msg.message_size,
> +    change->data.msg.message);
>
> Don't we need to set any_data_sent flag while streaming messages as we
> do for other types of changes?

Actually, pgoutput plugin don't send any data on stream_message.  But,
I agree that how other plugin will handle.  I will analyze this part
again, maybe we have to such flag at the plugin level and whether stop
is sent to not can also be handled at the plugin level.

> 2.
> + if (streaming)
> + {
> + /*
> + * Set the last of the stream as the final lsn before calling
> + * stream stop.
> + */
> + if (!XLogRecPtrIsInvalid(prev_lsn))
> + txn->final_lsn = prev_lsn;
> + rb->stream_stop(rb, txn);
> + }
>
> I am not sure if it is good to use final_lsn for this purpose.  See
> comments for this variable in reorderbuffer.h.  Basically, it is used
> for a specific purpose on different occasions.  Now, if we want to
> start using it for a new purpose, we need to study its interaction
> with all other places and update the comments as well.  Can we pass an
> additional parameter to stream_stop() instead?

I think it was in sycn with the spill code right? I mean the last
change we spill is set as the final_lsn and same is done here.

Other comments looks fine so I will work on them and reply separatly.


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