Skip to content

When requested, load the "citus" library first #3761

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

Merged
merged 1 commit into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions internal/patroni/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,17 +227,24 @@ func DynamicConfiguration(
// Override the above with mandatory parameters.
if pgParameters.Mandatory != nil {
for k, v := range pgParameters.Mandatory.AsMap() {
// Unlike other PostgreSQL parameters that have mandatory values,
// shared_preload_libraries is a comma separated list that can have
// other values appended in addition to the mandatory values. Below,
// any values provided in the CRD are appended after the mandatory
// values.
s, ok := parameters[k].(string)
if k == "shared_preload_libraries" && ok {
parameters[k] = v + "," + s
} else {
parameters[k] = v

// This parameter is a comma-separated list. Rather than overwrite the
// user-defined value, we want to combine it with the mandatory one.
// Some libraries belong at specific positions in the list, so figure
// that out as well.
if k == "shared_preload_libraries" {
// Load mandatory libraries ahead of user-defined libraries.
if s, ok := parameters[k].(string); ok && len(s) > 0 {
v = v + "," + s
}
// Load "citus" ahead of any other libraries.
// - https://github.com/citusdata/citus/blob/v12.0.0/src/backend/distributed/shared_library_init.c#L417-L419
if strings.Contains(v, "citus") {
v = "citus," + v
Comment on lines +242 to +243
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making sure I've got this (though the test helps):

  1. we want citus to be first;
  2. citus can be present several times without side effect;
  3. removing citus from that list (from the non-first position) would be brittle

Is that a fair summation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a fair summation?

Yep.

2. citus can be present several times without side effect;

There is a set of related parameters for loading libraries that amount to LOAD statements at certain times.

Postgres compares inodes to detect symlink chains as well.

3. removing citus from that list (from the non-first position) would be brittle

A variety of names can refer to the same library. As I recall, citus, citus.so, and a full path are acceptable. This does not handle the full path case, but in my experience that is quite unusual.

Being more deliberate with these names requires parsing, which #3530 did partially. Ultimately, matching on names is still a step removed from the issue: the Citus code that complains could be in a file with any name. :neckbeard:

}
}

parameters[k] = v
}
}
postgresql["parameters"] = parameters
Expand Down
29 changes: 28 additions & 1 deletion internal/patroni/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ func TestDynamicConfiguration(t *testing.T) {
},
},
{
name: "postgresql.parameters: mandatory shared_preload_libraries bad type",
name: "postgresql.parameters: mandatory shared_preload_libraries wrong-type is ignored",
input: map[string]any{
"postgresql": map[string]any{
"parameters": map[string]any{
Expand All @@ -420,6 +420,33 @@ func TestDynamicConfiguration(t *testing.T) {
},
},
},
{
name: "postgresql.parameters: shared_preload_libraries order",
input: map[string]any{
"postgresql": map[string]any{
"parameters": map[string]any{
"shared_preload_libraries": "given, citus, more",
},
},
},
params: postgres.Parameters{
Mandatory: parameters(map[string]string{
"shared_preload_libraries": "mandatory",
}),
},
expected: map[string]any{
"loop_wait": int32(10),
"ttl": int32(30),
"postgresql": map[string]any{
"parameters": map[string]any{
"shared_preload_libraries": "citus,mandatory,given, citus, more",
},
"pg_hba": []string{},
"use_pg_rewind": true,
"use_slots": false,
},
},
},
{
name: "postgresql.pg_hba: wrong-type is ignored",
input: map[string]any{
Expand Down
5 changes: 1 addition & 4 deletions internal/pgaudit/postgres.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package pgaudit

import (
"context"
"strings"

"github.com/crunchydata/postgres-operator/internal/logging"
"github.com/crunchydata/postgres-operator/internal/postgres"
Expand Down Expand Up @@ -67,7 +66,5 @@ func PostgreSQLParameters(outParameters *postgres.Parameters) {
// PostgreSQL must be restarted when changing this value.
// - https://github.com/pgaudit/pgaudit#settings
// - https://www.postgresql.org/docs/current/runtime-config-client.html
shared := outParameters.Mandatory.Value("shared_preload_libraries")
outParameters.Mandatory.Add("shared_preload_libraries",
strings.TrimPrefix(shared+",pgaudit", ","))
outParameters.Mandatory.AppendToList("shared_preload_libraries", "pgaudit")
}
9 changes: 1 addition & 8 deletions internal/pgmonitor/postgres.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,7 @@ func PostgreSQLParameters(inCluster *v1beta1.PostgresCluster, outParameters *pos
// Exporter expects that shared_preload_libraries are installed
// pg_stat_statements: https://access.crunchydata.com/documentation/pgmonitor/latest/exporter/
// pgnodemx: https://github.com/CrunchyData/pgnodemx
libraries := []string{"pg_stat_statements", "pgnodemx"}

defined, found := outParameters.Mandatory.Get("shared_preload_libraries")
if found {
libraries = append(libraries, defined)
}
Comment on lines -53 to -58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making sure I'm following: this code here would've prepended "pg_stat_statements", "pgnodemx" to the list -- and we don't want them prepended, we want them appended, yeah?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a change.


outParameters.Mandatory.Add("shared_preload_libraries", strings.Join(libraries, ","))
outParameters.Mandatory.AppendToList("shared_preload_libraries", "pg_stat_statements", "pgnodemx")
outParameters.Mandatory.Add("pgnodemx.kdapi_path",
postgres.DownwardAPIVolumeMount().MountPath)
}
Expand Down
16 changes: 16 additions & 0 deletions internal/postgres/parameters.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,22 @@ func (ps *ParameterSet) Add(name, value string) {
ps.values[ps.normalize(name)] = value
}

// AppendToList adds each value to the right-hand side of parameter name
// as a comma-separated list without quoting.
func (ps *ParameterSet) AppendToList(name string, value ...string) {
result := ps.Value(name)

if len(value) > 0 {
if len(result) > 0 {
result += "," + strings.Join(value, ",")
} else {
result = strings.Join(value, ",")
}
}

ps.Add(name, result)
}

// Get returns the value of parameter name and whether or not it was present in ps.
func (ps ParameterSet) Get(name string) (string, bool) {
value, ok := ps.values[ps.normalize(name)]
Expand Down
23 changes: 23 additions & 0 deletions internal/postgres/parameters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,3 +68,26 @@ func TestParameterSet(t *testing.T) {
ps2.Add("x", "n")
assert.Assert(t, ps2.Value("x") != ps.Value("x"))
}

func TestParameterSetAppendToList(t *testing.T) {
ps := NewParameterSet()

ps.AppendToList("empty")
assert.Assert(t, ps.Has("empty"))
assert.Equal(t, ps.Value("empty"), "")

ps.AppendToList("empty")
assert.Equal(t, ps.Value("empty"), "", "expected no change")

ps.AppendToList("full", "a")
assert.Equal(t, ps.Value("full"), "a")

ps.AppendToList("full", "b")
assert.Equal(t, ps.Value("full"), "a,b")

ps.AppendToList("full")
assert.Equal(t, ps.Value("full"), "a,b", "expected no change")

ps.AppendToList("full", "a", "cd", `"e"`)
assert.Equal(t, ps.Value("full"), `a,b,a,cd,"e"`)
}