On Wed, Apr 28, 2021 at 8:51 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Amit Kapila <amit.kapil...@gmail.com> writes: > > On Wed, Apr 28, 2021 at 5:31 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > >> It's griping about this: > >> curtxn->concurrent_abort = true; > >> and I think it's got a point. There is little if any reason to have > >> confidence that curtxn must be non-NULL when this code is reached, > >> because it's in a PG_CATCH segment and there is a lot of code within > >> the PG_TRY that could throw an error before the spot where curtxn > >> is set. Not to mention that curtxn is set only conditionally. > > > We can do that to silence this Warning, otherwise, there won't be any > > problem because it is set only when we get a particular error code and > > we can get that error code only when the conditions that lead to > > curtxn being set are met. > > To be blunt, that argument is hopelessly naive. You cannot safely > make assumptions about a CATCH block having omniscient knowledge > of where an error was thrown from. At least, not with a check > no stronger than checking the SQLSTATE. All you need is some > extension ... or a user-defined function ... deciding to throw > that same SQLSTATE, and you're hosed. >
I see your point and it is possible that some callback can throw this error code. I think we need a stronger check than just SQLSTATE to ensure that we set concurrent abort flag and do other things in that check only when we are decoding non-committed xacts. I'll write about this on -hackers and do a further discussion there. -- With Regards, Amit Kapila.
