Re: Deferring some AtStart* allocations?

On Fri, Oct 24, 2014 at 9:17 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> If
>> that subtransaction abouts, AtEOSubXact_Inval() gets called again,
>> sees that it has messages (that it inherited from the innermost
>> subtransaction), and takes the exact same code-path that it would have
>> pre-patch.
>
> Consider what happens if the innermost transaction commits and the
> second innermost one rolls back. AtEOSubXact_Inval() won't do anything
> because it doesn't have any entries itself.

This is the part I don't understand.  After the innermost transaction
commits, it *does* have entries itself.  This whole block is basically
just an optimization:

+               if (myInfo->parent == NULL || myInfo->parent->my_level
< my_level - 1)
+               {
+                       myInfo->my_level--;
+                       return;
+               }

If we removed that code, then we'd just do this instead:
       /* Pass up my inval messages to parent */       AppendInvalidationMessages(&myInfo->parent->PriorCmdInvalidMsgs,
                                &myInfo->PriorCmdInvalidMsgs);
 
       /* Pending relcache inval becomes parent's problem too */       if (myInfo->RelcacheInitFileInval)
myInfo->parent->RelcacheInitFileInval= true;
 

That should be basically equivalent.  I mean, we need a bit of surgery
to ensure that the parent entry actually exists before we attach stuff
to it, but that's mechanical.  The whole point here though is that,
pre-patch, the parent has an empty entry and we have one with stuff in
it.  What I think happens in the current code is that we take all of
the stuff in our non-empty entry and move it into the parent's empty
entry, so that the parent ends up with an entry identical to ours but
with a level one less than we had.  We could do that here too,
manufacturing the empty entry and then moving our stuff into it and
then deleting our original entry, but that seems silly; just change
the level and call it good.

IOW, I don't see how I'm *changing* the logic here at all.  Why
doesn't whatever problem you're concerned about exist in the current
coding, too?

> Even though there's still
> valid cache inval entries containing the innermost xact's version of the
> catalog, now not valid anymore.

This part doesn't make sense to me either.  The invalidation entries
don't put data into the caches; they take data out.  When we change
stuff, we generate invalidation messages.  At end of statement, once
we've done CommandCounterIncrement(), we execute those invalidation
messages locally, so that the next cache reload the updated state.  At
end of transaction, once we've committed, we send those invalidation
messages to the shared queue to be executed by everyone else, so that
they also see the updated state.  If we abort a transaction or
sub-transaction, we re-execute those same invalidation messages so
that we flush the new state, forcing the next set of cache reloads to
again pull in the old state.  The patch isn't changing - or not
intentionally anyway - anything about any of that logic.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company