On Wed, May 22, 2019 at 02:39:54PM -0400, Tom Lane wrote:
> Andres Freund <andres@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
