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

On Wed, Jun 24, 2020 at 4:04 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> Comments on other patches:
> =========================

Replying to the pending comments.

> 6.
> In v29-0002-Issue-individual-invalidations-with-wal_level-lo,
> xact_desc_invalidations seems to be a subset of
> standby_desc_invalidations, can we have a common code for them?

Done

> 7.
> I think we can avoid sending v29-0007-Track-statistics-for-streaming
> this each time. We can do this after the main patch is complete.
> Also, we might need to change how and where these stats will be
> tracked. See the related discussion [1].

Removed

> 8. In v29-0005-Implement-streaming-mode-in-ReorderBuffer,
> * Return oldest transaction in reorderbuffer
> @@ -863,6 +909,9 @@ ReorderBufferAssignChild(ReorderBuffer *rb,
> TransactionId xid,
> /* set the reference to top-level transaction */
> subtxn->toptxn = txn;
>
> + /* set the reference to toplevel transaction */
> + subtxn->toptxn = txn;
> +
>
> There is a double initialization of subtxn->toptxn. You need to
> remove this line from 0005 patch as we have now added it in an earlier
> patch.

Done

> 9. I think you forgot to update the patch to execute invalidations in
> Abort case or I might be missing something. I don't see any changes
> in ReorderBufferAbort. You have agreed in one of the emails above [2]
> about handling the same.

Done, check 0005

> 10. In v29-0008-Add-support-for-streaming-to-built-in-replicatio,
> apply_handle_stream_commit(StringInfo s)
> {
> ..
> + /*
> + * send feedback to upstream
> + *
> + * XXX Probably should send a valid LSN. But which one?
> + */
> + send_feedback(InvalidXLogRecPtr, false, false);
> ..
> }
>
> I have given a comment on this code that we don't need this feedback
> and you mentioned on June 02 [3] that you will think on it and let me
> know your opinion but I don't see a response from you yet. Can you
> get back to me regarding this point?

Yeah, I have analyzed this and this seems we don't need this. Because
like non-streaming mode here also sending feedback mechanisms shall be
the same. I don't see any reason for sending extra feedback on
commit.

> 11. Add some comments as to why we have used Shared BufFile interface
> instead of Temp BufFile interface?

Done

> 12. In v29-0013-Change-buffile-interface-required-for-streaming,
> + * Initialize a space for temporary files that can be opened other backends.
>
> /opened other backends/opened for access by other backends

Done

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