Hello 2013/7/25 Tom Lane <t...@sss.pgh.pa.us>: > Stephen Frost <sfr...@snowman.net> writes: >> I've just pushed up some much needed improvements to the comments which >> hopefully clarify that this function is using error_context_stack and >> calling the callbacks set up there by callers above on the PG call >> stack. > > Now that I'm a bit more awake, I realize that my comments last night > were off-target. As you say, the purpose of this function is to extract > the context information that's been stacked against the possibility of a > future error, so it's unrelated to actual error processing, and the > FlushErrorState call is *not* destroying its input data as I claimed. > >> Also, hopefully this makes it clear that errrordata is required >> to be empty when this function is called and therefore it can be cleaned >> up when exiting with FlushErrorState. > > But having said that, "unrelated" does not mean "cannot be used together > with". I think this implementation of the function is dangerous (PANIC? > really?), overly restrictive, and overcomplicated to boot. The only > reason you need to call FlushErrorState is to get rid of any palloc's > leaked by errcontext functions, and the only reason that that's even > of concern is that you're allocating them in ErrorContext which is a > precious resource. I don't think this function has any business > touching ErrorContext at all, precisely because it isn't part of error > recovery. Indeed, by eating up reserved ErrorContext space, you > increase the risk of an error within this function being unrecoverable. > Saner would be either: > > 1. Just run the whole business in the caller's context and leave it up > to the caller to worry about whether it needs to clean up memory usage. > > 2. Create a temporary context underneath CurrentMemoryContext, use that, > and then delete it when done. > > The only thing that needs to be considered an error condition is if the > errordata stack is too full to allow creation of the extra entry needed > by this function, which is an improbable situation. Other than > temporarily stacking and then unstacking that entry, you don't need to > have any side-effects on the state of the error subsystem. > >> I'm happy to rework this or even revert it if this use of the >> error_context_stack is just too grotty, but this certainly looks like a >> useful capability to have. > > I'm not objecting to the functionality, just to the implementation: > I think you could decouple this from error handling a lot better. > > One other minor gripe is that the function is documented as returning > the "call stack", which a C programmer would think means something > entirely different from what is actually output. I think you need to > use a different phrase there. "error context stack" would be OK.
I used a ErrorContext because I wasn't sure so errcontext and similar function can work in different context. Now I look there and there should be well initialized ErrorDataStack, due int set_errcontext_domain(const char *domain) { <------>ErrorData *edata = &errordata[errordata_stack_depth]; <------>/* we don't bother incrementing recursion_depth */ <------>CHECK_STACK_DEPTH(); <------>edata->context_domain = domain; <------>return 0; } but MemoryContext can be any - so probably some private context is ideal. Regards Pavel > > regards, tom lane > > > -- > Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-committers -- Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-committers