Pgx: Fallback valuer option

[Oliver-Fish]

I am currently moving my code base over to PGX, and what I really like is that native go types automatically map into the relevant PostgreSQL columns without having to use custom types.

However I am also looking to implement Pgbouncer and was quite disappointed to see the following code stops working when I set PreferSimpleProtocol to true as required for pgbouncer to work.

    a := testStruct{
        Attributes: map[string]interface{}{
            "num":  5,
            "num2": "test",
        },
    }

    _, err = conn.Exec(context.TODO(), `
    INSERT INTO
        test --JSONB column
    (attributes)
    VALUES($1)
    `, a.Attributes)
    if err != nil {
        panic(err)
    }

Cannot encode map[string]interface {} in simple protocol - map[string]interface {} must implement driver.Valuer, pgtype.TextEncoder, or be a native type

The reason that this happens makes complete sense to me, however I would like to avoid changing the types in all my structs from a built-in type to something custom like the types in the pgtype package.

I find these types increase the complexity of my code and I much prefer working with the built-in types and don't really like the idea of using custom types just so I can push values to the database.

Would you consider adding a fallback valuer method that can be set from outside the package? much like the below

    config, err := pgx.ParseConfig("--ommited--")
    if err != nil {
        panic(err)
    }

    config.PreferSimpleProtocol = true

    config.FallbackValuer = func(arg interface{}) (interface{}, error) {
        switch arg := arg.(type) {
        case map[string]interface{}:
            b, err := json.Marshal(&arg)
            return string(b), err
        }
        return nil, nil
    }

    conn, err := pgx.ConnectConfig(context.TODO(), config)
    if err != nil {
        panic(err)
    }

This would allow me to define logic for any extra types that are not defined inside convertSimpleArgument.

Alternatively I had the idea of extending convertSimpleArgument to support slices and maps but I think that might not be possible. This is because we don't know the actual column type since as far as I know this is all done in a single round trip with PreferSimpleProtocol enabled.

I am happy to do the implementation myself and create a PR if we can agree on a method that would be acceptable.

[redbaron]

set PreferSimpleProtocol to true as required for pgbouncer to work.

Out of curiosity, why is that? pgbouncer works with extended protocol, it even has an option to disable simple protocol

[redbaron]

As for the question, what if for testStruct you implement Valuer and do .Attributes on SQL side?

Or you can introduce new wrapper which implements relevant interfaces and you use only when making a query, so your structs remain untouched:

conn.Exec(context.TODO(), `
    INSERT INTO
        test --JSONB column
    (attributes)
    VALUES($1)
    `, testStructAttributes(a.Attributes))

where

type testStructAttributes  map[string]interface{}

func (t testStructAttributes) EncodeText(ci *ConnInfo, buf []byte) (newBuf []byte, err error) {
   ...
}

[jackc]

As @redbaron mentioned, it is possible to use pgbouncer with the extended protocol. You would need to connect with the statement_cache_mode=describe option.

As far as the original suggestion of FallbackValuer, I have mixed feelings. I see how it would make this particular use case easier, but there is a downside.

It would only affect the simple protocol. This could cause a confusing situation where the behavior of the same query could vary depending on whether the simple or extended protocol is in use. This is already unavoidable in some circumstances, but I don't want to do anything to make it worse.

There are a few alternatives that might work.

First, as mentioned above, you can use a type wrapper just when querying. For a standard example see the zeronull package.

Second, you could use your own query function wrappers. For example:

type Execer interface {
    Exec(ctx context.Context, sql string, args ...interface{}) (pgconn.CommandTag, error)
}

func Exec(ctx context.Context, db Execer, sql string, args []interface{}) (pgconn.CommandTag, error) {
    for i := range args {
        switch arg := args[i].(type) {
        case map[string]interface{}:
            j := &pgtype.JSON{}
            err := j.Set(arg)
            if err != nil {
                return nil, err
            }
            args[i] = j
        // define any other type mappings here 
        }
    }

    // call underlying exec
    return db.Exec(ctx, sql, args...)
}

[Oliver-Fish]

Thanks both @jackc and @redbaron I was having issues with PGBouncer that was fixed by moving to the simple protocol but I guess I missed the alternative of changing statement cache instead.

I agree with the following comment

It would only affect the simple protocol. This could cause a confusing situation where the behavior of the same query could vary depending on whether the simple or extended protocol is in use. This is already unavoidable in some circumstances, but I don't want to do anything to make it worse.

I will go ahead and close this ticket since you have offered some viable alternatives so I don't see a need for this change.