Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?

>
> >
> > 1.
> > +void
> > +pgstat_report_queryid(uint64 queryId, bool force)
> > +{
> > + volatile PgBackendStatus *beentry = MyBEEntry;
> > +
> > + if (!beentry)
> > + return;
> > +
> > + /*
> > + * if track_activities is disabled, st_queryid should already have been
> > + * reset
> > + */
> > + if (!pgstat_track_activities)
> > + return;
> >
> > The above two conditions can be clubbed together in a single condition.
> Right, I just kept it separate as the comment is only relevant for the 2nd
> test. I'm fine with merging both if needed.

I feel we should merge both of the conditions as it is done in
pgstat_report_xact_timestamp(). Probably we can write a common comment to
explain both the conditions.

> 2.
> > +/* ----------
> > + * pgstat_get_my_queryid() -
> > + *
> > + * Return current backend's query identifier.
> > + */
> > +uint64
> > +pgstat_get_my_queryid(void)
> > +{
> > + if (!MyBEEntry)
> > + return 0;
> > +
> > + return MyBEEntry->st_queryid;
> > +}
> >
> > Is it safe to directly read the data from MyBEEntry without
> > calling pgstat_begin_read_activity() and pgstat_end_read_activity().
> Kindly
> > ref pgstat_get_backend_current_activity() for more information. Kindly
> let
> > me know if I am wrong.
> This field is only written by a backend for its own entry.
> pg_stat_get_activity already has required protection, so the rest of the
> calls
> to read that field shouldn't have any risk of reading torn values on
> platform
> where this isn't an atomic operation due to concurrent write, as it will be
> from the same backend that originally wrote it. It avoids some overhead to
> retrieve the queryid, but if people think it's worth having the loop (or a
> comment explaining why there's no loop) I'm also fine with it.

Thanks for the explanation. Please add a comment explaining why there is no
loop.

Thanks and Regards,
Nitin Jadhav

On Tue, Apr 6, 2021 at 8:40 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:

> On Tue, Apr 06, 2021 at 08:05:19PM +0530, Nitin Jadhav wrote:
> >
> > 1.
> > +void
> > +pgstat_report_queryid(uint64 queryId, bool force)
> > +{
> > + volatile PgBackendStatus *beentry = MyBEEntry;
> > +
> > + if (!beentry)
> > + return;
> > +
> > + /*
> > + * if track_activities is disabled, st_queryid should already have been
> > + * reset
> > + */
> > + if (!pgstat_track_activities)
> > + return;
> >
> > The above two conditions can be clubbed together in a single condition.
>
> Right, I just kept it separate as the comment is only relevant for the 2nd
> test. I'm fine with merging both if needed.
>
> > 2.
> > +/* ----------
> > + * pgstat_get_my_queryid() -
> > + *
> > + * Return current backend's query identifier.
> > + */
> > +uint64
> > +pgstat_get_my_queryid(void)
> > +{
> > + if (!MyBEEntry)
> > + return 0;
> > +
> > + return MyBEEntry->st_queryid;
> > +}
> >
> > Is it safe to directly read the data from MyBEEntry without
> > calling pgstat_begin_read_activity() and pgstat_end_read_activity().
> Kindly
> > ref pgstat_get_backend_current_activity() for more information. Kindly
> let
> > me know if I am wrong.
>
> This field is only written by a backend for its own entry.
> pg_stat_get_activity already has required protection, so the rest of the
> calls
> to read that field shouldn't have any risk of reading torn values on
> platform
> where this isn't an atomic operation due to concurrent write, as it will be
> from the same backend that originally wrote it. It avoids some overhead to
> retrieve the queryid, but if people think it's worth having the loop (or a
> comment explaining why there's no loop) I'm also fine with it.
>