From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Petr Jelinek <petr(dot)jelinek(at)enterprisedb(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> |
Subject: | Re: Decoding speculative insert with toast leaks memory |
Date: | 2021-06-02 04:12:23 |
Message-ID: | CAA4eK1LbWD2ZvfYpJKMcx+XmTNLFyLZ2564cmNCn7RGpCWsq_w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Jun 1, 2021 at 8:01 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
>
> 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.
>
I don't think that is required especially because we intend to
backpatch this, so I would like to keep such optimization for another
day. Few comments:
Comments:
------------
/*
* Super deletions are irrelevant for logical decoding, it's driven by the
* confirmation records.
*/
1. The above comment is not required after your other changes.
/*
* Either speculative insertion was confirmed, or it was
* unsuccessful and the record isn't needed anymore.
*/
if (specinsert != NULL)
2. The above comment needs some adjustment.
/*
* There's a speculative insertion remaining, just clean in up, it
* can't have been successful, otherwise we'd gotten a confirmation
* record.
*/
if (specinsert)
{
ReorderBufferReturnChange(rb, specinsert, true);
specinsert = NULL;
}
3. Ideally, we should have an Assert here because we shouldn't reach
without cleaning up specinsert. If there is still a chance then we
should mention that in the comments.
4.
+ case REORDER_BUFFER_CHANGE_INTERNAL_SPEC_ABORT:
+
+ /*
+ * Abort for speculative insertion arrived.
I think here we should explain why we can't piggyback cleanup on next
insert/update/delete.
5. Can we write a test case for it? I guess we don't need to use
multiple sessions if the conflicting record is already present.
Please see if the same patch works on back-branches? I guess this
makes the change bit tricky as it involves decoding a new message but
not sure if there is a better way.
--
With Regards,
Amit Kapila.
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2021-06-02 04:29:26 | Re: Fixup some appendStringInfo and appendPQExpBuffer calls |
Previous Message | Peter Smith | 2021-06-02 03:37:38 | Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options |