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

On Wed, Jul 22, 2020 at 4:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Jul 22, 2020 at 10:20 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Wed, Jul 22, 2020 at 9:18 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Jul 20, 2020 at 6:46 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > >
> > > > There was one warning in release mode in the last version in 0004 so
> > > > attaching a new version.
> > > >
> > >
> > > Today, I was reviewing patch
> > > v38-0001-WAL-Log-invalidations-at-command-end-with-wal_le and found a
> > > small problem with it.
> > >
> > > + /*
> > > + * Execute the invalidations for xid-less transactions,
> > > + * otherwise, accumulate them so that they can be processed at
> > > + * the commit time.
> > > + */
> > > + if (!ctx->fast_forward)
> > > + {
> > > + if (TransactionIdIsValid(xid))
> > > + {
> > > + ReorderBufferAddInvalidations(reorder, xid, buf->origptr,
> > > + invals->nmsgs, invals->msgs);
> > > + ReorderBufferXidSetCatalogChanges(ctx->reorder, xid,
> > > + buf->origptr);
> > > + }
> > >
> > > I think we need to set ReorderBufferXidSetCatalogChanges even when
> > > ctx->fast-forward is true because we are dependent on that flag for
> > > snapshot build (see SnapBuildCommitTxn). We are already doing the
> > > same way in DecodeCommit where even though we skip adding
> > > invalidations for fast-forward cases but we do set the flag to
> > > indicate that this txn has catalog changes. Is there any reason to do
> > > things differently here?
> >
> > I think it is wrong, we should set the
> > ReorderBufferXidSetCatalogChanges, even if it is in fast-forward mode.
> >
>
> Thanks for the change. I have one more minor comment in the patch
> 0001-WAL-Log-invalidations-at-command-end-with-wal_le.
>
> /*
> + * Invalidations logged with wal_level=logical.
> + */
> +typedef struct xl_xact_invalidations
> +{
> + int nmsgs; /* number of shared inval msgs */
> + SharedInvalidationMessage msgs[FLEXIBLE_ARRAY_MEMBER];
> +} xl_xact_invalidations;
>
> I see that we already have a structure xl_xact_invals in the code
> which has the same members, so I think it is better to use that
> instead of defining a new one.

You are right. I have changed it.

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