On Wed, May 22, 2019 at 02:39:54PM -0400, Tom Lane wrote: > Andres Freund <and...@anarazel.de> writes: >> Well, if we explicitly have to check for -1, it's not really an error of >> omission for everything. Yes, we could forget returning the amname in a >> newer version of the query, but given that we usually just forward copy >> the query that's not that likely. > > No, the concerns I have are about (1) failure to insert the extra return > column into all branches where it's needed; (2) some unexpected run-time > problem causing the PGresult to not have the expected column.
Using a -1 check is not something I find much helpful, because this masks the real problem that some queries may not have the output they expect. > I'm not sure if it'd work quite that cleanly when we need changes in the > FROM part, but certainly for newly-added result fields this would be > hugely better than repeating the whole query. And yes, I'd still insist > that explicitly providing the alternative NULL value is not optional. This makes the addition of JOIN clauses and WHERE quals harder to follow and read, and it makes back-patching harder (with testing to older versions it is already complicated enough) so I don't think that this is a good idea. One extra idea I have would be to add a compile-time flag which we could use to enforce a hard failure with an assertion or such in those code paths, because we never expect it in the in-core clients. And that would cause any failure to be immediately visible, at the condition of using the flag of course. > int > PQgetisnull(const PGresult *res, int tup_num, int field_num) > { > if (!check_tuple_field_number(res, tup_num, field_num)) > return 1; /* pretend it is null */ > > which just happens to be the right thing --- in this case --- for > the back branches. Yes. I don't think that this is completely wrong. So, are there any objections if I just apply the patch at the top of this thread and fix the issue? -- Michael
signature.asc
Description: PGP signature