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