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

On Mon, Aug 23, 2021 at 11:16 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Aug 17, 2021 at 6:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Amit Kapila <amit.kapila16@gmail.com> writes:
> > > On Tue, Aug 17, 2021 at 6:40 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >> On Mon, Aug 16, 2021 at 11:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >>> Abstractly it'd be
> > >>>
> > >>> createpub := CREATE PUBLICATION pubname FOR cpitem [, ... ] [ WITH ... ]
> > >>>
> > >>> cpitem := ALL TABLES |
> > >>>     TABLE name |
> > >>>     ALL TABLES IN SCHEMA name |
> > >>>     ALL SEQUENCES |
> > >>>     SEQUENCE name |
> > >>>     ALL SEQUENCES IN SCHEMA name |
> > >>>     name
> > >>>
> > >>> The grammar output would need some post-analysis to attribute the
> > >>> right type to bare "name" items, but that doesn't seem difficult.
> >
> > >> That last bare "name" cpitem. looks like it would permit the following syntax:
> > >> CREATE PUBLICATION pub FOR a,b,c;
> > >> Was that intentional?
> >
> > > I think so.
> >
> > I had supposed that we could throw an error at the post-processing stage,
> > or alternatively default to assuming that such names are tables.
> >
> > Now you could instead make the grammar work like
> >
> > cpitem := ALL TABLES |
> >           TABLE name [, ...] |
> >           ALL TABLES IN SCHEMA name [, ...] |
> >           ALL SEQUENCES |
> >           SEQUENCE name [, ...] |
> >           ALL SEQUENCES IN SCHEMA name [, ...]
> >
> > which would result in a two-level-list data structure.  I'm not sure
> > that this is better, as any sort of mistake would result in a very
> > uninformative generic "syntax error" from Bison.  Errors out of a
> > post-processing stage could be more specific than that.
>
> I preferred the implementation in the lines Tom Lane had proposed at [1]. Is it ok if the implementation is something
likebelow: 
> CreatePublicationStmt:
> CREATE PUBLICATION name FOR pub_obj_list opt_definition
> {
> CreatePublicationStmt *n = makeNode(CreatePublicationStmt);
> n->pubname = $3;
> n->options = $6;
> n->pubobjects = (List *)$5;
> $$ = (Node *)n;
> }
> ;
> pub_obj_list: PublicationObjSpec
> { $$ = list_make1($1); }
> | pub_obj_list ',' PublicationObjSpec
> { $$ = lappend($1, $3); }
> ;
> /* FOR TABLE and FOR ALL TABLES IN SCHEMA specifications */
> PublicationObjSpec: TABLE pubobj_expr
> { ....}
> | ALL TABLES IN_P SCHEMA pubobj_expr
> { ....}
> | pubobj_expr
> { ....}
> ;
> pubobj_expr:
> any_name
> { ....}
> | any_name '*'
> { ....}
> | ONLY any_name
> { ....}
> | ONLY '(' any_name ')'
> { ....}
> | CURRENT_SCHEMA
> { ....}
> ;

"FOR ALL TABLES” (that includes all tables in the database) is missing
in this syntax?

>
> I needed pubobj_expr to support the existing syntaxes supported by relation_expr and also to handle CURRENT_SCHEMA
supportin case of the "FOR ALL TABLES IN SCHEMA" feature. I changed the name to any_name to support objects like
"sch1.t1".

I think that relation_expr also accepts objects like "sch1.t1", no?

> I felt if a user specified "FOR ALL TABLES", the user should not be allowed to combine it with "FOR TABLE" and "FOR
ALLTABLES IN SCHEMA" as "FOR ALL TABLES" anyway will include all the tables. 

I think so too.

> Should we support the similar syntax in case of alter publication, like "ALTER PUBLICATION pub1 ADD TABLE t1,t2, ALL
TABLESIN SCHEMA sch1, sch2" or shall we keep these separate like "ALTER PUBLICATION pub1 ADD TABLE t1, t2"  and "ALTER
PUBLICATIONpub1 ADD ALL TABLES IN SCHEMA sch1, sch2". I preferred to keep it separate as we have kept ADD/DROP
separatelywhich cannot be combined currently. 

If we support the former syntax, the latter two syntaxes are also
supported. Why do we want to support only the latter separate two
syntaxes?

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/