Improving contrib/tablefunc's error reporting

On 3/5/24 17:04, Tom Lane wrote:

After reading the thread at [1], I could not escape the feeling
that contrib/tablefunc's error reporting is very confusing.
Looking into the source code, I soon found that it is also
very inconsistent, with similar error reports being phrased
quite differently. The terminology for column names doesn't
match the SGML docs either. And there are some places that are
so confused about whether they are complaining about the calling
query or the called query that the output is flat-out backwards.
So at that point my nascent OCD wouldn't let me rest without
fixing it. Here's a quick patch series to do that.

For review purposes, I split this into two patches. 0001 simply
adds some more test cases to reach currently-unexercised error
reports. Then 0002 makes my proposed code changes and shows
how the existing error messages change.

I'm not necessarily wedded to the phrasings I used here,
in case anyone has better ideas.

BTW, while I didn't touch it here, it seems fairly bogus that
connectby() checks both type OID and typmod for its output
columns while crosstab() only checks type OID. I think
crosstab() is in the wrong and needs to be checking typmod.
That might be fit material for a separate patch though.

Been a long time since I gave contrib/tablefunc any love I guess ;-)

I will have a look at your patches, and the other issue you mention, but
it might be a day or three before I can give it some quality time.

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Joe Conway <mail@joeconway.com> writes:

I will have a look at your patches, and the other issue you mention, but
it might be a day or three before I can give it some quality time.

No hurry certainly. Thanks for looking.

regards, tom lane

On 3/5/24 17:04, Tom Lane wrote:

After reading the thread at [1], I could not escape the feeling
that contrib/tablefunc's error reporting is very confusing.
Looking into the source code, I soon found that it is also
very inconsistent, with similar error reports being phrased
quite differently. The terminology for column names doesn't
match the SGML docs either. And there are some places that are
so confused about whether they are complaining about the calling
query or the called query that the output is flat-out backwards.
So at that point my nascent OCD wouldn't let me rest without
fixing it. Here's a quick patch series to do that.

For review purposes, I split this into two patches. 0001 simply
adds some more test cases to reach currently-unexercised error
reports. Then 0002 makes my proposed code changes and shows
how the existing error messages change.

I'm not necessarily wedded to the phrasings I used here,
in case anyone has better ideas.

The changes all look good to me and indeed more consistent with the docs.

Do you want me to push these? If so, development tip only, or backpatch?

BTW, while I didn't touch it here, it seems fairly bogus that
connectby() checks both type OID and typmod for its output
columns while crosstab() only checks type OID. I think
crosstab() is in the wrong and needs to be checking typmod.
That might be fit material for a separate patch though.

I can take a look at this. Presumably this would not be for backpatching.

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Joe Conway <mail@joeconway.com> writes:

On 3/5/24 17:04, Tom Lane wrote:

After reading the thread at [1], I could not escape the feeling
that contrib/tablefunc's error reporting is very confusing.

The changes all look good to me and indeed more consistent with the docs.
Do you want me to push these? If so, development tip only, or backpatch?

I can push that. I was just thinking HEAD, we aren't big on changing
error reporting in back branches.

BTW, while I didn't touch it here, it seems fairly bogus that
connectby() checks both type OID and typmod for its output
columns while crosstab() only checks type OID. I think
crosstab() is in the wrong and needs to be checking typmod.
That might be fit material for a separate patch though.

I can take a look at this. Presumably this would not be for backpatching.

I'm not sure whether that could produce results bad enough to be
called a bug or not. But I too would lean towards not back-patching,
in view of the lack of field complaints.

regards, tom lane

On 3/9/24 13:07, Tom Lane wrote:

Joe Conway <mail@joeconway.com> writes:

On 3/5/24 17:04, Tom Lane wrote:

After reading the thread at [1], I could not escape the feeling
that contrib/tablefunc's error reporting is very confusing.

The changes all look good to me and indeed more consistent with the docs.
Do you want me to push these? If so, development tip only, or backpatch?

I can push that. I was just thinking HEAD, we aren't big on changing
error reporting in back branches.

BTW, while I didn't touch it here, it seems fairly bogus that
connectby() checks both type OID and typmod for its output
columns while crosstab() only checks type OID. I think
crosstab() is in the wrong and needs to be checking typmod.
That might be fit material for a separate patch though.

I can take a look at this. Presumably this would not be for backpatching.

I'm not sure whether that could produce results bad enough to be
called a bug or not. But I too would lean towards not back-patching,
in view of the lack of field complaints.

Something like the attached what you had in mind? (applies on top of
your two patches)

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Joe Conway <mail@joeconway.com> writes:

On 3/9/24 13:07, Tom Lane wrote:

BTW, while I didn't touch it here, it seems fairly bogus that
connectby() checks both type OID and typmod for its output
columns while crosstab() only checks type OID. I think
crosstab() is in the wrong and needs to be checking typmod.
That might be fit material for a separate patch though.

Something like the attached what you had in mind? (applies on top of
your two patches)

Yeah, exactly.

As far as the comment change goes:

-   * - attribute [1] of the sql tuple is the category; no need to check it -
-   * attribute [2] of the sql tuple should match attributes [1] to [natts]
+   * attribute [1] of the sql tuple is the category; no need to check it
+   * attribute [2] of the sql tuple should match attributes [1] to [natts - 1]
    * of the return tuple

I suspect that this block looked better when originally committed but
didn't survive contact with pgindent. You should check whether your
version will; if not, some dashes on the /* line will help.

regards, tom lane

On 3/9/24 15:39, Tom Lane wrote:

Joe Conway <mail@joeconway.com> writes:

On 3/9/24 13:07, Tom Lane wrote:

BTW, while I didn't touch it here, it seems fairly bogus that
connectby() checks both type OID and typmod for its output
columns while crosstab() only checks type OID. I think
crosstab() is in the wrong and needs to be checking typmod.
That might be fit material for a separate patch though.

Something like the attached what you had in mind? (applies on top of
your two patches)

Yeah, exactly.

As far as the comment change goes:

-   * - attribute [1] of the sql tuple is the category; no need to check it -
-   * attribute [2] of the sql tuple should match attributes [1] to [natts]
+   * attribute [1] of the sql tuple is the category; no need to check it
+   * attribute [2] of the sql tuple should match attributes [1] to [natts - 1]
* of the return tuple

I suspect that this block looked better when originally committed but
didn't survive contact with pgindent. You should check whether your
version will; if not, some dashes on the /* line will help.

Thanks for the review and heads up. I fiddled with it a bit to make it
pgindent clean. I saw you commit your patches so I just pushed mine.

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com