On Wed, Jan 27, 2016 at 10:40 PM, Noah Misch <noah@leadboat.com> wrote:
> On Sun, Jan 03, 2016 at 12:35:56AM -0500, Noah Misch wrote:
>> On Sat, Jan 02, 2016 at 07:22:13PM -0500, Tom Lane wrote:
>> > Noah Misch <noah@leadboat.com> writes:
>> > > I am inclined to add an Assert(portal->status != PORTAL_ACTIVE) to emphasize
>> > > that this is backup only. MarkPortalActive() callers remain responsible for
>> > > updating the status to something else before relinquishing control.
>> >
>> > No, I do not think that would be an improvement. There is no contract
>> > saying that this must be done earlier, IMO.
>>
>> Indeed, nobody wrote a contract. The assertion would record what has been the
>> sole standing practice for eleven years (since commit a393fbf9). It would
>> prompt discussion if a proposed patch would depart from that practice, and
>> that is a good thing. Also, every addition of dead code should label that
>> code to aid future readers.
>
> Here's the patch I have in mind. AtAbort_Portals() has an older
> MarkPortalFailed() call, and the story is somewhat different there per its new
> comment. That call is plausible to reach with no bug involved, but
> MarkPortalFailed() would then be the wrong thing.
+ Assert(portal->status != PORTAL_ACTIVE); if (portal->status == PORTAL_ACTIVE)
MarkPortalFailed(portal);
Now that just looks kooky to me. We assert that the portal isn't
active, but then cater to the possibility that it might be anyway?
That seems totally contrary to our usual programming practices, and a
bad idea for that reason.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company