Re: Decoding speculative insert with toast leaks memory

Dilip Kumar <dilipbalaut@gmail.com>

From: Dilip Kumar <dilipbalaut@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, Petr Jelinek <petr.jelinek@enterprisedb.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Tomas Vondra <tomas.vondra@enterprisedb.com>
Date: 2021-06-01T14:30:42Z
Lists: pgsql-hackers

Attachments

On Tue, Jun 1, 2021 at 5:22 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Jun 1, 2021 at 12:25 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > >
> > > IMHO, as I stated earlier one way to fix this problem is that we add
> > > the spec abort operation (DELETE + XLH_DELETE_IS_SUPER flag) to the
> > > queue, maybe with action name
> > > "REORDER_BUFFER_CHANGE_INTERNAL_SPEC_ABORT" and as part of processing
> > > that just cleans up the toast and specinsert tuple and nothing else.
> > > If we think that makes sense then I can work on that patch?
> > >
> >
> > I think this should solve the problem but let's first try to see if we
> > have any other problems. Because, say, if we don't have any other
> > problem, then maybe removing Assert might work but I guess we still
> > need to process the tuple to find that we don't need to assemble toast
> > for it which again seems like overkill.
>
> Yeah, other operation will also fail, basically, if txn->toast_hash is
> not NULL then we assume that we need to assemble the tuple using
> toast, but if there is next insert in another relation and if that
> relation doesn't have a toast table then it will fail with below
> error.  And otherwise also, if it is the same relation, then the toast
> chunks of previous tuple will be used for constructing this new tuple.
> I think we must have to clean the toast before processing the next
> tuple so I think we can go with the solution I mentioned above.
>
> static void
> ReorderBufferToastReplace
> {
> ...
>  toast_rel = RelationIdGetRelation(relation->rd_rel->reltoastrelid);
>   if (!RelationIsValid(toast_rel))
>   elog(ERROR, "could not open relation with OID %u",
>   relation->rd_rel->reltoastrelid);

The attached patch fixes by queuing the spec abort change and cleaning
up the toast hash on spec abort.  Currently, in this patch I am
queuing up all the spec abort changes, but as an optimization we can
avoid
queuing the spec abort for toast tables but for that we need to log
that as a flag in WAL. that this XLH_DELETE_IS_SUPER is for a toast
relation.

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

Commits

  1. Fix ABI break introduced by commit 4daa140a2f.

  2. Fix valgrind issue in pgoutput.c.

  3. Fix decoding of speculative aborts.

  4. pgoutput: Fix memory leak due to RelationSyncEntry.map.

  5. Fix initialization of RelationSyncEntry for streaming transactions.

  6. Implement streaming mode in ReorderBuffer.