Remove FLUSH from CF integration

[tmthecoder]

The intermediate use of FLUSH in the current implementation for Cloudflare workers causes incompatibilities with Hyperdrive's caching service.

In order to support caching, this PR aims to remove the intermediate use of FLUSH when executing extended Postgres queries favoring sending the query all-together. This allows Hyperdrive's caching service to cache the full queries sent and serve results from cache for extended queries, resulting in less round-trips overall.

I believe all existing tests should pass (I ported the connection.js changes onto the deno implementation and ran the tests there). I also ran local tests to ensure compatibility.

[porsager]

Interesting. During my initial testing there was a noticeable perf difference directly against PostgreSQL.

Even so, the cloudflare code you edited is generated from src/ (npm run build:cf), so this would need to be a part of the build step if it's the solution needed, you're welcome to come up with an idea to maintain it the best way.

[tmthecoder]

Ah alright, thanks for letting me know! Would you mind letting me know what the perf testing looked like? I'd love to reproduce it on my end to see the results if possible.

I'll look into integrating this into the main src directory as well then

[porsager]

Yeah, check out https://github.com/porsager/postgres-benchmarks

[tmthecoder]

@porsager Should be updated for all tests and in the main source directory. The changes made essentially check if existing known type mappings exist and cut the round trip used by the FLUSH query. If the types aren't known by us, the FLUSH is sent and that round trip occurs. I'm unable to get the benchmarks running locally currently, but will try to soon to see how it looks in terms of performance, I'm curious to know what it looked like when you ran them

[tmthecoder]

@porsager Benchmarks look like the fluctuate between this implementation and the existing implementation, so I don't see a visible constant difference on my end.

EDIT: Removed the log I missed, seems like benchmarks stay close to the same locally. Anything else needed to merge this in?

[porsager]

Interesting.

Can you check if your PR makes sense on: https://github.com/porsager/postgres/pull/392 ?

[tmthecoder]

@porsager If I understand #392 correctly, it allows explicit definitions of types or inference from postgres, so in the case where all types in a query are explicitly defined, this would provide a fast path of sorts and not have to wait for the ParameterDescription response. Please let me know if I misunderstand what the PR does and the discussion around it, but from that understanding it should still provide a fast-path and cache-compatibility with Hyperdrive in the case where all types are mapped/defined

[porsager]

No, it's actually the opposite 🙂 It removes any js type inference to only rely on what is provided by PostgreSQL in ParameterDescription.

I guess the PR description needs a touchup 😬

[tmthecoder]

Ah makes sense. What was the conversation about the user explicitly specifying a type? would that be mapped prior to receiving the ParameterDescription message?

[porsager]

No, that was about explicitly specifying the type through sql, so eg sql`select ${ 1 }` would not have any kind of type inference (it would just turn to string), so sql`select ${ 1 }::int` would be the desired way to achieve that.

[tmthecoder]

In that case then, where no inference is needed, would there still be a need to receive information from the ParameterDescription response? If not, then this should still apply so we don't wait on the response from server when not needed

[porsager]

Hmm.. maybe I'm misunderstanding something. The way I see it we would always need to wait for ParameterDescription because it is Postgres that tells us the type so we can properly serialize the values before passing them to Postgres. That's why I was thinking it conflicted with this PR because there would never be any js inference of types.

[tmthecoder]

@porsager Ah my bad, I misunderstood then. I'd still be in favor of merging this PR for now as it seems like the JS inference will take a while to deprecate completely (a new major version release it seems), and if any other inference is thought out in the future this would still apply if or when that is implemented if that makes sense

[elithrar]

@porsager is this something you'd still want to merge + tag a release on? We'd love to get Postgres.js working w/ Hyperdrive + Workers again!

[porsager]

Definitely! I've just been too busy to look at it, but today and tomorrow I'll be diving into these things again!

[john-griffin]

Would also like to use Hyperdrive caching, any idea where the commit that closed this issue ended up? When clicking through it says "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository."

[porsager]

Hi John. The PR didn't solve the issue, it's a bit deeper than that, and requires ignoring the describe part of the protocol which will change the behavior of Postgres.js. I'm talking with CloudFlare to see if we can find a good solution.

[john-griffin]

@porsager thanks for clarifying 🙏