feat(new sink): new postgres sink

[Ichmed]

Summary

A zero-copy postgres sink that requires no new dependencies (it adds one feature on tokio-postgres).
The sink uses a prepared statement to insert the data in pure SQL instead of serializing the data to JSON and deserializing it in the database.

For now the sink can only handle Logs and Traces.

Tests are still missing but it can be E2E tested it using this setup:

sources:
  stdin:
    type: stdin

transforms:
  foobar:
    type: remap
    inputs:
      - stdin
    source: |-
      .json_field = del(.)
      .array_field = [true, true, true]
      .id = "some_id"
      .ignored_field = 1324

sinks:
  posti:
    type: postgres
    host: localhost
    port: 5432
    table: jsontest
    inputs:
      - foobar

CREATE TABLE IF NOT EXISTS public.jsontest
(
    id character varying(255) COLLATE pg_catalog."default",
    json_field json,
    array_field boolean[]
)

Change Type

• Bug fix
• New feature
• Non-functional (chore, refactoring, docs)
• Performance

Is this a breaking change?

• Yes
• No

How did you test this PR?

Does this PR include user facing changes?

• Yes. Please add a changelog fragment based on our guidelines.
• No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

• Please read our Vector contributor resources.
  • make check-all is a good command to run locally. This check is
    defined here. Some of these
    checks might not be relevant to your PR. For Rust changes, at the very least you should run:
    • cargo fmt --all
    • cargo clippy --workspace --all-targets -- -D warnings
    • cargo nextest run --workspace (alternatively, you can run cargo test --all)
• If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
  run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

[bits-bot]

All committers have signed the CLA.

[pront]

Hi @Ichmed, thank you this PR! There is an existing PR that introduces a postgres sink which is almost there: #21248

[isbm]

@pront We are fully aware of it and analysed it. 😉 And yet we don't think it is the right way to do. Please take a closer look at the code. Features we can add, no worries. But we have a really enormous data and we need it in large amounts into Postgres and TimescaleDB. We specifically need that optimised for cloud usage (mem/CPU matters!).

In a worst case you will have two sinks! 😆 Call it "lightweight PgSink".

[pront]

@pront We are fully aware of it and analysed it. 😉 And yet we don't think it is the right way to do. Please take a closer look at the code. Features we can add, no worries.

Sure will do. It will take some time though so please bear with me.

But we have a really enormous data and we need it in large amounts into Postgres and TimescaleDB. We specifically need that optimised for cloud usage (mem/CPU matters!).

Did you compare both implementations against some benchmarks?

In a worst case you will have two sinks! 😆 Call it "lightweight PgSink".

Having two sinks doing the same thing is probably not what we want. I do like that #21248 has support for all telemetry data, Vector features such as ACKs and good UX. And most importantly, a lot of testing.

Again, I didn't dive into the differences and I need some time to do so. I wonder, since you looked the existing PR, can you work on optimizing that after it lands?

[isbm]

Sure will do. It will take some time though so please bear with me.

Thanks!

Having two sinks doing the same thing is probably not what we want. I do like that #21248 has support for all telemetry data, Vector features such as ACKs and good UX. And most importantly, a lot of testing.

To our defence, our day one Chapter 1 is not half-year Chapter 128 😛. We specifically focused on having it zero-copy, no dependencies generic micro-sink. Adding features is not a problem, ACKs are coming, as it is a necessity.

Again, I didn't dive into the differences and I need some time to do so. I wonder, since you looked the existing PR, can you work on optimizing that after it lands?

We would definitely support and maintain ours — that's for sure, because it will go into production straight away. Alternatively, it can land in "contrib" section: more options to choose from is always better. We are interested in bringing more sinks/transforms in a near future.

[Ichmed]

Hi, @pront with these changes we should have feature parity with the other PR aside from configuration.

Is there a nice way to do benchmarks? I looked at the benches directory but didn't really understand how to apply that to this use case.
AFAIK this implementation should be faster than the other, since we are simply doing less work, should have zero allocations per event, are using a prepared statement and have no deserialization happening on the DB side, so if we are slower in any usecase I would consider that a bug that can be fixed.

[jorgehermo9]

Hi, I would like to drop my opinion on this.

AFAIK this implementation should be faster than the other, since we are simply doing less work

I'm not really sure about this and claiming about performance improvements and optimizations without measuring it, is a mistake.

I see that you are not batching events and every ingested event results in a network trip. I would be surprised to see that this approach results in a higher throughput than batching them.

Moreover, as you are loading the table column's on sink's startup

vector/src/sinks/postgres/mod.rs

Line 90 in e9b0c8c

let columns: Vec<_> = client.query("SELECT column_name from INFORMATION_SCHEMA.COLUMNS WHERE table_name = $1 AND table_schema = $2", &[&table, &schema]).await?.into_iter().map(|x| x.get(0)).collect();

your implementation does not allow to alter tables and insert new columns while runnning (which #21248 does), you must have to restart the sink so new columns are taken into account. Also, deleting columns while running would cause all events to fail until Vector is restarted.

And also, I'm not sure your implementation works for Composite types (maybe does, but I'm currently not sure if it does).

The implementations are not feature-wise equal so I don't think that a performance comparison makes sense in this case though (whichever would be the fastest).

should have zero allocations per event

No allocation does not always imply to be faster. It generally is faster to not allocate, but does not imply to be faster.

are using a prepared statement

so does #21248. https://docs.rs/sqlx/latest/sqlx/fn.query.html
The connection will transparently prepare and cache the statement, which means it only needs to be parsed once in the connection’s lifetime

more options to choose from is always better

This is also a fallacy. From a new user experience, not having a single solution is actually worse, as users would struggle deciding which one to use, for example. Moreover, it is a maintenance overhead for maintainers to have multiple implementations for nearly the same.

From my point of view, we should not be talking about should be faster and actually measuring it, but as I think this is not feature-wise equal to #21248, I don't know if it makes sense to just choose the fastest

[jorgehermo9]

Also, you state that

should have zero allocations per event

but clearly using a BytesMut to encode every event's field

vector/src/sinks/postgres/mod.rs

Line 192 in e9b0c8c

.map(Wrapper);

, you are potentially doing several allocations per event. Those statements about no allocation done should come from validating it with tools like valgrind. Stating that allocations are not done purely based on your written code and not on your dependencies' code (which also must be taken into account) is wrong.

[isbm]

@pront, @jorgehermo9 Hey, thanks for taking a look at it and adding your thoughts! OK, so I've spent kind of a day for this to measure it thoroughly and found that "native approach" is still faster than @jorgehermo9 's, however it is paper-thin faster when preparing the statements and using JSONB and net-win is pretty much useless. My tests was made on PgSQL 17.3. Meanwhile, adding batching to all this turns a bit difficult because the interface is a bit weird.

So I think that it would be indeed a good idea to go with @jorgehermo9 's solution!

@pront since @jorgehermo9 is about to finish that, would be nice to finally merge that. 😉 We will then take this sink and might also look after it, as we "badly" need PostgreSQL sink in Vector.

That said, have a nice day.

[pront]

So I think that it would be indeed a good idea to go with @jorgehermo9 's solution!

@pront since @jorgehermo9 is about to finish that, would be nice to finally merge that. 😉 We will then take this sink and might also look after it, as we "badly" need PostgreSQL sink in Vector.

That said, have a nice day.

Thank you @isbm, this sounds like a good plan! You are very welcome to optimize the postgres sink after it lands. And thanks for using Vector :)

[jorgehermo9]

So I think that it would be indeed a good idea to go with @jorgehermo9 's solution!

Thank you @isbm. I'm sorry that PR got delayed this much, I'm doing it outside of work hours and it's difficult to find enough time. I think all I have left is to write some documentation about the usage, I plan to do it this through this week!

[isbm]

Thank you @isbm, this sounds like a good plan! You are very welcome to optimize the postgres sink after it lands.

@pront after it lands. 😉 We tried also valgrind, but it doesn't do required measures. I spent a day of tuning/finding/narrowing down possible performance issues, but this is not that easy, apparently. For example, earlier versions of Postgres dealt with JSON/B quite poorly and caching the statement wasn't that helpful. But seems v17 is quite improved, which was an impressive discovery on its own. Since we have quite high cardinality of telemetry, let's see how that performs in real life, i.e. memory/CPU consumption, throughput etc etc.

And thanks for using Vector :)

Did you guys thought about "contrib" repo anyways? Because indeed not all modules should go into the main distribution, and some of those I would like not to have — after all it is ISO standard: if you don't use it, remove it. While I've managed to get vector in "just" 60M binary, it is still huge, if we are talking about embedded devices. While binary size doesn't necessarily affects the performance (only loading time), yet we have cases where our storage is strictly limited to a bare minimum, so we count almost every byte...

[isbm]

Thank you for you @isbm. I'm sorry that PR got delayed this much, I'm doing it outside of work hours and it's difficult to find enough time.

@pront actually another very good point ☝️ by @jorgehermo9 why contrib approach would be actually beneficial for the rest of us: as we do all that outside of working hours, we need iterations. It would tremendously help if @jorgehermo9 just wrote a basic HOWTO/Readme and let it all get released ASAP, and then we all could just iterate improving it, rather then bubble-up one huge PR since September last year, isn't it? Downside: could be many plugins/modules "half-baked" or not ideal. But you could just label them as "good enough" or "needs improvement here/there" etc. E.g. I dont have much time to make it sales-perfect at one go, but overtime it will be...