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

On Mon, May 18, 2020 at 4:10 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sun, May 17, 2020 at 12:41 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >

Few comments on v20-0010-Bugfix-handling-of-incomplete-toast-tuple
1.
+ /*
+ * If this is a toast insert then set the corresponding bit. Otherwise, if
+ * we have toast insert bit set and this is insert/update then clear the
+ * bit.
+ */
+ if (toast_insert)
+ toptxn->txn_flags |= RBTXN_HAS_TOAST_INSERT;
+ else if (rbtxn_has_toast_insert(txn) &&
+ ChangeIsInsertOrUpdate(change->action))
+ {

Here, it might better to add a comment on why we expect only
Insert/Update? Also, it might be better that we add an assert for
other operations.

2.
@@ -1865,8 +1920,8 @@ ReorderBufferProcessTXN(ReorderBuffer *rb,
ReorderBufferTXN *txn,
* disk.
*/
dlist_delete(&change->node);
- ReorderBufferToastAppendChunk(rb, txn, relation,
- change);
+ ReorderBufferToastAppendChunk(rb, txn, relation,
+ change);
}

This seems to be a spurious change.

3.
+ /*
+ * If streaming is enable and we have serialized this transaction because
+ * it had incomplete tuple. So if now we have got the complete tuple we
+ * can stream it.
+ */
+ if (ReorderBufferCanStream(rb) && can_stream && rbtxn_is_serialized(toptxn)
+ && !(rbtxn_has_toast_insert(txn)) && !(rbtxn_has_spec_insert(txn)))
+ {

This comment is just saying what you are doing in the if-check. I
think you need to explain the rationale behind it. I don't like the
variable name 'can_stream' because it matches ReorderBufferCanStream
whereas it is for a different purpose, how about naming it as
'change_complete' or something like that. The check has many
conditions, can we move it to a separate function to make the code
here look clean?

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