Re: includedir_internal headers are not self-contained

On 2014-04-28 13:20:47 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Mon, Apr 28, 2014 at 12:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> As far as pg_xlogdump goes, I agree that symbolic fork names are probably
> >> nice, but I think the case for printing db/ts/rel OIDs as a pathname is a
> >> whole lot weaker --- to my taste, that's actually an anti-feature.
>
> > I might be missing something, but, why?
>
> It's more verbose, it's not actually any more information, and in many
> cases it's actively misleading, because what's printed is NOT the real
> file name --- it omits segment numbers for instance. As a particularly
> egregious example, in xact_desc_commit() we print a pathname including
> MAIN_FORKNUM, which is a flat out lie to the reader, because what will
> actually get deleted is all forks.
>
> So yeah, it's an anti-feature.
>
> BTW, for the same reasons I'm less than impressed with the uses of
> relpath in error messages in, eg, reorderbuffer.c:
>
> relation = RelationIdGetRelation(reloid);
>
> if (relation == NULL)
> elog(ERROR, "could open relation descriptor %s",
> relpathperm(change->data.tp.relnode, MAIN_FORKNUM));
>
> Printing anything other than the relation OID here is irrelevant,
> misleading, and inconsistent with our practice everywhere else.

I don't think it's really comparable to the other scenarios. We should
print the oid, just as relation_open() does, but the filenode is also
rather helpful here. How about the attached?

If we had a version of relpath() that didn't require the fsm, I'd use
it. If you prefer, I'd be happy enough to replace it with
spcNode/dbNode/relNode. That's more than sufficient here.

> Let's not even mention the missing "not" in the message text.

That's clearly wrong.

Greetings,

Andres Freund

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