Re: [HACKERS] Re: pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <

Hi,

On 2016-05-06 14:18:22 -0500, Kevin Grittner wrote:
> I rebased the patch Ants posted (attached), and am running
> benchmarks on a cthulhu (a big NUMA machine with 8 memory nodes).
> Normally I wouldn't post results without a lot more data points
> with multiple samples at each, but the initial results have me
> wondering whether people would like to see this pushed later today
> so that it has some time in the buildfarm and then into beta1.

I think that generally would make sense. We quite possibly need some
further changes, but it seems more likely that we can find them if the
patch runs close to the disabled performance.

> Running the r/w TPC-B (sort of) load with scale, jobs, and threads
> at 1000, and the database configured as I would for a production
> server of that size, preliminary TPS results are:
>
> master, -1: 8158
> master, 10min: 2019
> Ants' patch, 10min: 7804

That's rather nice. Did you test read-only as well?

If you'd feel more comfortable committing after I've run some
performance tests, I could kick off some soon.

> I can see arguments for tuning this far in time for the beta, as
> well as the argument to wait until after the beta, so I'm just
> throwing it out there to see what other people think. I wouldn't
> do it unless I have three runs at -1 and 10min with the patch, all
> showing similar numbers. If the BF chokes on it I would revert
> this optimization attempt.

+1 for going forward. I'm still doubtful that it's a good idea to the
map maintenance from GetSnapshotData(), but the issue becomes much less
severe when addressed like this.

The primary reasons why I'd like to move it is because of the
significant amount of added gettimeofday() calls which are a real hog in
some virtualized environments, and because I'm doubtful of tying the
current time to the xmin horizon.

> diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
> index e1551a3..b7d965a 100644
> --- a/src/backend/utils/time/snapmgr.c
> +++ b/src/backend/utils/time/snapmgr.c
> @@ -80,8 +80,11 @@ typedef struct OldSnapshotControlData
> */
> slock_t mutex_current; /* protect current timestamp */
> int64 current_timestamp; /* latest snapshot timestamp */
> - slock_t mutex_latest_xmin; /* protect latest snapshot xmin */
> + slock_t mutex_latest_xmin; /* protect latest snapshot xmin
> + * and next_map_update
> + */
> TransactionId latest_xmin; /* latest snapshot xmin */
> + int64 next_map_update; /* latest snapshot valid up to */
> slock_t mutex_threshold; /* protect threshold fields */
> int64 threshold_timestamp; /* earlier snapshot is old */
> TransactionId threshold_xid; /* earlier xid may be gone */

Overly nitpickily I'd refer to the actual variable name (instead of
"latest snapshot xmin") in the mutex_latest_xmin comment.

> if (!same_ts_as_threshold)
> {
> + if (ts == update_ts)
> + {
> + xlimit = latest_xmin;
> + if (NormalTransactionIdFollows(xlimit, recentXmin))
> + SetOldSnapshotThresholdTimestamp(ts, xlimit);
> + }
> + else
> + {
> LWLockAcquire(OldSnapshotTimeMapLock, LW_SHARED);
>
> if (oldSnapshotControl->count_used > 0

I guess it's just an issue in my mail-reader, but the indentation looks
funky here.

Looks roughly sensible.

Greetings,

Andres Freund