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
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 0bdd398..a8a41d3 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -2529,6 +2529,7 @@ dumpDatabase(Archive *fout) PQExpBuffer dbQry = createPQExpBuffer(); PQExpBuffer delQry = createPQExpBuffer(); PQExpBuffer creaQry = createPQExpBuffer(); + PQExpBuffer labelq = createPQExpBuffer(); PGconn *conn = GetConnection(fout); PGresult *res; int i_tableoid, @@ -2787,16 +2788,20 @@ dumpDatabase(Archive *fout) destroyPQExpBuffer(loOutQry); } + /* Compute correct tag for comments etc */ + appendPQExpBuffer(labelq, "DATABASE %s", fmtId(datname)); + /* Dump DB comment if any */ if (fout->remoteVersion >= 80200) { /* - * 8.2 keeps comments on shared objects in a shared table, so we - * cannot use the dumpComment used for other database objects. + * 8.2 and up keep comments on shared objects in a shared table, so we + * cannot use the dumpComment() code used for other database objects. + * Be careful that the ArchiveEntry parameters match that function. */ char *comment = PQgetvalue(res, 0, PQfnumber(res, "description")); - if (comment && strlen(comment)) + if (comment && *comment) { resetPQExpBuffer(dbQry); @@ -2808,17 +2813,17 @@ dumpDatabase(Archive *fout) appendStringLiteralAH(dbQry, comment, fout); appendPQExpBufferStr(dbQry, ";\n"); - ArchiveEntry(fout, dbCatId, createDumpId(), datname, NULL, NULL, - dba, false, "COMMENT", SECTION_NONE, + ArchiveEntry(fout, nilCatalogId, createDumpId(), + labelq->data, NULL, NULL, dba, + false, "COMMENT", SECTION_NONE, dbQry->data, "", NULL, - &dbDumpId, 1, NULL, NULL); + &(dbDumpId), 1, + NULL, NULL); } } else { - resetPQExpBuffer(dbQry); - appendPQExpBuffer(dbQry, "DATABASE %s", fmtId(datname)); - dumpComment(fout, dbQry->data, NULL, "", + dumpComment(fout, labelq->data, NULL, dba, dbCatId, 0, dbDumpId); } @@ -2834,11 +2839,13 @@ dumpDatabase(Archive *fout) shres = ExecuteSqlQuery(fout, seclabelQry->data, PGRES_TUPLES_OK); resetPQExpBuffer(seclabelQry); emitShSecLabels(conn, shres, seclabelQry, "DATABASE", datname); - if (strlen(seclabelQry->data)) - ArchiveEntry(fout, dbCatId, createDumpId(), datname, NULL, NULL, - dba, false, "SECURITY LABEL", SECTION_NONE, + if (seclabelQry->len > 0) + ArchiveEntry(fout, nilCatalogId, createDumpId(), + labelq->data, NULL, NULL, dba, + false, "SECURITY LABEL", SECTION_NONE, seclabelQry->data, "", NULL, - &dbDumpId, 1, NULL, NULL); + &(dbDumpId), 1, + NULL, NULL); destroyPQExpBuffer(seclabelQry); PQclear(shres); } @@ -2848,6 +2855,7 @@ dumpDatabase(Archive *fout) destroyPQExpBuffer(dbQry); destroyPQExpBuffer(delQry); destroyPQExpBuffer(creaQry); + destroyPQExpBuffer(labelq); } /* @@ -9707,7 +9715,7 @@ dumpNamespace(Archive *fout, NamespaceInfo *nspinfo) if (nspinfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, nspinfo->dobj.catId, nspinfo->dobj.dumpId, "SCHEMA", - qnspname, NULL, nspinfo->dobj.name, NULL, + qnspname, NULL, labelq->data, NULL, nspinfo->rolname, nspinfo->nspacl, nspinfo->rnspacl, nspinfo->initnspacl, nspinfo->initrnspacl); @@ -10003,7 +10011,7 @@ dumpEnumType(Archive *fout, TypeInfo *tyinfo) if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE", - qtypname, NULL, tyinfo->dobj.name, + qtypname, NULL, labelq->data, tyinfo->dobj.namespace->dobj.name, tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl, tyinfo->inittypacl, tyinfo->initrtypacl); @@ -10143,7 +10151,7 @@ dumpRangeType(Archive *fout, TypeInfo *tyinfo) if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE", - qtypname, NULL, tyinfo->dobj.name, + qtypname, NULL, labelq->data, tyinfo->dobj.namespace->dobj.name, tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl, tyinfo->inittypacl, tyinfo->initrtypacl); @@ -10220,7 +10228,7 @@ dumpUndefinedType(Archive *fout, TypeInfo *tyinfo) if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE", - qtypname, NULL, tyinfo->dobj.name, + qtypname, NULL, labelq->data, tyinfo->dobj.namespace->dobj.name, tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl, tyinfo->inittypacl, tyinfo->initrtypacl); @@ -10509,7 +10517,7 @@ dumpBaseType(Archive *fout, TypeInfo *tyinfo) if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE", - qtypname, NULL, tyinfo->dobj.name, + qtypname, NULL, labelq->data, tyinfo->dobj.namespace->dobj.name, tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl, tyinfo->inittypacl, tyinfo->initrtypacl); @@ -10678,7 +10686,7 @@ dumpDomain(Archive *fout, TypeInfo *tyinfo) if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE", - qtypname, NULL, tyinfo->dobj.name, + qtypname, NULL, labelq->data, tyinfo->dobj.namespace->dobj.name, tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl, tyinfo->inittypacl, tyinfo->initrtypacl); @@ -10914,7 +10922,7 @@ dumpCompositeType(Archive *fout, TypeInfo *tyinfo) if (tyinfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, tyinfo->dobj.catId, tyinfo->dobj.dumpId, "TYPE", - qtypname, NULL, tyinfo->dobj.name, + qtypname, NULL, labelq->data, tyinfo->dobj.namespace->dobj.name, tyinfo->rolname, tyinfo->typacl, tyinfo->rtypacl, tyinfo->inittypacl, tyinfo->initrtypacl); @@ -11233,7 +11241,7 @@ dumpProcLang(Archive *fout, ProcLangInfo *plang) if (plang->lanpltrusted && plang->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, plang->dobj.catId, plang->dobj.dumpId, "LANGUAGE", - qlanname, NULL, plang->dobj.name, + qlanname, NULL, labelq->data, lanschema, plang->lanowner, plang->lanacl, plang->rlanacl, plang->initlanacl, plang->initrlanacl); @@ -11867,7 +11875,7 @@ dumpFunc(Archive *fout, FuncInfo *finfo) if (finfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, finfo->dobj.catId, finfo->dobj.dumpId, keyword, - funcsig, NULL, funcsig_tag, + funcsig, NULL, labelq->data, finfo->dobj.namespace->dobj.name, finfo->rolname, finfo->proacl, finfo->rproacl, finfo->initproacl, finfo->initrproacl); @@ -13939,15 +13947,13 @@ dumpAgg(Archive *fout, AggInfo *agginfo) * syntax for zero-argument aggregates and ordered-set aggregates. */ free(aggsig); - free(aggsig_tag); aggsig = format_function_signature(fout, &agginfo->aggfn, true); - aggsig_tag = format_function_signature(fout, &agginfo->aggfn, false); if (agginfo->aggfn.dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, agginfo->aggfn.dobj.catId, agginfo->aggfn.dobj.dumpId, "FUNCTION", - aggsig, NULL, aggsig_tag, + aggsig, NULL, labelq->data, agginfo->aggfn.dobj.namespace->dobj.name, agginfo->aggfn.rolname, agginfo->aggfn.proacl, agginfo->aggfn.rproacl, @@ -14393,7 +14399,7 @@ dumpForeignDataWrapper(Archive *fout, FdwInfo *fdwinfo) if (fdwinfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, fdwinfo->dobj.catId, fdwinfo->dobj.dumpId, "FOREIGN DATA WRAPPER", - qfdwname, NULL, fdwinfo->dobj.name, + qfdwname, NULL, labelq->data, NULL, fdwinfo->rolname, fdwinfo->fdwacl, fdwinfo->rfdwacl, fdwinfo->initfdwacl, fdwinfo->initrfdwacl); @@ -14490,7 +14496,7 @@ dumpForeignServer(Archive *fout, ForeignServerInfo *srvinfo) if (srvinfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, srvinfo->dobj.catId, srvinfo->dobj.dumpId, "FOREIGN SERVER", - qsrvname, NULL, srvinfo->dobj.name, + qsrvname, NULL, labelq->data, NULL, srvinfo->rolname, srvinfo->srvacl, srvinfo->rsrvacl, srvinfo->initsrvacl, srvinfo->initrsrvacl); @@ -14701,7 +14707,8 @@ dumpDefaultACL(Archive *fout, DefaultACLInfo *daclinfo) * FOREIGN DATA WRAPPER, SERVER, or LARGE OBJECT. * 'name' is the formatted name of the object. Must be quoted etc. already. * 'subname' is the formatted name of the sub-object, if any. Must be quoted. - * 'tag' is the tag for the archive entry (typ. unquoted name of object). + * 'tag' is the tag for the archive entry (should be the same tag as would be + * used for comments etc; for example "TABLE foo"). * 'nspname' is the namespace the object is in (NULL if none). * 'owner' is the owner, NULL if there is no owner (for languages). * 'acls' contains the ACL string of the object from the appropriate system @@ -15105,13 +15112,18 @@ dumpTable(Archive *fout, TableInfo *tbinfo) /* Handle the ACL here */ namecopy = pg_strdup(fmtId(tbinfo->dobj.name)); if (tbinfo->dobj.dump & DUMP_COMPONENT_ACL) + { + const char *objtype = + (tbinfo->relkind == RELKIND_SEQUENCE) ? "SEQUENCE" : "TABLE"; + char *acltag = psprintf("%s %s", objtype, namecopy); + dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId, - (tbinfo->relkind == RELKIND_SEQUENCE) ? "SEQUENCE" : - "TABLE", - namecopy, NULL, tbinfo->dobj.name, + objtype, namecopy, NULL, acltag, tbinfo->dobj.namespace->dobj.name, tbinfo->rolname, tbinfo->relacl, tbinfo->rrelacl, tbinfo->initrelacl, tbinfo->initrrelacl); + free(acltag); + } /* * Handle column ACLs, if any. Note: we pull these with a separate query @@ -15195,7 +15207,7 @@ dumpTable(Archive *fout, TableInfo *tbinfo) char *acltag; attnamecopy = pg_strdup(fmtId(attname)); - acltag = psprintf("%s.%s", tbinfo->dobj.name, attname); + acltag = psprintf("COLUMN %s.%s", namecopy, attnamecopy); /* Column's GRANT type is always TABLE */ dumpACL(fout, tbinfo->dobj.catId, tbinfo->dobj.dumpId, "TABLE", namecopy, attnamecopy, acltag,
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index a8a41d3..db65552 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -2696,6 +2696,68 @@ dumpDatabase(Archive *fout) NULL, /* Dumper */ NULL); /* Dumper Arg */ + /* Compute correct tag for comments etc */ + appendPQExpBuffer(labelq, "DATABASE %s", fmtId(datname)); + + /* Dump DB comment if any */ + if (fout->remoteVersion >= 80200) + { + /* + * 8.2 and up keep comments on shared objects in a shared table, so we + * cannot use the dumpComment() code used for other database objects. + * Be careful that the ArchiveEntry parameters match that function. + */ + char *comment = PQgetvalue(res, 0, PQfnumber(res, "description")); + + if (comment && *comment) + { + resetPQExpBuffer(dbQry); + + /* + * Generates warning when loaded into a differently-named + * database. + */ + appendPQExpBuffer(dbQry, "COMMENT ON DATABASE %s IS ", fmtId(datname)); + appendStringLiteralAH(dbQry, comment, fout); + appendPQExpBufferStr(dbQry, ";\n"); + + ArchiveEntry(fout, nilCatalogId, createDumpId(), + labelq->data, NULL, NULL, dba, + false, "COMMENT", SECTION_NONE, + dbQry->data, "", NULL, + &(dbDumpId), 1, + NULL, NULL); + } + } + else + { + dumpComment(fout, labelq->data, NULL, dba, + dbCatId, 0, dbDumpId); + } + + /* Dump shared security label. */ + if (!dopt->no_security_labels && fout->remoteVersion >= 90200) + { + PGresult *shres; + PQExpBuffer seclabelQry; + + seclabelQry = createPQExpBuffer(); + + buildShSecLabelQuery(conn, "pg_database", dbCatId.oid, seclabelQry); + shres = ExecuteSqlQuery(fout, seclabelQry->data, PGRES_TUPLES_OK); + resetPQExpBuffer(seclabelQry); + emitShSecLabels(conn, shres, seclabelQry, "DATABASE", datname); + if (seclabelQry->len > 0) + ArchiveEntry(fout, nilCatalogId, createDumpId(), + labelq->data, NULL, NULL, dba, + false, "SECURITY LABEL", SECTION_NONE, + seclabelQry->data, "", NULL, + &(dbDumpId), 1, + NULL, NULL); + destroyPQExpBuffer(seclabelQry); + PQclear(shres); + } + /* * pg_largeobject and pg_largeobject_metadata come from the old system * intact, so set their relfrozenxids and relminmxids. @@ -2788,68 +2850,6 @@ dumpDatabase(Archive *fout) destroyPQExpBuffer(loOutQry); } - /* Compute correct tag for comments etc */ - appendPQExpBuffer(labelq, "DATABASE %s", fmtId(datname)); - - /* Dump DB comment if any */ - if (fout->remoteVersion >= 80200) - { - /* - * 8.2 and up keep comments on shared objects in a shared table, so we - * cannot use the dumpComment() code used for other database objects. - * Be careful that the ArchiveEntry parameters match that function. - */ - char *comment = PQgetvalue(res, 0, PQfnumber(res, "description")); - - if (comment && *comment) - { - resetPQExpBuffer(dbQry); - - /* - * Generates warning when loaded into a differently-named - * database. - */ - appendPQExpBuffer(dbQry, "COMMENT ON DATABASE %s IS ", fmtId(datname)); - appendStringLiteralAH(dbQry, comment, fout); - appendPQExpBufferStr(dbQry, ";\n"); - - ArchiveEntry(fout, nilCatalogId, createDumpId(), - labelq->data, NULL, NULL, dba, - false, "COMMENT", SECTION_NONE, - dbQry->data, "", NULL, - &(dbDumpId), 1, - NULL, NULL); - } - } - else - { - dumpComment(fout, labelq->data, NULL, dba, - dbCatId, 0, dbDumpId); - } - - /* Dump shared security label. */ - if (!dopt->no_security_labels && fout->remoteVersion >= 90200) - { - PGresult *shres; - PQExpBuffer seclabelQry; - - seclabelQry = createPQExpBuffer(); - - buildShSecLabelQuery(conn, "pg_database", dbCatId.oid, seclabelQry); - shres = ExecuteSqlQuery(fout, seclabelQry->data, PGRES_TUPLES_OK); - resetPQExpBuffer(seclabelQry); - emitShSecLabels(conn, shres, seclabelQry, "DATABASE", datname); - if (seclabelQry->len > 0) - ArchiveEntry(fout, nilCatalogId, createDumpId(), - labelq->data, NULL, NULL, dba, - false, "SECURITY LABEL", SECTION_NONE, - seclabelQry->data, "", NULL, - &(dbDumpId), 1, - NULL, NULL); - destroyPQExpBuffer(seclabelQry); - PQclear(shres); - } - PQclear(res); destroyPQExpBuffer(dbQry); @@ -14395,6 +14395,12 @@ dumpForeignDataWrapper(Archive *fout, FdwInfo *fdwinfo) NULL, 0, NULL, NULL); + /* Dump Foreign Data Wrapper Comments */ + if (fdwinfo->dobj.dump & DUMP_COMPONENT_COMMENT) + dumpComment(fout, labelq->data, + NULL, fdwinfo->rolname, + fdwinfo->dobj.catId, 0, fdwinfo->dobj.dumpId); + /* Handle the ACL */ if (fdwinfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, fdwinfo->dobj.catId, fdwinfo->dobj.dumpId, @@ -14404,12 +14410,6 @@ dumpForeignDataWrapper(Archive *fout, FdwInfo *fdwinfo) fdwinfo->fdwacl, fdwinfo->rfdwacl, fdwinfo->initfdwacl, fdwinfo->initrfdwacl); - /* Dump Foreign Data Wrapper Comments */ - if (fdwinfo->dobj.dump & DUMP_COMPONENT_COMMENT) - dumpComment(fout, labelq->data, - NULL, fdwinfo->rolname, - fdwinfo->dobj.catId, 0, fdwinfo->dobj.dumpId); - free(qfdwname); destroyPQExpBuffer(q); @@ -14492,6 +14492,12 @@ dumpForeignServer(Archive *fout, ForeignServerInfo *srvinfo) NULL, 0, NULL, NULL); + /* Dump Foreign Server Comments */ + if (srvinfo->dobj.dump & DUMP_COMPONENT_COMMENT) + dumpComment(fout, labelq->data, + NULL, srvinfo->rolname, + srvinfo->dobj.catId, 0, srvinfo->dobj.dumpId); + /* Handle the ACL */ if (srvinfo->dobj.dump & DUMP_COMPONENT_ACL) dumpACL(fout, srvinfo->dobj.catId, srvinfo->dobj.dumpId, @@ -14508,12 +14514,6 @@ dumpForeignServer(Archive *fout, ForeignServerInfo *srvinfo) srvinfo->rolname, srvinfo->dobj.catId, srvinfo->dobj.dumpId); - /* Dump Foreign Server Comments */ - if (srvinfo->dobj.dump & DUMP_COMPONENT_COMMENT) - dumpComment(fout, labelq->data, - NULL, srvinfo->rolname, - srvinfo->dobj.catId, 0, srvinfo->dobj.dumpId); - free(qsrvname); destroyPQExpBuffer(q);