RE: Added schema level support for publication. - Mailing list pgsql-hackers

From Mon, Aug 30, 2021 11:26 PM vignesh C <vignesh21@gmail.com> wrote:
> On Mon, Aug 30, 2021 at 9:10 AM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:
> >
> > 5)
> > +                       if (list_length(pubobj->name) == 1 &&
> > +                               (strcmp(relname, "CURRENT_SCHEMA") ==
> 0))
> > +                               ereport(ERROR,
> > +
> errcode(ERRCODE_SYNTAX_ERROR),
> > +                                               errmsg("invalid relation
> name at or near"),
> > +
> > + parser_errposition(pstate, pubobj->location));
> >
> > Maybe we don't need this check, because it will report an error in
> > OpenTableList() anyway, "relation "CURRENT_SCHEMA" does not exist" ,
> > and that message seems readable to me.
> 
> Allowing CURRENT_SCHEMA is required to support current schema for schema
> publications, currently I'm allowing this syntax during parsing and this error is
> thrown for relations later, this is done to keep the similar error as earlier before
> this feature support. I felt we can keep it like this to maintain the similar error.
> Thoughts?

Thanks for the explanation, I got the point.

Here are some other comments for v23-000x patches.

1)

@@ -6225,6 +6342,9 @@ describePublications(const char *pattern)
     bool        has_pubtruncate;
     bool        has_pubviaroot;
 
+    PQExpBufferData title;
+    printTableContent cont;
+
     if (pset.sversion < 100000)
     {
        ...
        PQExpBufferData title;
         printTableOpt myopt = pset.popt.topt;
         printTableContent cont;

Should we delete the inner declaration of 'title' and 'cont' ?

2)
-    /* parameters used for ALTER PUBLICATION ... ADD/DROP TABLE */
+    /* parameters used for ALTER PUBLICATION ... ADD/DROP TABLE/SCHEMA */

SCHEMA => ALL TABLES IN SCHEMA

3)

+                              .description = "PUBLICATION SCHEMA",
+                              .section = SECTION_POST_DATA,
+                              .createStmt = query->data));

Is it better to use something like 'PUBLICATION TABLES IN SCHEMA' to describe
the schema level table publication ? Because there could be some other type
publication such as 'ALL SEQUENCES IN SCHEMA' in the future, it will be better
to make it clear that we only publish table in schema in this patch.

Best regards,
Hou zj