On Thu, Jan 28, 2016 at 12: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.
+ * fresh transaction. No part of core PostgreSQL functions that way,
+ * though it's a fair thing to want. Such code would wish the portal
From the point of view of core code, this stands true, but, for my 2c,
honestly, I think that is just going to annoy more people working on
plugins and forks of Postgres. When working on Postgres-XC and
developing stuff for the core code, I recall having been annoyed a
couple of times by similar assert limitations, because sometimes they
did not actually make sense in the context of what I was doing, and
other times things got suddendly broken after bumping the forks code
base to a major upgrades because a routine used up to now broke its
contract. Perhaps I was doing it wrong at this point though, and
should have used something else.
--
Michael
