On 2014-04-28 13:20:47 -0400, Tom Lane wrote:
> Robert Haas <robertmh...@gmail.com> writes:
> > On Mon, Apr 28, 2014 at 12:14 PM, Tom Lane <t...@sss.pgh.pa.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
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 4493930..0b9731b 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1362,14 +1362,14 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
change->data.tp.oldtuple == NULL)
continue;
else if (reloid == InvalidOid)
- elog(ERROR, "could not lookup relation %s",
+ elog(ERROR, "could not map filenode %s to its oid",
relpathperm(change->data.tp.relnode, MAIN_FORKNUM));
relation = RelationIdGetRelation(reloid);
if (relation == NULL)
- elog(ERROR, "could open relation descriptor %s",
- relpathperm(change->data.tp.relnode, MAIN_FORKNUM));
+ elog(ERROR, "could not open relation with oid %u, filenode %s",
+ reloid, relpathperm(change->data.tp.relnode, MAIN_FORKNUM));
if (RelationIsLogicallyLogged(relation))
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
