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

On Tue, Aug 4, 2020 at 10:12 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Jul 29, 2020 at 10:46 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > Thanks, please find the rebased patch set.
> >
>
> Few comments on v44-0001-Implement-streaming-mode-in-ReorderBuffer:
> ============================================================
> 1.
> +-- streaming with subxact, nothing in main
> +BEGIN;
> +savepoint s1;
> +SELECT 'msg5' FROM pg_logical_emit_message(true, 'test', repeat('a', 50));
> +INSERT INTO stream_test SELECT repeat('a', 2000) || g.i FROM
> generate_series(1, 35) g(i);
> +TRUNCATE table stream_test;
> +rollback to s1;
> +INSERT INTO stream_test SELECT repeat('a', 10) || g.i FROM
> generate_series(1, 20) g(i);
> +COMMIT;
>
> Is the above comment true?  Because it seems to me that Insert is
> getting streamed in the main transaction.

Changed the comments.

> 2.
> +<programlisting>
> +postgres[33712]=#* SELECT * FROM
> pg_logical_slot_get_changes('test_slot', NULL, NULL, 'stream-changes',
> '1');
> +    lsn    | xid |                       data
> +-----------+-----+--------------------------------------------------
> + 0/16B21F8 | 503 | opening a streamed block for transaction TXN 503
> + 0/16B21F8 | 503 | streaming change for TXN 503
> + 0/16B2300 | 503 | streaming change for TXN 503
> + 0/16B2408 | 503 | streaming change for TXN 503
> + 0/16BEBA0 | 503 | closing a streamed block for transaction TXN 503
> + 0/16B21F8 | 503 | opening a streamed block for transaction TXN 503
> + 0/16BECA8 | 503 | streaming change for TXN 503
> + 0/16BEDB0 | 503 | streaming change for TXN 503
> + 0/16BEEB8 | 503 | streaming change for TXN 503
> + 0/16BEBA0 | 503 | closing a streamed block for transaction TXN 503
> +(10 rows)
> +</programlisting>
> + </para>
> +
>
> Is the above example correct?  Because we should include XID in the
> stream message only when include_xids option is specified.

include_xids is true if we don't set it to false explicitly

> 3.
>  /*
> - * Queue a change into a transaction so it can be replayed upon commit.
> + * Record the partial change for the streaming of in-progress transactions.  We
> + * can stream only complete changes so if we have a partial change like toast
> + * table insert or speculative then we mark such a 'txn' so that it can't be
> + * streamed.
>
> /speculative then/speculative insert then

Done

> 4.  I think we can explain the problems (like we can see the wrong
> tuple or see two versions of the same tuple or whatever else wrong can
> happen, if possible with some example) related to concurrent aborts
> somewhere in comments.

Done

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