Re: [BUG] failed assertion in EnsurePortalSnapshotExists() - Mailing list pgsql-hackers

From Drouvot, Bertrand
Subject Re: [BUG] failed assertion in EnsurePortalSnapshotExists()
Date
Msg-id d66da5f7-eb6c-4753-84a2-0c3811f93e5a@amazon.com
Whole thread Raw
In response to Re: [BUG] failed assertion in EnsurePortalSnapshotExists()  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: [BUG] failed assertion in EnsurePortalSnapshotExists()
List pgsql-hackers
On 9/30/21 7:16 PM, Tom Lane wrote:
> "Drouvot, Bertrand" <bdrouvot@amazon.com> writes:
>> [ v2-0003-EnsurePortalSnapshotExists-failed-assertion.patch ]
> Looking through this, I think you were overenthusiastic about applying
> PushActiveSnapshotWithLevel.  We don't really need to use it except in
> the places where we're setting portalSnapshot, because other than those
> cases we don't have a risk of portalSnapshot becoming a dangling pointer.
> Also, I'm quite nervous about applying PushActiveSnapshotWithLevel to
> snapshots that aren't created by the portal machinery itself, because
> we don't know very much about where passed-in snapshots came from or
> what the caller thinks their lifespan is.

Oh right, I did not think about it, thanks!

>
> The attached revision therefore backs off to only using the new code
> in the two places where we really need it.

Ok, so in PortalRunUtility() and EnsurePortalSnapshotExists().

>    I made a number of
> more-cosmetic revisions too.
thanks!
> Notably, I think it's useful to frame
> the testing shortcoming as "we were not testing COMMIT/ROLLBACK
> inside a plpgsql exception block".  So I moved the test code to the
> plpgsql tests and made it check ROLLBACK too.
Indeed, makes sense.
>
>                          regards, tom lane
>
> PS: Memo to self: in the back branches, the new field has to be
> added at the end of struct Portal.

out of curiosity, why?

Thanks

Bertrand




pgsql-hackers by date:

Previous
From: James Coleman
Date:
Subject: Re: Document spaces in .pgpass need to be escaped
Next
From: Tom Lane
Date:
Subject: Re: [EXTERNAL] Re: Add ETIMEDOUT to ALL_CONNECTION_FAILURE_ERRNOS