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

On Wed, Dec 11, 2019 at 11:46 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Dec 2, 2019 at 3:32 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > I have rebased the patch set on the latest head.
>
> 0001 looks like a clever approach, but are you sure it doesn't hurt
> performance when many small XLOG records are being inserted? I think
> XLogRecordAssemble() can get pretty hot in some workloads.
>
> With regard to 0002, logging a separate WAL record for each
> invalidation seems painful; I think most operations that generate
> invalidations generate a bunch of them all at once. Perhaps you could
> just queue up invalidations as they happen, and then force anything
> that's been queued up to be emitted into WAL just before you emit any
> WAL record that might need to be decoded.
>

I feel we can log the invalidations of the entire command at one go if
we log at CommandEndInvalidationMessages. We already have all the
invalidations of current command in
transInvalInfo->CurrentCmdInvalidMsgs. This can save us the effort of
maintaining a new separate list/queue for invalidations and to a good
extent, it will ameliorate your concern of logging each invalidation
separately.

>
> 0006 contains lots of XXX comments that look like real issues. I guess
> those need to be fixed. Also, why don't we do the thing that the
> commit message for 0006 says we could "theoretically" do? I don't
> understand why we need the k-way merge at all,
>

I think we can do what is written in the commit message, but then we
need to maintain two paths (one for streaming contexts and other for
non-streaming contexts) unless we want to entirely get rid of storing
subtransaction changes separately which seems like a more fundamental
change. Right now, also to some extent such things are there, but I
have already given a comment to minimize it. Having said that, I
think we can go either way. I think the original intention was to
avoid doing more stuff unless it is really required as this is already
a big patchset, but maybe Tomas has a different idea about this.

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