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

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?

>  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.

> > > 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.  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.

> > >
> > > 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.

> > >
> > >
> > > 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 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?

> > > 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]?

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?

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?

[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

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