Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions

On Wed, Mar 4, 2020 at 3:16 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> Hi,
>
> I started looking at this patch series again, hoping to get it moving
> for PG13.

Nice.

 There's been a tremendous amount of work done since I last
> worked on it, and a lot was discussed on this thread, so it'll take a
> while to get familiar with the new code ...
>
> The first thing I realized that WAL-logging of assignments in v12 does
> both the "old" logging (using dedicated message) and "new" with
> toplevel-XID embedded in the first message. Yes, the patch was wrong,
> because it eliminated all calls to ProcArrayApplyXidAssignment() and so
> it was trivial to crash the replica due to KnownAssignedXids overflow.
> But I don't think re-introducing XLOG_XACT_ASSIGNMENT message is the
> right fix.
>
> I actually proposed doing this (having both ways to log assignments) so
> that there's no regression risk with (wal_level < logical). But IIRC
> Andres objected to it, argumenting that we should not log the same piece
> of information in two very different ways at the same time (IIRC it was
> discussed on the FOSDEM dev meeting, so I don't have a link to share).
> And I do agree with him ...
>
> The question is, why couldn't the replica use the same assignment info
> we already write for logical decoding? The main challenge is that now
> the assignment can be sent in many different xlog messages, from a bunch
> of resource managers (essentially, any xlog message with a xid can have
> embedded XID of the toplevel xact). So the handling would either need to
> happen in every rmgr, or we need to move it before we call the rmgr.
>
> For exampple, we might do this e.g. in StartupXLOG() I think, per the
> attached patch (FWIW this particular fix was written by Masahiko Sawada,
> not me). This does the trick for me - I'm no longer able to reproduce
> the KnownAssignedXids overflow.
>
> The one difference is that we used to call ProcArrayApplyXidAssignment
> for larger groups of XIDs, as sent in the assignment message. Now we
> call it for each individual assignment. I don't know if this is an
> issue, but I suppose we might introduce some sort of local caching
> (accumulate the assignments into a local array, call the function only
> when we have enough of them).

Thanks for the pointers,  I will think over these points.

>
> Aside from that, I think there's a minor bug in xact.c - the patch adds
> a "assigned" field to TransactionStateData, but then it fails to add a
> default value into TopTransactionStateData. We probably interpret NULL
> as false, but then there's nothing for the pointer. I suspect it might
> leave some random garbage there, leading to strange things later.

Actually, we will never access that field for the
TopTransactionStateData, right?
See below code,  we have a check that only if IsSubTransaction(), then
we access the "assigned" filed.

+bool
+IsSubTransactionAssignmentPending(void)
+{
+ if (!XLogLogicalInfoActive())
+ return false;
+
+ /* we need to be in a transaction state */
+ if (!IsTransactionState())
+ return false;
+
+ /* it has to be a subtransaction */
+ if (!IsSubTransaction())
+ return false;
+
+ /* the subtransaction has to have a XID assigned */
+ if (!TransactionIdIsValid(GetCurrentTransactionIdIfAny()))
+ return false;
+
+ /* and it needs to have 'assigned' */
+ return !CurrentTransactionState->assigned;
+
+}

>
> Another thing I noticed is LogicalDecodingProcessRecord() extracts the
> toplevel XID using a macro
>
>    txid = XLogRecGetTopXid(record);
>
> but then it just starts accessing the fields directly again in the
> ReorderBufferAssignChild call. I think we should do this instead:
>
>      ReorderBufferAssignChild(ctx->reorder,
>                               txid,
>                              XLogRecGetXid(record),
>                               buf.origptr);

Make sense.  I will change this in the patch.

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