Bogus tags for comments, ACLs, and security labels in pg_dump

There is a convention in pg_dump/pg_restore that the "tag" field in a
COMMENT TOC entry should be exactly what you'd write after COMMENT ON
to specify the comment's target; for example it might be "INDEX foo".
This is depended on in _tocEntryRequired, which wants to know which
comments are for large objects:

if (strcmp(te->desc, "SEQUENCE SET") == 0 ||
strcmp(te->desc, "BLOB") == 0 ||
(strcmp(te->desc, "ACL") == 0 &&
strncmp(te->tag, "LARGE OBJECT ", 13) == 0) ||
--> (strcmp(te->desc, "COMMENT") == 0 &&
--> strncmp(te->tag, "LARGE OBJECT ", 13) == 0) ||
(strcmp(te->desc, "SECURITY LABEL") == 0 &&
strncmp(te->tag, "LARGE OBJECT ", 13) == 0))
res = res & REQ_DATA;
else
res = res & ~REQ_DATA;

While fooling with the pg_dump/pg_dumpall refactoring patch, I noticed
that somebody broke this for comments on databases, back around 8.2:
if you look at the code path for emitting database comments from >= 8.2,
it just uses the raw database name as the tag. The pre-8.2 code path gets
that right, and it's easy to see by inspection that everyplace else that
creates a COMMENT TOC entry also gets it right.

This is problematic because a database named "LARGE OBJECT something"
will fool the above-mentioned code into misclassifying its comment
as data rather than schema.

As you can see from the above stanza, there's a similar expectation
for SECURITY LABEL TOC entries, and again dumpDatabase() is alone in
getting it wrong.

And we also see a similar expectation for ACLs, and that's a bit more of
a problem because there is not, in fact, any convention about including
an object type prefix in ACL tags. Large objects do use tags like "LARGE
OBJECT nnn", but for other kinds of objects the tag is generally just the
bare object name. So any object named "LARGE OBJECT something" will
have its ACL misclassified by this code.

While there's no visible effect in a pg_dump run with default options,
it's easy to demonstrate that pg_dump with -s will indeed omit comments,
ACLs, etc on objects named this way, which in turn means that pg_upgrade
would lose those properties. Conversely, a data-only dump might
unexpectedly include commands to set comments, ACLs, etc on objects
named this way.

There's a potential security issue here, but the security team discussed
it and decided that there's not much chance for an interesting exploit.
An object owner can just change the comment or ACL on his object; there's
no need to try to trick someone else into doing it. The security-label
case is more interesting, but since it only applies to databases the
attack surface seems small, and anyway there seem to be few people using
security labels so far. Hence, I'm just treating this as a regular bug.

What I think we should do is fix pg_dump so that these classes of
TOC entries do indeed have tags that meet the expectation held by
_tocEntryRequired, as per the first attached patch. (That patch also
fixes some lesser errors in the same place: dumpDatabase was passing the
database's OID as catalogId for its comment and seclabel TOC entries,
and the pre-8.2 code path neglected to specify the owner of the comment.
I'm not sure that these mistakes can cause any visible symptoms right
now, but they're still wrong.)

The second attached patch just rearranges some code so that pg_dump
uniformly emits comments, sec labels, and ACLs immediately after emitting
the TOC entry for the object they belong to. In dumpDatabase, somebody
decided to stick a bunch of binary_upgrade TOC items in between. That's
harmless at the moment, AFAIK, since pg_restore isn't tasked with making
very interesting choices during a binary upgrade, but it seems like
trouble waiting to happen; at minimum it violates the API contract
specified for dumpComment. Similarly, dumpForeignDataWrapper was randomly
doing things in an atypical order (perhaps as a result of careless patch
application?) while dumpForeignServer dumped a server's comment only after
dumping user mappings for it. Again I don't know of live bugs there, but
I have little patience for code that seems to have been assembled with the
aid of a dartboard.

I propose to apply the first patch to all supported branches. I have
no reason to think the second patch is more than cosmetics and perhaps
future-proofing, so I only propose it for HEAD.

If there are not objections, I plan to push these pretty soon.

regards, tom lane