A GUC variable to replace PGBE_ACTIVITY_SIZE

On Mon, 2008-06-23 at 03:52 +1000, Thomas Lee wrote:

* Should it be possible to set this new variable via a command-line
option ala shared_buffers?

I would say not: most parameters cannot be set by special command-line
parameters, and this one is not important enough to warrant special
treatment. Instead, "--track_activity_query_size=x" can be used as-is.

* I'm not exactly sure about the best way to add unit/regr tests for
this change.

AFAICS there isn't an easy way.

* I'm also not sure what's required in the way of updates to the
documentation.

postgresql.conf.sample and doc/src/sgml/config.sgml should be updated at
a minimum.

-Neil

I'd like to see some quick testing of this thing mentioned in the comments:

/*
* XXX if pgstat_track_activity_query_size is really large,
* it might be best to use strcpy not memcpy for copying the
* activity string?
*/

If we make it a GUC, there will be people running with "really large"
pgstat_track_activity_query_size settings. In fact I wouldn't be
surprised if people routinely cranked it up to the 10-100 KB range, just
in case.

It's going to be platform-dependent, for sure, but some quick
micro-benchmarks of where the break-even point between memcpy and strcpy
lies would be nice.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

An updated patch for the track_activity_query_size GUC variable.

Made sure it's a context diff this time around, as per the wiki
documentation.

Cheers,
T

Thomas Lee wrote:

An updated patch for the track_activity_query_size GUC variable.

Thanks, committed with some changes.

There was one bug: BackendStatusShmemSize() needs to report the memory
needed for the activity string buffers; it's used for sizing the single
fixed size shmem block on postmaster startup. I also fixed the
documentation to refer to the setting with the right name, noted that
it's a startup time option, and changed the wording a bit.

I tested the performance between strcpy and memcpy a little bit. As
expected, strcpy is a lot cheaper if the string is small. I chose
strcpy, since it's very likely that the buffer is over-sized, and even
if it's just the right size to hold typical queries, it's going to be
"<IDLE>" for idle connections.

Another simple optimization occurred to me while looking at this: we
should skip the memcpy/strcpy altogether if the BackendActivity slot is
not in use. That seems like a good bet, you usually don't try to max out
max_connections.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:

Another simple optimization occurred to me while looking at this: we
should skip the memcpy/strcpy altogether if the BackendActivity slot is
not in use. That seems like a good bet, you usually don't try to max out
max_connections.

Huh? How could we be assigning to a slot that is not in use?

regards, tom lane

Tom Lane wrote:

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:

Another simple optimization occurred to me while looking at this: we
should skip the memcpy/strcpy altogether if the BackendActivity slot is
not in use. That seems like a good bet, you usually don't try to max out
max_connections.

Huh? How could we be assigning to a slot that is not in use?

Before the patch, we loop through the shared PgBackendStatus slots
(there is MaxBackends of them), and issue a memcpy for each to copy it
to our local slot. After that, we check if it was actually in use.

After the patch, we still loop through the shared slots, but only issue
the memcpy for slots that are in use.

Hope this answers your question, I'm not sure what you meant...

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

"Heikki Linnakangas" <heikki@enterprisedb.com> writes:

Tom Lane wrote:

Huh? How could we be assigning to a slot that is not in use?

Before the patch, we loop through the shared PgBackendStatus slots
(there is MaxBackends of them), and issue a memcpy for each to copy it
to our local slot. After that, we check if it was actually in use.
After the patch, we still loop through the shared slots, but only issue
the memcpy for slots that are in use.

Oh, you're talking about *fetching* the slot contents, not *assigning*
to them. Sorry, probably should have read the patch instead of just
reacting to the comment ...

regards, tom lane