Re: Logical replication CPU-bound with TRUNCATE/DROP/CREATE many tables

On Thu, Oct 1, 2020 at 4:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 1, 2020 at 10:12 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Wed, Sep 30, 2020 at 3:00 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Sep 30, 2020 at 12:16 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > > And you might need to update the below comment as well:
> > > typedef struct ReorderBufferTXN
> > > {
> > > ..
> > > /*
> > > * List of ReorderBufferChange structs, including new Snapshots and new
> > > * CommandIds
> > > */
> > > dlist_head changes;
> > >
>
> You forgot to address the above comment.

Fixed

> Few other comments:
> ==================
> 1.
> void
>  ReorderBufferAddInvalidations(ReorderBuffer *rb, TransactionId xid,
> @@ -2813,12 +2830,27 @@ ReorderBufferAddInvalidations(ReorderBuffer
> *rb, TransactionId xid,
>     SharedInvalidationMessage *msgs)
>  {
>   ReorderBufferTXN *txn;
> + MemoryContext oldcontext;
> + ReorderBufferChange *change;
>
>   txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
>
> + oldcontext = MemoryContextSwitchTo(rb->context);
> +
> + change = ReorderBufferGetChange(rb);
> + change->action = REORDER_BUFFER_CHANGE_INVALIDATION;
> + change->data.inval.ninvalidations = nmsgs;
> + change->data.inval.invalidations = (SharedInvalidationMessage *)
> + MemoryContextAlloc(rb->context,
> +    sizeof(SharedInvalidationMessage) * nmsgs);
> + memcpy(change->data.inval.invalidations, msgs,
> +    sizeof(SharedInvalidationMessage) * nmsgs);
> +
> + ReorderBufferQueueChange(rb, xid, lsn, change, false);
> +
>   /*
> - * We collect all the invalidations under the top transaction so that we
> - * can execute them all together.
> + * Additionally, collect all the invalidations under the top transaction so
> + * that we can execute them all together.  See comment atop this function
>   */
>   if (txn->toptxn)
>   txn = txn->toptxn;
> @@ -2830,8 +2862,7 @@ ReorderBufferAddInvalidations(ReorderBuffer *rb,
> TransactionId xid,
>   {
>   txn->ninvalidations = nmsgs;
>   txn->invalidations = (SharedInvalidationMessage *)
>
> Here what is the need for using MemoryContextAlloc, can't we directly
> use palloc? Also, isn't it better to store the invalidation in txn
> before queuing it for change because doing so can lead to the
> processing of this and other changes accumulated till that point
> before recording the same in txn. As such, there is no problem with it
> but still, I think if any error happens while processing those changes
> we would be able to clear the cache w.r.t this particular
> invalidation.

Fixed

> 2.
> XXX Do we need to care about relcacheInitFileInval and
> + * the other fields added to ReorderBufferChange, or just
> + * about the message itself?
>
> I don't think we need this comment in the patch.
>
> 3.
> - * This needs to be called for each XLOG_XACT_INVALIDATIONS message and
> - * accumulates all the invalidation messages in the toplevel transaction.
> - * This is required because in some cases where we skip processing the
> - * transaction (see ReorderBufferForget), we need to execute all the
> - * invalidations together.
> + * This needs to be called for each XLOG_XACT_INVALIDATIONS message.  The
> + * invalidation messages will be added in the reorder buffer as a change as
> + * well as all the invalidations will be accumulated under the toplevel
> + * transaction.  We collect them as a change so that during decoding, we can
> + * execute only those invalidations which are specific to the command instead
> + * of executing all the invalidations, OTOH after decoding is complete or on
> + * transaction abort (see ReorderBufferForget) we need to execute all the
> + * invalidations to avoid any cache pollution so it is better to keep them
> + * together
>
> Can we rewrite the comment as below?
> This needs to be called for each XLOG_XACT_INVALIDATIONS message and
> accumulates all the invalidation messages in the toplevel transaction
> as well as in the form of change in reorder buffer. We require to
> record it in form of change so that we can execute only required
> invalidations instead of executing all the invalidations on each
> CommandId increment. We also need to accumulate these in top-level txn
> because in some cases where we skip processing the transaction (see
> ReorderBufferForget), we need to execute all the invalidations
> together.

Done

> 4.
> +void ReorderBufferAddInvalidation(ReorderBuffer *, TransactionId,
> XLogRecPtr lsn,
> +   int nmsgs, SharedInvalidationMessage *msgs);
> As pointed by Keisuke-San as well, this is not required.

Fixed

> 5. Can you please once repeat the performance test done by Keisuke-San
> to see if you have similar observations? Additionally, see if you are
> also seeing the inconsistency related to the Truncate message reported
> by him and if so why?
>

Okay, I will set up and do this test early next week.  Keisuke-San,
can you send me your complete test script?

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