Skip to content

2.27.0.0-b137

2.27.0.0-b137: [#22421] YSQL: Address YB_TODOs in pg_stat_statements.c
@karthik-ramanathan-3006 karthik-ramanathan-3006 tagged this 21 May 13:09 
Summary:
This revision resolves two sets of YB_TODOs in pg_stat_statements:
1. Re-enabling password redaction
2. Removing a TODO related to sanitizing error output

**Enabling password redaction**
This feature was disabled by D34937 due to changes in postgres' parse interface (upstream commit 844fe9f159a948377907a63d0ef3fb16dc51ce50).
This change allows the parser to support multiple parse targets. In other words, it allows the parser to process not only SQL statements but also pl/pgSQL statements (amongst others).
With this change, it becomes necessary to specify the "parse target" or the kind of statement that is being input to the parser.
Given a query string, there is no good way to find out if it is a regular SQL statement(s), a pl/pgSQL expression or other. The current password redaction mechanism requires parsing the query to check for password tokens.
Since the "parse target" of the query is not known at the time of password redaction, it is possible for the parser to error out. This caused us to disable password redaction in D34937.
 - Prior to D34937, password redaction was originally supported only for CREATE/ALTER USER/ROLE statements as these are the only commands that contain a password token.
 - Between D34937 and the current revision, password redaction was disabled.

This revision re-enables password redaction by making the following changes:
 - Only UTILITY statements are subjected to the password redaction logic. This information is already available to the caller of `YbGetRedactedQueryString`.
 - CREATE/ALTER USER/ROLE statements fall under the category of UTILITY statements. Since it is known that the input is a regular SQL statement, we not have to deal with parse targets.
 - In fact, re-parsing the statement for password redaction is skipped altogether.
 - Although the need for password redaction is computed higher up the call stack, the actual redaction can be performed only just before the redacted string is required.
    - This is because extensions are irregular in the way they handle query strings. Some extensions do not make copies of the query string and just pass along a reference.
    - As a result, because of extension chaining, a redacted query may be passed up to the succeeding extension as the input query to a subsequent extension if redaction is performed higher up the call stack. This is a problem as the extension requires the actual query to work with.
    - Fixing the above is beyond the scope of this revision. So, extensions will now relay whether the given statement needs redaction all the way to the function that requires the redacted string.

Further, there was a memory leak in the function that computes the redacted string. This revision fixes the leak and simplifies the computation.

**Sanitizing error output**
Upstream commit 56df07bb9e50a3ca4d148c537524f00bccc6650e standardized file I/O error outputs in pg_stat_statements.c. D30859 violates this convention for an out-of-memory error message.
The latter has proven to be useful. So, marking the TODO as resolved by retaining the latter.
Jira: DB-11332

Test Plan:
Run the following test:
```
./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressPgStatStatements#schedule
./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressThirdPartyExtensionsOrafce#schedule'
./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressThirdPartyExtensionsPgStatMonitor#schedule'
```

Reviewers: ishan.chhangani, asaha, telgersma

Reviewed By: telgersma

Subscribers: jason, smishra, yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D43289
Assets 2
Loading