RE: Skipping schema changes in publication

[shiy.f...@fujitsu.com]

On Tue, Apr 12, 2022 2:23 PM vignesh C <vignes...@gmail.com> wrote:
> 
> The patch does not apply on top of HEAD because of the recent commit,
> attached patch is rebased on top of HEAD.
> 

Thanks for your patch. Here are some comments for 0001 patch.

1. doc/src/sgml/catalogs.sgml
@@ -6438,6 +6438,15 @@ SCRAM-SHA-256$<replaceable>&lt;iteration 
count&gt;</replaceable>:<replaceable>&l
        A null value indicates that all columns are published.
       </para></entry>
      </row>
+
+    <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>pnskip</structfield> <type>bool</type>
+      </para>
+      <para>
+       True if the schema is skip schema
+      </para></entry>
+     </row>
     </tbody>
    </tgroup>
   </table>

This change is added to pg_publication_rel, I think it should be added to
pg_publication_namespace, right?

2.
postgres=# alter publication p1 add skip all tables in schema s1,s2;
ERROR:  schema "s1" is already member of publication "p1"

This error message seems odd to me, can we improve it? Something like:
schema "s1" is already skipped in publication "p1"

3.
create table tbl (a int primary key);
create schema s1;
create schema s2;
create table s1.tbl (a int);
create publication p1 for all tables skip all tables in schema s1,s2;

postgres=# \dRp+
                               Publication p1
  Owner   | All tables | Inserts | Updates | Deletes | Truncates | Via root
----------+------------+---------+---------+---------+-----------+----------
 postgres | t          | t       | t       | t       | t         | f
Skip tables from schemas:
    "s1"
    "s2"

postgres=# select * from pg_publication_tables;
 pubname | schemaname | tablename
---------+------------+-----------
 p1      | public     | tbl
 p1      | s1         | tbl
(2 rows)

There shouldn't be a record of s1.tbl, since all tables in schema s1 are 
skipped.

I found that it is caused by the following code:

src/backend/catalog/pg_publication.c
+       foreach(cell, pubschemalist)
+       {
+               PublicationSchInfo *pubsch = (PublicationSchInfo *) 
lfirst(cell);
+
+               skipschemaidlist = lappend_oid(result, pubsch->oid);
+       }

The first argument to append_oid() seems wrong, should it be:

skipschemaidlist = lappend_oid(skipschemaidlist, pubsch->oid);


4. src/backend/commands/publicationcmds.c

/*
 * Convert the PublicationObjSpecType list into schema oid list and
 * PublicationTable list.
 */
static void
ObjectsInPublicationToOids(List *pubobjspec_list, ParseState *pstate,
                                                   List **rels, List **schemas)

Should we modify the comment of ObjectsInPublicationToOids()?
"schema oid list" should be "PublicationSchInfo list".

Regards,
Shi yu