On Wed, Apr 28, 2021 at 8:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > On Wed, Apr 28, 2021 at 5:31 AM Tom Lane <tgl@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.