Re: pg_dump throwing "column number -1 is out of range 0..36" on HEAD

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Andrew Dunstan <andrew(at)dunslane(dot)net>
Subject: Re: pg_dump throwing "column number -1 is out of range 0..36" on HEAD
Date: 2019-05-23 00:11:33
Message-ID: 20190523001133.GA1426@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Gierth 2019-05-23 00:36:10 Re: Patch to fix write after end of array in hashed agg initialization
Previous Message Ashwin Agrawal 2019-05-23 00:07:45 Re: Zedstore - compressed in-core columnar storage