Hi,
On 2019-08-01 22:49:48 +0200, Julien Rouhaud wrote:
> On Thu, Aug 1, 2019 at 8:36 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > On 2019-08-01 14:20:46 -0400, Robert Haas wrote:
> > > However, I think that the fact that this patch adds 15 new calls to
> > > pg_atomic_write_u64(&MyProc->queryId, ...) is probably not a good
> > > sign. It seems like we ought to be able to centralize it better than
> > > that.
> >
> > +1
>
> Unfortunately I didn't find a better way to do that. Since you can
> have nested execution, I don't see how to avoid adding extra code in
> every parts of query execution.
At least my +1 is not primarily about the number of sites that need to
handle queryid changes, but that they all need to know about the way the
queryid is stored. Including how atomicity etc is handled. That
knowledge should be in one or two places, not more. In a file where that
knowledge makes sense.
I'm *also* concerned about the number of places, as that makes it likely
that some have been missed/new ones will be introduced without the
queryid handling. But that wasn't what I was referring to above.
I'm actually quite unconvinced that it's sensible to update the global
value for nested queries. That'll mean e.g. the log_line_prefix and
pg_stat_activity values are most of the time going to be bogus while
nested, because the querystring that's associated with those will *not*
be the value that the queryid corresponds to. elog.c uses
debug_query_string to log the statement, which is only updated for
top-level queries (outside of some exceptions like parallel workers for
parallel queries in a function or stuff like that). And pg_stat_activity
is also only updated for top level queries.
Greetings,
Andres Freund