Skip to content

Commit e3a07dd

Browse files
authored
Collection helper detection: Improve handling of default arguments (#691)
This previously crashed in some cases due to an incomplete refactoring I had done in a prior commit. Add test coverage to prevent recurrence. In passing move SQL helper definitions to a dedicated util/helpers directory and add the helper definition for "get_stat_statements", which whilst not used much these days, is a good test case here.
1 parent 27d7793 commit e3a07dd

File tree

5 files changed

+110
-4
lines changed

5 files changed

+110
-4
lines changed

input/postgres/collection.go

+5-3
Original file line numberDiff line numberDiff line change
@@ -120,9 +120,11 @@ func (c *Collection) findHelperFunction(name string, inputTypes []string) (state
120120
parts := strings.Split(arg, " ")
121121

122122
// Check if function has more arguments required than we're expecting
123-
//
124-
// Note we allow extra arguments if they have default values.
125-
if idx >= len(inputTypes) && parts[2] != "DEFAULT" {
123+
if idx >= len(inputTypes) {
124+
// Allow extra arguments if they have default values.
125+
if len(parts) >= 3 && parts[2] == "DEFAULT" {
126+
break
127+
}
126128
mismatch = true
127129
break
128130
}

input/postgres/collection_test.go

+97
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
package postgres_test
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/pganalyze/collector/input/postgres"
8+
"github.com/pganalyze/collector/state"
9+
"github.com/pganalyze/collector/util"
10+
)
11+
12+
type collectionHelperTestpair struct {
13+
Name string
14+
InputTypes []string
15+
ExpectedExists bool
16+
ExpectedReturnType string
17+
}
18+
19+
var collectionHelperTests = []collectionHelperTestpair{
20+
{
21+
"explain_analyze",
22+
[]string{"text", "text[]", "text[]", "text[]"},
23+
true,
24+
"text",
25+
},
26+
{
27+
"get_stat_statements",
28+
[]string{"boolean"},
29+
true,
30+
"SETOF pg_stat_statements",
31+
},
32+
// Verify we don't match on a shorter set of arguments
33+
{
34+
"explain_analyze",
35+
[]string{"text", "text[]", "text[]"},
36+
false,
37+
"",
38+
},
39+
// Verify we don't match on a longer set of arguments
40+
{
41+
"explain_analyze",
42+
[]string{"text", "text[]", "text[]", "text[]", "text[]"},
43+
false,
44+
"",
45+
},
46+
// Verify we don't match on different argument types
47+
{
48+
"explain_analyze",
49+
[]string{"text", "text[]", "text[]", "float"},
50+
false,
51+
"",
52+
},
53+
// Verify we allow default arguments to be ommitted
54+
{
55+
"get_stat_statements",
56+
[]string{},
57+
true,
58+
"SETOF pg_stat_statements",
59+
},
60+
}
61+
62+
func TestCollectionFindHelper(t *testing.T) {
63+
db := setupTest(t)
64+
defer db.Close()
65+
66+
_, err := db.Exec("CREATE EXTENSION IF NOT EXISTS pg_stat_statements")
67+
if err != nil {
68+
t.Fatalf("Could not create pg_stat_statements extension: %s", err)
69+
}
70+
71+
_, err = db.Exec(util.GetStatStatementsHelper)
72+
if err != nil {
73+
t.Fatalf("Could not load get_stat_statements helper: %s", err)
74+
}
75+
76+
ctx := context.Background()
77+
logger := &util.Logger{}
78+
server := &state.Server{}
79+
opts := state.CollectionOpts{}
80+
81+
collection, err := postgres.NewCollection(ctx, logger, server, opts, db)
82+
if err != nil {
83+
t.Fatalf("Could not initialize collection struct: %s", err)
84+
}
85+
86+
for _, pair := range collectionHelperTests {
87+
helperExists := collection.HelperExists(pair.Name, pair.InputTypes)
88+
if helperExists != pair.ExpectedExists {
89+
t.Errorf("Incorrect exists state for helper %s(%+v):\n got: %+v\n expected: %+v", pair.Name, pair.InputTypes, helperExists, pair.ExpectedExists)
90+
}
91+
92+
helperReturnType := collection.HelperReturnType(pair.Name, pair.InputTypes)
93+
if helperReturnType != pair.ExpectedReturnType {
94+
t.Errorf("Incorrect return type for helper %s(%+v):\n got: %s\n expected: %s", pair.Name, pair.InputTypes, helperReturnType, pair.ExpectedReturnType)
95+
}
96+
}
97+
}
File renamed without changes.

util/helpers/get_stat_statements.sql

+4
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
CREATE OR REPLACE FUNCTION pganalyze.get_stat_statements(showtext boolean = true) RETURNS SETOF pg_stat_statements AS
2+
$$
3+
/* pganalyze-collector */ SELECT * FROM public.pg_stat_statements(showtext);
4+
$$ LANGUAGE sql VOLATILE SECURITY DEFINER;

util/sql_helpers.go

+4-1
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,8 @@ import (
44
_ "embed"
55
)
66

7-
//go:embed explain_analyze_helper.sql
7+
//go:embed helpers/explain_analyze.sql
88
var ExplainAnalyzeHelper string
9+
10+
//go:embed helpers/get_stat_statements.sql
11+
var GetStatStatementsHelper string

0 commit comments

Comments
 (0)