Changed query methods to accept slices

[YohDeadfall]

Why to allocate query arguments on the heap only while they can also be placed on the stack? Why not both? (there should be that meme)

Another option is to introduce a little bit of generics and use AsRef<T> to accept anything, but deeply under the hood make non-generic methods using slices. Would it be better?

I'm also thinking about that Option<T> used for the arguments. As for me, it's not different from having an empty slice or whatever from the implementation perspective, but from the usability point of view it's much clearer.

Depends on #1837.

[YohDeadfall]

Converted to a draft to prevent an accidental merge before #1837.

[eeeebbbbrrrr]

Thanks for this. It’s a good change.

Looks like CI didn’t like it.

[eeeebbbbrrrr]

This isn't something to address here, but a soundness problem with Spi is that query args are just Datums. We can't verify that the provided Datum is of a type that's compatible with the query argument position.

let foo = Spi::get_one_with_args::<pg_sys::Oid>(
          "SELECT oid FROM pg_class WHERE relname = $1", 
          &[42.into_datum()]
);

That's not good. I've never been able to think of a way to safely handle this. I don't recall @workingjubilee and I talking about this problem in particular (just Spi soundness in general). Do you have any ideas, @YohDeadfall?

[YohDeadfall]

Thanks for this. It’s a good change.

That's for the evening.

That's not good. I've never been able to think of a way to safely handle this. I don't recall @workingjubilee and I talking about this problem in particular (just Spi soundness in general). Do you have any ideas, @YohDeadfall?

You're nerd snipping me, don't you? But that's actually what I usually enjoy :D

I have an idea in mind which requires storing the oid as a part of the datum. It can also include nullability either in another field or as a sign in the ``oid` field because it cannot be negative as far as I remember.

I will take a look closer when get back from my daily job.

[workingjubilee]

we can't merge this without either releasing 0.13 after or using a generic that hopefully infers well?

[YohDeadfall]

Generics will turn that into a non-breaking change, and as far as I know that trick is used somewhere in Bevy API, but maybe for a different reason.

[YohDeadfall]

Ugh, that Option<T> for arguments gives a lot of headache and is a show stopper for AsRef<T> tricks. It plays nice when there's Some, but None doesn't allow the compiler to deduce the parameter type, and default type parameters are disallowed for quite a while. Therefore, it would be better to nuke that Option<T> wrapper as well and use &[][..] when no arguments to pass.

[workingjubilee]

Yeah, I'm generally not a fan of Option<&[T]> for these reasons.

[workingjubilee]

Were you gonna try the generic/AsRef-using variant of this out so it doesn't require a breaking change?

[YohDeadfall]

I tried it locally and noticed that it's a breaking change anyway due to a compilation error for None. It's ambiguous without T specified, and T cannot have a default type. It could be worked around with implicit casts and a proxy type, but Rust doesn't have them for a good reason.

What I can do then is accumulate breaking changes in some branch and ship them at once. It can be made on your side to avoid squashing commits there and reviewing changes one by one.

Another option is to wait till I create enough pull requests and then merge them at once.

[workingjubilee]

Oh, I see.

[YohDeadfall]

Superseded by #1858.