pg_dump PublicationRelInfo objects need to be marked with owner - Mailing list pgsql-hackers

Although pg_publication_rel entries don't have an owner per se,
it's still important for the ArchiveEntry created for one to be
marked with an appropriate owner, because that is what determines
which role will be used to run the ALTER PUBLICATION command when
doing a restore with --use-set-session-authorization.  As the
code stands, the session's original role will be used to run
ALTER PUBLICATION.  Admittedly, to have enough privileges to
create the publication, the original role probably has to be
superuser, so that generally this'd be moot.  Still, in principle
it's wrong, and maybe there are edge cases where you'd get a
failure that you shouldn't.

Accordingly, I propose the attached patch to ensure that the ALTER
is done as the owner of the publication.  While I was at it I rewrote
getPublicationTables to do just one query instead of one per table ---
the existing coding seems like it could be quite inefficient for lots
of tables.

Upon looking at the code, I notice that the ALTER PUBLICATION ref
page is lying about the permissions required.  Yes, you do have to
own the publication, but you *also* have to own the table(s) you
are adding.  So this raises the question of exactly which user
we should try to run the command as.  I judged that the publication
owner is more likely to be the right thing.

Thoughts?

            regards, tom lane

diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 255f891432..b0f02bc1f6 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -52,6 +52,7 @@ static DumpableObject **oprinfoindex;
 static DumpableObject **collinfoindex;
 static DumpableObject **nspinfoindex;
 static DumpableObject **extinfoindex;
+static DumpableObject **pubinfoindex;
 static int    numTables;
 static int    numTypes;
 static int    numFuncs;
@@ -59,6 +60,7 @@ static int    numOperators;
 static int    numCollations;
 static int    numNamespaces;
 static int    numExtensions;
+static int    numPublications;

 /* This is an array of object identities, not actual DumpableObjects */
 static ExtensionMemberId *extmembers;
@@ -93,6 +95,7 @@ getSchemaData(Archive *fout, int *numTablesPtr)
     CollInfo   *collinfo;
     NamespaceInfo *nspinfo;
     ExtensionInfo *extinfo;
+    PublicationInfo *pubinfo;
     InhInfo    *inhinfo;
     int            numAggregates;
     int            numInherits;
@@ -247,7 +250,9 @@ getSchemaData(Archive *fout, int *numTablesPtr)
     getPolicies(fout, tblinfo, numTables);

     pg_log_info("reading publications");
-    getPublications(fout);
+    pubinfo = getPublications(fout, &numPublications);
+    pubinfoindex = buildIndexArray(pubinfo, numPublications,
+                                   sizeof(PublicationInfo));

     pg_log_info("reading publication membership");
     getPublicationTables(fout, tblinfo, numTables);
@@ -937,6 +942,17 @@ findExtensionByOid(Oid oid)
     return (ExtensionInfo *) findObjectByOid(oid, extinfoindex, numExtensions);
 }

+/*
+ * findPublicationByOid
+ *      finds the entry (in pubinfo) of the publication with the given oid
+ *      returns NULL if not found
+ */
+PublicationInfo *
+findPublicationByOid(Oid oid)
+{
+    return (PublicationInfo *) findObjectByOid(oid, pubinfoindex, numPublications);
+}
+
 /*
  * findIndexByOid
  *        find the entry of the index with the given oid
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index f2c88fa6b5..52973d9e9c 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -3865,8 +3865,8 @@ dumpPolicy(Archive *fout, PolicyInfo *polinfo)
  * getPublications
  *      get information about publications
  */
-void
-getPublications(Archive *fout)
+PublicationInfo *
+getPublications(Archive *fout, int *numPublications)
 {
     DumpOptions *dopt = fout->dopt;
     PQExpBuffer query;
@@ -3886,7 +3886,10 @@ getPublications(Archive *fout)
                 ntups;

     if (dopt->no_publications || fout->remoteVersion < 100000)
-        return;
+    {
+        *numPublications = 0;
+        return NULL;
+    }

     query = createPQExpBuffer();

@@ -3964,6 +3967,9 @@ getPublications(Archive *fout)
     PQclear(res);

     destroyPQExpBuffer(query);
+
+    *numPublications = ntups;
+    return pubinfo;
 }

 /*
@@ -4072,7 +4078,8 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables)
     DumpOptions *dopt = fout->dopt;
     int            i_tableoid;
     int            i_oid;
-    int            i_pubname;
+    int            i_prpubid;
+    int            i_prrelid;
     int            i,
                 j,
                 ntups;
@@ -4082,15 +4089,39 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables)

     query = createPQExpBuffer();

-    for (i = 0; i < numTables; i++)
+    /* Collect all publication membership info. */
+    appendPQExpBufferStr(query,
+                         "SELECT tableoid, oid, prpubid, prrelid "
+                         "FROM pg_catalog.pg_publication_rel");
+    res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+
+    ntups = PQntuples(res);
+
+    i_tableoid = PQfnumber(res, "tableoid");
+    i_oid = PQfnumber(res, "oid");
+    i_prpubid = PQfnumber(res, "prpubid");
+    i_prrelid = PQfnumber(res, "prrelid");
+
+    /* this allocation may be more than we need */
+    pubrinfo = pg_malloc(ntups * sizeof(PublicationRelInfo));
+    j = 0;
+
+    for (i = 0; i < ntups; i++)
     {
-        TableInfo  *tbinfo = &tblinfo[i];
+        Oid            prpubid = atooid(PQgetvalue(res, i, i_prpubid));
+        Oid            prrelid = atooid(PQgetvalue(res, i, i_prrelid));
+        PublicationInfo *pubinfo;
+        TableInfo  *tbinfo;

         /*
-         * Only regular and partitioned tables can be added to publications.
+         * Ignore any entries for which we aren't interested in either the
+         * publication or the rel.
          */
-        if (tbinfo->relkind != RELKIND_RELATION &&
-            tbinfo->relkind != RELKIND_PARTITIONED_TABLE)
+        pubinfo = findPublicationByOid(prpubid);
+        if (pubinfo == NULL)
+            continue;
+        tbinfo = findTableByOid(prrelid);
+        if (tbinfo == NULL)
             continue;

         /*
@@ -4100,55 +4131,24 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables)
         if (!(tbinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
             continue;

-        pg_log_info("reading publication membership for table \"%s.%s\"",
-                    tbinfo->dobj.namespace->dobj.name,
-                    tbinfo->dobj.name);
-
-        resetPQExpBuffer(query);
-
-        /* Get the publication membership for the table. */
-        appendPQExpBuffer(query,
-                          "SELECT pr.tableoid, pr.oid, p.pubname "
-                          "FROM pg_publication_rel pr, pg_publication p "
-                          "WHERE pr.prrelid = '%u'"
-                          "  AND p.oid = pr.prpubid",
-                          tbinfo->dobj.catId.oid);
-        res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
-
-        ntups = PQntuples(res);
-
-        if (ntups == 0)
-        {
-            /*
-             * Table is not member of any publications. Clean up and return.
-             */
-            PQclear(res);
-            continue;
-        }
-
-        i_tableoid = PQfnumber(res, "tableoid");
-        i_oid = PQfnumber(res, "oid");
-        i_pubname = PQfnumber(res, "pubname");
+        /* OK, make a DumpableObject for this relationship */
+        pubrinfo[j].dobj.objType = DO_PUBLICATION_REL;
+        pubrinfo[j].dobj.catId.tableoid =
+            atooid(PQgetvalue(res, i, i_tableoid));
+        pubrinfo[j].dobj.catId.oid = atooid(PQgetvalue(res, i, i_oid));
+        AssignDumpId(&pubrinfo[j].dobj);
+        pubrinfo[j].dobj.namespace = tbinfo->dobj.namespace;
+        pubrinfo[j].dobj.name = tbinfo->dobj.name;
+        pubrinfo[j].publication = pubinfo;
+        pubrinfo[j].pubtable = tbinfo;

-        pubrinfo = pg_malloc(ntups * sizeof(PublicationRelInfo));
+        /* Decide whether we want to dump it */
+        selectDumpablePublicationTable(&(pubrinfo[j].dobj), fout);

-        for (j = 0; j < ntups; j++)
-        {
-            pubrinfo[j].dobj.objType = DO_PUBLICATION_REL;
-            pubrinfo[j].dobj.catId.tableoid =
-                atooid(PQgetvalue(res, j, i_tableoid));
-            pubrinfo[j].dobj.catId.oid = atooid(PQgetvalue(res, j, i_oid));
-            AssignDumpId(&pubrinfo[j].dobj);
-            pubrinfo[j].dobj.namespace = tbinfo->dobj.namespace;
-            pubrinfo[j].dobj.name = tbinfo->dobj.name;
-            pubrinfo[j].pubname = pg_strdup(PQgetvalue(res, j, i_pubname));
-            pubrinfo[j].pubtable = tbinfo;
-
-            /* Decide whether we want to dump it */
-            selectDumpablePublicationTable(&(pubrinfo[j].dobj), fout);
-        }
-        PQclear(res);
+        j++;
     }
+
+    PQclear(res);
     destroyPQExpBuffer(query);
 }

@@ -4159,6 +4159,7 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables)
 static void
 dumpPublicationTable(Archive *fout, PublicationRelInfo *pubrinfo)
 {
+    PublicationInfo *pubinfo = pubrinfo->publication;
     TableInfo  *tbinfo = pubrinfo->pubtable;
     PQExpBuffer query;
     char       *tag;
@@ -4166,22 +4167,26 @@ dumpPublicationTable(Archive *fout, PublicationRelInfo *pubrinfo)
     if (!(pubrinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
         return;

-    tag = psprintf("%s %s", pubrinfo->pubname, tbinfo->dobj.name);
+    tag = psprintf("%s %s", pubinfo->dobj.name, tbinfo->dobj.name);

     query = createPQExpBuffer();

     appendPQExpBuffer(query, "ALTER PUBLICATION %s ADD TABLE ONLY",
-                      fmtId(pubrinfo->pubname));
+                      fmtId(pubinfo->dobj.name));
     appendPQExpBuffer(query, " %s;\n",
                       fmtQualifiedDumpable(tbinfo));

     /*
-     * There is no point in creating drop query as the drop is done by table
-     * drop.
+     * There is no point in creating a drop query as the drop is done by table
+     * drop.  (If you think to change this, see also _printTocEntry().)
+     * Although this object doesn't really have ownership as such, set the
+     * owner field anyway to ensure that the command is run by the correct
+     * role at restore time.
      */
     ArchiveEntry(fout, pubrinfo->dobj.catId, pubrinfo->dobj.dumpId,
                  ARCHIVE_OPTS(.tag = tag,
                               .namespace = tbinfo->dobj.namespace->dobj.name,
+                              .owner = pubinfo->rolname,
                               .description = "PUBLICATION TABLE",
                               .section = SECTION_POST_DATA,
                               .createStmt = query->data));
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index 731d0fe7ba..1290f9659b 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -623,8 +623,8 @@ typedef struct _PublicationInfo
 typedef struct _PublicationRelInfo
 {
     DumpableObject dobj;
+    PublicationInfo *publication;
     TableInfo  *pubtable;
-    char       *pubname;
 } PublicationRelInfo;

 /*
@@ -675,6 +675,7 @@ extern OprInfo *findOprByOid(Oid oid);
 extern CollInfo *findCollationByOid(Oid oid);
 extern NamespaceInfo *findNamespaceByOid(Oid oid);
 extern ExtensionInfo *findExtensionByOid(Oid oid);
+extern PublicationInfo *findPublicationByOid(Oid oid);

 extern void setExtensionMembership(ExtensionMemberId *extmems, int nextmems);
 extern ExtensionInfo *findOwningExtension(CatalogId catalogId);
@@ -727,7 +728,8 @@ extern void processExtensionTables(Archive *fout, ExtensionInfo extinfo[],
                                    int numExtensions);
 extern EventTriggerInfo *getEventTriggers(Archive *fout, int *numEventTriggers);
 extern void getPolicies(Archive *fout, TableInfo tblinfo[], int numTables);
-extern void getPublications(Archive *fout);
+extern PublicationInfo *getPublications(Archive *fout,
+                                        int *numPublications);
 extern void getPublicationTables(Archive *fout, TableInfo tblinfo[],
                                  int numTables);
 extern void getSubscriptions(Archive *fout);