On Sat, Jan 30, 2016 at 04:28:47PM -0500, Robert Haas wrote:
> I think I've
> pretty much said what I have to say about this; if nothing I wrote up
> until now swayed you, it's unlikely that anything else I say after
> this point will either.
Say I drop the parts that change the binary. Does the attached v2 manage to
improve PostgreSQL, or is it neutral-or-harmful like v1?
On Sat, Jan 30, 2016 at 04:28:47PM -0500, Robert Haas wrote:
> On Sat, Jan 30, 2016 at 2:13 PM, Noah Misch <n...@leadboat.com> wrote:
> > On Sat, Jan 30, 2016 at 07:37:45AM -0500, Robert Haas wrote:
> > > On Fri, Jan 29, 2016 at 11:19 PM, Noah Misch <n...@leadboat.com> wrote:
> > > > On Thu, Jan 28, 2016 at 11:12:15AM -0500, Robert Haas wrote:
> > > >> The code you quote emits a warning
> > > >> about a reasonably forseeable situation that can never be right, but
> > > >> there's no particular reason to think that MarkPortalFailed is the
> > > >> wrong thing to do here if that situation came up.
> > > >
> > > > I have two reasons to expect these MarkPortalFailed() calls will be the
> > > > wrong
> > > > thing for hypothetical code reaching them. First, PortalRun() and
> > > > peers reset
> > > > ActivePortal and PortalContext on error in addition to fixing portal
> > > > status
> > > > with MarkPortalFailed(). If code forgets to update the status, there's
> > > > a
> > > > substantial chance it forgot the other two. My patch added a comment
> > > > explaining the second reason:
> > > >
> > > > + /*
> > > > + * See similar code in AtSubAbort_Portals(). This
> > > > would fire if code
> > > > + * orchestrating multiple top-level transactions within
> > > > a portal, such
> > > > + * as VACUUM, caught errors and continued under the
> > > > same portal with a
> > > > + * fresh transaction. No part of core PostgreSQL
> > > > functions that way,
> > > > + * though it's a fair thing to want. Such code would
> > > > wish the portal
> > > > + * to remain ACTIVE, as in PreCommit_Portals(); we
> > > > don't cater to
> > > > + * that. Instead, like AtSubAbort_Portals(), interpret
> > > > this as a bug.
> > > > + */
> > >
> > > You may be right, but then again Tom had a different opinion, even
> > > after seeing your patch, and he's no dummy.
> >
> > Eh? Tom last posted to this thread before I first posted a patch.
>
> http://www.postgresql.org/message-id/29758.1451780...@sss.pgh.pa.us
> seems to me to be a vote against the concept embodied by the patch.
That much is true. The order of postings is the opposite of what you stated,
and there's no mailing list evidence that anyone other than you has read my
explanation of why MarkPortalFailed() is wrong here. Alternately, I demand
the schematics for Tom's time machine.
diff --git a/src/backend/utils/mmgr/portalmem.c
b/src/backend/utils/mmgr/portalmem.c
index a53673c..c840aa6 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -765,7 +765,14 @@ AtAbort_Portals(void)
{
Portal portal = hentry->portal;
- /* Any portal that was actually running has to be considered
broken */
+ /*
+ * See similar code in AtSubAbort_Portals(). This would fire
if code
+ * orchestrating multiple top-level transactions within a
portal, such
+ * as VACUUM, caught errors and continued under the same portal
with a
+ * fresh transaction. No part of core PostgreSQL functions
that way.
+ * XXX Such code would wish the portal to remain ACTIVE, as in
+ * PreCommit_Portals().
+ */
if (portal->status == PORTAL_ACTIVE)
MarkPortalFailed(portal);
@@ -919,9 +926,11 @@ AtSubAbort_Portals(SubTransactionId mySubid,
portal->activeSubid = parentSubid;
/*
- * Upper-level portals that failed while
running in this
- * subtransaction must be forced into FAILED
state, for the
- * same reasons discussed below.
+ * If a bug in a MarkPortalActive() caller has
it miss cleanup
+ * after having failed while running an
upper-level portal in
+ * this subtransaction, we don't know what else
in the portal
+ * is wrong. Force it into FAILED state, for
the same reasons
+ * discussed below.
*
* We assume we can get away without forcing
upper-level READY
* portals to fail, even if they were run and
then suspended.
@@ -960,7 +969,11 @@ AtSubAbort_Portals(SubTransactionId mySubid,
* Force any live portals of my own subtransaction into FAILED
state.
* We have to do this because they might refer to objects
created or
* changed in the failed subtransaction, leading to crashes
within
- * ExecutorEnd when portalcmds.c tries to close down the portal.
+ * ExecutorEnd when portalcmds.c tries to close down the
portal. Each
+ * MarkPortalActive() caller ensures it updates the portal
status
+ * again before relinquishing control, so ACTIVE can't happen
here.
+ * If a bug does make it happen, dispose the portal like a
normal
+ * MarkPortalActive() caller would.
*/
if (portal->status == PORTAL_READY ||
portal->status == PORTAL_ACTIVE)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
