Should `Sql.executeTransactionAsync` fail if a parameterised has empty parameters

[absolutejam]

As we discussed on Slack, I experienced an Error when (inadvertently) running Sql.executeTransactionAsync with empty parameters.

I was actually doing the following, without realising items was empty:

/// Persist all `TicketRatings` to the DB
let persistTicketRatings connection (items: (DateTime * TicketRatings) list) =
    connection
    |> Sql.connect
    |> Sql.executeTransactionAsync
        [
            """
            INSERT INTO livechat_ticket_ratings
            (id, datetime, good, bad)
            VALUES
            (@id, @datetime, @good, @bad)
            ON CONFLICT DO NOTHING;
            """,
            [ for (dateTime, x) in items ->
                [
                    "@id",       Sql.uuid      (Guid.NewGuid ())
                    "@datetime", Sql.timestamp dateTime
                    "@good",     Sql.int       x.Good
                    "@bad",      Sql.int       x.Bad
                ]
            ]

which returns the following error message:

Error
  System.AggregateException: One or more errors occurred. (42P01: relation "livechat_ticket_rankings" does not exist)
 ---> Npgsql.PostgresException (0x80004005): 42P01: relation "livechat_ticket_rankings" does not exist
   at Npgsql.NpgsqlConnector.<ReadMessage>g__ReadMessageLong|194_0(NpgsqlConnector connector, Boolean async, DataRowLoadingMode dataRowLoadingMode, Boolean readingNotifications, Boolean isReadingPrependedMessage)
   at Npgsql.NpgsqlDataReader.NextResult(Boolean async, Boolean isConsuming, CancellationToken cancellationToken)
   at Npgsql.NpgsqlCommand.ExecuteReader(CommandBehavior behavior, Boolean async, CancellationToken cancellationToken)
   at Npgsql.NpgsqlCommand.ExecuteReader(CommandBehavior behavior, Boolean async, CancellationToken cancellationToken)
   at Npgsql.NpgsqlCommand.ExecuteNonQuery(Boolean async, CancellationToken cancellationToken)
  Exception data:
    Severity: ERROR
    SqlState: 42P01
    MessageText: relation "livechat_ticket_rankings" does not exist
    Position: 26
    File: parse_relation.c
    Line: 1379
    Routine: parserOpenTable
   --- End of inner exception stack trace ---

As suggested, I've created a simple failing test case which I can expand upon if needed.

I don't mind making some changes to fix this, but I guess it means deciding what the 'fix' is...

• Should Sql.executeTransactionAsync require each query to have parameters?
• Should Sql.executeTransactionAsync detect parameterisation in the query - which could be error-prone & unreliable - then fail if the parameters are empty?
• Is the current behaviour fine and it's up to the consumer?
  As you can see, the error I got back from Postgres was pretty obscure when trying to actually correlate this with the F# code, and I had 2 practically identical queries but the latter failed because of the list being passed in to the function.
• Should this be split into its own functionality?
  The issue here is that a single Sql.executeTransactionAsync could have some queries with parameters and some without.
  The obvious way to approach this is to use a DU of parameterised/unparameterised queries, but this which muddies the slick API you've built.
  Or, perhaps an approach like #54 (comment) where the transaction is contained within a function and you perform other queries inside the scope of this.

And as an side, if it's decided that queries should not have empty parameters, that will break the tests as they stand:

Npgsql.FSharp/tests/Tests.fs

Lines 74 to 87 in a9d0660

	database.ConnectionString
	|> Sql.connect
	|> Sql.executeTransaction [
	createFSharpTable, [ ]
	createJsonbTable, [ ]
	createTimestampzTable, [ ]
	createTimespanTable, [ ]
	createStringArrayTable, [ ]
	createExtensionHStore, [ ]
	createIntArrayTable, [ ]
	createExtensionUuid, [ ]
	createUuidArrayTable, []
	createPointTable, []
	]

[Zaid-Ajaj]

And as an side, if it's decided that queries should not have empty parameters, that will break the tests as they stand:

No these will work just fine like they are now because these are not parameterized. Right now the issue occurs when executing a parameterised query in a transaction without parameters.

[absolutejam]

And as an side, if it's decided that queries should not have empty parameters, that will break the tests as they stand:

No these will work just fine like they are now because these are not parameterized. Right now the issue occurs when executing a parameterised query in a transaction without parameters.

Sorry, I meant if its decided that empty parameters are not allowed at all. But it sounds like you're erring on the side of detecting if the query contains parameterised values, then evaluating the queries parameters.

If this is the case, I can look at adding it as an MR. Do we simply regex parse for VALUES (, (including whitespace) or go as far as and parsing each @value and ensure it is in the parameters list?

[ericsampson]

If the decision is to do scanning like that, could it be done when an exception occurs and then used to return a friendly message? That way every query doesn't pay the cost of a scan, when 99.8% of the time it's not required.

[Zaid-Ajaj]

@absolutejam No need to do any regex on our end, Npgsql has a nice function called DeriveParameters from a command which we can use 😄 I have fixed the issue and a new version of Npgsql.FSharp v3.10 should be available soon.

@ericsampson The scan only happens only once and only after we check that the parameter sets are empty so I believe in the grand scheme of things, the perf impact is insignificant

[absolutejam]

Oh wow, that's awesome! No need to overthink this one then, thanks!