Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit pgbouncer statistics to what the wire protocol supports. #360

Closed
wants to merge 1 commit into from

Conversation

@ferringb
Copy link

ferringb commented Jan 22, 2019

Pgbouncer internal statistics are uint8 (64 bit), but the pg protocol is int8;
pgbouncer doesn't roll over the value it writes, instead assuming that the
wire protocol is uint when in fact it's int: see https://www.postgresql.org/docs/9.3/datatype-numeric.html .

To address this, the simplest fix is to force a modulus on what we serialize for uint8
values to limit them to int8 space.

It may be worth refactoring the internals of stats.c/admin.c rather than the
fix I'm proposing here- what is implemented here should cover all stats, but
it feels fragile in the sense that it's a duct tape fix tucked into an internal-
any consuming code that directly uses the underlying pktbuf functions can
bypass and reintroduce a variant of this bug.

I don't see a clean way to address this beyond vigilance combined w/ this
patch, thus proposing this fix.

Backing ticket for this is #350

Pgbouncer internal statistics are uint8 (64 bit), but the pg protocol is int8;
pgbouncer doesn't roll over the value it writes, instead assuming that the
wire protocol is uint when in fact it's int: see https://www.postgresql.org/docs/9.3/datatype-numeric.html .

To address this, the simplest fix is to force a modulus on what we serialize for uint8
values to limit them to int8 space.

It may be worth refactoring the internals of stats.c/admin.c rather than the
fix I'm proposing here- what is implemented here should cover all stats, but
it feels fragile in the sense that it's a duct tape fix tucked into an internal-
any consuming code that directly uses the underlying pktbuf functions can
bypass and reintroduce a variant of this bug.

I don't see a clean way to address this beyond vigilance combined w/ this
patch, thus proposing this fix.

Backing ticket for this is #150.
@eulerto

This comment has been minimized.

Copy link
Contributor

eulerto commented Jan 31, 2019

@ferringb The PostgreSQL protocol isn't limited to int8 (see [1]). PgBouncer is limiting that data to int4 or int8. It seems what you want is change the tupdesc key from i to q.

[1] https://www.postgresql.org/docs/11/protocol-message-formats.html

@ferringb

This comment has been minimized.

Copy link
Author

ferringb commented Feb 2, 2019

@eulerto pardon, but it's not 2^32 vs 2^64- the problem is that pgbouncer is exceeding the OID's value range it sets for stat columens. Protocol breakdown for one of the columns is thus from the RowDescription message:

column ID: 0
OID: 20
data type size: 8
type modifier: 0
format code: 0

OID20 is int8- per https://www.postgresql.org/docs/9.3/datatype-numeric.html that is limited to 63 bits; in tracing the postgres code I also find multiple capping functions (including in postgres's backend/utils/adt/int8.c that all use int64- signed, not unsigned). I'm pretty sure my analysis is correct here on that angle, but I'd appreciate a second set of eyes should I have missed something.

see larseen/pgbouncer_exporter#10 for the context this was caught via- pgbouncer_exporter enforces that int8 OID's are type int64 (long long) and was catching that it was being sent a value outside that range- 17606157912109483021, which is between 2^63 and 2^64.

I've seen multiple clients tolerate int8 exceeding 2^63; that said, golang lib/pg doesn't have that toleration, an int8 is a signed 64bit integer per the OID definitions.

Capping the pgbouncer statistics to 2^63 resolves this either way.

@stanhu

This comment has been minimized.

Copy link

stanhu commented Apr 22, 2019

We ran into the same issue: PgBouncer exports a 64-bit unsigned integer, but PostgreSQL only supports 64-bit signed integers, so the lib/pq client library flagged the errors here: https://github.com/lib/pq/blob/90697d60dd844d5ef6ff15135d0203f65d2f53b8/encode.go#L114

I suggest we either:

  1. Merge this pull request to cap the values as this pull request does
  2. Change the exported PgBouncer types of the 64-bit integers to use NUMERIC
@petere

This comment has been minimized.

Copy link
Contributor

petere commented Aug 3, 2019

I think the best solution would be to use numeric.

stanhu added a commit to stanhu/pgbouncer that referenced this pull request Aug 3, 2019
Internally these statistics are stored as unsigned 64-bit values,
but the PostgreSQL INT8 types can only express signed 64-bit values.
Convert all the stats to NUMERIC(20, 0) to avoid losing data.

Closes pgbouncer#360
stanhu added a commit to stanhu/pgbouncer that referenced this pull request Aug 3, 2019
Internally these statistics are stored as unsigned 64-bit values,
but the PostgreSQL INT8 types can only express signed 64-bit values.
Convert all the stats to NUMERIC(20, 0) to avoid losing data.

Closes pgbouncer#360
stanhu added a commit to stanhu/pgbouncer that referenced this pull request Aug 4, 2019
Internally these statistics are stored as unsigned 64-bit values,
but the PostgreSQL INT8 types can only express signed 64-bit values.
Convert all the stats to NUMERIC(20, 0) to avoid losing data.

Closes pgbouncer#360
@petere petere closed this in 2bc1747 Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.