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

On Sun, Jun 7, 2020 at 5:08 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Fri, Jun 5, 2020 at 11:37 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> >
> > Let me know what you think of the changes? If you find them okay,
> > then feel to include them in the next patch-set.
> >
> > [1] - https://www.postgresql.org/message-id/CAONYFtOv%2BEr1p3WAuwUsy1zsCFrSYvpHLhapC_fMD-zNaRWxYg%40mail.gmail.com
>
> Thanks for the patch, I will review it and include it in my next version.
>

Okay, I have done review of
0002-Issue-individual-invalidations-with-wal_level-lo.patch and below
are my comments:

1. I don't think it is a good idea that logical decoding process the
new XLOG_XACT_INVALIDATIONS and existing WAL records for invalidations
like XLOG_INVALIDATIONS and what we do in DecodeCommit (see code in
the check "if (parsed->nmsgs > 0)"). I think if that is required for
some particular reason then we should write detailed comments about
the same. I have tried some experiments to see if those are really
required:
a. After applying patch 0002, I have tried by commenting out the
processing of invalidations via DecodeCommit and found some regression
tests were failing but the reason for failure was that we are not
setting RBTXN_HAS_CATALOG_CHANGES for the toptxn when subtxn has
catalog changes and when I did that all regression tests started
passing. See the attached diff patch
(v27-0003-Incremental-patch-for-0002-to-test-removal-of-du) atop 0002
patch.
b. The processing of invalidations for XLOG_INVALIDATIONS is added by
commit c6ff84b06a for xid-less transactions. See
https://postgr.es/m/CAB-SwXY6oH=9twBkXJtgR4UC1NqT-vpYAtxCseME62ADwyK5OA@mail.gmail.com
to know why that has been added. Now, after this patch we will
process the same invalidations via XLOG_XACT_INVALIDATIONS and
XLOG_INVALIDATIONS which doesn't seem warranted. Also, the below
assertion will fail for xid-less transactions (try create index
concurrently statement):
+ case XLOG_XACT_INVALIDATIONS:
+ {
+ TransactionId xid;
+ xl_xact_invalidations *invals;
+
+ xid = XLogRecGetXid(r);
+ invals = (xl_xact_invalidations *) XLogRecGetData(r);
+
+ Assert(TransactionIdIsValid(xid));

I feel we don't need the processing of XLOG_INVALIDATIONS in logical
decoding after this patch but to prove that first we need to write a
test case which need XLOG_INVALIDATIONS in the HEAD as commit
c6ff84b06a doesn't add one. I think we need two code paths in
XLOG_XACT_INVALIDATIONS where if it is for xid-less transactions, then
execute actions immediately as we are doing in processing of
XLOG_INVALIDATIONS, otherwise, do what we are doing currently in the
patch. If the above point (b) is correct, I am not sure if it is a
good idea to use RM_XACT_ID as resource manager if for this WAL in
LogLogicalInvalidations, what do you think?

I think one of the usages we still need is in ReorderBufferForget
because it can be called when we skip processing the txn. See the
comments in DecodeCommit where we call this function. If I am
correct, we need to probably collect all invalidations in
ReorderBufferTxn as we are collecting tuplecids and use them here. We
can do the same during processing of XLOG_XACT_INVALIDATIONS.

I had also thought a bit about removing logging of invalidations at
commit time altogether but it seems processing hot-standby is somewhat
tightly coupled with existing WAL logging. See xact_redo_commit (a
comment atop call to ProcessCommittedInvalidationMessages). It says
we need to maintain the order when we process invalidations. If we
can later find a way to avoid that we can probably remove it but for
now maybe we can live with it.

2.
+ /* not expected, but print something anyway */
+ else if (msg->id == SHAREDINVALSMGR_ID)
+ appendStringInfoString(buf, " smgr");
+ /* not expected, but print something anyway */
+ else if (msg->id == SHAREDINVALRELMAP_ID)

I think the above comment is not valid after we started logging at CCI.

3.
+
+ xid = XLogRecGetXid(r);
+ invals = (xl_xact_invalidations *) XLogRecGetData(r);
+
+ Assert(TransactionIdIsValid(xid));
+ ReorderBufferAddInvalidation(reorder, xid, buf->origptr,
+ invals->nmsgs, invals->msgs);

Here, it should check !ctx->forward as we do in DecodeCommit, do we
have any reason for not doing so. We can test once by changing this.

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