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

On Wed, Aug 25, 2021 at 3:07 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, Aug 25, 2021 at 1:19 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > 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
somethinglike below: 
> > > 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?
>
> "FOR ALL TABLES" is present in CreatePublicationStmt rule, sorry for
> not including all of CreatePublicationStmt rule in the previous mail,
> I thought of keeping the contents shorter:
>
> CreatePublicationStmt:
> CREATE PUBLICATION name opt_definition
> {....}
> | CREATE PUBLICATION name FOR ALL TABLES opt_definition
> {....}
> | CREATE PUBLICATION name FOR pub_obj_list opt_definition
> {....}
> ;
>
> It is not in  pub_obj_list as the user will be able to specify either
> of "FOR ALL TABLES" or "FOR TABLE/ FOR ALL TABLES IN SCHEMA" along
> with create publication.
>
> > >
> > > I needed pubobj_expr to support the existing syntaxes supported by relation_expr and also to handle
CURRENT_SCHEMAsupport in case of the "FOR ALL TABLES IN SCHEMA" feature. I changed the name to any_name to support
objectslike "sch1.t1". 
> >
> > I think that relation_expr also accepts objects like "sch1.t1", no?
>
> Earlier syntax only supported relations, the relations were parsed
> into RangeVar datatype. The new feature supports schema for which only
> the schema name is required. To keep the parsing rule common, I used
> any_name which will store the dotted name into a list for both
> relation and schema. I will later convert it into rangevar for
> relation and schema oid for schema names during the processing and
> create the publication. I felt relation_expr was able to handle dotted
> names because of qualified_name having "ColId indirection", here
> indirection rule takes care of handling the dotted names.
>
> > > I felt if a user specified "FOR ALL TABLES", the user should not be allowed to combine it with "FOR TABLE" and
"FORALL TABLES 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,
ALLTABLES IN SCHEMA sch1, sch2" or shall we keep these separate like "ALTER PUBLICATION pub1 ADD TABLE t1, t2"  and
"ALTERPUBLICATION pub1 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?
>
> We can support either syntax, I was not sure which one is better. If
> alter also should support similar syntax I can do it as a separate
> patch so as to not increase the main patch size. Thoughts?

I have implemented this in the 0003 patch, I have kept it separate to
reduce the testing effort and also it will be easier if someone
disagrees with the syntax. I will merge it to the main patch later
based on the feedback. Attached v22 patch has the changes for the
same.
Thoughts?

Regards,
Vignesh