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

On Fri, Sep 27, 2019 at 12:06 AM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> On Thu, Sep 26, 2019 at 06:58:17PM +0530, Amit Kapila wrote:
>
> >3.
> >+ * Find the largest transaction (toplevel or subxact) to evict (spill to disk).
> >+ *
> >+ * XXX With many subtransactions this might be quite slow, because we'll have
> >+ * to walk through all of them. There are some options how we could improve
> >+ * that: (a) maintain some secondary structure with transactions sorted by
> >+ * amount of changes, (b) not looking for the entirely largest transaction,
> >+ * but e.g. for transaction using at least some fraction of the memory limit,
> >+ * and (c) evicting multiple transactions at once, e.g. to free a given portion
> >+ * of the memory limit (e.g. 50%).
> >+ */
> >+static ReorderBufferTXN *
> >+ReorderBufferLargestTXN(ReorderBuffer *rb)
> >
> >What is the guarantee that after evicting largest transaction, we
> >won't immediately hit the memory limit? Say, all of the transactions
> >are of almost similar size which I don't think is that uncommon a
> >case.
>
> Not sure I understand - what do you mean 'immediately hit'?
>
> We do check the limit after queueing a change, and we know that this
> change is what got us over the limit. We pick the largest transaction
> (which has to be larger than the change we just entered) and evict it,
> getting below the memory limit again.
>
> The next change can get us over the memory limit again, of course,
>

Yeah, this is what I want to say when I wrote that it can immediately hit again.

> but
> there's not much we could do about that.
>
> > Instead, the strategy mentioned in point (c) or something like
> >that seems more promising. In that strategy, there is some risk that
> >it might lead to many smaller disk writes which we might want to
> >control via some threshold (like we should not flush more than N
> >xacts). In this, we also need to ensure that the total memory freed
> >must be greater than the current change.
> >
> >I think we have some discussion around this point but didn't reach any
> >conclusion which means some more brainstorming is required.
> >
>
> I agree it's worth investigating, but I'm not sure it's necessary before
> committing v1 of the feature. I don't think there's a clear winner
> strategy, and the current approach works fairly well I think.
>
> The comment is concerned with the cost of ReorderBufferLargestTXN with
> many transactions, but we can only have certain number of top-level
> transactions (max_connections + certain number of not-yet-assigned
> subtransactions). And 0002 patch essentially gets rid of the subxacts
> entirely, further reducing the maximum number of xacts to walk.
>

That would be good, but I don't understand how. The second patch will
update the subxacts in top-level ReorderBufferTxn, but it won't remove
it from hash table. It also doesn't seem to be caring for considering
the size of subxacts in top-level xact, so not sure how will it reduce
the number of xacts to walk. I might be missing something here. Can
you explain a bit how 0002 patch would help in reducing the maximum
number of xacts to walk?

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