Thread: Incorrect format in error message

The attached fixes an error message which is incorrectly using an
unsigned format specifier instead of a signed one.

# create table t ();
# create unique index t_idx on t (ctid);
# alter table t replica identity using index t_idx;
ERROR:  internal column 4294967295 in unique index "t_idx"

I was just going to submit a patch to change the %u to a %d, but this
error message should use ereport() instead of elog(), so I fixed that
up too and added the missing regression test.

I picked ERRCODE_INVALID_COLUMN_REFERENCE as I thought it was the most
suitable. I'm not sure if it's correct though.

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

David Rowley <david.rowley@2ndquadrant.com> writes:
> The attached fixes an error message which is incorrectly using an
> unsigned format specifier instead of a signed one.

I think that's the tip of the iceberg :-(.  For starters, the code
allows ObjectIdAttributeNumber without regard for the fact that the
next line will dump core on a negative attno.  Really though, what
astonishes me about this example is that we allow indexes at all on
system columns other than OID.  None of the other ones can possibly
have either a use-case or sensible semantics, can they?  We certainly
would not stop to update indexes after changing xmax, for example.
        regards, tom lane

On 1 April 2016 at 17:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> The attached fixes an error message which is incorrectly using an
>> unsigned format specifier instead of a signed one.
>
> I think that's the tip of the iceberg :-(.  For starters, the code
> allows ObjectIdAttributeNumber without regard for the fact that the
> next line will dump core on a negative attno.  Really though, what
> astonishes me about this example is that we allow indexes at all on
> system columns other than OID.  None of the other ones can possibly
> have either a use-case or sensible semantics, can they?  We certainly
> would not stop to update indexes after changing xmax, for example.

ouch. Yeah that's not going to work very well. I guess nobody's tried
that with a unique index on an OID column yet then.

I've changed the patch around a little to fix the crash. I was a bit
worried as logical decoding obviously has not yet been tested with an
OID unique index as the replica indentity, so I gave it a quick test
to try to give myself a little piece of mind that this won't uncover
something else;

# create table t (a int) with oids;
CREATE TABLE
# create unique index t_oid_idx on t(oid);
CREATE INDEX
# alter table t replica identity using index t_oid_idx;
ALTER TABLE
# insert into t values(123);
INSERT 24593 1
# delete from t;

On the receive side:

BEGIN 606
table public.t: INSERT: oid[oid]:24593 a[integer]:123
COMMIT 606
BEGIN 607
table public.t: DELETE: oid[oid]:24593
COMMIT 607

--
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 1 April 2016 at 17:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> The attached fixes an error message which is incorrectly using an
>> unsigned format specifier instead of a signed one.
>
> Really though, what
> astonishes me about this example is that we allow indexes at all on
> system columns other than OID.  None of the other ones can possibly
> have either a use-case or sensible semantics, can they?  We certainly
> would not stop to update indexes after changing xmax, for example.

As for this part. I really don't see how we could disable this without
breaking pg_restore for database who have such indexes.  My best
thought is to add some sort of warning during CREATE INDEX, like we do
for HASH indexes.

-- David Rowley                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 2016-04-01 20:18:29 +1300, David Rowley wrote:
> On 1 April 2016 at 17:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > David Rowley <david.rowley@2ndquadrant.com> writes:
> >> The attached fixes an error message which is incorrectly using an
> >> unsigned format specifier instead of a signed one.
> >
> > Really though, what
> > astonishes me about this example is that we allow indexes at all on
> > system columns other than OID.  None of the other ones can possibly
> > have either a use-case or sensible semantics, can they?  We certainly
> > would not stop to update indexes after changing xmax, for example.
> 
> As for this part. I really don't see how we could disable this without
> breaking pg_restore for database who have such indexes.  My best
> thought is to add some sort of warning during CREATE INDEX, like we do
> for HASH indexes.

As they're currently already not working correctly as indexes, I don't
see throwing an error during pg_restore as being overly harmful.

Andres