Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions - Mailing list pgsql-hackers

On Sat, Jan 4, 2020 at 4:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Dec 30, 2019 at 3:11 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Thu, Dec 12, 2019 at 9:44 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > Yesterday, Tomas has posted the latest version of the patch set which
> > contain the fix for schema send part.  Meanwhile, I was working on few
> > review comments/bugfixes and refactoring.  I have tried to merge those
> > changes with the latest patch set except the refactoring related to
> > "0006-Implement-streaming-mode-in-ReorderBuffer" patch, because Tomas
> > has also made some changes in the same patch.
> >
>
> I don't see any changes by Tomas in that particular patch, am I
> missing something?
He has created some sub-patch from the main patch for handling
schema-sent issue.  So if I make change in that patch all other
patches will conflict.

>
> >  I have created a
> > separate patch for the same so that we can review the changes and then
> > we can merge them to the main patch.
> >
>
> It is better to merge it with the main patch for
> "Implement-streaming-mode-in-ReorderBuffer", otherwise, it is a bit
> difficult to review.
Actually, we can merge 0008, 0009, 0012, 0018 to the main patch
(0007).  Basically, if we merge all of them then we don't need to deal
with the conflict.  I think Tomas has kept them separate so that we
can review the solution for the schema sent.  And, I kept 0018 as a
separate patch to avoid conflict and rebasing in 0008, 0009 and 0012.
In the next patch set, I will merge all of them to 0007.

>
> > > > 0002-Issue-individual-invalidations-with-wal_level-log
> > > > ----------------------------------------------------------------------------
> > > > 1.
> > > > xact_desc_invalidations(StringInfo buf,
> > > > {
> > > > ..
> > > > + else if (msg->id == SHAREDINVALSNAPSHOT_ID)
> > > > + appendStringInfo(buf, " snapshot %u", msg->sn.relId);
> > > >
> > > > You have removed logging for the above cache but forgot to remove its
> > > > reference from one of the places.  Also, I think you need to add a
> > > > comment somewhere in inval.c to say why you are writing for WAL for
> > > > some types of invalidations and not for others?
> > Done
> >
>
> I don't see any new comments as asked by me.
Oh, I just fixed one part of the comment and overlooked the rest.  Will fix.
  I think we should also
> consider WAL logging at each command end instead of doing piecemeal as
> discussed in another email [1], which will have lesser code changes
> and maybe better in performance.  You might want to evaluate the
> performance of both approaches.
Ok
>
> > > >
> > > > 0003-Extend-the-output-plugin-API-with-stream-methods
> > > > --------------------------------------------------------------------------------
> > > >
> > > > 4.
> > > > stream_start_cb_wrapper()
> > > > {
> > > > ..
> > > > + /* state.report_location = apply_lsn; */
> > > > ..
> > > > + /* FIXME ctx->write_location = apply_lsn; */
> > > > ..
> > > > }
> > > >
> > > > See, if we can fix these and similar in the callback for the stop.  I
> > > > think we don't have final_lsn till we commit/abort.  Can we compute
> > > > before calling these API's?
> > Done
> >
>
> You have just used final_lsn, but I don't see where you have ensured
> that it is set before the API stream_stop_cb_wrapper.  I think we need
> something similar to what Vignesh has done in one of his bug-fix patch
> [2].  See my comment below in this regard.

You can refer below hunk in 0018.

+ /*
+ * Done with current changes, call stream_stop callback for streaming
+ * transaction, commit callback otherwise.
+ */
+ if (streaming)
+ {
+ /*
+ * Set the last last of the stream as the final lsn before calling
+ * stream stop.
+ */
+ txn->final_lsn = prev_lsn;
+ rb->stream_stop(rb, txn);
+ }

>
> > > >
> > > >
> > > > 0005-Gracefully-handle-concurrent-aborts-of-uncommitte
> > > > ----------------------------------------------------------------------------------
> > > > 1.
> > > > @@ -1877,6 +1877,7 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
> > > >   PG_CATCH();
> > > >   {
> > > >   /* TODO: Encapsulate cleanup
> > > > from the PG_TRY and PG_CATCH blocks */
> > > > +
> > > >   if (iterstate)
> > > >   ReorderBufferIterTXNFinish(rb, iterstate);
> > > >
> > > > Spurious line change.
> > > >
> > Done
>
> + /*
> + * We don't expect direct calls to heap_getnext with valid
> + * CheckXidAlive for regular tables. Track that below.
> + */
> + if (unlikely(TransactionIdIsValid(CheckXidAlive) &&
> + !(IsCatalogRelation(scan->rs_base.rs_rd) ||
> +   RelationIsUsedAsCatalogTable(scan->rs_base.rs_rd))))
> + elog(ERROR, "improper heap_getnext call");
>
> Earlier, I thought we don't need to check if it is a regular table in
> this check, but it is required because output plugins can try to do
> that
I did not understand that, can you give some example?
and if they do so during decoding (with historic snapshots), the
> same should be not allowed.
>
> How about changing the error message to "unexpected heap_getnext call
> during logical decoding" or something like that?
Ok
>
> > > > 2. The commit message of this patch refers to Prepared transactions.
> > > > I think that needs to be changed.
> > > >
> > > > 0006-Implement-streaming-mode-in-ReorderBuffer
> > > > -------------------------------------------------------------------------
>
> Few comments on v4-0018-Review-comment-fix-and-refactoring:
> 1.
> + if (streaming)
> + {
> + /*
> + * Set the last last of the stream as the final lsn before calling
> + * stream stop.
> + */
> + txn->final_lsn = prev_lsn;
> + rb->stream_stop(rb, txn);
> + }
>
> Shouldn't we try to final_lsn as is done by Vignesh's patch [2]?
Isn't it the same, there we are doing while serializing and here we
are doing while streaming?  Basically, the last LSN we streamed.  Am I
missing something?

>
> 2.
> + if (streaming)
> + {
> + /*
> + * Set the CheckXidAlive to the current (sub)xid for which this
> + * change belongs to so that we can detect the abort while we are
> + * decoding.
> + */
> + CheckXidAlive = change->txn->xid;
> +
> + /* Increment the stream count. */
> + streamed++;
> + }
>
> Is the variable 'streamed' used anywhere?
>
> 3.
> + /*
> + * Destroy the (relfilenode, ctid) hashtable, so that we don't leak
> + * any memory. We could also keep the hash table and update it with
> + * new ctid values, but this seems simpler and good enough for now.
> + */
> + ReorderBufferDestroyTupleCidHash(rb, txn);
>
> Won't this be required only when we are streaming changes?

I will work on this review comments and reply to them separately along
with the patch.
>
> As per my understanding apart from the above comments, the known
> pending work for this patchset is as follows:
> a. The two open items agreed to you in the email [3].
> b. Complete the handling of schema_sent as discussed above [4].
> c. Few comments by Vignesh and the response on the same by me [5][6].
> d. WAL overhead and performance testing for additional WAL logging by
> this patchset.
> e. Some way to see the tuple for streamed transactions by decoding API
> as speculated by you [7].
>
> Have I missed anything?
I think this is the list I remember.  Apart from these few points by
Robert which are still under discussion[8].

>
> [1] -
https://www.postgresql.org/message-id/CAA4eK1LOa%2B2KqNX%3Dm%3D1qMBDW%2Bo50AuwjAOX6ZqL-rWGiH1F9MQ%40mail.gmail.com
> [2] -
https://www.postgresql.org/message-id/CALDaNm3MDxFnsZsnSqVhPBLS3%3DqzNH6%2BYzB%3DxYuX2vbtsUeFgw%40mail.gmail.com
> [3] - https://www.postgresql.org/message-id/CAFiTN-uT5YZE0egGhKdTteTjcGrPi8hb%3DFMPpr9_hEB7hozQ-Q%40mail.gmail.com
> [4] - https://www.postgresql.org/message-id/CAA4eK1KjD9x0mS4JxzCbu3gu-r6K7XJRV%2BZcGb3BH6U3x2uxew%40mail.gmail.com
> [5] - https://www.postgresql.org/message-id/CALDaNm0DNUojjt7CV-fa59_kFbQQ3rcMBtauvo44ttea7r9KaA%40mail.gmail.com
> [6] - https://www.postgresql.org/message-id/CAA4eK1%2BZvupW00c--dqEg8f3dHZDOGmA9xOQLyQHjRSoDi6AkQ%40mail.gmail.com
> [7] - https://www.postgresql.org/message-id/CAFiTN-t8PmKA1X4jEqKmkvs0ggWpy0APWpPuaJwpx2YpfAf97w%40mail.gmail.com

[8] https://www.postgresql.org/message-id/CA%2BTgmoYH6N_YDvKH9AaAJo5ZTHn142K%3DB75VO9yKvjjjHcoZhA%40mail.gmail.com


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