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

On Wed, Jun 24, 2020 at 4:04 PM Amit Kapila <amit.kapila16@gmail.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